| Message ID | 20201101191620.589272-10-vladimir.oltean@nxp.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Netdev Maintainers |
| 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>
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.
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;
diff --git a/net/dsa/tag_brcm.c b/net/dsa/tag_brcm.c index ad72dff8d524..e934dace3922 100644 --- a/net/dsa/tag_brcm.c +++ b/net/dsa/tag_brcm.c @@ -66,9 +66,6 @@ static struct sk_buff *brcm_tag_xmit_ll(struct sk_buff *skb, u16 queue = skb_get_queue_mapping(skb); u8 *brcm_tag; - if (skb_cow_head(skb, BRCM_TAG_LEN) < 0) - return NULL; - /* 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
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> --- Changes in v3: None. Changes in v2: None. net/dsa/tag_brcm.c | 3 --- 1 file changed, 3 deletions(-)