Date:	Tue, 19 Jul 2011 21:59:41 -0400
From:	Neil Horman <nhorman@...driver.com>
To:	Eric Dumazet <eric.dumazet@...il.com>
Cc:	Jiri Pirko <jpirko@...hat.com>, netdev@...r.kernel.org,
	Alexey Dobriyan <adobriyan@...il.com>,
	"David S. Miller" <davem@...emloft.net>
Subject: Re: [PATCH] pktgen: Clone skb to avoid corruption of skbs in
 ndo_start_xmit methods

On Wed, Jul 20, 2011 at 02:43:12AM +0200, Eric Dumazet wrote:
> Le mardi 19 juillet 2011 à 20:19 -0400, Neil Horman a écrit :
> > > 
> > You are correct Eric, this can cause a significant performance regression, but I
> > think that beats causing a panic or other unexpected behavior.  I read your
> > previous threads with others regarding fixing this with vlans, but I don't think
> > its fair to just say 'its fast, but it might cause oopses'. 
> > 
> > And its not sufficient to simply forbid soft drivers to make use of pktgen, its
> > not just a soft driver problem, its systemic.  Any driver which assumes that it
> > has exclusive access to an skb submitted for transmit is at risk from pktgen in
> > its current implementation.  That of course as a subset includes all the soft
> > drivers, but others are also suceptible.  As examples (some of which I noted in
> > the origional post) virtio_net uses the skb->cb to hold vnet header information
> > which will be corrupted on sucessive sends.  bnx2x linearizes skbs under certain
> > circumstances, which means pktgen, if it marshals a fragmented frame will not
> > send a fragmented frame after the first iteration.  The PPP and Slip drivers
> > skb_push the skb to prepend a header to the frame on send, meaning sucessive
> > uses, up until they get an skb_under_panic will get iteratively more malformed
> > frames on the wire as ppp headers get stacked on top of one another.  These are
> > ust a few of the examples I've found.
> > 
> > The long and the short of it in my mind, is that we have a fundamental
> > disconnect between driver asumptions and pktgen.  If its ok to submit shared
> > skbs to drivers, then we need to augment drivers that modify skbs on transmit to
> > clone the skb (likey not an efficient solution), or if its not ok to do so, we
> > need to change pktgen to not behave that way.
> > 
> 
> Its a known problem, please check mail archives. Nobody felt a fix was
> needed.
> 
As I said in my origional note, I said I read the archives, I didn't agree with
the conclusion that a fix was unnecessecary.  I'm sorry if you don't care for
dissenting opinions.

> > > Note : a sysadmin has other ways to make a machine panic or reboot or
> > > halt...
> > Yes, predictable ways, that the sysadmin can see coming based on what they're
> > doing (i.e. no one should be shocked if they dd /dev/random to /dev/kmem and get
> > a hang or panic, or if they issue a sysrq-c, etc).  This case is different.  A
> > sysadmin reasonably expects pktgen to send the frames they configure on the
> > interface they specify.  While its arguably reasonable to forsee that it may not
> > work with soft interfaces, pktgen just won't work with some hardware drivers (as
> > per the examples above).  And it won't always be an oops, it may be occasional
> > random behvaior in the output data, and its highly dependent not just on the use
> > of pktgen, but rather the specific command(s) issued.
> > 
> > 
> > I'm sensitive to the performance impact, but I would much rather see a lower
> > performing pktgen that doesn't randomly crash, and bring the performance back up
> > in a safe, reliable way.  To that end, I've been starting to think about
> > pre-allocating a ring buffer of skbs with a skb->users count biased up to
> > prevent driver freeing.  That way we could detect 'unused skb's' by a user count
> > that was at the bias level.  Thoughts?
> > 
> 
> I dont know. I use pktgen maybe once per week and never got a single
> crash like this. We probably are very few pktgen users in the world, and
> we use it exactly to avoid calling skb_clone() or other expensive per
> xmit setup.
Please re-read my origional post.  Lots of drivers work just fine, some don't.
Some just behave differently.  Its the random results thats broken and I feel
needs fixing.  I get that performance is an issue, and I'm open to other
solutions, I'm not open to just saying 'its fine, mostly'.

> 
> Just remove pktgen from RedHat kernels, if you dont trust sysadmins.
> # CONFIG_PKTGEN is not set
> 
You're twisting my words.  At what point in time did I say I
don't trust sysadmins?  I want to give them a tool that works reliably without
them having to comb through their nic drivers xmit patch to ensure that pktgen
works without crashing or causing other odd behavior.  I don't think thats too
much to ask.
 
> Alternatively, add a check to problematic drivers to _not_ mess skb if
> skb_shared(skb) is true : eventually use skb_share_check()
> 
The former isn't feasible, as many skb modifications are neccesitated by the
nature of the hardware.  The latter is possible, but far less scalable than just
modifying pktgen.

FWIW, I like Ben's solution, adding a flag to drivers noting that they can't
handle multi-skb.  Then we can dynamically enforce a clone in pktgen when needed
(or buffer up additional skbs)
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists