Re: [PATCH 0/2] pktgen: Clone skb to avoid corruption of skbs in ndo_start_xmit methods (v3)

[Ben Hutchings <bhutchings@solarflare.com>]

On Sun, 2011-08-21 at 20:27 -0400, Neil Horman wrote:
> On Wed, Aug 17, 2011 at 04:07:17PM +0100, Ben Hutchings wrote:
> > On Tue, 2011-07-26 at 12:05 -0400, Neil Horman wrote:
> > > Ok, after considering all your comments, Dave suggested this as an alternate
> > > approach:
> > > 
> > > 1) We create a new priv_flag, IFF_SKB_TX_SHARED, to identify drivers capable of
> > > handling shared skbs.  Default is to not set this flag
> > > 
> > > 2) Modify ether_setup to enable this flag, under the assumption that any driver
> > > calling this  function is initalizing a real ethernet device and as such can
> > > handle shared skbs since they don't tend to store state in the skb struct.
> > > Pktgen can then query this flag when a user script attempts to issue the
> > > clone_skb command and decide if it is to be alowed or not.
> > [...]
> > 
> > A bunch of Ethernet drivers do skb_pad() or skb_padto() in their
> > ndo_start_xmit implementations, either to avoid hardware bugs or because
> > the MAC doesn't automatically pad to the minimum frame length.  This
> > presumably means they can't generally handle shared skbs, though in the
> > specific case of pktgen it should be safe as long as a single skb is not
> > submitted by multiple threads at once.
> > 
> Agreed, given that pktgen is doing skb sharing in a serialized manner (i.e. one
> thread of execution increasing skb->users rather than in multiple threads), the
> skb_pad[to] cases are safe.  Are there cases in which shared skbs are
> transmitted in parallel threads that we need to check for?

Not that I'm aware of.  However, you have defined the flag to mean 'safe
to send shared skbs', and this is not generally true for those drivers.

So please decide whether:
a. The flag means 'safe to send shared skbs' (i.e. the TX path does not
modify the skb) and drivers which pad must clear it on their devices.
b. The flag means 'safe to send shared skbs in the way ptkgen does', and
the restrictions this places on the user and driver should be specified.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.