Netdev Archive on lore.kernel.org
 help / color / Atom feed
From: Vladimir Oltean <vladimir.oltean@nxp.com>
To: Christian Eggers <ceggers@arri.de>
Cc: "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	"andrew@lunn.ch" <andrew@lunn.ch>,
	"f.fainelli@gmail.com" <f.fainelli@gmail.com>,
	"vivien.didelot@gmail.com" <vivien.didelot@gmail.com>,
	"kuba@kernel.org" <kuba@kernel.org>,
	Kurt Kanzenbach <kurt@linutronix.de>
Subject: Re: [RFC PATCH 02/13] net: dsa: implement a central TX reallocation procedure
Date: Sun, 18 Oct 2020 11:42:06 +0000
Message-ID: <20201018114205.dk5mcr7mcnqgo65w@skbuf> (raw)
In-Reply-To: <2267996.Y80grUFxSa@n95hx1g2>

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@arri.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

  reply index

Thread overview: 49+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-17 21:35 [RFC PATCH 00/13] Generic TX reallocation for DSA Vladimir Oltean
2020-10-17 21:35 ` [RFC PATCH 01/13] net: dsa: add plumbing for custom netdev statistics Vladimir Oltean
2020-10-17 22:13   ` Florian Fainelli
2020-10-17 23:15   ` Andrew Lunn
2020-10-18  0:23     ` Vladimir Oltean
2020-10-18 12:02   ` Heiner Kallweit
2020-10-18 12:16     ` Vladimir Oltean
2020-10-18 13:09       ` Heiner Kallweit
2020-10-18 13:48         ` Vladimir Oltean
2020-10-18 14:13           ` Heiner Kallweit
2020-10-18 22:58             ` Vladimir Oltean
2020-10-18 23:11               ` Florian Fainelli
2020-10-19  0:21                 ` Vladimir Oltean
2020-10-19  3:49                   ` Florian Fainelli
2020-10-19 12:05                     ` Andrew Lunn
2020-10-19 16:27                       ` Florian Fainelli
2020-10-18 16:49   ` Jakub Kicinski
2020-10-18 17:36     ` Andrew Lunn
2020-10-18 18:26       ` Jakub Kicinski
2020-10-18 18:33         ` Jakub Kicinski
2020-10-17 21:36 ` [RFC PATCH 02/13] net: dsa: implement a central TX reallocation procedure Vladimir Oltean
2020-10-17 22:01   ` Vladimir Oltean
2020-10-17 22:11     ` Florian Fainelli
2020-10-17 22:17       ` Vladimir Oltean
2020-10-18  0:37         ` Florian Fainelli
2020-10-19  8:33       ` David Laight
2020-10-19 10:30         ` Vladimir Oltean
2020-10-19 11:14           ` David Laight
2020-10-19 11:41             ` Vladimir Oltean
2020-10-19 12:19           ` Andrew Lunn
2020-10-17 22:31     ` Vladimir Oltean
2020-10-18  0:13       ` Vladimir Oltean
2020-10-18 10:36         ` Christian Eggers
2020-10-18 11:42           ` Vladimir Oltean [this message]
2020-10-18 11:59             ` Christian Eggers
2020-10-18 12:15               ` Vladimir Oltean
2020-10-17 21:36 ` [RFC PATCH 03/13] net: dsa: tag_ksz: don't allocate additional memory for padding/tagging Vladimir Oltean
2020-10-17 21:36 ` [RFC PATCH 04/13] net: dsa: trailer: " Vladimir Oltean
2020-10-17 21:36 ` [RFC PATCH 05/13] net: dsa: tag_qca: let DSA core deal with TX reallocation Vladimir Oltean
2020-10-17 21:36 ` [RFC PATCH 06/13] net: dsa: tag_ocelot: " Vladimir Oltean
2020-10-17 21:36 ` [RFC PATCH 07/13] net: dsa: tag_mtk: " Vladimir Oltean
2020-10-17 21:36 ` [RFC PATCH 08/13] net: dsa: tag_lan9303: " Vladimir Oltean
2020-10-17 21:36 ` [RFC PATCH 09/13] net: dsa: tag_edsa: " Vladimir Oltean
2020-10-17 21:36 ` [RFC PATCH 10/13] net: dsa: tag_brcm: " Vladimir Oltean
2020-10-17 21:36 ` [RFC PATCH 11/13] net: dsa: tag_dsa: " Vladimir Oltean
2020-10-17 21:36 ` [RFC PATCH 12/13] net: dsa: tag_gswip: " Vladimir Oltean
2020-10-17 22:19   ` Vladimir Oltean
2020-10-17 21:36 ` [RFC PATCH 13/13] net: dsa: tag_ar9331: " Vladimir Oltean
2020-10-17 23:07 ` [RFC PATCH 00/13] Generic TX reallocation for DSA Andrew Lunn

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20201018114205.dk5mcr7mcnqgo65w@skbuf \
    --to=vladimir.oltean@nxp.com \
    --cc=andrew@lunn.ch \
    --cc=ceggers@arri.de \
    --cc=f.fainelli@gmail.com \
    --cc=kuba@kernel.org \
    --cc=kurt@linutronix.de \
    --cc=netdev@vger.kernel.org \
    --cc=vivien.didelot@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Netdev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/netdev/0 netdev/git/0.git
	git clone --mirror https://lore.kernel.org/netdev/1 netdev/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 netdev netdev/ https://lore.kernel.org/netdev \
		netdev@vger.kernel.org
	public-inbox-index netdev

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.netdev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git