archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/3] net: dsa: move skb reallocation to dsa_slave_xmit
@ 2020-10-16 20:02 Christian Eggers
  2020-10-16 20:02 ` [PATCH net-next 1/3] net: dsa: don't pass cloned skb's to drivers xmit function Christian Eggers
                   ` (3 more replies)
  0 siblings, 4 replies; 11+ messages in thread
From: Christian Eggers @ 2020-10-16 20:02 UTC (permalink / raw)
  To: Andrew Lunn, Vivien Didelot, Florian Fainelli, Vladimir Oltean,
	Jakub Kicinski, Kurt Kanzenbach
  Cc: David S . Miller, Woojung Huh, Microchip Linux Driver Support,
	netdev, linux-kernel

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.

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2020-10-17 21:35 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-16 20:02 [PATCH 0/3] net: dsa: move skb reallocation to dsa_slave_xmit Christian Eggers
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

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).