Netdev Archive on
 help / color / Atom feed
From: Christian Eggers <>
To: Andrew Lunn <>,
	Vivien Didelot <>,
	Florian Fainelli <>,
	Vladimir Oltean <>,
	Jakub Kicinski <>,
	Kurt Kanzenbach <>
Cc: "David S . Miller" <>,
	Woojung Huh <>,
	Microchip Linux Driver Support <>,
	<>, <>
Subject: [PATCH 0/3] net: dsa: move skb reallocation to dsa_slave_xmit
Date: Fri, 16 Oct 2020 22:02:23 +0200
Message-ID: <> (raw)

This series moves the reallocation of a skb which may be required due to
tail tagging or padding, from the tag_trailer and tag_ksz drivers to
dsa_slave_xmit. Additionally it prevents a skb_panic in a very special
corner case described here:

This series has been tested with KSZ9563 and my preliminary PTP patches.

On Friday, 16 October 2020, 17:56:45 CEST, Vladimir Oltean wrote:
> On Fri, Oct 16, 2020 at 02:44:46PM +0200, Christian Eggers wrote:
> > Machine:
> > - ARMv7 (i.MX6ULL), SMP_CACHE_BYTES is 64
> > - DSA device: Microchip KSZ9563 (I am currently working on time stamping
> > support)
> I have a board very similar to this on which I am going to test.
hopefully you are not just developing on PTP support for KSZ9563 ;-)
Which hardware do you exactly own? The problem I described to (link
above) can only be reproduced with my (not yes published) PTP patches.

> > Last, CONFIG_SLOB must be selected.
> Interesting, do you know why?
Yes. The other allocaters will actually allocate 512 byte instead of 320
if 64+256 bytes are requested. This will then be reported by ksize() and
let to more skb tailroom. The SLOB allocator will really allocate only
320 byte in this case, so that the skb will be run out of tail room when
tail tagging...

> > 3. "Manually" unsharing in dsa_slave_xmit(), reserving enough tailroom
> > for the tail tag (and ETH_ZLEN?). Would moving the "else" clause from
> > ksz_common_xmit()  to dsa_slave_xmit() do the job correctly?
> I was thinking about something like that, indeed. DSA knows everything
> about the tagger: its overhead, whether it's a tail tag or not. The xmit
> callback of the tagger should only be there to populate the tag where it
> needs to be. But reallocation, padding, etc etc, should all be dealt
> with by the common DSA xmit procedure. We want the taggers to be simple
> and reuse as much logic as possible, not to be bloated.
This series is the first draft for it. Some additional changes my be
done later:
1. All xmit() function now return either the supplied skb or NULL. No
reallocation will be done anymore. Maybe the type of the return value may
be changed to reflect this (e.g. to bool).
2. There is no path left which calls __skb_put_padto()/skb_pad() with
free_on_error set to false. So the following commit may be reverted in
order to simply the code:

cd0a137acbb6 ("net: core: Specify skb_pad()/skb_put_padto() SKB freeing")

On Friday, 16 October 2020, 11:05:27 CEST, Vladimir Oltean wrote:
> Kurt is asking, and rightfully so, because his tag_hellcreek.c driver
> (for a 1588 switch with tail tags) is copied from tag_ksz.c.
@Kurt: If this series (or a later version) is accepted, please update
your tagging driver. Ensure that your dsa_device_ops::overhead contains
the "maximum" possible tail tag len for xmit and that
dsa_device_ops::tail_tag is set to true.

On Friday, 16 October 2020, 20:03:11 CEST, Jakub Kicinski wrote:
> FWIW if you want to avoid the reallocs you may want to set
> needed_tailroom on the netdev.
I haven't looked for this yet. If this can really solve the tagging AND
padding problem, I would like to do this in a follow up patch.

Wishing a nice weekend for netdev.

             reply index

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-16 20:02 Christian Eggers [this message]
2020-10-16 20:02 ` [PATCH net-next 1/3] net: dsa: don't pass cloned skb's to drivers xmit function Christian Eggers
2020-10-17  0:48   ` Vladimir Oltean
2020-10-17 18:53     ` Christian Eggers
2020-10-17 19:12       ` Vladimir Oltean
2020-10-17 20:56         ` Christian Eggers
2020-10-17 21:35           ` Vladimir Oltean
2020-10-17  2:35   ` Florian Fainelli
2020-10-16 20:02 ` [PATCH net-next 2/3] net: dsa: tag_ksz: don't allocate additional memory for padding/tagging Christian Eggers
2020-10-16 20:02 ` [PATCH net-next 3/3] net: dsa: trailer: " Christian Eggers
2020-10-17  2:44 ` [PATCH 0/3] net: dsa: move skb reallocation to dsa_slave_xmit Florian Fainelli

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:

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

  git send-email \ \ \ \ \ \ \ \ \ \ \ \ \ \

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

Netdev Archive on

Archives are clonable:
	git clone --mirror netdev/git/0.git
	git clone --mirror 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/ \
	public-inbox-index netdev

Example config snippet for mirrors

Newsgroup available over NNTP:

AGPL code for this site: git clone