| Message ID | 20231027-fix-rtl8366rb-v1-1-d565d905535a@linaro.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | dsa: tag_rtl4_a: Bump min packet size | expand |
You would want your subject to be: net: dsa: tag_rtl4_a: Bump min packet size On 10/27/23 13:21, Linus Walleij wrote: > It was reported that the "LuCI" web UI was not working properly > with a device using the RTL8366RB switch. Disabling the egress > port tagging code made the switch work again, but this is not > a good solution as we want to be able to direct traffic to a > certain port. > > It turns out that sometimes, but not always, small packets are > dropped by the switch for no reason. And we are positive that the Ethernet MAC is also properly padding frames before having them ingress the switch? > > If we pad the ethernet frames to a minimum of ETH_FRAME_LEN + FCS > (1518 bytes) everything starts working fine. That is quite unprecedented, either the switch is very bogus or there is something else we do not fully understand...
On Fri, Oct 27, 2023 at 02:23:13PM -0700, Florian Fainelli wrote: > You would want your subject to be: > > net: dsa: tag_rtl4_a: Bump min packet size > > On 10/27/23 13:21, Linus Walleij wrote: > > It was reported that the "LuCI" web UI was not working properly > > with a device using the RTL8366RB switch. Disabling the egress > > port tagging code made the switch work again, but this is not > > a good solution as we want to be able to direct traffic to a > > certain port. > > > > It turns out that sometimes, but not always, small packets are > > dropped by the switch for no reason. > > And we are positive that the Ethernet MAC is also properly padding frames > before having them ingress the switch? It might be interesting to run a script which systematically does a ping, or similar, for all frame sizes. > > If we pad the ethernet frames to a minimum of ETH_FRAME_LEN + FCS > > (1518 bytes) everything starts working fine. > > That is quite unprecedented, either the switch is very bogus or there is > something else we do not fully understand... It would also be interesting to know if the frames on the wire have the padding removed when needed. Its not going to be good for performance if a TCP ACK is 1500bytes in size, rather than the usual ~64. Have problems also been noticed with traffic going from user port to user port? Andrew
On Fri, Oct 27, 2023 at 10:21:39PM +0200, Linus Walleij wrote: > It was reported that the "LuCI" web UI was not working properly > with a device using the RTL8366RB switch. Disabling the egress > port tagging code made the switch work again, but this is not > a good solution as we want to be able to direct traffic to a > certain port. > > It turns out that sometimes, but not always, small packets are > dropped by the switch for no reason. "For no reason" is a technical statement which means "an unspecific/inconclusive drop reason in the ethtool -S output on the conduit interface (which also shows the hardware counters of the CPU port", or is it just a figure of speech? If just a figure of speech, could you please determine which counter gets incremented when the switch drops packets? What user port is being targeted when the switch drops packets? Any user port, or just specific ones? What protocol headers do those packets that are dropped have? Is it size that they have in common, I wonder (given that you say that small packets are not always dropped), or is it something else? > If we pad the ethernet frames to a minimum of ETH_FRAME_LEN + FCS > (1518 bytes) everything starts working fine. > > As we completely lack datasheet or any insight into how this > switch works, this trial-and-error solution is the best we > can do. > > I have tested smaller sizes, ETH_FRAME_LEN doesn't work for > example. > > Fixes: 0e90dfa7a8d8 ("net: dsa: tag_rtl4_a: Fix egress tags") Have you actually checked out this sha1sum and confirmed that the packet drop can be reproduced there? Ideally you could also go back to a bit earlier, to commit 9eb8bc593a5e ("net: dsa: tag_rtl4_a: fix egress tags") (this is a different commit from Qingfang with the same description) and test on user port 0 only? > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > --- > net/dsa/tag_rtl4_a.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/net/dsa/tag_rtl4_a.c b/net/dsa/tag_rtl4_a.c > index c327314b95e3..8b1e81a6377b 100644 > --- a/net/dsa/tag_rtl4_a.c > +++ b/net/dsa/tag_rtl4_a.c > @@ -41,8 +41,11 @@ static struct sk_buff *rtl4a_tag_xmit(struct sk_buff *skb, > u8 *tag; > u16 out; > > - /* Pad out to at least 60 bytes */ > - if (unlikely(__skb_put_padto(skb, ETH_ZLEN, false))) > + /* We need to pad out to at least ETH_FRAME_LEN + FCS bytes. This was > + * found by trial-and-error. Sometimes smaller frames work, but sadly > + * not always. > + */ > + if (unlikely(__skb_put_padto(skb, ETH_FRAME_LEN + ETH_FCS_LEN, false))) > return NULL; > > netdev_dbg(dev, "add realtek tag to package to port %d\n", > > --- > base-commit: 58720809f52779dc0f08e53e54b014209d13eebb > change-id: 20231027-fix-rtl8366rb-e752bd5901ca > > Best regards, > -- > Linus Walleij <linus.walleij@linaro.org> > Until more is known: pw-bot: cr
On Fri, Oct 27, 2023 at 11:23 PM Florian Fainelli <f.fainelli@gmail.com> wrote: > > It turns out that sometimes, but not always, small packets are > > dropped by the switch for no reason. > > And we are positive that the Ethernet MAC is also properly padding > frames before having them ingress the switch? I don't fully follow, this code is the one adding the padding isn't it? Then the result is transmitted to the switch from the ethernet MAC (drivers/net/ethernet/cortina/gemini.c). What am I getting wrong here... > > If we pad the ethernet frames to a minimum of ETH_FRAME_LEN + FCS > > (1518 bytes) everything starts working fine. > > That is quite unprecedented, either the switch is very bogus or there is > something else we do not fully understand... The switch is pretty bogus, all documentation we have of it is a vendor code drop, no data sheet. The format for ingress and egress tags was discovered using trial-and-error. Yours, Linus Walleij
> The switch is pretty bogus, all documentation we have of it is a vendor > code drop, no data sheet. The format for ingress and egress tags > was discovered using trial-and-error. The code drop does not include tagging? Does the code drop also pad to full size Ethernet frames? I still think it would be good to do some systematic testing to figure out if there is any pattern to the drops. Andrew
Hi Vladimir, first: THANKS, because you ALWAYS ask the *right* questions, and I quickly get closer to the real solution! On Sun, Oct 29, 2023 at 12:04 AM Vladimir Oltean <olteanv@gmail.com> wrote: > On Fri, Oct 27, 2023 at 10:21:39PM +0200, Linus Walleij wrote: > > It was reported that the "LuCI" web UI was not working properly > > with a device using the RTL8366RB switch. Disabling the egress > > port tagging code made the switch work again, but this is not > > a good solution as we want to be able to direct traffic to a > > certain port. > > > > It turns out that sometimes, but not always, small packets are > > dropped by the switch for no reason. > > "For no reason" is a technical statement which means "an unspecific/inconclusive > drop reason in the ethtool -S output on the conduit interface (which also > shows the hardware counters of the CPU port", or is it just a figure of > speech? If just a figure of speech, could you please determine which > counter gets incremented when the switch drops packets? I ran ethtool -S on the switch port before and after trying to access the web UI (LuCI) on the router with the chip, i.e. putting http://192.168.1.1 into the address bar of a browser and hitting enter three times. Then I ran diff -ur on the two outputs (before and after) and get this disturbing and symmetric stat: - Dot1dTpPortInDiscards: 0 + Dot1dTpPortInDiscards: 3 (Counters defined in drivers/net/dsa/realtek/rtl8366rb.c) > What user port is being targeted when the switch drops packets? Any user > port, or just specific ones? I tried on lan0, lan1, lan2 and lan3 (DSA ports 0,1,2,3): same result. On each of the ports, the same Dot1dTpPortInDiscards counter goes up. > What protocol headers do those packets that are dropped have? HTTP > Is it size > that they have in common, I wonder (given that you say that small > packets are not always dropped), or is it something else? I got it wrong, it's big packets getting dropped, not small ones... :( Some tcpdump:ing gives at hand that the problem is that the httpd is sending 1500 byte packages. Anything over 1496 fails in ping tests. 1496 is suspiciously much 1500 - DSA tag size. However the MTU of the parent ethernet is bumped nicely to 1504 and the device MTU is set up to accomodate it as well. Modifying the patch to just pad out packets >= 1496 bytes solves the problem in a better way, but maybe that is not the last thing we try here... I'll resend the patch with more elaborate commit log and test description in the commit. > > Fixes: 0e90dfa7a8d8 ("net: dsa: tag_rtl4_a: Fix egress tags") > > Have you actually checked out this sha1sum and confirmed that the packet > drop can be reproduced there? Ideally you could also go back to a bit > earlier, to commit 9eb8bc593a5e ("net: dsa: tag_rtl4_a: fix egress tags") > (this is a different commit from Qingfang with the same description) and > test on user port 0 only? Yes it should be an earlier commit indeed. I'll fix! Yours, Linus Walleij
From: Andrew Lunn > Sent: 28 October 2023 00:04 > > On Fri, Oct 27, 2023 at 02:23:13PM -0700, Florian Fainelli wrote: > > You would want your subject to be: > > > > net: dsa: tag_rtl4_a: Bump min packet size > > > > On 10/27/23 13:21, Linus Walleij wrote: > > > It was reported that the "LuCI" web UI was not working properly > > > with a device using the RTL8366RB switch. Disabling the egress > > > port tagging code made the switch work again, but this is not > > > a good solution as we want to be able to direct traffic to a > > > certain port. > > > > > > It turns out that sometimes, but not always, small packets are > > > dropped by the switch for no reason. > > > > And we are positive that the Ethernet MAC is also properly padding frames > > before having them ingress the switch? > > It might be interesting to run a script which systematically does a > ping, or similar, for all frame sizes. > > > > If we pad the ethernet frames to a minimum of ETH_FRAME_LEN + FCS > > > (1518 bytes) everything starts working fine. Thought - is that just because it slows everything down?? > > That is quite unprecedented, either the switch is very bogus or there is > > something else we do not fully understand... > > It would also be interesting to know if the frames on the wire have > the padding removed when needed. Its not going to be good for > performance if a TCP ACK is 1500bytes in size, rather than the usual > ~64. Non IP protocols are very likely to object to unexpected frame padding. I'm also sure I've seen systems do (the equivalent of) printk for overlong UDP packets. If you search the right place you'll find reports of packets being discarded before one of the VM network implementations padded ethernet frames to an even byte length. (I can't remember which, but have some fpga logic that adjusts the MAC address based on the TCP port number and manages to require there be no unexpected padding - yes it is broken.) David > > Have problems also been noticed with traffic going from user port to > user port? > > Andrew - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
> 1496 is suspiciously much 1500 - DSA tag size. However the > MTU of the parent ethernet is bumped nicely to 1504 and the > device MTU is set up to accomodate it as well. > > Modifying the patch to just pad out packets >= 1496 bytes > solves the problem in a better way, but maybe that is not the > last thing we try here... Have you tried playing with RTL8366RB_SGCR in rtl8366rb_change_mtu()? I had an annoying bug in the mv88e6xxx driver where the MTU configuration register was up to, but not including... So i had to change a <= to <. Andrew
On Mon, Oct 30, 2023 at 12:00 AM Andrew Lunn <andrew@lunn.ch> wrote: > > 1496 is suspiciously much 1500 - DSA tag size. However the > > MTU of the parent ethernet is bumped nicely to 1504 and the > > device MTU is set up to accomodate it as well. > > > > Modifying the patch to just pad out packets >= 1496 bytes > > solves the problem in a better way, but maybe that is not the > > last thing we try here... > > Have you tried playing with RTL8366RB_SGCR in rtl8366rb_change_mtu()? Yes I set the MTU to increasingly larger sizes. Sadly MTU has nothing to do with this, it's really annoying, I have a v2 patch that is better and working, I'll include some test text so you see where the problem is. Yours, Linus Walleij
On Sun, Oct 29, 2023 at 04:38:04PM +0100, Linus Walleij wrote: > On Fri, Oct 27, 2023 at 11:23 PM Florian Fainelli <f.fainelli@gmail.com> wrote: > > > > It turns out that sometimes, but not always, small packets are > > > dropped by the switch for no reason. > > > > And we are positive that the Ethernet MAC is also properly padding > > frames before having them ingress the switch? > > I don't fully follow, this code is the one adding the padding isn't it? > Then the result is transmitted to the switch from the ethernet > MAC (drivers/net/ethernet/cortina/gemini.c). > > What am I getting wrong here... What you are missing is that the existing padding done by rtl4a_tag_xmit() shouldn't be normally needed except for exceptional cases. Socket buffers smaller than ETH_ZLEN can be passed to any network device, and it is expected that either the driver or the hardware pads up to ETH_ZLEN automatically. Thus, the conduit driver should already know that it needs to pad packets to ETH_ZLEN. The exceptional cases are: - This is a tail tag (not the case here), which by definition needs to be located at the end of the skb. If you first put the tag then let the conduit interface pad, then the tail tag is no longer at the tail. So in that case, DSA pads first in generic code - dsa_user_xmit(). - The switch must handle the case where, after stripping the DSA tag from a ETH_ZLEN sized packet coming from the CPU port, it re-pads the packet on user port egress. Some switches don't handle that properly, and thus, we have isolated __skb_put_padto() calls within certain tagging protocols which address just that case. So, what Florian was asking is whether the conduit interface is not doing its expected job properly. You clarified that the problem is big rather than small packets, but we still need an explanation for the existing __skb_put_padto() call, given that it seems that it was placed there due to a misunderstanding rather than due to an explicit need for an exceptional case.
On Mon, Oct 30, 2023 at 1:51 PM Vladimir Oltean <olteanv@gmail.com> wrote: > So, what Florian was asking is whether the conduit interface is not > doing its expected job properly. You clarified that the problem is big > rather than small packets, but we still need an explanation for the > existing __skb_put_padto() call, given that it seems that it was placed > there due to a misunderstanding rather than due to an explicit need for > an exceptional case. Hm Deng added that and it was because the same was needed for some other tagger IIRC, let's see what he says. I will do some tests to just remove it and see what happens. Yours, Linus Walleij
Hi Deng, do you have some comments on the below, pertaining to commit 9eb8bc593a5eed167dac2029abef343854c5ba75 "net: dsa: tag_rtl4_a: fix egress tags"? I plan to test without the ZLEN padding and see what happens. IIRC it wasn't working without that, but I may just misremember the whole thing so let's rehash this. On Mon, Oct 30, 2023 at 1:51 PM Vladimir Oltean <olteanv@gmail.com> wrote: > What you are missing is that the existing padding done by rtl4a_tag_xmit() > shouldn't be normally needed except for exceptional cases. > > Socket buffers smaller than ETH_ZLEN can be passed to any network > device, and it is expected that either the driver or the hardware pads > up to ETH_ZLEN automatically. Thus, the conduit driver should already > know that it needs to pad packets to ETH_ZLEN. > > The exceptional cases are: > - This is a tail tag (not the case here), which by definition needs to > be located at the end of the skb. If you first put the tag then let > the conduit interface pad, then the tail tag is no longer at the tail. > So in that case, DSA pads first in generic code - dsa_user_xmit(). > - The switch must handle the case where, after stripping the DSA tag > from a ETH_ZLEN sized packet coming from the CPU port, it re-pads the > packet on user port egress. Some switches don't handle that properly, > and thus, we have isolated __skb_put_padto() calls within certain > tagging protocols which address just that case. > > So, what Florian was asking is whether the conduit interface is not > doing its expected job properly. You clarified that the problem is big > rather than small packets, but we still need an explanation for the > existing __skb_put_padto() call, given that it seems that it was placed > there due to a misunderstanding rather than due to an explicit need for > an exceptional case. Yours, Linus Walleij
diff --git a/net/dsa/tag_rtl4_a.c b/net/dsa/tag_rtl4_a.c index c327314b95e3..8b1e81a6377b 100644 --- a/net/dsa/tag_rtl4_a.c +++ b/net/dsa/tag_rtl4_a.c @@ -41,8 +41,11 @@ static struct sk_buff *rtl4a_tag_xmit(struct sk_buff *skb, u8 *tag; u16 out; - /* Pad out to at least 60 bytes */ - if (unlikely(__skb_put_padto(skb, ETH_ZLEN, false))) + /* We need to pad out to at least ETH_FRAME_LEN + FCS bytes. This was + * found by trial-and-error. Sometimes smaller frames work, but sadly + * not always. + */ + if (unlikely(__skb_put_padto(skb, ETH_FRAME_LEN + ETH_FCS_LEN, false))) return NULL; netdev_dbg(dev, "add realtek tag to package to port %d\n",
It was reported that the "LuCI" web UI was not working properly with a device using the RTL8366RB switch. Disabling the egress port tagging code made the switch work again, but this is not a good solution as we want to be able to direct traffic to a certain port. It turns out that sometimes, but not always, small packets are dropped by the switch for no reason. If we pad the ethernet frames to a minimum of ETH_FRAME_LEN + FCS (1518 bytes) everything starts working fine. As we completely lack datasheet or any insight into how this switch works, this trial-and-error solution is the best we can do. I have tested smaller sizes, ETH_FRAME_LEN doesn't work for example. Fixes: 0e90dfa7a8d8 ("net: dsa: tag_rtl4_a: Fix egress tags") Signed-off-by: Linus Walleij <linus.walleij@linaro.org> --- net/dsa/tag_rtl4_a.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) --- base-commit: 58720809f52779dc0f08e53e54b014209d13eebb change-id: 20231027-fix-rtl8366rb-e752bd5901ca Best regards,