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

[Ben Hutchings <bhutchings@solarflare.com>]

On Mon, 2011-08-22 at 14:14 -0400, Neil Horman wrote:
> On Mon, Aug 22, 2011 at 05:17:37PM +0100, Ben Hutchings wrote:
> > 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.
> > 
> 
> The flag means safe to send shared skbs.
> 
> Actually looking closer at this, I don't think this is much of a problem at all.
> The flag is cleared on devices for which it is unsafe to send shared skbs, not
> because there are multiple users of the skb in parallel, but because the driver
> expects to have continued exclusive access to the skb after the drivers
> ndo_start_xmit method returns.
> 
> In the pktgen case, skbs have skb->users > 1, but all users exist in the same
> execution context, making their transmission serialized.
[...]

So it is not *generally* safe to send shared skbs to drivers that make
idempotent changes to the skb.  There is a restriction and while pktgen
operates within it today I don't want it to be an unwritten assumption.

> So even though drivers that call skb_pad[to] modify the skb, its perfectly ok to
> do so, as long as they don't assume that the struct skb will remain in that
> state after the driver is done with it.
> 
> This works out perfectly well for skb_padto calls, since the function reduces to
> a no-op after the minimal tail size has been reached.
[...]

Right, that's what I want to be specified.  Did you miss my own
follow-up?  I proposed this description for the interface flag:

        The ndo_start_xmit operation for this interface either does not
        modify the given skb or modifies it idempotently. A single skb
        may be transmitted repeatedly on a single queue of this
        interface, but not on multiple queues or on multiple interfaces.

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.