netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jakub Kicinski <kuba@kernel.org>
To: Vladimir Oltean <olteanv@gmail.com>
Cc: Christian Eggers <ceggers@arri.de>,
	Kurt Kanzenbach <kurt@linutronix.de>,
	Woojung Huh <woojung.huh@microchip.com>,
	Andrew Lunn <andrew@lunn.ch>,
	Vivien Didelot <vivien.didelot@gmail.com>,
	Florian Fainelli <f.fainelli@gmail.com>,
	Microchip Linux Driver Support <UNGLinuxDriver@microchip.com>,
	"David S . Miller" <davem@davemloft.net>,
	netdev@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH net] net: dsa: ksz: fix padding size of skb
Date: Fri, 16 Oct 2020 11:52:20 -0700	[thread overview]
Message-ID: <20201016115220.771c7169@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com> (raw)
In-Reply-To: <20201016181319.2jrbdp5h7avzjczj@skbuf>

On Fri, 16 Oct 2020 21:13:19 +0300 Vladimir Oltean wrote:
> On Fri, Oct 16, 2020 at 11:03:11AM -0700, Jakub Kicinski wrote:
> > On Fri, 16 Oct 2020 18:56:45 +0300 Vladimir Oltean wrote:  
> > > > 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.  
> > 
> > FWIW if you want to avoid the reallocs you may want to set
> > needed_tailroom on the netdev.  
> 
> Tell me more about that, I've been meaning since forever to try it out.
> I read about needed_headroom and needed_tailroom, and it's one of the
> reasons why I added the .tail_tag option in the DSA tagger (to
> distinguish whether a switch needs headroom or tailroom), but I can't
> figure out, just from static analysis of the code, where exactly is the
> needed tailroom being accounted for. 

My understanding that the tail tag use case matches pretty well,
needed_tailroom is supposed to be a hit to the higher layers of 
the stack to leave some extra space at the end.

Now, it's probably of limited use in practice since I'd imagine 
most data comes in fragments. 

> For example, if I'm in Christian's
> situation, e.g. I have a packet smaller than ETH_ZLEN, would the
> tailroom be enough to hold just the dev->needed_tailroom, or would there
> be enough space in the skb for the entire ETH_ZLEN + dev->needed_tailroom?

I don't think we account for padding in general. Also looking at
__pskb_pull_tail() we're already seem to be provisioning some extra 
128B. So I guess it won't matter and the DSA tags are too small to
need the head/tailroom hints anyway..

      reply	other threads:[~2020-10-16 18:52 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-14 16:17 Christian Eggers
2020-10-14 16:47 ` Vladimir Oltean
2020-10-14 16:54   ` Vladimir Oltean
2020-10-14 17:02     ` Christian Eggers
2020-10-14 17:31       ` Vladimir Oltean
2020-10-15 17:58         ` Christian Eggers
2020-10-16  7:45           ` Kurt Kanzenbach
2020-10-16  9:00             ` Christian Eggers
2020-10-16  9:05               ` Vladimir Oltean
2020-10-16 12:44                 ` Christian Eggers
2020-10-16 15:56                   ` Vladimir Oltean
2020-10-16 18:03                     ` Jakub Kicinski
2020-10-16 18:13                       ` Vladimir Oltean
2020-10-16 18:52                         ` Jakub Kicinski [this message]

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=20201016115220.771c7169@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com \
    --to=kuba@kernel.org \
    --cc=UNGLinuxDriver@microchip.com \
    --cc=andrew@lunn.ch \
    --cc=ceggers@arri.de \
    --cc=davem@davemloft.net \
    --cc=f.fainelli@gmail.com \
    --cc=kurt@linutronix.de \
    --cc=linux-kernel@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=olteanv@gmail.com \
    --cc=vivien.didelot@gmail.com \
    --cc=woojung.huh@microchip.com \
    --subject='Re: [PATCH net] net: dsa: ksz: fix padding size of skb' \
    /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).