From: Christian Eggers <email@example.com> To: Andrew Lunn <firstname.lastname@example.org>, Vivien Didelot <email@example.com>, Florian Fainelli <firstname.lastname@example.org>, Vladimir Oltean <email@example.com>, Jakub Kicinski <firstname.lastname@example.org>, Kurt Kanzenbach <email@example.com> Cc: "David S . Miller" <firstname.lastname@example.org>, Woojung Huh <email@example.com>, Microchip Linux Driver Support <UNGLinuxDriver@microchip.com>, <firstname.lastname@example.org>, <email@example.com> Subject: [PATCH 0/3] net: dsa: move skb reallocation to dsa_slave_xmit Date: Fri, 16 Oct 2020 22:02:23 +0200 [thread overview] Message-ID: <firstname.lastname@example.org> (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: https://email@example.com/#2554896 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. Christian
next reply other threads:[~2020-10-16 20:03 UTC|newest] 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: 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 \ --firstname.lastname@example.org \ --email@example.com \ --cc=UNGLinuxDriver@microchip.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --subject='Re: [PATCH 0/3] net: dsa: move skb reallocation to dsa_slave_xmit' \ /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
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).