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.faine...@gmail.com>
> > > Signed-off-by: Vladimir Oltean <vladimir.olt...@nxp.com>
> >
> > Reviewed-by: Florian Fainelli <f.faine...@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.faine...@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.faine...@gmail.com>
> Signed-off-by: David S. Miller <da...@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.
