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

[Ben Hutchings <bhutchings@solarflare.com>]

On Tue, 2011-08-23 at 11:13 -0400, Neil Horman wrote:
> On Tue, Aug 23, 2011 at 03:01:24PM +0100, Ben Hutchings wrote:
[...]
> > 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.
> > 
> No, I read it, I just don't agree with it. :).  Specifically I disagree with the
> langauge indicating that you cannot transmit a shared skb on multiple queues or
> on multiple interfaces.  You can in fact do that sanely with shared skbs,
> because to do so you are required to serialize their transmission anyway.  By
> definition they're shared, and you can't send them to multiple devices without
> modifing data in the skb that may be read in parallel in an alternate execution
> context.
>
> In short, what I'm saying is that there is no way to safely send a shared skb in
> parallel to multiple queues/interfaces without introducing other bugs orthogonal
> to the one prevented by the flag I added.  The only thing the flag indicates is
> that the driver can't handle non-idempotent changes to skbs (like being added to
> an sk_buff_head list)

In fact the caller must commit to a particular queue by setting skb->dev
and skb->queue_mapping.  So I really was talking nonsense.

> I think if you really want to clarify the meaning of the flag, I would add
> language to it like:
> 	The ndo_start_xmit operation either makes no changes to the skb data,
> 	or makes only idempotent changes, and does not expect any changes to 
> 	persist after the return from nod_start_xmit
>
> Its really the expectation of persistence that we need to worry about here.  If
> a driver adds an skb to a list for deferred transmission, for example, it
> assumes that it owns the skb, and that its state will remain unchanged after the
> return from ndo_start_xmit, but in the shared case thats not a safe assumption
> to make because in the shared case teh network stack is once again free to
> modify the skb.

The skb data (including padding) needs to persist until DMA is complete,
even if the driver ignores the actual struct sk_buff from then on.  And
pktgen certainly doesn't want to modify it.

> If you're ok with my language, I'll put a patch together for that.

I don't think we're quite there yet.

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.