| Message ID | 20201101191620.589272-1-vladimir.oltean@nxp.com |
|---|---|
| Headers | show |
| Series | Generic TX reallocation for DSA | expand |
On 11/1/2020 11:16 AM, Vladimir Oltean wrote: > Now that we have a central TX reallocation procedure that accounts for > the tagger's needed headroom in a generic way, we can remove the > skb_cow_head call. > > Cc: Florian Fainelli <f.fainelli@gmail.com> > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> -- Florian
On 11/1/2020 11:16 AM, Vladimir Oltean wrote: > From: Christian Eggers <ceggers@arri.de> > > The caller (dsa_slave_xmit) guarantees that the frame length is at least > ETH_ZLEN and that enough memory for tail tagging is available. > > Signed-off-by: Christian Eggers <ceggers@arri.de> > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> -- Florian
On 11/1/2020 11:16 AM, Vladimir Oltean wrote: > Now that we have a central TX reallocation procedure that accounts for > the tagger's needed headroom in a generic way, we can remove the > skb_cow_head call. > > Cc: John Crispin <john@phrozen.org> > Cc: Alexander Lobakin <alobakin@pm.me> > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> -- Florian
On 11/1/2020 11:16 AM, Vladimir Oltean wrote: > Now that we have a central TX reallocation procedure that accounts for > the tagger's needed headroom in a generic way, we can remove the > skb_cow_head call. > > Cc: DENG Qingfang <dqfext@gmail.com> > Cc: Sean Wang <sean.wang@mediatek.com> > Cc: John Crispin <john@phrozen.org> > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> -- Florian
On 11/1/2020 11:16 AM, Vladimir Oltean wrote: > Now that we have a central TX reallocation procedure that accounts for > the tagger's needed headroom in a generic way, we can remove the > skb_cow_head call. > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> -- Florian
On 11/1/2020 11:16 AM, Vladimir Oltean wrote: > Now that we have a central TX reallocation procedure that accounts for > the tagger's needed headroom in a generic way, we can remove the > skb_cow_head call. > > Similar to the EtherType DSA tagger, the old Marvell tagger can > transform an 802.1Q header if present into a DSA tag, so there is no > headroom required in that case. But we are ensuring that it exists, > regardless (practically speaking, the headroom must be 4 bytes larger > than it needs to be). > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> -- Florian
On Sun, 1 Nov 2020 21:16:08 +0200 Vladimir Oltean wrote: > Christian has reported buggy usage of skb_put() in tag_ksz.c, which is > only triggerable in real life using his not-yet-published patches for > IEEE 1588 timestamping on Micrel KSZ switches. > > The concrete problem there is that the driver can end up calling > skb_put() and exceed the end of the skb data area, because even though > it had reallocated the frame once before, it hadn't reallocated it large > enough. Christian explained it in more detail here: > > https://lore.kernel.org/netdev/20201014161719.30289-1-ceggers@arri.de/ > https://lore.kernel.org/netdev/20201016200226.23994-1-ceggers@arri.de/ > > But actually there's a bigger problem, which is that some taggers which > get more rarely tested tend to do some shenanigans which are uncaught > for the longest time, and in the meanwhile, their code gets copy-pasted > into other taggers, creating a mess. For example, the tail tagging > driver for Marvell 88E6060 currently reallocates _every_single_frame_ on > TX. Is that an obvious indication that nobody is using it? Sure. Is it a > good model to follow when developing a new tail tagging driver? No. > > DSA has all the information it needs in order to simplify the job of a > tagger on TX. It knows whether it's a normal or a tail tagger, and what > is the protocol overhead it incurs. So this series performs the > reallocation centrally. Applied, thank you!
On Mon, Nov 02, 2020 at 12:34:11PM -0800, Florian Fainelli wrote: > On 11/1/2020 11:16 AM, Vladimir Oltean wrote: > > Now that we have a central TX reallocation procedure that accounts for > > the tagger's needed headroom in a generic way, we can remove the > > skb_cow_head call. > > > > Cc: Florian Fainelli <f.fainelli@gmail.com> > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> > > Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> Florian, I just noticed that tag_brcm.c has an __skb_put_padto call, even though it is not a tail tagger. This comes from commit: commit bf08c34086d159edde5c54902dfa2caa4d9fbd8c Author: Florian Fainelli <f.fainelli@gmail.com> Date: Wed Jan 3 22:13:00 2018 -0800 net: dsa: Move padding into Broadcom tagger Instead of having the different master network device drivers potentially used by DSA/Broadcom tags, move the padding necessary for the switches to accept short packets where it makes most sense: within tag_brcm.c. This avoids multiplying the number of similar commits to e.g: bgmac, bcmsysport, etc. Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> Signed-off-by: David S. Miller <davem@davemloft.net> Do you remember why this was needed? As far as I understand, either the DSA master driver or the MAC itself should pad frames automatically. Is that not happening on Broadcom SoCs, or why do you need to pad from DSA? How should we deal with this? Having tag_brcm.c still do some potential reallocation defeats the purpose of doing it centrally, in a way. I was trying to change the prototype of struct dsa_device_ops::xmit to stop returning a struct sk_buff *, and I stumbled upon this. Should we just go ahead and pad everything unconditionally in DSA?
On Tue, 3 Nov 2020 10:51:00 +0000 Vladimir Oltean wrote: > On Mon, Nov 02, 2020 at 12:34:11PM -0800, Florian Fainelli wrote: > > On 11/1/2020 11:16 AM, Vladimir Oltean wrote: > > > Now that we have a central TX reallocation procedure that accounts for > > > the tagger's needed headroom in a generic way, we can remove the > > > skb_cow_head call. > > > > > > Cc: Florian Fainelli <f.fainelli@gmail.com> > > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> > > > > Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> > > Florian, I just noticed that tag_brcm.c has an __skb_put_padto call, > even though it is not a tail tagger. This comes from commit: > > commit bf08c34086d159edde5c54902dfa2caa4d9fbd8c > Author: Florian Fainelli <f.fainelli@gmail.com> > Date: Wed Jan 3 22:13:00 2018 -0800 > > net: dsa: Move padding into Broadcom tagger > > Instead of having the different master network device drivers > potentially used by DSA/Broadcom tags, move the padding necessary for > the switches to accept short packets where it makes most sense: within > tag_brcm.c. This avoids multiplying the number of similar commits to > e.g: bgmac, bcmsysport, etc. > > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> > Signed-off-by: David S. Miller <davem@davemloft.net> > > Do you remember why this was needed? > As far as I understand, either the DSA master driver or the MAC itself > should pad frames automatically. Is that not happening on Broadcom SoCs, > or why do you need to pad from DSA? > How should we deal with this? Having tag_brcm.c still do some potential > reallocation defeats the purpose of doing it centrally, in a way. I was > trying to change the prototype of struct dsa_device_ops::xmit to stop > returning a struct sk_buff *, and I stumbled upon this. > Should we just go ahead and pad everything unconditionally in DSA? In a recent discussion I was wondering if it makes sense to add the padding len to struct net_device, with similar best-effort semantics to needed_*room. It'd be a u8, so little worry about struct size. You could also make sure DSA always provisions for padding if it has to reallocate, you don't need to actually pad: @@ -568,6 +568,9 @@ static int dsa_realloc_skb(struct sk_buff *skb, struct net_device *dev) /* No reallocation needed, yay! */ return 0; + if (skb->len < ETH_ZLEN) + needed_tailroom += ETH_ZLEN; + return pskb_expand_head(skb, needed_headroom, needed_tailroom, GFP_ATOMIC); } That should save the realloc for all reasonable drivers while not costing anything (other than extra if()) to drivers which don't care.
On 11/3/2020 2:51 AM, Vladimir Oltean wrote: > On Mon, Nov 02, 2020 at 12:34:11PM -0800, Florian Fainelli wrote: >> On 11/1/2020 11:16 AM, Vladimir Oltean wrote: >>> Now that we have a central TX reallocation procedure that accounts for >>> the tagger's needed headroom in a generic way, we can remove the >>> skb_cow_head call. >>> >>> Cc: Florian Fainelli <f.fainelli@gmail.com> >>> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> >> >> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> > > Florian, I just noticed that tag_brcm.c has an __skb_put_padto call, > even though it is not a tail tagger. This comes from commit: > > commit bf08c34086d159edde5c54902dfa2caa4d9fbd8c > Author: Florian Fainelli <f.fainelli@gmail.com> > Date: Wed Jan 3 22:13:00 2018 -0800 > > net: dsa: Move padding into Broadcom tagger > > Instead of having the different master network device drivers > potentially used by DSA/Broadcom tags, move the padding necessary for > the switches to accept short packets where it makes most sense: within > tag_brcm.c. This avoids multiplying the number of similar commits to > e.g: bgmac, bcmsysport, etc. > > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> > Signed-off-by: David S. Miller <davem@davemloft.net> > > Do you remember why this was needed? Yes, it is explained in the comment surrounding the padding: /* The Ethernet switch we are interfaced with needs packets to be at * least 64 bytes (including FCS) otherwise they will be discarded when * they enter the switch port logic. When Broadcom tags are enabled, we * need to make sure that packets are at least 68 bytes * (including FCS and tag) because the length verification is done after * the Broadcom tag is stripped off the ingress packet. > As far as I understand, either the DSA master driver or the MAC itself > should pad frames automatically. Is that not happening on Broadcom SoCs, > or why do you need to pad from DSA? Some of the Ethernet MACs were not doing that automatic padding and/or had no option to turn this on. This is true for at least SYSTEMPORT (not Lite) and BGMAC. GENET is also commonly used but does support automatic RUNT frame padding. > How should we deal with this? Having tag_brcm.c still do some potential > reallocation defeats the purpose of doing it centrally, in a way. I was > trying to change the prototype of struct dsa_device_ops::xmit to stop > returning a struct sk_buff *, and I stumbled upon this. > Should we just go ahead and pad everything unconditionally in DSA? > This is really a problem specific to Broadcom tags and how the switch operate so it seems reasonable to leave those details down to the tagger. -- Florian
On Tue, Nov 03, 2020 at 09:32:38AM -0800, Florian Fainelli wrote: > the length verification is done after the Broadcom tag is stripped off > the ingress packet. Aha, that makes sense. So to the DSA master it has the proper length, but to the switch it doesn't. Interesting problem. It must mean that my switches pad short frames automatically.
On Tue, Nov 03, 2020 at 09:04:11AM -0800, Jakub Kicinski wrote: > In a recent discussion I was wondering if it makes sense to add the > padding len to struct net_device, with similar best-effort semantics > to needed_*room. It'd be a u8, so little worry about struct size. What would that mean in practice? Modify the existing alloc_skb calls which have an expression e that depends on LL_RESERVED_SPACE(dev), into max(e, dev->padding_len)? There's a lot of calls to alloc_skb to modify though... > You could also make sure DSA always provisions for padding if it has to > reallocate, you don't need to actually pad: > > @@ -568,6 +568,9 @@ static int dsa_realloc_skb(struct sk_buff *skb, struct net_device *dev) > /* No reallocation needed, yay! */ > return 0; > > + if (skb->len < ETH_ZLEN) > + needed_tailroom += ETH_ZLEN; > + > return pskb_expand_head(skb, needed_headroom, needed_tailroom, > GFP_ATOMIC); > } > > That should save the realloc for all reasonable drivers while not > costing anything (other than extra if()) to drivers which don't care. DSA does already provision for padding if it has to reallocate, but only for the case where it needs to add a frame header at the end of the skb (i.e. "tail taggers"). My question here was whether there would be any drawback to doing that for all types of switches, including ones that might deal with padding in some other way (i.e. in hardware).
On Tue, 3 Nov 2020 18:15:29 +0000 Vladimir Oltean wrote: > On Tue, Nov 03, 2020 at 09:04:11AM -0800, Jakub Kicinski wrote: > > In a recent discussion I was wondering if it makes sense to add the > > padding len to struct net_device, with similar best-effort semantics > > to needed_*room. It'd be a u8, so little worry about struct size. > > What would that mean in practice? Modify the existing alloc_skb calls > which have an expression e that depends on LL_RESERVED_SPACE(dev), into > max(e, dev->padding_len)? There's a lot of calls to alloc_skb to modify > though... Yeah, separate helper would probably be warranted, so we don't have to touch multiple sites every time we adjust things. > > You could also make sure DSA always provisions for padding if it has to > > reallocate, you don't need to actually pad: > > > > @@ -568,6 +568,9 @@ static int dsa_realloc_skb(struct sk_buff *skb, struct net_device *dev) > > /* No reallocation needed, yay! */ > > return 0; > > > > + if (skb->len < ETH_ZLEN) > > + needed_tailroom += ETH_ZLEN; > > + > > return pskb_expand_head(skb, needed_headroom, needed_tailroom, > > GFP_ATOMIC); > > } > > > > That should save the realloc for all reasonable drivers while not > > costing anything (other than extra if()) to drivers which don't care. > > DSA does already provision for padding if it has to reallocate, but only > for the case where it needs to add a frame header at the end of the skb > (i.e. "tail taggers"). My question here was whether there would be any > drawback to doing that for all types of switches, including ones that > might deal with padding in some other way (i.e. in hardware). Well, we may re-alloc unnecessarily if we provision for padding of all frames. So what I was trying to achieve was to add the padding space _after_ the "do we need to realloc" check. /* over-provision space for pad, if we realloc anyway */ if (!needed_tailroom && skb->len < ETH_ZLEN) needed_tailroom = ETH_ZLEN - skb->len;
Christian has reported buggy usage of skb_put() in tag_ksz.c, which is only triggerable in real life using his not-yet-published patches for IEEE 1588 timestamping on Micrel KSZ switches. The concrete problem there is that the driver can end up calling skb_put() and exceed the end of the skb data area, because even though it had reallocated the frame once before, it hadn't reallocated it large enough. Christian explained it in more detail here: https://lore.kernel.org/netdev/20201014161719.30289-1-ceggers@arri.de/ https://lore.kernel.org/netdev/20201016200226.23994-1-ceggers@arri.de/ But actually there's a bigger problem, which is that some taggers which get more rarely tested tend to do some shenanigans which are uncaught for the longest time, and in the meanwhile, their code gets copy-pasted into other taggers, creating a mess. For example, the tail tagging driver for Marvell 88E6060 currently reallocates _every_single_frame_ on TX. Is that an obvious indication that nobody is using it? Sure. Is it a good model to follow when developing a new tail tagging driver? No. DSA has all the information it needs in order to simplify the job of a tagger on TX. It knows whether it's a normal or a tail tagger, and what is the protocol overhead it incurs. So this series performs the reallocation centrally. Changes in v3: - Use dev_kfree_skb_any due to potential hardirq context in xmit path. Changes in v2: - Dropped the tx_realloc counters for now, since the patch was pretty controversial and I lack the time at the moment to introduce new UAPI for that. - Do padding for tail taggers irrespective of whether they need to reallocate the skb or not. Christian Eggers (2): net: dsa: tag_ksz: don't allocate additional memory for padding/tagging net: dsa: trailer: don't allocate additional memory for padding/tagging Vladimir Oltean (10): net: dsa: implement a central TX reallocation procedure net: dsa: tag_qca: let DSA core deal with TX reallocation net: dsa: tag_ocelot: let DSA core deal with TX reallocation net: dsa: tag_mtk: let DSA core deal with TX reallocation net: dsa: tag_lan9303: let DSA core deal with TX reallocation net: dsa: tag_edsa: let DSA core deal with TX reallocation net: dsa: tag_brcm: let DSA core deal with TX reallocation net: dsa: tag_dsa: let DSA core deal with TX reallocation net: dsa: tag_gswip: let DSA core deal with TX reallocation net: dsa: tag_ar9331: let DSA core deal with TX reallocation net/dsa/slave.c | 45 ++++++++++++++++++++++++++ net/dsa/tag_ar9331.c | 3 -- net/dsa/tag_brcm.c | 3 -- net/dsa/tag_dsa.c | 5 --- net/dsa/tag_edsa.c | 4 --- net/dsa/tag_gswip.c | 5 --- net/dsa/tag_ksz.c | 73 ++++++------------------------------------- net/dsa/tag_lan9303.c | 9 ------ net/dsa/tag_mtk.c | 3 -- net/dsa/tag_ocelot.c | 7 ----- net/dsa/tag_qca.c | 3 -- net/dsa/tag_trailer.c | 31 ++---------------- 12 files changed, 56 insertions(+), 135 deletions(-)