Re: [PATCH] pktgen: Clone skb to avoid corruption of skbs in ndo_start_xmit methods

Neil Horman
2011-07-20T01:59:55Z

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)