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;
