Message-ID: <20201018114205.dk5mcr7mcnqgo65w@skbuf>
Date:   Sun, 18 Oct 2020 11:42:06 +0000
From:   Vladimir Oltean <vladimir.oltean@....com>
To:     Christian Eggers <ceggers@...i.de>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "andrew@...n.ch" <andrew@...n.ch>,
        "f.fainelli@...il.com" <f.fainelli@...il.com>,
        "vivien.didelot@...il.com" <vivien.didelot@...il.com>,
        "kuba@...nel.org" <kuba@...nel.org>,
        Kurt Kanzenbach <kurt@...utronix.de>
Subject: Re: [RFC PATCH 02/13] net: dsa: implement a central TX reallocation
 procedure

On Sun, Oct 18, 2020 at 12:36:03PM +0200, Christian Eggers wrote:
> >       /* For tail taggers, we need to pad short frames ourselves, to ensure
> >        * that the tail tag does not fail at its role of being at the end of
> >        * the packet, once the master interface pads the frame.
> >        */
> >       if (unlikely(needed_tailroom && skb->len < ETH_ZLEN))
> Can "needed_tailroom" be used equivalently for dsa_device_ops::tail_tag?

For what I care, it's "equivalent enough".
Since needed_tailroom comes from slave->needed_tailroom + master->needed_tailroom,
there might be a situation when slave->needed_tailroom is 0, but padding
is performed nonetheless. I am not sure that this is something that will
occur in practice. If you grep drivers/net/ethernet, only Sun Virtual Network
devices set dev->needed_tailroom. I would prefer avoiding to touch any
other cache line, and not duplicating the tail_tag or overhead members
if I can avoid it. If it becomes a problem, I'll make that change.

> >               padlen = ETH_ZLEN - skb->len;
> >       needed_tailroom += padlen;
> >       needed_headroom -= skb_headroom(skb);
> >       needed_tailroom -= skb_tailroom(skb);
> >
> >       if (likely(needed_headroom <= 0 && needed_tailroom <= 0 &&
> >                  !skb_cloned(skb)))
> >               /* No reallocation needed, yay! */
> >               return 0;
> >
> >       e = this_cpu_ptr(p->extra_stats);
> >       u64_stats_update_begin(&e->syncp);
> >       e->tx_reallocs++;
> >       u64_stats_update_end(&e->syncp);
> >
> >       err = pskb_expand_head(skb, max(needed_headroom, 0),
> >                              max(needed_tailroom, 0), GFP_ATOMIC);
> You may remove the second max() statement (around needed_tailroom). This would
> size the reallocated skb more exactly to the size actually required an may save
> some RAM (already tested too).

Please explain more. needed_tailroom can be negative, why should I
shrink the tailroom?

> Alternatively I suggest moving the max() statements up in order to completely
> avoid negative values for needed_headroom / needed_tailroom:
> 
>         needed_headroom = max(needed_headroom - skb_headroom(skb), 0);
>         needed_tailroom = max(needed_tailroom - skb_tailroom(skb), 0);
> 
>         if (likely(needed_headroom == 0 && needed_tailroom == 0 &&
>                    !skb_cloned(skb)))
>                 /* No reallocation needed, yay! */
>                 return 0;
>         ...
>         err = pskb_expand_head(skb, needed_headroom, needed_tailroom, GFP_ATOMIC);
> 

Ok, this looks better, thanks.

> >       if (err < 0 || !padlen)
> >               return err;
> This is correct but looks misleading for me. What about...
>         if (err < 0)
>                 return err;
> 
>         return padlen ? __skb_put_padto(skb, ETH_ZLEN, false) : 0;
> 

Ok, I suppose it can be misleading. Will do this even if it's one more
branch. It's in the unlikely path anyway.

> FYI two dumps of a padded skb (before/after) dsa_realloc_skb():
> [ 1983.621180] old:skb len=58 headroom=2 headlen=58 tailroom=68
> [ 1983.621180] mac=(2,14) net=(16,-1) trans=-1
> [ 1983.621180] shinfo(txflags=1 nr_frags=0 gso(size=0 type=0 segs=0))
> [ 1983.621180] csum(0x0 ip_summed=0 complete_sw=0 valid=0 level=0)
> [ 1983.621180] hash(0x0 sw=0 l4=0) proto=0x88f7 pkttype=0 iif=0
> [ 1983.635575] old:dev name=lan0 feat=0x0x0002000000005220
> [ 1983.638205] old:sk family=17 type=3 proto=0
> [ 1983.640323] old:skb linear:   00000000: 01 1b 19 00 00 00 ee 97 1f aa 93 21 88 f7 01 02
> [ 1983.644416] old:skb linear:   00000010: 00 2c 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> [ 1983.648656] old:skb linear:   00000020: 00 00 ee 97 1f ff fe aa 93 21 00 01 06 79 01 7f
> [ 1983.652726] old:skb linear:   00000030: 00 00 00 00 00 00 00 00 00 00
> 
> [ 1983.656040] new:skb len=60 headroom=2 headlen=60 tailroom=66
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ok
> [ 1983.656040] mac=(2,14) net=(16,-1) trans=-1
> [ 1983.656040] shinfo(txflags=1 nr_frags=0 gso(size=0 type=0 segs=0))
> [ 1983.656040] csum(0x0 ip_summed=0 complete_sw=0 valid=0 level=0)
> [ 1983.656040] hash(0x0 sw=0 l4=0) proto=0x88f7 pkttype=0 iif=0
> [ 1983.670427] new:dev name=lan0 feat=0x0x0002000000005220
> [ 1983.673082] new:sk family=17 type=3 proto=0
> [ 1983.675233] new:skb linear:   00000000: 01 1b 19 00 00 00 ee 97 1f aa 93 21 88 f7 01 02
> [ 1983.679329] new:skb linear:   00000010: 00 2c 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> [ 1983.683411] new:skb linear:   00000020: 00 00 ee 97 1f ff fe aa 93 21 00 01 06 79 01 7f
> [ 1983.687506] new:skb linear:   00000030: 00 00 00 00 00 00 00 00 00 00 00 00

The dumps look ok, and in line with what I saw.
For what it's worth, anybody can test with any tagger, you don' need
dedicated hardware. You just need to replace the value returned by your
dsa_switch_ops::get_tag_protocol method. This is enough to get skb dumps.
For more complicated things like ensuring 1588 timestamping
works, it won't be enough, of course, so your testing is still very
valuable to ensure that keeps working for you (it does for me).

> 
> Tested-by: Christian Eggers <ceggers@...i.de>  # For tail taggers only

Thanks, I'll resend this in about 2 weeks. In the meantime I'll update
this branch:
https://github.com/vladimiroltean/linux/commits/dsa-tx-realloc

Powered by blists - more mailing lists