Re: [PATCH net] net: dsa: ksz: fix padding size of skb

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