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

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

[Neil Horman]

This oops was reported recently when running a pktgen script that called for a
transmitted skb to be cloned and sent 100 times using the clone_skb command to a
bridge device with several attached interface:

BUG: unable to handle kernel paging request at ffff880136994528
RIP: 0010:[<ffff880136994528>]  [<ffff880136994528>] 0xffff880136994528
RSP: 0018:ffff8801384e5b78  EFLAGS: 00010286
RAX: ffff880136994528 RBX: ffff880137e52800 RCX: 0000000000000000
RDX: ffffffffa0529ba0 RSI: ffff880137e52800 RDI: ffff8801369944b0
RBP: ffff8801384e5ba0 R08: ffff8801379cb49c R09: ffff8801384e5c78
R10: 0000000000000000 R11: 0000000000000400 R12: ffff880137e52ec0
R13: ffff8801369944b0 R14: ffff880136ed9480 R15: ffff880137e52800
FS:  0000000000000000(0000) GS:ffff880028200000(0000) knlGS:0000000000000000
CS:  0010 DS: 0018 ES: 0018 CR0: 000000008005003b
CR2: ffff880136994528 CR3: 00000001395f2000 CR4: 00000000000026e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
Process kpktgend_0 (pid: 2601, threadinfo ffff8801384e4000, task
ffff880137fe5560)
Stack:
 ffffffffa0527425 0000000000000000 ffff8801369944b0 ffff880137e52800
<0> ffff880136ed9480 ffff8801384e5bf0 ffffffff8141047a ffffffffa0529ba0
<0> ffff880134813e40 0000000080000000 ffff8801379cb400 ffff880137e52800
Call Trace:
 [<ffffffffa0527425>] ? tun_net_xmit+0x135/0x200 [tun]
 [<ffffffff8141047a>] dev_hard_start_xmit+0x20a/0x370
 [<ffffffff814288aa>] sch_direct_xmit+0x15a/0x1c0
 [<ffffffff81413ab8>] dev_queue_xmit+0x378/0x4a0
 [<ffffffffa0488400>] ? br_dev_queue_push_xmit+0x0/0xa0 [bridge]
 [<ffffffffa048846c>] br_dev_queue_push_xmit+0x6c/0xa0 [bridge]
 [<ffffffffa04884f8>] br_forward_finish+0x58/0x60 [bridge]
 [<ffffffffa0488690>] __br_deliver+0x60/0x70 [bridge]
 [<ffffffffa04883aa>] br_flood+0xca/0xe0 [bridge]
 [<ffffffffa0488630>] ? __br_deliver+0x0/0x70 [bridge]
 [<ffffffffa04883f7>] br_flood_deliver+0x17/0x20 [bridge]
 [<ffffffffa048748b>] br_dev_xmit+0xfb/0x100 [bridge]
 [<ffffffffa053790e>] pktgen_thread_worker+0x83e/0x1bc8 [pktgen]
 [<ffffffff81059d12>] ? finish_task_switch+0x42/0xd0
 [<ffffffffa0487390>] ? br_dev_xmit+0x0/0x100 [bridge]
 [<ffffffff81091ca0>] ? autoremove_wake_function+0x0/0x40
 [<ffffffff81091ca0>] ? autoremove_wake_function+0x0/0x40
 [<ffffffffa05370d0>] ? pktgen_thread_worker+0x0/0x1bc8 [pktgen]
 [<ffffffff81091936>] kthread+0x96/0xa0
 [<ffffffff810141ca>] child_rip+0xa/0x20
 [<ffffffff810918a0>] ? kthread+0x0/0xa0
 [<ffffffff810141c0>] ? child_rip+0x0/0x20

Turns out the pktgen driver doesn't actually clone skbs, but rather shares them,
increasing the reference count of the skb for each send operation.  This works
for most drivers because most drivers don't store or care about any state in the
skb itself, but several do.  For instance, the above tun/tap driver and other
soft drivers (vlans, bonding/bridging), all requeue frames to a physical device,
meaning the skb next and prev pointers will be set.  Other drivers also care
about skb state.  The virtio_net driver for instance uses the skb->cb space to
store a vnet header and several converged adapters adjust the data pointer of an
skb to prepend a device control header to the skb.  Drivers expect skbs
submitted for i/o to be in their control and unshared with other users, an
assumption which pktgen is violating, the result being multiple skb users
corrupting one antohers state and producing oopses like the one above.  The
solution is to make pktgen clone the skb for each transmit so as to ensure the
drivers assumptions about private exclusive access to the skb is maintained.

Tested successfully by myself
Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
Reported-by: Jiri Pirko <jpirko@redhat.com>
CC: Eric Dumazet <eric.dumazet@gmail.com>
CC: Alexey Dobriyan <adobriyan@gmail.com>
CC: David S. Miller <davem@davemloft.net>
---
 net/core/pktgen.c |   21 +++++++++++++--------
 1 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/net/core/pktgen.c b/net/core/pktgen.c
index f76079c..c8e0e08 100644
--- a/net/core/pktgen.c
+++ b/net/core/pktgen.c
@@ -3297,6 +3297,7 @@ static void pktgen_xmit(struct pktgen_dev *pkt_dev)
 	netdev_tx_t (*xmit)(struct sk_buff *, struct net_device *)
 		= odev->netdev_ops->ndo_start_xmit;
 	struct netdev_queue *txq;
+	struct sk_buff *tx_skb = NULL;
 	u16 queue_map;
 	int ret;
 
@@ -3316,9 +3317,7 @@ static void pktgen_xmit(struct pktgen_dev *pkt_dev)
 
 	/* If no skb or clone count exhausted then get new one */
 	if (!pkt_dev->skb || (pkt_dev->last_ok &&
-			      ++pkt_dev->clone_count >= pkt_dev->clone_skb)) {
-		/* build a new pkt */
-		kfree_skb(pkt_dev->skb);
+			      ++pkt_dev->clone_count > pkt_dev->clone_skb)) {
 
 		pkt_dev->skb = fill_packet(odev, pkt_dev);
 		if (pkt_dev->skb == NULL) {
@@ -3332,10 +3331,13 @@ static void pktgen_xmit(struct pktgen_dev *pkt_dev)
 		pkt_dev->clone_count = 0;	/* reset counter */
 	}
 
+	tx_skb = (pkt_dev->clone_count == pkt_dev->clone_skb) ?
+		 pkt_dev->skb : skb_clone(pkt_dev->skb, GFP_KERNEL);
+
 	if (pkt_dev->delay && pkt_dev->last_ok)
 		spin(pkt_dev, pkt_dev->next_tx);
 
-	queue_map = skb_get_queue_mapping(pkt_dev->skb);
+	queue_map = skb_get_queue_mapping(tx_skb);
 	txq = netdev_get_tx_queue(odev, queue_map);
 
 	__netif_tx_lock_bh(txq);
@@ -3345,8 +3347,8 @@ static void pktgen_xmit(struct pktgen_dev *pkt_dev)
 		pkt_dev->last_ok = 0;
 		goto unlock;
 	}
-	atomic_inc(&(pkt_dev->skb->users));
-	ret = (*xmit)(pkt_dev->skb, odev);
+
+	ret = (*xmit)(tx_skb, odev);
 
 	switch (ret) {
 	case NETDEV_TX_OK:
@@ -3369,8 +3371,11 @@ static void pktgen_xmit(struct pktgen_dev *pkt_dev)
 		/* fallthru */
 	case NETDEV_TX_LOCKED:
 	case NETDEV_TX_BUSY:
-		/* Retry it next time */
-		atomic_dec(&(pkt_dev->skb->users));
+		/*
+		 * Only free it if its not the last in the clone series
+		 */
+		if (tx_skb != pkt_dev->skb)
+			kfree_skb(tx_skb);
 		pkt_dev->last_ok = 0;
 	}
 unlock:
-- 
1.7.6

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

[Eric Dumazet]

Le mardi 19 juillet 2011 à 15:52 -0400, Neil Horman a écrit :
> This oops was reported recently when running a pktgen script that called for a
> transmitted skb to be cloned and sent 100 times using the clone_skb command to a
> bridge device with several attached interface:
> 
...

> Turns out the pktgen driver doesn't actually clone skbs, but rather shares them,
> increasing the reference count of the skb for each send operation.  This works
> for most drivers because most drivers don't store or care about any state in the
> skb itself, but several do.  For instance, the above tun/tap driver and other
> soft drivers (vlans, bonding/bridging), all requeue frames to a physical device,
> meaning the skb next and prev pointers will be set.  Other drivers also care
> about skb state.  The virtio_net driver for instance uses the skb->cb space to
> store a vnet header and several converged adapters adjust the data pointer of an
> skb to prepend a device control header to the skb.  Drivers expect skbs
> submitted for i/o to be in their control and unshared with other users, an
> assumption which pktgen is violating, the result being multiple skb users
> corrupting one antohers state and producing oopses like the one above.  The
> solution is to make pktgen clone the skb for each transmit so as to ensure the
> drivers assumptions about private exclusive access to the skb is maintained.
> 
> Tested successfully by myself
> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> Reported-by: Jiri Pirko <jpirko@redhat.com>
> CC: Eric Dumazet <eric.dumazet@gmail.com>
> CC: Alexey Dobriyan <adobriyan@gmail.com>
> CC: David S. Miller <davem@davemloft.net>
> ---

This will kill pktgen performance ?

pktgen is only for sysadmins, and very skilled ones :)

BTW you forgot to CC pktgen author, Robert Olsson

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

[Joe Perches]

On Tue, 2011-07-19 at 22:02 +0200, Eric Dumazet wrote:
> Le mardi 19 juillet 2011 à 15:52 -0400, Neil Horman a écrit :
> > This oops was reported recently when running a pktgen script that called for a
> > transmitted skb to be cloned and sent 100 times using the clone_skb command to a
> > bridge device with several attached interface:
> BTW you forgot to CC pktgen author, Robert Olsson

Robert, you're not getting cc's on pktgen when
people use get_maintainer.

Are you still interested in getting cc'd on patches
to pktgen?

If so, perhaps you want to add a MAINTAINERS entry
for it.

Maybe something like:

diff --git a/MAINTAINERS b/MAINTAINERS
index 378fccf..5a6c16a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4999,6 +4999,11 @@ S:	Maintained
 F:	drivers/block/pktcdvd.c
 F:	include/linux/pktcdvd.h
 
+PKTGEN
+M:	Robert Olsson <robert.olsson@its.uu.se>
+S:	Odd Fixes
+F:	net/core/pktgen.c
+
 PKUNITY SOC DRIVERS
 M:	Guan Xuetao <gxt@mprc.pku.edu.cn>
 W:	http://mprc.pku.edu.cn/~guanxuetao/linux

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

[Jiri Pirko]

Tue, Jul 19, 2011 at 10:02:18PM CEST, eric.dumazet@gmail.com wrote:
>Le mardi 19 juillet 2011 à 15:52 -0400, Neil Horman a écrit :
>> This oops was reported recently when running a pktgen script that called for a
>> transmitted skb to be cloned and sent 100 times using the clone_skb command to a
>> bridge device with several attached interface:
>> 
>...
>
>> Turns out the pktgen driver doesn't actually clone skbs, but rather shares them,
>> increasing the reference count of the skb for each send operation.  This works
>> for most drivers because most drivers don't store or care about any state in the
>> skb itself, but several do.  For instance, the above tun/tap driver and other
>> soft drivers (vlans, bonding/bridging), all requeue frames to a physical device,
>> meaning the skb next and prev pointers will be set.  Other drivers also care
>> about skb state.  The virtio_net driver for instance uses the skb->cb space to
>> store a vnet header and several converged adapters adjust the data pointer of an
>> skb to prepend a device control header to the skb.  Drivers expect skbs
>> submitted for i/o to be in their control and unshared with other users, an
>> assumption which pktgen is violating, the result being multiple skb users
>> corrupting one antohers state and producing oopses like the one above.  The
>> solution is to make pktgen clone the skb for each transmit so as to ensure the
>> drivers assumptions about private exclusive access to the skb is maintained.
>> 
>> Tested successfully by myself
>> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
>> Reported-by: Jiri Pirko <jpirko@redhat.com>
>> CC: Eric Dumazet <eric.dumazet@gmail.com>
>> CC: Alexey Dobriyan <adobriyan@gmail.com>
>> CC: David S. Miller <davem@davemloft.net>
>> ---
>
>This will kill pktgen performance ?
>
>pktgen is only for sysadmins, and very skilled ones :)

You are right, but it may not cause panic, right? In case this patch
would cause significant performance regression, how about to just forbid
pktgen to run on soft-netdevs ?

Jirka

>
>BTW you forgot to CC pktgen author, Robert Olsson
>
>
>
>--
>To unsubscribe from this list: send the line "unsubscribe netdev" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Eric Dumazet]

Le mardi 19 juillet 2011 à 22:29 +0200, Jiri Pirko a écrit :

> You are right, but it may not cause panic, right? In case this patch
> would cause significant performance regression, how about to just forbid
> pktgen to run on soft-netdevs ?
> 

Please do

Note : a sysadmin has other ways to make a machine panic or reboot or
halt...

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

[Ben Greear]

On 07/19/2011 01:41 PM, Eric Dumazet wrote:
> Le mardi 19 juillet 2011 à 22:29 +0200, Jiri Pirko a écrit :
>
>> You are right, but it may not cause panic, right? In case this patch
>> would cause significant performance regression, how about to just forbid
>> pktgen to run on soft-netdevs ?
>>
>
> Please do

No, please do not.  Pktgen *can* work on soft-devs if you do not
use multi-skb, and it can be useful for testing highs speed vlan
traffic and various other things.

If you must, forbid multi-skb on soft devices, but don't just
break pktgen for all soft-devices.

Thanks,
Ben

>
> Note : a sysadmin has other ways to make a machine panic or reboot or
> halt...
>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

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

[Neil Horman]

On Tue, Jul 19, 2011 at 10:41:47PM +0200, Eric Dumazet wrote:
> Le mardi 19 juillet 2011 à 22:29 +0200, Jiri Pirko a écrit :
> 
> > You are right, but it may not cause panic, right? In case this patch
> > would cause significant performance regression, how about to just forbid
> > pktgen to run on soft-netdevs ?
> > 
> 
> Please do
> 
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.

> 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?

Neil

> 
> 
> 
> 

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

[Rick Jones]

How "everyday use" is pktgen considered?  As "everyday use" as 
netperf/iperf/whatnot? Is it actually considered something one would run 
on production rather than test systems?

rick jones

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

[Eric Dumazet]

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.

> > 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.

Just remove pktgen from RedHat kernels, if you dont trust sysadmins.
# CONFIG_PKTGEN is not set

Alternatively, add a check to problematic drivers to _not_ mess skb if
skb_shared(skb) is true : eventually use skb_share_check()

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

[Ben Greear]

On 07/19/2011 05:43 PM, Eric Dumazet wrote:
> Le mardi 19 juillet 2011 à 20:19 -0400, Neil Horman a écrit :

>> 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.
>
> Just remove pktgen from RedHat kernels, if you dont trust sysadmins.
> # CONFIG_PKTGEN is not set
>
> Alternatively, add a check to problematic drivers to _not_ mess skb if
> skb_shared(skb) is true : eventually use skb_share_check()

When the features-flags work gets completed so that we can start adding
new flags, we could add a CANT_DO_MULTI_SKB flag to drivers with known
issues and then restrict pktgen config accordingly.

Upstream code already clears skb memory to avoid leaking kernel memory
contents, so if you take away multi-skb too, pktgen is going to suck
at what it is supposed to do:  run fast as possible.

If you want real fun, use pktgen on a wlan0 device...it will crash
regardless of whether you use multi-skb or not because of xmit-queue
number issues :P

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

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

[Neil Horman]

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)

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

[Neil Horman]

On Tue, Jul 19, 2011 at 05:52:49PM -0700, Ben Greear wrote:
> On 07/19/2011 05:43 PM, Eric Dumazet wrote:
> >Le mardi 19 juillet 2011 à 20:19 -0400, Neil Horman a écrit :
> 
> >>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.
> >
> >Just remove pktgen from RedHat kernels, if you dont trust sysadmins.
> ># CONFIG_PKTGEN is not set
> >
> >Alternatively, add a check to problematic drivers to _not_ mess skb if
> >skb_shared(skb) is true : eventually use skb_share_check()
> 
> When the features-flags work gets completed so that we can start adding
> new flags, we could add a CANT_DO_MULTI_SKB flag to drivers with known
> issues and then restrict pktgen config accordingly.
> 
I think this is a good idea.  It lets pktgen dynamically make the clone/share
decision dynamically and only impacts performance for those systems.

> Upstream code already clears skb memory to avoid leaking kernel memory
> contents, so if you take away multi-skb too, pktgen is going to suck
> at what it is supposed to do:  run fast as possible.
> 
I don't want to take away multi-skb, but I do want pktgen to work reliably.  I
think flagging drivers that need unshared skbs is the way to go.

> If you want real fun, use pktgen on a wlan0 device...it will crash
> regardless of whether you use multi-skb or not because of xmit-queue
> number issues :P
> 
I'll try that, thanks :)

Regards
Neil

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

[Ben Greear]

On 07/19/2011 07:07 PM, Neil Horman wrote:
> On Tue, Jul 19, 2011 at 05:52:49PM -0700, Ben Greear wrote:
>> On 07/19/2011 05:43 PM, Eric Dumazet wrote:
>>> Le mardi 19 juillet 2011 à 20:19 -0400, Neil Horman a écrit :

>> When the features-flags work gets completed so that we can start adding
>> new flags, we could add a CANT_DO_MULTI_SKB flag to drivers with known
>> issues and then restrict pktgen config accordingly.
>>
> I think this is a good idea.  It lets pktgen dynamically make the clone/share
> decision dynamically and only impacts performance for those systems.
>
>> Upstream code already clears skb memory to avoid leaking kernel memory
>> contents, so if you take away multi-skb too, pktgen is going to suck
>> at what it is supposed to do:  run fast as possible.
>>
> I don't want to take away multi-skb, but I do want pktgen to work reliably.  I
> think flagging drivers that need unshared skbs is the way to go.

Lets all cheer on the Intel NIC driver guys and Mr. Miraslaw then!

>> If you want real fun, use pktgen on a wlan0 device...it will crash
>> regardless of whether you use multi-skb or not because of xmit-queue
>> number issues :P
>>
> I'll try that, thanks :)

Here's a probably-white-space damaged, and deemed-unfit-for-kernel patch
that at least works around the problem...my users don't really need pktgen
on wlans, so I didn't put any extra effort into it, but for someone
more motivated..this might be a good starting point :)

I guess it doesn't really crash, but if I recall correctly, it filled
up the queues in a bad way and effectively blocked all wifi traffic
until reboot, or maybe rmmod ath9k.


 From d57130f29843cab30196b11d4476231f245e3f92 Mon Sep 17 00:00:00 2001
From: Ben Greear <greearb@candelatech.com>
Date: Wed, 9 Feb 2011 16:58:42 -0800
Subject: [PATCH 31/38] mac80211: Set up tx-queue-mapping in subif_start_xmit.

Otherwise, ath9k gets confused about which queue to use
and spews a warning like this when driving traffic with
pktgen.

WARNING: at /home/greearb/git/linux.wireless-testing-ct/drivers/net/wireless/ath/ath9k/xmit.c:1748 ath_tx_start+0x4a2/0x662 [ath9k]()
Hardware name: To Be Filled By O.E.M.
Modules linked in: ath5k arc4 ath9k mac80211 ath9k_common ath9k_hw ath cfg80211 nfs lockd bluetooth cryptd aes_i586 aes_generic veth 8021q garp stp l]
Pid: 1729, comm: kpktgend_0 Tainted: G        W   2.6.38-rc4-wl+ #21
Call Trace:
  [<c043091b>] ? warn_slowpath_common+0x65/0x7a
  [<fabe784e>] ? ath_tx_start+0x4a2/0x662 [ath9k]
  [<c043093f>] ? warn_slowpath_null+0xf/0x13
  [<fabe784e>] ? ath_tx_start+0x4a2/0x662 [ath9k]
  [<fabe14d0>] ? ath9k_tx+0x14f/0x183 [ath9k]
  [<fab9026d>] ? __ieee80211_tx+0x10c/0x18c [mac80211]
  [<fab90397>] ? ieee80211_tx+0xaa/0x188 [mac80211]
  [<fab905f3>] ? ieee80211_xmit+0x17e/0x186 [mac80211]
  [<fab8ecc0>] ? ieee80211_skb_resize+0x8e/0xd2 [mac80211]
  [<fab9148b>] ? ieee80211_subif_start_xmit+0x643/0x65c [mac80211]
  [<c0440000>] ? rescuer_thread+0x25/0x1c8
  [<f92cd354>] ? pktgen_thread_worker+0x114c/0x1b44 [pktgen]
  [<fab90e48>] ? ieee80211_subif_start_xmit+0x0/0x65c [mac80211]
  [<c042d612>] ? default_wake_function+0xb/0xd
  [<c04254c7>] ? __wake_up_common+0x34/0x5c
  [<c0443a29>] ? autoremove_wake_function+0x0/0x2f
  [<f92cc208>] ? pktgen_thread_worker+0x0/0x1b44 [pktgen]
  [<c044371a>] ? kthread+0x62/0x67
  [<c04436b8>] ? kthread+0x0/0x67
  [<c04035f6>] ? kernel_thread_helper+0x6/0x10

Signed-off-by: Ben Greear <greearb@candelatech.com>
---
:100644 100644 64e0f75... e8ff199... M	net/mac80211/tx.c
  net/mac80211/tx.c |    2 ++
  1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 64e0f75..e8ff199 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -1971,6 +1971,8 @@ netdev_tx_t ieee80211_subif_start_xmit(struct sk_buff *skb,
  	} else
  		memcpy(skb_push(skb, hdrlen), &hdr, hdrlen);

+	skb_set_queue_mapping(skb, ieee80211_select_queue(sdata, skb));
+
  	nh_pos += hdrlen;
  	h_pos += hdrlen;

-- 
1.7.3.4


>
> Regards
> Neil


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

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

[Eric Dumazet]

Le mardi 19 juillet 2011 à 22:07 -0400, Neil Horman a écrit :
> > 
> I think this is a good idea.  It lets pktgen dynamically make the clone/share
> decision dynamically and only impacts performance for those systems.
> 

Just let pktgen refuse to use clone_skb command for these devices.

At that point, an userland application is going to be faster and more
flexible than pktgen.

So when I said "remove pktgen" from distro it was not a joke.
Maybe its time to admit pktgen has to be removed from kernel sources.

$ wc net/core/pktgen.c
 3788 10881 92771 net/core/pktgen.c

Hmmm, 3788 lines, patched 180 times for a thing that only sends UDP
frames...

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

[Loke, Chetan]

> So when I said "remove pktgen" from distro it was not a joke.
> Maybe its time to admit pktgen has to be removed from kernel sources.
> 

Please, let's not remove pktgen from kernel sources. We can add a CONFIG_PKTGEN as someone mentioned in the email thread.


> $ wc net/core/pktgen.c
>  3788 10881 92771 net/core/pktgen.c
> 
> Hmmm, 3788 lines, patched 180 times for a thing that only sends UDP
> frames...

It really doesn't matter what traffic it sends. As long as I can get some high speed throughput I am good.
There are quite a few use-cases where one doesn't really need to know the traffic profile.

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

[Neil Horman]

On Wed, Jul 20, 2011 at 06:24:15AM +0200, Eric Dumazet wrote:
> Le mardi 19 juillet 2011 à 22:07 -0400, Neil Horman a écrit :
> > > 
> > I think this is a good idea.  It lets pktgen dynamically make the clone/share
> > decision dynamically and only impacts performance for those systems.
> > 
> 
> Just let pktgen refuse to use clone_skb command for these devices.
> 
copy that, This is by no means final, but what do you think of this?  If its
agreeable to you, Ben, et al. I can add this to my local tree and start auditing
all the drivers that may need to have the flag set.

Regards
Neil


diff --git a/include/linux/if.h b/include/linux/if.h
index 3bc63e6..ae904fe 100644
--- a/include/linux/if.h
+++ b/include/linux/if.h
@@ -76,6 +76,7 @@
 #define IFF_BRIDGE_PORT	0x4000		/* device used as bridge port */
 #define IFF_OVS_DATAPATH	0x8000	/* device used as Open vSwitch
 					 * datapath port */
+#define IFF_CANT_SHARE_SKB	0x10000	/* Device can't share skbs in tx path */
 
 #define IF_GET_IFACE	0x0001		/* for querying only */
 #define IF_GET_PROTO	0x0002
diff --git a/net/core/pktgen.c b/net/core/pktgen.c
index f76079c..bf6d88d 100644
--- a/net/core/pktgen.c
+++ b/net/core/pktgen.c
@@ -1071,6 +1071,9 @@ static ssize_t pktgen_if_write(struct file *file,
 		if (len < 0)
 			return len;
 
+		if (pkt_dev->priv_flags & IFF_CANT_SHARE_SKB)
+			return -EOPNOTSUPP;
+
 		i += len;
 		pkt_dev->clone_skb = value;
 

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

[Loke, Chetan]

> -----Original Message-----
> From: netdev-owner@vger.kernel.org [mailto:netdev-
> owner@vger.kernel.org] On Behalf Of Rick Jones
> Sent: July 19, 2011 8:25 PM
> To: Neil Horman
> Cc: Eric Dumazet; Jiri Pirko; netdev@vger.kernel.org; Alexey Dobriyan;
> David S. Miller
> Subject: Re: [PATCH] pktgen: Clone skb to avoid corruption of skbs in
> ndo_start_xmit methods
> 
> How "everyday use" is pktgen considered?  As "everyday use" as
> netperf/iperf/whatnot? Is it actually considered something one would
> run
> on production rather than test systems?
> 

Totally agree. There are quite a few ways to send traffic from
user-space. Build a pcap file and then blast it from user-space. No sane
sysadmin would use pktgen on production systems to begin with.

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

[Eric Dumazet]

Le mercredi 20 juillet 2011 à 11:18 -0400, Neil Horman a écrit :
> On Wed, Jul 20, 2011 at 06:24:15AM +0200, Eric Dumazet wrote:
> > Le mardi 19 juillet 2011 à 22:07 -0400, Neil Horman a écrit :
> > > > 
> > > I think this is a good idea.  It lets pktgen dynamically make the clone/share
> > > decision dynamically and only impacts performance for those systems.
> > > 
> > 
> > Just let pktgen refuse to use clone_skb command for these devices.
> > 
> copy that, This is by no means final, but what do you think of this?  If its
> agreeable to you, Ben, et al. I can add this to my local tree and start auditing
> all the drivers that may need to have the flag set.
> 
> Regards
> Neil
> 
> 
> diff --git a/include/linux/if.h b/include/linux/if.h
> index 3bc63e6..ae904fe 100644
> --- a/include/linux/if.h
> +++ b/include/linux/if.h
> @@ -76,6 +76,7 @@
>  #define IFF_BRIDGE_PORT	0x4000		/* device used as bridge port */
>  #define IFF_OVS_DATAPATH	0x8000	/* device used as Open vSwitch
>  					 * datapath port */
> +#define IFF_CANT_SHARE_SKB	0x10000	/* Device can't share skbs in tx path */
>  
>  #define IF_GET_IFACE	0x0001		/* for querying only */
>  #define IF_GET_PROTO	0x0002
> diff --git a/net/core/pktgen.c b/net/core/pktgen.c
> index f76079c..bf6d88d 100644
> --- a/net/core/pktgen.c
> +++ b/net/core/pktgen.c
> @@ -1071,6 +1071,9 @@ static ssize_t pktgen_if_write(struct file *file,
>  		if (len < 0)
>  			return len;
>  
> +		if (pkt_dev->priv_flags & IFF_CANT_SHARE_SKB)
> +			return -EOPNOTSUPP;
> +

Well, the general idea was to intercept the "clone_skb XXX" command and
cap XXX to 0 for said devices.

So some admin can still use pktgen without clone_skb stuff.

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

[Michał Mirosław]

2011/7/20 Neil Horman <nhorman@tuxdriver.com>:
> On Wed, Jul 20, 2011 at 06:24:15AM +0200, Eric Dumazet wrote:
>> Le mardi 19 juillet 2011 à 22:07 -0400, Neil Horman a écrit :
>> > >
>> > I think this is a good idea.  It lets pktgen dynamically make the clone/share
>> > decision dynamically and only impacts performance for those systems.
>> >
>>
>> Just let pktgen refuse to use clone_skb command for these devices.
>>
> copy that, This is by no means final, but what do you think of this?  If its
> agreeable to you, Ben, et al. I can add this to my local tree and start auditing
> all the drivers that may need to have the flag set.
>
> Regards
> Neil
>
>
> diff --git a/include/linux/if.h b/include/linux/if.h
> index 3bc63e6..ae904fe 100644
> --- a/include/linux/if.h
> +++ b/include/linux/if.h
> @@ -76,6 +76,7 @@
>  #define IFF_BRIDGE_PORT        0x4000          /* device used as bridge port */
>  #define IFF_OVS_DATAPATH       0x8000  /* device used as Open vSwitch
>                                         * datapath port */
> +#define IFF_CANT_SHARE_SKB     0x10000 /* Device can't share skbs in tx path */
>
>  #define IF_GET_IFACE   0x0001          /* for querying only */
>  #define IF_GET_PROTO   0x0002
> diff --git a/net/core/pktgen.c b/net/core/pktgen.c
> index f76079c..bf6d88d 100644
> --- a/net/core/pktgen.c
> +++ b/net/core/pktgen.c
> @@ -1071,6 +1071,9 @@ static ssize_t pktgen_if_write(struct file *file,
>                if (len < 0)
>                        return len;
>
> +               if (pkt_dev->priv_flags & IFF_CANT_SHARE_SKB)
> +                       return -EOPNOTSUPP;
> +
>                i += len;
>                pkt_dev->clone_skb = value;
>

I would prefer that the flag be inclusive, i.e. it should mark drivers
which can use shared skbs. And it might be better to clone the skb in
case the flag is disabled to keep functionality working.

Best Regards,
Michał Mirosław

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

[Neil Horman]

On Wed, Jul 20, 2011 at 05:30:17PM +0200, Eric Dumazet wrote:
> Le mercredi 20 juillet 2011 à 11:18 -0400, Neil Horman a écrit :
> > On Wed, Jul 20, 2011 at 06:24:15AM +0200, Eric Dumazet wrote:
> > > Le mardi 19 juillet 2011 à 22:07 -0400, Neil Horman a écrit :
> > > > > 
> > > > I think this is a good idea.  It lets pktgen dynamically make the clone/share
> > > > decision dynamically and only impacts performance for those systems.
> > > > 
> > > 
> > > Just let pktgen refuse to use clone_skb command for these devices.
> > > 
> > copy that, This is by no means final, but what do you think of this?  If its
> > agreeable to you, Ben, et al. I can add this to my local tree and start auditing
> > all the drivers that may need to have the flag set.
> > 
> > Regards
> > Neil
> > 
> > 
> > diff --git a/include/linux/if.h b/include/linux/if.h
> > index 3bc63e6..ae904fe 100644
> > --- a/include/linux/if.h
> > +++ b/include/linux/if.h
> > @@ -76,6 +76,7 @@
> >  #define IFF_BRIDGE_PORT	0x4000		/* device used as bridge port */
> >  #define IFF_OVS_DATAPATH	0x8000	/* device used as Open vSwitch
> >  					 * datapath port */
> > +#define IFF_CANT_SHARE_SKB	0x10000	/* Device can't share skbs in tx path */
> >  
> >  #define IF_GET_IFACE	0x0001		/* for querying only */
> >  #define IF_GET_PROTO	0x0002
> > diff --git a/net/core/pktgen.c b/net/core/pktgen.c
> > index f76079c..bf6d88d 100644
> > --- a/net/core/pktgen.c
> > +++ b/net/core/pktgen.c
> > @@ -1071,6 +1071,9 @@ static ssize_t pktgen_if_write(struct file *file,
> >  		if (len < 0)
> >  			return len;
> >  
> > +		if (pkt_dev->priv_flags & IFF_CANT_SHARE_SKB)
> > +			return -EOPNOTSUPP;
> > +
> 
> Well, the general idea was to intercept the "clone_skb XXX" command and
> cap XXX to 0 for said devices.
> 
Isn't that exactly what this does?  Disallows a user from issuing the cone_skb
command in a pktgen script if the underlying driver can't support the sharing of
skbs.

> So some admin can still use pktgen without clone_skb stuff.
> 
Yes. this doesn't preclude that unless you see something I dont
Neil

> 
> 

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

[Neil Horman]

On Wed, Jul 20, 2011 at 05:35:47PM +0200, Michał Mirosław wrote:
> 2011/7/20 Neil Horman <nhorman@tuxdriver.com>:
> > On Wed, Jul 20, 2011 at 06:24:15AM +0200, Eric Dumazet wrote:
> >> Le mardi 19 juillet 2011 à 22:07 -0400, Neil Horman a écrit :
> >> > >
> >> > I think this is a good idea.  It lets pktgen dynamically make the clone/share
> >> > decision dynamically and only impacts performance for those systems.
> >> >
> >>
> >> Just let pktgen refuse to use clone_skb command for these devices.
> >>
> > copy that, This is by no means final, but what do you think of this?  If its
> > agreeable to you, Ben, et al. I can add this to my local tree and start auditing
> > all the drivers that may need to have the flag set.
> >
> > Regards
> > Neil
> >
> >
> > diff --git a/include/linux/if.h b/include/linux/if.h
> > index 3bc63e6..ae904fe 100644
> > --- a/include/linux/if.h
> > +++ b/include/linux/if.h
> > @@ -76,6 +76,7 @@
> >  #define IFF_BRIDGE_PORT        0x4000          /* device used as bridge port */
> >  #define IFF_OVS_DATAPATH       0x8000  /* device used as Open vSwitch
> >                                         * datapath port */
> > +#define IFF_CANT_SHARE_SKB     0x10000 /* Device can't share skbs in tx path */
> >
> >  #define IF_GET_IFACE   0x0001          /* for querying only */
> >  #define IF_GET_PROTO   0x0002
> > diff --git a/net/core/pktgen.c b/net/core/pktgen.c
> > index f76079c..bf6d88d 100644
> > --- a/net/core/pktgen.c
> > +++ b/net/core/pktgen.c
> > @@ -1071,6 +1071,9 @@ static ssize_t pktgen_if_write(struct file *file,
> >                if (len < 0)
> >                        return len;
> >
> > +               if (pkt_dev->priv_flags & IFF_CANT_SHARE_SKB)
> > +                       return -EOPNOTSUPP;
> > +
> >                i += len;
> >                pkt_dev->clone_skb = value;
> >
> 
> I would prefer that the flag be inclusive, i.e. it should mark drivers
> which can use shared skbs. And it might be better to clone the skb in
> case the flag is disabled to keep functionality working.
> 
Ok, I can agree with that.  But you're ok with the general approach?
Neil

> Best Regards,
> Michał Mirosław
> 

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

[Eric Dumazet]

Le mercredi 20 juillet 2011 à 11:39 -0400, Neil Horman a écrit :

> Isn't that exactly what this does?  Disallows a user from issuing the cone_skb
> command in a pktgen script if the underlying driver can't support the sharing of
> skbs.

My bad, I misread your patch. Seems fine to me ;)

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

[Michał Mirosław]

W dniu 20 lipca 2011 17:40 użytkownik Neil Horman
<nhorman@tuxdriver.com> napisał:
> On Wed, Jul 20, 2011 at 05:35:47PM +0200, Michał Mirosław wrote:
>> 2011/7/20 Neil Horman <nhorman@tuxdriver.com>:
>> > On Wed, Jul 20, 2011 at 06:24:15AM +0200, Eric Dumazet wrote:
>> >> Le mardi 19 juillet 2011 à 22:07 -0400, Neil Horman a écrit :
>> >> > >
>> >> > I think this is a good idea.  It lets pktgen dynamically make the clone/share
>> >> > decision dynamically and only impacts performance for those systems.
>> >> >
>> >>
>> >> Just let pktgen refuse to use clone_skb command for these devices.
>> >>
>> > copy that, This is by no means final, but what do you think of this?  If its
>> > agreeable to you, Ben, et al. I can add this to my local tree and start auditing
>> > all the drivers that may need to have the flag set.
>> >
>> > Regards
>> > Neil
>> >
>> >
>> > diff --git a/include/linux/if.h b/include/linux/if.h
>> > index 3bc63e6..ae904fe 100644
>> > --- a/include/linux/if.h
>> > +++ b/include/linux/if.h
>> > @@ -76,6 +76,7 @@
>> >  #define IFF_BRIDGE_PORT        0x4000          /* device used as bridge port */
>> >  #define IFF_OVS_DATAPATH       0x8000  /* device used as Open vSwitch
>> >                                         * datapath port */
>> > +#define IFF_CANT_SHARE_SKB     0x10000 /* Device can't share skbs in tx path */
>> >
>> >  #define IF_GET_IFACE   0x0001          /* for querying only */
>> >  #define IF_GET_PROTO   0x0002
>> > diff --git a/net/core/pktgen.c b/net/core/pktgen.c
>> > index f76079c..bf6d88d 100644
>> > --- a/net/core/pktgen.c
>> > +++ b/net/core/pktgen.c
>> > @@ -1071,6 +1071,9 @@ static ssize_t pktgen_if_write(struct file *file,
>> >                if (len < 0)
>> >                        return len;
>> >
>> > +               if (pkt_dev->priv_flags & IFF_CANT_SHARE_SKB)
>> > +                       return -EOPNOTSUPP;
>> > +
>> >                i += len;
>> >                pkt_dev->clone_skb = value;
>> >
>>
>> I would prefer that the flag be inclusive, i.e. it should mark drivers
>> which can use shared skbs. And it might be better to clone the skb in
>> case the flag is disabled to keep functionality working.
> Ok, I can agree with that.  But you're ok with the general approach?

I assumed you wanted to use netdev->features and I noticed priv_flags
just now. If the flag is supposed to be permanent then its fine by me.
Actually this makes me wonder if NETIF_F_LLTX and similar should get
moved to netdev->priv_flags.

Best Regards,
Michał Mirosław

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

[Neil Horman]

On Wed, Jul 20, 2011 at 06:08:39PM +0200, Michał Mirosław wrote:
> W dniu 20 lipca 2011 17:40 użytkownik Neil Horman
> <nhorman@tuxdriver.com> napisał:
> > On Wed, Jul 20, 2011 at 05:35:47PM +0200, Michał Mirosław wrote:
> >> 2011/7/20 Neil Horman <nhorman@tuxdriver.com>:
> >> > On Wed, Jul 20, 2011 at 06:24:15AM +0200, Eric Dumazet wrote:
> >> >> Le mardi 19 juillet 2011 à 22:07 -0400, Neil Horman a écrit :
> >> >> > >
> >> >> > I think this is a good idea.  It lets pktgen dynamically make the clone/share
> >> >> > decision dynamically and only impacts performance for those systems.
> >> >> >
> >> >>
> >> >> Just let pktgen refuse to use clone_skb command for these devices.
> >> >>
> >> > copy that, This is by no means final, but what do you think of this?  If its
> >> > agreeable to you, Ben, et al. I can add this to my local tree and start auditing
> >> > all the drivers that may need to have the flag set.
> >> >
> >> > Regards
> >> > Neil
> >> >
> >> >
> >> > diff --git a/include/linux/if.h b/include/linux/if.h
> >> > index 3bc63e6..ae904fe 100644
> >> > --- a/include/linux/if.h
> >> > +++ b/include/linux/if.h
> >> > @@ -76,6 +76,7 @@
> >> >  #define IFF_BRIDGE_PORT        0x4000          /* device used as bridge port */
> >> >  #define IFF_OVS_DATAPATH       0x8000  /* device used as Open vSwitch
> >> >                                         * datapath port */
> >> > +#define IFF_CANT_SHARE_SKB     0x10000 /* Device can't share skbs in tx path */
> >> >
> >> >  #define IF_GET_IFACE   0x0001          /* for querying only */
> >> >  #define IF_GET_PROTO   0x0002
> >> > diff --git a/net/core/pktgen.c b/net/core/pktgen.c
> >> > index f76079c..bf6d88d 100644
> >> > --- a/net/core/pktgen.c
> >> > +++ b/net/core/pktgen.c
> >> > @@ -1071,6 +1071,9 @@ static ssize_t pktgen_if_write(struct file *file,
> >> >                if (len < 0)
> >> >                        return len;
> >> >
> >> > +               if (pkt_dev->priv_flags & IFF_CANT_SHARE_SKB)
> >> > +                       return -EOPNOTSUPP;
> >> > +
> >> >                i += len;
> >> >                pkt_dev->clone_skb = value;
> >> >
> >>
> >> I would prefer that the flag be inclusive, i.e. it should mark drivers
> >> which can use shared skbs. And it might be better to clone the skb in
> >> case the flag is disabled to keep functionality working.
> > Ok, I can agree with that.  But you're ok with the general approach?
> 
> I assumed you wanted to use netdev->features and I noticed priv_flags
> just now. If the flag is supposed to be permanent then its fine by me.
> Actually this makes me wonder if NETIF_F_LLTX and similar should get
> moved to netdev->priv_flags.
> 
Yeah, I expect it will be both permanent and invisible to user space.
Neil

> Best Regards,
> Michał Mirosław
> 

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

[Neil Horman]

On Wed, Jul 20, 2011 at 05:44:48PM +0200, Eric Dumazet wrote:
> Le mercredi 20 juillet 2011 à 11:39 -0400, Neil Horman a écrit :
> 
> > Isn't that exactly what this does?  Disallows a user from issuing the cone_skb
> > command in a pktgen script if the underlying driver can't support the sharing of
> > skbs.
> 
> My bad, I misread your patch. Seems fine to me ;)
> 
Ok, thanks.  I'll put this in a local tree and stat auditing drivers to see who
needs it set/cleared.  I'll post a new series when I have it done.

Thanks!
Neil

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

[Ben Greear]

On 07/20/2011 08:18 AM, Neil Horman wrote:
> On Wed, Jul 20, 2011 at 06:24:15AM +0200, Eric Dumazet wrote:
>> Le mardi 19 juillet 2011 à 22:07 -0400, Neil Horman a écrit :
>>>>
>>> I think this is a good idea.  It lets pktgen dynamically make the clone/share
>>> decision dynamically and only impacts performance for those systems.
>>>
>>
>> Just let pktgen refuse to use clone_skb command for these devices.
>>
> copy that, This is by no means final, but what do you think of this?  If its
> agreeable to you, Ben, et al. I can add this to my local tree and start auditing
> all the drivers that may need to have the flag set.
>
> Regards
> Neil
>
>
> diff --git a/include/linux/if.h b/include/linux/if.h
> index 3bc63e6..ae904fe 100644
> --- a/include/linux/if.h
> +++ b/include/linux/if.h
> @@ -76,6 +76,7 @@
>   #define IFF_BRIDGE_PORT	0x4000		/* device used as bridge port */
>   #define IFF_OVS_DATAPATH	0x8000	/* device used as Open vSwitch
>   					 * datapath port */
> +#define IFF_CANT_SHARE_SKB	0x10000	/* Device can't share skbs in tx path */
>
>   #define IF_GET_IFACE	0x0001		/* for querying only */
>   #define IF_GET_PROTO	0x0002
> diff --git a/net/core/pktgen.c b/net/core/pktgen.c
> index f76079c..bf6d88d 100644
> --- a/net/core/pktgen.c
> +++ b/net/core/pktgen.c
> @@ -1071,6 +1071,9 @@ static ssize_t pktgen_if_write(struct file *file,
>   		if (len<  0)
>   			return len;
>
> +		if (pkt_dev->priv_flags&  IFF_CANT_SHARE_SKB)
> +			return -EOPNOTSUPP;
> +
>   		i += len;
>   		pkt_dev->clone_skb = value;
>

Please only return an error if value > 1.

Also, if there is any place where user can configure pkt_dev,
you would want code there to check for the CANT_SHARE_SKB flag
and force clone_skb to zero if user changes to a different
device that cannot share skbs.

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

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

[Neil Horman]

On Wed, Jul 20, 2011 at 09:33:20AM -0700, Ben Greear wrote:
> On 07/20/2011 08:18 AM, Neil Horman wrote:
> >On Wed, Jul 20, 2011 at 06:24:15AM +0200, Eric Dumazet wrote:
> >>Le mardi 19 juillet 2011 à 22:07 -0400, Neil Horman a écrit :
> >>>>
> >>>I think this is a good idea.  It lets pktgen dynamically make the clone/share
> >>>decision dynamically and only impacts performance for those systems.
> >>>
> >>
> >>Just let pktgen refuse to use clone_skb command for these devices.
> >>
> >copy that, This is by no means final, but what do you think of this?  If its
> >agreeable to you, Ben, et al. I can add this to my local tree and start auditing
> >all the drivers that may need to have the flag set.
> >
> >Regards
> >Neil
> >
> >
> >diff --git a/include/linux/if.h b/include/linux/if.h
> >index 3bc63e6..ae904fe 100644
> >--- a/include/linux/if.h
> >+++ b/include/linux/if.h
> >@@ -76,6 +76,7 @@
> >  #define IFF_BRIDGE_PORT	0x4000		/* device used as bridge port */
> >  #define IFF_OVS_DATAPATH	0x8000	/* device used as Open vSwitch
> >  					 * datapath port */
> >+#define IFF_CANT_SHARE_SKB	0x10000	/* Device can't share skbs in tx path */
> >
> >  #define IF_GET_IFACE	0x0001		/* for querying only */
> >  #define IF_GET_PROTO	0x0002
> >diff --git a/net/core/pktgen.c b/net/core/pktgen.c
> >index f76079c..bf6d88d 100644
> >--- a/net/core/pktgen.c
> >+++ b/net/core/pktgen.c
> >@@ -1071,6 +1071,9 @@ static ssize_t pktgen_if_write(struct file *file,
> >  		if (len<  0)
> >  			return len;
> >
> >+		if (pkt_dev->priv_flags&  IFF_CANT_SHARE_SKB)
> >+			return -EOPNOTSUPP;
> >+
> >  		i += len;
> >  		pkt_dev->clone_skb = value;
> >
> 
> Please only return an error if value > 1.
> 
> Also, if there is any place where user can configure pkt_dev,
> you would want code there to check for the CANT_SHARE_SKB flag
> and force clone_skb to zero if user changes to a different
> device that cannot share skbs.
> 
> Thanks,
> Ben
> 
Understood, thank you Ben.
Neil

> -- 
> Ben Greear <greearb@candelatech.com>
> Candela Technologies Inc  http://www.candelatech.com
> 
> 

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

[Ben Greear]

On 07/20/2011 08:35 AM, Michał Mirosław wrote:
> 2011/7/20 Neil Horman<nhorman@tuxdriver.com>:
>> On Wed, Jul 20, 2011 at 06:24:15AM +0200, Eric Dumazet wrote:
>>> Le mardi 19 juillet 2011 à 22:07 -0400, Neil Horman a écrit :
>>>>>
>>>> I think this is a good idea.  It lets pktgen dynamically make the clone/share
>>>> decision dynamically and only impacts performance for those systems.
>>>>
>>>
>>> Just let pktgen refuse to use clone_skb command for these devices.
>>>
>> copy that, This is by no means final, but what do you think of this?  If its
>> agreeable to you, Ben, et al. I can add this to my local tree and start auditing
>> all the drivers that may need to have the flag set.
>>
>> Regards
>> Neil
>>
>>
>> diff --git a/include/linux/if.h b/include/linux/if.h
>> index 3bc63e6..ae904fe 100644
>> --- a/include/linux/if.h
>> +++ b/include/linux/if.h
>> @@ -76,6 +76,7 @@
>>   #define IFF_BRIDGE_PORT        0x4000          /* device used as bridge port */
>>   #define IFF_OVS_DATAPATH       0x8000  /* device used as Open vSwitch
>>                                          * datapath port */
>> +#define IFF_CANT_SHARE_SKB     0x10000 /* Device can't share skbs in tx path */
>>
>>   #define IF_GET_IFACE   0x0001          /* for querying only */
>>   #define IF_GET_PROTO   0x0002
>> diff --git a/net/core/pktgen.c b/net/core/pktgen.c
>> index f76079c..bf6d88d 100644
>> --- a/net/core/pktgen.c
>> +++ b/net/core/pktgen.c
>> @@ -1071,6 +1071,9 @@ static ssize_t pktgen_if_write(struct file *file,
>>                 if (len<  0)
>>                         return len;
>>
>> +               if (pkt_dev->priv_flags&  IFF_CANT_SHARE_SKB)
>> +                       return -EOPNOTSUPP;
>> +
>>                 i += len;
>>                 pkt_dev->clone_skb = value;
>>
>
> I would prefer that the flag be inclusive, i.e. it should mark drivers
> which can use shared skbs. And it might be better to clone the skb in
> case the flag is disabled to keep functionality working.

The whole point of clone-skb is for speedup due to not allocating
new memory..so if the netdev can't support it, lets just fail instead
of giving user false sense of it working...

User can just use clone-skb value of 0 in that case...

Thanks,
Ben

>
> Best Regards,
> Michał Mirosław
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

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

[David Miller]

From: Neil Horman <nhorman@tuxdriver.com>
Date: Wed, 20 Jul 2011 11:18:27 -0400

> On Wed, Jul 20, 2011 at 06:24:15AM +0200, Eric Dumazet wrote:
>> Le mardi 19 juillet 2011 à 22:07 -0400, Neil Horman a écrit :
>> > > 
>> > I think this is a good idea.  It lets pktgen dynamically make the clone/share
>> > decision dynamically and only impacts performance for those systems.
>> > 
>> 
>> Just let pktgen refuse to use clone_skb command for these devices.
>> 
> copy that, This is by no means final, but what do you think of this?  If its
> agreeable to you, Ben, et al. I can add this to my local tree and start auditing
> all the drivers that may need to have the flag set.

I think there is a much simpler solution.

Set a flag in the SKB when pktgen does SKB sharing.

In dev_queue_xmit() (or perhaps, dev_hard_start_xmit()), check the flag
and if it's set then we copy the SKB.

If this works, then we fix the crash and no driver changes are
necessary both now and in the future.

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

[Ben Greear]

On 07/21/2011 03:01 PM, David Miller wrote:
> From: Neil Horman<nhorman@tuxdriver.com>
> Date: Wed, 20 Jul 2011 11:18:27 -0400
>
>> On Wed, Jul 20, 2011 at 06:24:15AM +0200, Eric Dumazet wrote:
>>> Le mardi 19 juillet 2011 à 22:07 -0400, Neil Horman a écrit :
>>>>>
>>>> I think this is a good idea.  It lets pktgen dynamically make the clone/share
>>>> decision dynamically and only impacts performance for those systems.
>>>>
>>>
>>> Just let pktgen refuse to use clone_skb command for these devices.
>>>
>> copy that, This is by no means final, but what do you think of this?  If its
>> agreeable to you, Ben, et al. I can add this to my local tree and start auditing
>> all the drivers that may need to have the flag set.
>
> I think there is a much simpler solution.
>
> Set a flag in the SKB when pktgen does SKB sharing.
>
> In dev_queue_xmit() (or perhaps, dev_hard_start_xmit()), check the flag
> and if it's set then we copy the SKB.
>
> If this works, then we fix the crash and no driver changes are
> necessary both now and in the future.

Doesn't that make clone-skb in pktgen much less efficient
in all cases?

Thanks,
Ben


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

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

[David Miller]

From: Ben Greear <greearb@candelatech.com>
Date: Thu, 21 Jul 2011 15:14:32 -0700

> On 07/21/2011 03:01 PM, David Miller wrote:
>> From: Neil Horman<nhorman@tuxdriver.com>
>> Date: Wed, 20 Jul 2011 11:18:27 -0400
>>
>>> On Wed, Jul 20, 2011 at 06:24:15AM +0200, Eric Dumazet wrote:
>>>> Le mardi 19 juillet 2011 à 22:07 -0400, Neil Horman a écrit :
>>>>>>
>>>>> I think this is a good idea.  It lets pktgen dynamically make the
>>>>> clone/share
>>>>> decision dynamically and only impacts performance for those systems.
>>>>>
>>>>
>>>> Just let pktgen refuse to use clone_skb command for these devices.
>>>>
>>> copy that, This is by no means final, but what do you think of this?
>>> If its
>>> agreeable to you, Ben, et al. I can add this to my local tree and
>>> start auditing
>>> all the drivers that may need to have the flag set.
>>
>> I think there is a much simpler solution.
>>
>> Set a flag in the SKB when pktgen does SKB sharing.
>>
>> In dev_queue_xmit() (or perhaps, dev_hard_start_xmit()), check the
>> flag
>> and if it's set then we copy the SKB.
>>
>> If this works, then we fix the crash and no driver changes are
>> necessary both now and in the future.
> 
> Doesn't that make clone-skb in pktgen much less efficient
> in all cases?

No, the copy only happens if we enter dev_queue_xmit() which pktgen
doesn't do, it calls the driver's ->ndo_start_xmit() method directly.

That's the whole idea.  Only these encapsulating software devices
will trigger the condition.

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

[Ben Greear]

On 07/21/2011 03:19 PM, David Miller wrote:
> From: Ben Greear<greearb@candelatech.com>
> Date: Thu, 21 Jul 2011 15:14:32 -0700
>
>> On 07/21/2011 03:01 PM, David Miller wrote:
>>> From: Neil Horman<nhorman@tuxdriver.com>
>>> Date: Wed, 20 Jul 2011 11:18:27 -0400
>>>
>>>> On Wed, Jul 20, 2011 at 06:24:15AM +0200, Eric Dumazet wrote:
>>>>> Le mardi 19 juillet 2011 à 22:07 -0400, Neil Horman a écrit :
>>>>>>>
>>>>>> I think this is a good idea.  It lets pktgen dynamically make the
>>>>>> clone/share
>>>>>> decision dynamically and only impacts performance for those systems.
>>>>>>
>>>>>
>>>>> Just let pktgen refuse to use clone_skb command for these devices.
>>>>>
>>>> copy that, This is by no means final, but what do you think of this?
>>>> If its
>>>> agreeable to you, Ben, et al. I can add this to my local tree and
>>>> start auditing
>>>> all the drivers that may need to have the flag set.
>>>
>>> I think there is a much simpler solution.
>>>
>>> Set a flag in the SKB when pktgen does SKB sharing.
>>>
>>> In dev_queue_xmit() (or perhaps, dev_hard_start_xmit()), check the
>>> flag
>>> and if it's set then we copy the SKB.
>>>
>>> If this works, then we fix the crash and no driver changes are
>>> necessary both now and in the future.
>>
>> Doesn't that make clone-skb in pktgen much less efficient
>> in all cases?
>
> No, the copy only happens if we enter dev_queue_xmit() which pktgen
> doesn't do, it calls the driver's ->ndo_start_xmit() method directly.
>
> That's the whole idea.  Only these encapsulating software devices
> will trigger the condition.

It seems there may be some Ethernet drivers that have similar issues,
though I don't know of any personally (many years ago, Chelsio 10G NICs had
this issue, but it may be fixed now.)

But, it should be no worse than what we have today, and now that I
better understand what you suggest, it sounds OK to me.

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

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

[Neil Horman]

On Thu, Jul 21, 2011 at 03:19:03PM -0700, David Miller wrote:
> From: Ben Greear <greearb@candelatech.com>
> Date: Thu, 21 Jul 2011 15:14:32 -0700
> 
> > On 07/21/2011 03:01 PM, David Miller wrote:
> >> From: Neil Horman<nhorman@tuxdriver.com>
> >> Date: Wed, 20 Jul 2011 11:18:27 -0400
> >>
> >>> On Wed, Jul 20, 2011 at 06:24:15AM +0200, Eric Dumazet wrote:
> >>>> Le mardi 19 juillet 2011 à 22:07 -0400, Neil Horman a écrit :
> >>>>>>
> >>>>> I think this is a good idea.  It lets pktgen dynamically make the
> >>>>> clone/share
> >>>>> decision dynamically and only impacts performance for those systems.
> >>>>>
> >>>>
> >>>> Just let pktgen refuse to use clone_skb command for these devices.
> >>>>
> >>> copy that, This is by no means final, but what do you think of this?
> >>> If its
> >>> agreeable to you, Ben, et al. I can add this to my local tree and
> >>> start auditing
> >>> all the drivers that may need to have the flag set.
> >>
> >> I think there is a much simpler solution.
> >>
> >> Set a flag in the SKB when pktgen does SKB sharing.
> >>
> >> In dev_queue_xmit() (or perhaps, dev_hard_start_xmit()), check the
> >> flag
> >> and if it's set then we copy the SKB.
> >>
> >> If this works, then we fix the crash and no driver changes are
> >> necessary both now and in the future.
> > 
> > Doesn't that make clone-skb in pktgen much less efficient
> > in all cases?
> 
> No, the copy only happens if we enter dev_queue_xmit() which pktgen
> doesn't do, it calls the driver's ->ndo_start_xmit() method directly.
> 
> That's the whole idea.  Only these encapsulating software devices
> will trigger the condition.
> 


I'm happy to go down this route Dave, and agree, its a more solid solution, but
I think the problem with it (which Ben may have been alluding to previously) is
that pktgen doesn't use dev_queue_xmit or dev_hard_start_xmit to send frames.
It mimics the locking of dev_hard_start_xmit (but ignores the other checks that
function does), and just calls the ndo_start_xmit routine of the driver
directly.  So theres no common code that an skb traverses from pktgen to a given
driver where we can check such a flag

Again, I'm happy to change that so that pktgen uses dev_hard_start_xmit, but I
wonder if thats going to get the same sort of pushback about performance that my
origional patch did.  Eric, et al., thoughts?

Regards
Neil

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

[David Miller]

From: Neil Horman <nhorman@tuxdriver.com>
Date: Thu, 21 Jul 2011 19:50:49 -0400

> I'm happy to go down this route Dave, and agree, its a more solid solution, but
> I think the problem with it (which Ben may have been alluding to previously) is
> that pktgen doesn't use dev_queue_xmit or dev_hard_start_xmit to send frames.

Neil, that's THE WHOLE POINT, and HOW MY IDEA WORKS.

Pktgen bypasses those functions, so it won't trigger the check and
therefore won't trigger making a copy of the SKB, since copying isn't
needed.

Only layering devices will end up eventually calling into those two
functions and trigger the "need to copy because this SKB is pktgen
shared" check.

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

[Neil Horman]

On Thu, Jul 21, 2011 at 05:08:55PM -0700, David Miller wrote:
> From: Neil Horman <nhorman@tuxdriver.com>
> Date: Thu, 21 Jul 2011 19:50:49 -0400
> 
> > I'm happy to go down this route Dave, and agree, its a more solid solution, but
> > I think the problem with it (which Ben may have been alluding to previously) is
> > that pktgen doesn't use dev_queue_xmit or dev_hard_start_xmit to send frames.
> 
> Neil, that's THE WHOLE POINT, and HOW MY IDEA WORKS.
> 
Dave, RELAX! I GET HOW YOUR IDEA SHOULD WORK!  CAPS ARENT NEEDED! :) 

> Pktgen bypasses those functions, so it won't trigger the check and
> therefore won't trigger making a copy of the SKB, since copying isn't
> needed.
> 
But thats part of the problem, we can't just have pktgen bypass those calls
entirely, assuming that the underlying driver will do so.  See the ppp driver as
an example, it does an skb_pull to add a header to the skb, so multiple
iterations of the multi-skb case will result in stacked header inadvertently.
Another example is the virtio_net driver, which uses the skb->cb area to store
information that would get corrupted in the process.  Neither of those would
pass through the dev_hard_start_xmit path and would be left uncovered by this
solution.

We could fix that by:

a) having pktgen call dev_hard_start_xmit for all frames, giving rise to the
performance issue I was bringing up in my last note.

b) augmenting each driver to check for the flag in the skb as you describe,
which in my mind is no better than having a flag in the netdevice informing
pktgen of weather or not it can use a shared skb approach.

> Only layering devices will end up eventually calling into those two
> functions and trigger the "need to copy because this SKB is pktgen
> shared" check.
> 
See above, that doesn't solve the entire problem.

Regards
Neil

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

[Neil Horman]

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.

3) Audit the network drivers calling ether_setup to identify any code doing so
that can't actualy handle shared skbs and manually disable the new flag.  There
are about 10 drivers in this category.

Thoughts/reviews aprpeciated.
Neil
 

[PATCH 1/2] net: add IFF_SKB_TX_SHARED flag to priv_flags

[Neil Horman]

Pktgen attempts to transmit shared skbs to net devices, which can't be used by
some drivers as they keep state information in skbs.  This patch adds a flag
marking drivers as being able to handle shared skbs in their tx path.  Drivers
are defaulted to being unable to do so, but calling ether_setup enables this
flag, as 90% of the drivers calling ether_setup touch real hardware and can
handle shared skbs.  A subsequent patch will audit drivers to ensure that the
flag is set properly

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
Reported-by: Jiri Pirko <jpirko@redhat.com>
CC: Robert Olsson <robert.olsson@its.uu.se>
CC: Eric Dumazet <eric.dumazet@gmail.com>
CC: Alexey Dobriyan <adobriyan@gmail.com>
CC: David S. Miller <davem@davemloft.net>
---
 include/linux/if.h |    2 ++
 net/core/pktgen.c  |    8 +++++---
 net/ethernet/eth.c |    1 +
 3 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/include/linux/if.h b/include/linux/if.h
index 3bc63e6..03489ca 100644
--- a/include/linux/if.h
+++ b/include/linux/if.h
@@ -76,6 +76,8 @@
 #define IFF_BRIDGE_PORT	0x4000		/* device used as bridge port */
 #define IFF_OVS_DATAPATH	0x8000	/* device used as Open vSwitch
 					 * datapath port */
+#define IFF_TX_SKB_SHARING	0x10000	/* The interface supports sharing
+					 * skbs on transmit */
 
 #define IF_GET_IFACE	0x0001		/* for querying only */
 #define IF_GET_PROTO	0x0002
diff --git a/net/core/pktgen.c b/net/core/pktgen.c
index f76079c..53f3f15 100644
--- a/net/core/pktgen.c
+++ b/net/core/pktgen.c
@@ -1070,7 +1070,9 @@ static ssize_t pktgen_if_write(struct file *file,
 		len = num_arg(&user_buffer[i], 10, &value);
 		if (len < 0)
 			return len;
-
+		if ((len > 0) &&
+		    (!(pkt_dev->odev->priv_flags & IFF_TX_SKB_SHARING)))
+			return -ENOTSUPP;
 		i += len;
 		pkt_dev->clone_skb = value;
 
@@ -3555,7 +3557,6 @@ static int pktgen_add_device(struct pktgen_thread *t, const char *ifname)
 	pkt_dev->min_pkt_size = ETH_ZLEN;
 	pkt_dev->max_pkt_size = ETH_ZLEN;
 	pkt_dev->nfrags = 0;
-	pkt_dev->clone_skb = pg_clone_skb_d;
 	pkt_dev->delay = pg_delay_d;
 	pkt_dev->count = pg_count_d;
 	pkt_dev->sofar = 0;
@@ -3563,7 +3564,6 @@ static int pktgen_add_device(struct pktgen_thread *t, const char *ifname)
 	pkt_dev->udp_src_max = 9;
 	pkt_dev->udp_dst_min = 9;
 	pkt_dev->udp_dst_max = 9;
-
 	pkt_dev->vlan_p = 0;
 	pkt_dev->vlan_cfi = 0;
 	pkt_dev->vlan_id = 0xffff;
@@ -3575,6 +3575,8 @@ static int pktgen_add_device(struct pktgen_thread *t, const char *ifname)
 	err = pktgen_setup_dev(pkt_dev, ifname);
 	if (err)
 		goto out1;
+	if (pkt_dev->odev->priv_flags & IFF_TX_SKB_SHARING)
+		pkt_dev->clone_skb = pg_clone_skb_d;
 
 	pkt_dev->entry = proc_create_data(ifname, 0600, pg_proc_dir,
 					  &pktgen_if_fops, pkt_dev);
diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c
index 5cffb63..4866330 100644
--- a/net/ethernet/eth.c
+++ b/net/ethernet/eth.c
@@ -339,6 +339,7 @@ void ether_setup(struct net_device *dev)
 	dev->addr_len		= ETH_ALEN;
 	dev->tx_queue_len	= 1000;	/* Ethernet wants good queues */
 	dev->flags		= IFF_BROADCAST|IFF_MULTICAST;
+	dev->priv_flags		= IFF_TX_SKB_SHARING;
 
 	memset(dev->broadcast, 0xFF, ETH_ALEN);
 
-- 
1.7.6

[PATCH 2/2] net: Audit drivers to identify those needing IFF_TX_SKB_SHARING cleared

[Neil Horman]

After the last patch, We are left in a state in which only drivers calling
ether_setup have IFF_TX_SKB_SHARING set (we assume that drivers touching real
hardware call ether_setup for their net_devices and don't hold any state in
their skbs.  There are a handful of drivers that violate this assumption of
course, and need to be fixed up.  This patch identifies those drivers, and marks
them as not being able to support the safe transmission of skbs by clearning the
IFF_TX_SKB_SHARING flag in priv_flags

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
CC: Karsten Keil <isdn@linux-pingi.de>
CC: "David S. Miller" <davem@davemloft.net>
CC: Jay Vosburgh <fubar@us.ibm.com>
CC: Andy Gospodarek <andy@greyhouse.net>
CC: Patrick McHardy <kaber@trash.net>
CC: Krzysztof Halasa <khc@pm.waw.pl>
CC: "John W. Linville" <linville@tuxdriver.com>
CC: Greg Kroah-Hartman <gregkh@suse.de>
CC: Marcel Holtmann <marcel@holtmann.org>
CC: Johannes Berg <johannes@sipsolutions.net>
---
 drivers/isdn/i4l/isdn_net.c                  |    3 +++
 drivers/net/bonding/bond_main.c              |    6 ++++--
 drivers/net/ifb.c                            |    2 +-
 drivers/net/macvlan.c                        |    2 +-
 drivers/net/tun.c                            |    1 +
 drivers/net/veth.c                           |    2 ++
 drivers/net/wan/hdlc_fr.c                    |    5 +++--
 drivers/net/wireless/airo.c                  |    1 +
 drivers/net/wireless/hostap/hostap_main.c    |    1 +
 drivers/staging/ath6kl/os/linux/ar6000_drv.c |    1 +
 net/8021q/vlan_dev.c                         |    2 +-
 net/bluetooth/bnep/netdev.c                  |    1 +
 net/l2tp/l2tp_eth.c                          |    2 +-
 net/mac80211/iface.c                         |    1 +
 14 files changed, 22 insertions(+), 8 deletions(-)

diff --git a/drivers/isdn/i4l/isdn_net.c b/drivers/isdn/i4l/isdn_net.c
index 48e9cc0..1f73d7f 100644
--- a/drivers/isdn/i4l/isdn_net.c
+++ b/drivers/isdn/i4l/isdn_net.c
@@ -2532,6 +2532,9 @@ static void _isdn_setup(struct net_device *dev)
 
 	/* Setup the generic properties */
 	dev->flags = IFF_NOARP|IFF_POINTOPOINT;
+
+	/* isdn prepends a header in the tx path, can't share skbs */
+	dev->priv_flags &= ~IFF_TX_SKB_SHARING;
 	dev->header_ops = NULL;
 	dev->netdev_ops = &isdn_netdev_ops;
 
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 02842d0..df21e84 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1557,8 +1557,10 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
 
 			if (slave_dev->type != ARPHRD_ETHER)
 				bond_setup_by_slave(bond_dev, slave_dev);
-			else
+			else {
 				ether_setup(bond_dev);
+				bond_dev->priv_flags &= ~IFF_TX_SKB_SHARING;
+			}
 
 			netdev_bonding_change(bond_dev,
 					      NETDEV_POST_TYPE_CHANGE);
@@ -4330,7 +4332,7 @@ static void bond_setup(struct net_device *bond_dev)
 	bond_dev->tx_queue_len = 0;
 	bond_dev->flags |= IFF_MASTER|IFF_MULTICAST;
 	bond_dev->priv_flags |= IFF_BONDING;
-	bond_dev->priv_flags &= ~IFF_XMIT_DST_RELEASE;
+	bond_dev->priv_flags &= ~(IFF_XMIT_DST_RELEASE | IFF_TX_SKB_SHARING);
 
 	/* At first, we block adding VLANs. That's the only way to
 	 * prevent problems that occur when adding VLANs over an
diff --git a/drivers/net/ifb.c b/drivers/net/ifb.c
index 6e82dd3..46b5f5f 100644
--- a/drivers/net/ifb.c
+++ b/drivers/net/ifb.c
@@ -183,7 +183,7 @@ static void ifb_setup(struct net_device *dev)
 
 	dev->flags |= IFF_NOARP;
 	dev->flags &= ~IFF_MULTICAST;
-	dev->priv_flags &= ~IFF_XMIT_DST_RELEASE;
+	dev->priv_flags &= ~(IFF_XMIT_DST_RELEASE | IFF_TX_SKB_SHARING);
 	random_ether_addr(dev->dev_addr);
 }
 
diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
index ba631fc..05172c3 100644
--- a/drivers/net/macvlan.c
+++ b/drivers/net/macvlan.c
@@ -572,7 +572,7 @@ void macvlan_common_setup(struct net_device *dev)
 {
 	ether_setup(dev);
 
-	dev->priv_flags	       &= ~IFF_XMIT_DST_RELEASE;
+	dev->priv_flags	       &= ~(IFF_XMIT_DST_RELEASE | IFF_TX_SKB_SHARING);
 	dev->netdev_ops		= &macvlan_netdev_ops;
 	dev->destructor		= free_netdev;
 	dev->header_ops		= &macvlan_hard_header_ops,
diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 9a6b382..71f3d1a 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -528,6 +528,7 @@ static void tun_net_init(struct net_device *dev)
 		dev->netdev_ops = &tap_netdev_ops;
 		/* Ethernet TAP Device */
 		ether_setup(dev);
+		dev->priv_flags &= ~IFF_TX_SKB_SHARING;
 
 		random_ether_addr(dev->dev_addr);
 
diff --git a/drivers/net/veth.c b/drivers/net/veth.c
index 7f78db7..5b23767 100644
--- a/drivers/net/veth.c
+++ b/drivers/net/veth.c
@@ -263,6 +263,8 @@ static void veth_setup(struct net_device *dev)
 {
 	ether_setup(dev);
 
+	dev->priv_flags &= ~IFF_TX_SKB_SHARING;
+
 	dev->netdev_ops = &veth_netdev_ops;
 	dev->ethtool_ops = &veth_ethtool_ops;
 	dev->features |= NETIF_F_LLTX;
diff --git a/drivers/net/wan/hdlc_fr.c b/drivers/net/wan/hdlc_fr.c
index b25c922..eb20281 100644
--- a/drivers/net/wan/hdlc_fr.c
+++ b/drivers/net/wan/hdlc_fr.c
@@ -1074,9 +1074,10 @@ static int fr_add_pvc(struct net_device *frad, unsigned int dlci, int type)
 
 	used = pvc_is_used(pvc);
 
-	if (type == ARPHRD_ETHER)
+	if (type == ARPHRD_ETHER) {
 		dev = alloc_netdev(0, "pvceth%d", ether_setup);
-	else
+		dev->priv_flags &= ~IFF_TX_SKB_SHARING;
+	} else
 		dev = alloc_netdev(0, "pvc%d", pvc_setup);
 
 	if (!dev) {
diff --git a/drivers/net/wireless/airo.c b/drivers/net/wireless/airo.c
index 55cf71f..e1b3e3c 100644
--- a/drivers/net/wireless/airo.c
+++ b/drivers/net/wireless/airo.c
@@ -2823,6 +2823,7 @@ static struct net_device *_init_airo_card( unsigned short irq, int port,
 	dev->wireless_data = &ai->wireless_data;
 	dev->irq = irq;
 	dev->base_addr = port;
+	dev->priv_flags &= ~IFF_TX_SKB_SHARING;
 
 	SET_NETDEV_DEV(dev, dmdev);
 
diff --git a/drivers/net/wireless/hostap/hostap_main.c b/drivers/net/wireless/hostap/hostap_main.c
index d508482..89a116f 100644
--- a/drivers/net/wireless/hostap/hostap_main.c
+++ b/drivers/net/wireless/hostap/hostap_main.c
@@ -855,6 +855,7 @@ void hostap_setup_dev(struct net_device *dev, local_info_t *local,
 
 	iface = netdev_priv(dev);
 	ether_setup(dev);
+	dev->priv_flags &= ~IFF_TX_SKB_SHARING;
 
 	/* kernel callbacks */
 	if (iface) {
diff --git a/drivers/staging/ath6kl/os/linux/ar6000_drv.c b/drivers/staging/ath6kl/os/linux/ar6000_drv.c
index 48dd9e3..8ff5289 100644
--- a/drivers/staging/ath6kl/os/linux/ar6000_drv.c
+++ b/drivers/staging/ath6kl/os/linux/ar6000_drv.c
@@ -6179,6 +6179,7 @@ int ar6000_create_ap_interface(struct ar6_softc *ar, char *ap_ifname)
     
     ether_setup(dev);
     init_netdev(dev, ap_ifname);
+    dev->priv_flags &= ~IFF_TX_SKB_SHARING;
 
     if (register_netdev(dev)) {
         AR_DEBUG_PRINTF(ATH_DEBUG_ERR,("ar6000_create_ap_interface: register_netdev failed\n"));
diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
index 934e221..9d40a07 100644
--- a/net/8021q/vlan_dev.c
+++ b/net/8021q/vlan_dev.c
@@ -695,7 +695,7 @@ void vlan_setup(struct net_device *dev)
 	ether_setup(dev);
 
 	dev->priv_flags		|= IFF_802_1Q_VLAN;
-	dev->priv_flags		&= ~IFF_XMIT_DST_RELEASE;
+	dev->priv_flags		&= ~(IFF_XMIT_DST_RELEASE | IFF_TX_SKB_SHARING);
 	dev->tx_queue_len	= 0;
 
 	dev->netdev_ops		= &vlan_netdev_ops;
diff --git a/net/bluetooth/bnep/netdev.c b/net/bluetooth/bnep/netdev.c
index 8c100c9..d4f5dff 100644
--- a/net/bluetooth/bnep/netdev.c
+++ b/net/bluetooth/bnep/netdev.c
@@ -231,6 +231,7 @@ void bnep_net_setup(struct net_device *dev)
 	dev->addr_len = ETH_ALEN;
 
 	ether_setup(dev);
+	dev->priv_flags &= ~IFF_TX_SKB_SHARING;
 	dev->netdev_ops = &bnep_netdev_ops;
 
 	dev->watchdog_timeo  = HZ * 2;
diff --git a/net/l2tp/l2tp_eth.c b/net/l2tp/l2tp_eth.c
index a8193f5..d2726a7 100644
--- a/net/l2tp/l2tp_eth.c
+++ b/net/l2tp/l2tp_eth.c
@@ -103,7 +103,7 @@ static struct net_device_ops l2tp_eth_netdev_ops = {
 static void l2tp_eth_dev_setup(struct net_device *dev)
 {
 	ether_setup(dev);
-
+	dev->priv_flags &= ~IFF_TX_SKB_SHARING;
 	dev->netdev_ops		= &l2tp_eth_netdev_ops;
 	dev->destructor		= free_netdev;
 }
diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
index cd5fb40..556e7e6 100644
--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c
@@ -698,6 +698,7 @@ static const struct net_device_ops ieee80211_monitorif_ops = {
 static void ieee80211_if_setup(struct net_device *dev)
 {
 	ether_setup(dev);
+	dev->priv_flags &= ~IFF_TX_SKB_SHARING;
 	dev->netdev_ops = &ieee80211_dataif_ops;
 	dev->destructor = free_netdev;
 }
-- 
1.7.6

Re: [PATCH 1/2] net: add IFF_SKB_TX_SHARED flag to priv_flags

[Eric Dumazet]

Le lundi 25 juillet 2011 à 15:45 -0400, Neil Horman a écrit :
> Pktgen attempts to transmit shared skbs to net devices, which can't be used by
> some drivers as they keep state information in skbs.  This patch adds a flag
> marking drivers as being able to handle shared skbs in their tx path.  Drivers
> are defaulted to being unable to do so, but calling ether_setup enables this
> flag, as 90% of the drivers calling ether_setup touch real hardware and can
> handle shared skbs.  A subsequent patch will audit drivers to ensure that the
> flag is set properly
> 
> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> Reported-by: Jiri Pirko <jpirko@redhat.com>
> CC: Robert Olsson <robert.olsson@its.uu.se>
> CC: Eric Dumazet <eric.dumazet@gmail.com>
> CC: Alexey Dobriyan <adobriyan@gmail.com>
> CC: David S. Miller <davem@davemloft.net>
> ---
>  include/linux/if.h |    2 ++
>  net/core/pktgen.c  |    8 +++++---
>  net/ethernet/eth.c |    1 +
>  3 files changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/if.h b/include/linux/if.h
> index 3bc63e6..03489ca 100644
> --- a/include/linux/if.h
> +++ b/include/linux/if.h
> @@ -76,6 +76,8 @@
>  #define IFF_BRIDGE_PORT	0x4000		/* device used as bridge port */
>  #define IFF_OVS_DATAPATH	0x8000	/* device used as Open vSwitch
>  					 * datapath port */
> +#define IFF_TX_SKB_SHARING	0x10000	/* The interface supports sharing
> +					 * skbs on transmit */
>  
>  #define IF_GET_IFACE	0x0001		/* for querying only */
>  #define IF_GET_PROTO	0x0002
> diff --git a/net/core/pktgen.c b/net/core/pktgen.c
> index f76079c..53f3f15 100644
> --- a/net/core/pktgen.c
> +++ b/net/core/pktgen.c
> @@ -1070,7 +1070,9 @@ static ssize_t pktgen_if_write(struct file *file,
>  		len = num_arg(&user_buffer[i], 10, &value);
>  		if (len < 0)
>  			return len;
> -
> +		if ((len > 0) &&

		if ((value > 0) ...

> +		    (!(pkt_dev->odev->priv_flags & IFF_TX_SKB_SHARING)))
> +			return -ENOTSUPP;
>  		i += len;
>  		pkt_dev->clone_skb = value;
>  

Re: [PATCH 1/2] net: add IFF_SKB_TX_SHARED flag to priv_flags

[Neil Horman]

On Mon, Jul 25, 2011 at 10:11:22PM +0200, Eric Dumazet wrote:
> Le lundi 25 juillet 2011 à 15:45 -0400, Neil Horman a écrit :
> > Pktgen attempts to transmit shared skbs to net devices, which can't be used by
> > some drivers as they keep state information in skbs.  This patch adds a flag
> > marking drivers as being able to handle shared skbs in their tx path.  Drivers
> > are defaulted to being unable to do so, but calling ether_setup enables this
> > flag, as 90% of the drivers calling ether_setup touch real hardware and can
> > handle shared skbs.  A subsequent patch will audit drivers to ensure that the
> > flag is set properly
> > 
> > Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> > Reported-by: Jiri Pirko <jpirko@redhat.com>
> > CC: Robert Olsson <robert.olsson@its.uu.se>
> > CC: Eric Dumazet <eric.dumazet@gmail.com>
> > CC: Alexey Dobriyan <adobriyan@gmail.com>
> > CC: David S. Miller <davem@davemloft.net>
> > ---
> >  include/linux/if.h |    2 ++
> >  net/core/pktgen.c  |    8 +++++---
> >  net/ethernet/eth.c |    1 +
> >  3 files changed, 8 insertions(+), 3 deletions(-)
> > 
> > diff --git a/include/linux/if.h b/include/linux/if.h
> > index 3bc63e6..03489ca 100644
> > --- a/include/linux/if.h
> > +++ b/include/linux/if.h
> > @@ -76,6 +76,8 @@
> >  #define IFF_BRIDGE_PORT	0x4000		/* device used as bridge port */
> >  #define IFF_OVS_DATAPATH	0x8000	/* device used as Open vSwitch
> >  					 * datapath port */
> > +#define IFF_TX_SKB_SHARING	0x10000	/* The interface supports sharing
> > +					 * skbs on transmit */
> >  
> >  #define IF_GET_IFACE	0x0001		/* for querying only */
> >  #define IF_GET_PROTO	0x0002
> > diff --git a/net/core/pktgen.c b/net/core/pktgen.c
> > index f76079c..53f3f15 100644
> > --- a/net/core/pktgen.c
> > +++ b/net/core/pktgen.c
> > @@ -1070,7 +1070,9 @@ static ssize_t pktgen_if_write(struct file *file,
> >  		len = num_arg(&user_buffer[i], 10, &value);
> >  		if (len < 0)
> >  			return len;
> > -
> > +		if ((len > 0) &&
> 
> 		if ((value > 0) ...
> 
Thank you Eric, I'll respin this shortly
Neil

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

[Neil Horman]

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.

3) Audit the network drivers calling ether_setup to identify any code doing so
that can't actualy handle shared skbs and manually disable the new flag.  There
are about 10 drivers in this category.

Change notes:
v3) Fixed Erics note in which I tested the length of the passed in string rather
than its converted value for beign > 0

Thoughts/reviews aprpeciated.
Neil

[PATCH 1/2] net: add IFF_SKB_TX_SHARED flag to priv_flags

[Neil Horman]

Pktgen attempts to transmit shared skbs to net devices, which can't be used by
some drivers as they keep state information in skbs.  This patch adds a flag
marking drivers as being able to handle shared skbs in their tx path.  Drivers
are defaulted to being unable to do so, but calling ether_setup enables this
flag, as 90% of the drivers calling ether_setup touch real hardware and can
handle shared skbs.  A subsequent patch will audit drivers to ensure that the
flag is set properly

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
Reported-by: Jiri Pirko <jpirko@redhat.com>
CC: Robert Olsson <robert.olsson@its.uu.se>
CC: Eric Dumazet <eric.dumazet@gmail.com>
CC: Alexey Dobriyan <adobriyan@gmail.com>
CC: David S. Miller <davem@davemloft.net>
---
 include/linux/if.h |    2 ++
 net/core/pktgen.c  |    8 +++++---
 net/ethernet/eth.c |    1 +
 3 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/include/linux/if.h b/include/linux/if.h
index 3bc63e6..03489ca 100644
--- a/include/linux/if.h
+++ b/include/linux/if.h
@@ -76,6 +76,8 @@
 #define IFF_BRIDGE_PORT	0x4000		/* device used as bridge port */
 #define IFF_OVS_DATAPATH	0x8000	/* device used as Open vSwitch
 					 * datapath port */
+#define IFF_TX_SKB_SHARING	0x10000	/* The interface supports sharing
+					 * skbs on transmit */
 
 #define IF_GET_IFACE	0x0001		/* for querying only */
 #define IF_GET_PROTO	0x0002
diff --git a/net/core/pktgen.c b/net/core/pktgen.c
index f76079c..e35a6fb 100644
--- a/net/core/pktgen.c
+++ b/net/core/pktgen.c
@@ -1070,7 +1070,9 @@ static ssize_t pktgen_if_write(struct file *file,
 		len = num_arg(&user_buffer[i], 10, &value);
 		if (len < 0)
 			return len;
-
+		if ((value > 0) &&
+		    (!(pkt_dev->odev->priv_flags & IFF_TX_SKB_SHARING)))
+			return -ENOTSUPP;
 		i += len;
 		pkt_dev->clone_skb = value;
 
@@ -3555,7 +3557,6 @@ static int pktgen_add_device(struct pktgen_thread *t, const char *ifname)
 	pkt_dev->min_pkt_size = ETH_ZLEN;
 	pkt_dev->max_pkt_size = ETH_ZLEN;
 	pkt_dev->nfrags = 0;
-	pkt_dev->clone_skb = pg_clone_skb_d;
 	pkt_dev->delay = pg_delay_d;
 	pkt_dev->count = pg_count_d;
 	pkt_dev->sofar = 0;
@@ -3563,7 +3564,6 @@ static int pktgen_add_device(struct pktgen_thread *t, const char *ifname)
 	pkt_dev->udp_src_max = 9;
 	pkt_dev->udp_dst_min = 9;
 	pkt_dev->udp_dst_max = 9;
-
 	pkt_dev->vlan_p = 0;
 	pkt_dev->vlan_cfi = 0;
 	pkt_dev->vlan_id = 0xffff;
@@ -3575,6 +3575,8 @@ static int pktgen_add_device(struct pktgen_thread *t, const char *ifname)
 	err = pktgen_setup_dev(pkt_dev, ifname);
 	if (err)
 		goto out1;
+	if (pkt_dev->odev->priv_flags & IFF_TX_SKB_SHARING)
+		pkt_dev->clone_skb = pg_clone_skb_d;
 
 	pkt_dev->entry = proc_create_data(ifname, 0600, pg_proc_dir,
 					  &pktgen_if_fops, pkt_dev);
diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c
index 5cffb63..4866330 100644
--- a/net/ethernet/eth.c
+++ b/net/ethernet/eth.c
@@ -339,6 +339,7 @@ void ether_setup(struct net_device *dev)
 	dev->addr_len		= ETH_ALEN;
 	dev->tx_queue_len	= 1000;	/* Ethernet wants good queues */
 	dev->flags		= IFF_BROADCAST|IFF_MULTICAST;
+	dev->priv_flags		= IFF_TX_SKB_SHARING;
 
 	memset(dev->broadcast, 0xFF, ETH_ALEN);
 
-- 
1.7.6

[PATCH 2/2] net: Audit drivers to identify those needing IFF_TX_SKB_SHARING cleared

[Neil Horman]

After the last patch, We are left in a state in which only drivers calling
ether_setup have IFF_TX_SKB_SHARING set (we assume that drivers touching real
hardware call ether_setup for their net_devices and don't hold any state in
their skbs.  There are a handful of drivers that violate this assumption of
course, and need to be fixed up.  This patch identifies those drivers, and marks
them as not being able to support the safe transmission of skbs by clearning the
IFF_TX_SKB_SHARING flag in priv_flags

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
CC: Karsten Keil <isdn@linux-pingi.de>
CC: "David S. Miller" <davem@davemloft.net>
CC: Jay Vosburgh <fubar@us.ibm.com>
CC: Andy Gospodarek <andy@greyhouse.net>
CC: Patrick McHardy <kaber@trash.net>
CC: Krzysztof Halasa <khc@pm.waw.pl>
CC: "John W. Linville" <linville@tuxdriver.com>
CC: Greg Kroah-Hartman <gregkh@suse.de>
CC: Marcel Holtmann <marcel@holtmann.org>
CC: Johannes Berg <johannes@sipsolutions.net>
---
 drivers/isdn/i4l/isdn_net.c                  |    3 +++
 drivers/net/bonding/bond_main.c              |    6 ++++--
 drivers/net/ifb.c                            |    2 +-
 drivers/net/macvlan.c                        |    2 +-
 drivers/net/tun.c                            |    1 +
 drivers/net/veth.c                           |    2 ++
 drivers/net/wan/hdlc_fr.c                    |    5 +++--
 drivers/net/wireless/airo.c                  |    1 +
 drivers/net/wireless/hostap/hostap_main.c    |    1 +
 drivers/staging/ath6kl/os/linux/ar6000_drv.c |    1 +
 net/8021q/vlan_dev.c                         |    2 +-
 net/bluetooth/bnep/netdev.c                  |    1 +
 net/l2tp/l2tp_eth.c                          |    2 +-
 net/mac80211/iface.c                         |    1 +
 14 files changed, 22 insertions(+), 8 deletions(-)

diff --git a/drivers/isdn/i4l/isdn_net.c b/drivers/isdn/i4l/isdn_net.c
index 48e9cc0..1f73d7f 100644
--- a/drivers/isdn/i4l/isdn_net.c
+++ b/drivers/isdn/i4l/isdn_net.c
@@ -2532,6 +2532,9 @@ static void _isdn_setup(struct net_device *dev)
 
 	/* Setup the generic properties */
 	dev->flags = IFF_NOARP|IFF_POINTOPOINT;
+
+	/* isdn prepends a header in the tx path, can't share skbs */
+	dev->priv_flags &= ~IFF_TX_SKB_SHARING;
 	dev->header_ops = NULL;
 	dev->netdev_ops = &isdn_netdev_ops;
 
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 02842d0..df21e84 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1557,8 +1557,10 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
 
 			if (slave_dev->type != ARPHRD_ETHER)
 				bond_setup_by_slave(bond_dev, slave_dev);
-			else
+			else {
 				ether_setup(bond_dev);
+				bond_dev->priv_flags &= ~IFF_TX_SKB_SHARING;
+			}
 
 			netdev_bonding_change(bond_dev,
 					      NETDEV_POST_TYPE_CHANGE);
@@ -4330,7 +4332,7 @@ static void bond_setup(struct net_device *bond_dev)
 	bond_dev->tx_queue_len = 0;
 	bond_dev->flags |= IFF_MASTER|IFF_MULTICAST;
 	bond_dev->priv_flags |= IFF_BONDING;
-	bond_dev->priv_flags &= ~IFF_XMIT_DST_RELEASE;
+	bond_dev->priv_flags &= ~(IFF_XMIT_DST_RELEASE | IFF_TX_SKB_SHARING);
 
 	/* At first, we block adding VLANs. That's the only way to
 	 * prevent problems that occur when adding VLANs over an
diff --git a/drivers/net/ifb.c b/drivers/net/ifb.c
index 6e82dd3..46b5f5f 100644
--- a/drivers/net/ifb.c
+++ b/drivers/net/ifb.c
@@ -183,7 +183,7 @@ static void ifb_setup(struct net_device *dev)
 
 	dev->flags |= IFF_NOARP;
 	dev->flags &= ~IFF_MULTICAST;
-	dev->priv_flags &= ~IFF_XMIT_DST_RELEASE;
+	dev->priv_flags &= ~(IFF_XMIT_DST_RELEASE | IFF_TX_SKB_SHARING);
 	random_ether_addr(dev->dev_addr);
 }
 
diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
index ba631fc..05172c3 100644
--- a/drivers/net/macvlan.c
+++ b/drivers/net/macvlan.c
@@ -572,7 +572,7 @@ void macvlan_common_setup(struct net_device *dev)
 {
 	ether_setup(dev);
 
-	dev->priv_flags	       &= ~IFF_XMIT_DST_RELEASE;
+	dev->priv_flags	       &= ~(IFF_XMIT_DST_RELEASE | IFF_TX_SKB_SHARING);
 	dev->netdev_ops		= &macvlan_netdev_ops;
 	dev->destructor		= free_netdev;
 	dev->header_ops		= &macvlan_hard_header_ops,
diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 9a6b382..71f3d1a 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -528,6 +528,7 @@ static void tun_net_init(struct net_device *dev)
 		dev->netdev_ops = &tap_netdev_ops;
 		/* Ethernet TAP Device */
 		ether_setup(dev);
+		dev->priv_flags &= ~IFF_TX_SKB_SHARING;
 
 		random_ether_addr(dev->dev_addr);
 
diff --git a/drivers/net/veth.c b/drivers/net/veth.c
index 7f78db7..5b23767 100644
--- a/drivers/net/veth.c
+++ b/drivers/net/veth.c
@@ -263,6 +263,8 @@ static void veth_setup(struct net_device *dev)
 {
 	ether_setup(dev);
 
+	dev->priv_flags &= ~IFF_TX_SKB_SHARING;
+
 	dev->netdev_ops = &veth_netdev_ops;
 	dev->ethtool_ops = &veth_ethtool_ops;
 	dev->features |= NETIF_F_LLTX;
diff --git a/drivers/net/wan/hdlc_fr.c b/drivers/net/wan/hdlc_fr.c
index b25c922..eb20281 100644
--- a/drivers/net/wan/hdlc_fr.c
+++ b/drivers/net/wan/hdlc_fr.c
@@ -1074,9 +1074,10 @@ static int fr_add_pvc(struct net_device *frad, unsigned int dlci, int type)
 
 	used = pvc_is_used(pvc);
 
-	if (type == ARPHRD_ETHER)
+	if (type == ARPHRD_ETHER) {
 		dev = alloc_netdev(0, "pvceth%d", ether_setup);
-	else
+		dev->priv_flags &= ~IFF_TX_SKB_SHARING;
+	} else
 		dev = alloc_netdev(0, "pvc%d", pvc_setup);
 
 	if (!dev) {
diff --git a/drivers/net/wireless/airo.c b/drivers/net/wireless/airo.c
index 55cf71f..e1b3e3c 100644
--- a/drivers/net/wireless/airo.c
+++ b/drivers/net/wireless/airo.c
@@ -2823,6 +2823,7 @@ static struct net_device *_init_airo_card( unsigned short irq, int port,
 	dev->wireless_data = &ai->wireless_data;
 	dev->irq = irq;
 	dev->base_addr = port;
+	dev->priv_flags &= ~IFF_TX_SKB_SHARING;
 
 	SET_NETDEV_DEV(dev, dmdev);
 
diff --git a/drivers/net/wireless/hostap/hostap_main.c b/drivers/net/wireless/hostap/hostap_main.c
index d508482..89a116f 100644
--- a/drivers/net/wireless/hostap/hostap_main.c
+++ b/drivers/net/wireless/hostap/hostap_main.c
@@ -855,6 +855,7 @@ void hostap_setup_dev(struct net_device *dev, local_info_t *local,
 
 	iface = netdev_priv(dev);
 	ether_setup(dev);
+	dev->priv_flags &= ~IFF_TX_SKB_SHARING;
 
 	/* kernel callbacks */
 	if (iface) {
diff --git a/drivers/staging/ath6kl/os/linux/ar6000_drv.c b/drivers/staging/ath6kl/os/linux/ar6000_drv.c
index 48dd9e3..8ff5289 100644
--- a/drivers/staging/ath6kl/os/linux/ar6000_drv.c
+++ b/drivers/staging/ath6kl/os/linux/ar6000_drv.c
@@ -6179,6 +6179,7 @@ int ar6000_create_ap_interface(struct ar6_softc *ar, char *ap_ifname)
     
     ether_setup(dev);
     init_netdev(dev, ap_ifname);
+    dev->priv_flags &= ~IFF_TX_SKB_SHARING;
 
     if (register_netdev(dev)) {
         AR_DEBUG_PRINTF(ATH_DEBUG_ERR,("ar6000_create_ap_interface: register_netdev failed\n"));
diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
index 934e221..9d40a07 100644
--- a/net/8021q/vlan_dev.c
+++ b/net/8021q/vlan_dev.c
@@ -695,7 +695,7 @@ void vlan_setup(struct net_device *dev)
 	ether_setup(dev);
 
 	dev->priv_flags		|= IFF_802_1Q_VLAN;
-	dev->priv_flags		&= ~IFF_XMIT_DST_RELEASE;
+	dev->priv_flags		&= ~(IFF_XMIT_DST_RELEASE | IFF_TX_SKB_SHARING);
 	dev->tx_queue_len	= 0;
 
 	dev->netdev_ops		= &vlan_netdev_ops;
diff --git a/net/bluetooth/bnep/netdev.c b/net/bluetooth/bnep/netdev.c
index 8c100c9..d4f5dff 100644
--- a/net/bluetooth/bnep/netdev.c
+++ b/net/bluetooth/bnep/netdev.c
@@ -231,6 +231,7 @@ void bnep_net_setup(struct net_device *dev)
 	dev->addr_len = ETH_ALEN;
 
 	ether_setup(dev);
+	dev->priv_flags &= ~IFF_TX_SKB_SHARING;
 	dev->netdev_ops = &bnep_netdev_ops;
 
 	dev->watchdog_timeo  = HZ * 2;
diff --git a/net/l2tp/l2tp_eth.c b/net/l2tp/l2tp_eth.c
index a8193f5..d2726a7 100644
--- a/net/l2tp/l2tp_eth.c
+++ b/net/l2tp/l2tp_eth.c
@@ -103,7 +103,7 @@ static struct net_device_ops l2tp_eth_netdev_ops = {
 static void l2tp_eth_dev_setup(struct net_device *dev)
 {
 	ether_setup(dev);
-
+	dev->priv_flags &= ~IFF_TX_SKB_SHARING;
 	dev->netdev_ops		= &l2tp_eth_netdev_ops;
 	dev->destructor		= free_netdev;
 }
diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
index cd5fb40..556e7e6 100644
--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c
@@ -698,6 +698,7 @@ static const struct net_device_ops ieee80211_monitorif_ops = {
 static void ieee80211_if_setup(struct net_device *dev)
 {
 	ether_setup(dev);
+	dev->priv_flags &= ~IFF_TX_SKB_SHARING;
 	dev->netdev_ops = &ieee80211_dataif_ops;
 	dev->destructor = free_netdev;
 }
-- 
1.7.6

Re: [PATCH 2/2] net: Audit drivers to identify those needing IFF_TX_SKB_SHARING cleared

[Krzysztof Halasa]

Neil Horman <nhorman@tuxdriver.com> writes:

> After the last patch, We are left in a state in which only drivers calling
> ether_setup have IFF_TX_SKB_SHARING set (we assume that drivers touching real
> hardware call ether_setup for their net_devices and don't hold any state in
> their skbs.  There are a handful of drivers that violate this assumption of
> course, and need to be fixed up.  This patch identifies those drivers, and marks
> them as not being able to support the safe transmission of skbs by clearning the
> IFF_TX_SKB_SHARING flag in priv_flags

> --- a/drivers/net/wan/hdlc_fr.c
> +++ b/drivers/net/wan/hdlc_fr.c
> @@ -1074,9 +1074,10 @@ static int fr_add_pvc(struct net_device *frad, unsigned int dlci, int type)
>  
>  	used = pvc_is_used(pvc);
>  
> -	if (type == ARPHRD_ETHER)
> +	if (type == ARPHRD_ETHER) {
>  		dev = alloc_netdev(0, "pvceth%d", ether_setup);
> -	else
> +		dev->priv_flags &= ~IFF_TX_SKB_SHARING;
> +	} else
>  		dev = alloc_netdev(0, "pvc%d", pvc_setup);
>  
>  	if (!dev) {

That's right, hdlc_fr has to add an additional header.

Acked-by: Krzysztof Hałasa <khc@pm.waw.pl>
-- 
Krzysztof Halasa

Re: [PATCH 1/2] net: add IFF_SKB_TX_SHARED flag to priv_flags

[David Miller]

From: Neil Horman <nhorman@tuxdriver.com>
Date: Tue, 26 Jul 2011 12:05:37 -0400

> Pktgen attempts to transmit shared skbs to net devices, which can't be used by
> some drivers as they keep state information in skbs.  This patch adds a flag
> marking drivers as being able to handle shared skbs in their tx path.  Drivers
> are defaulted to being unable to do so, but calling ether_setup enables this
> flag, as 90% of the drivers calling ether_setup touch real hardware and can
> handle shared skbs.  A subsequent patch will audit drivers to ensure that the
> flag is set properly
> 
> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> Reported-by: Jiri Pirko <jpirko@redhat.com>

Applied.

Re: [PATCH 2/2] net: Audit drivers to identify those needing IFF_TX_SKB_SHARING cleared

[David Miller]

From: Neil Horman <nhorman@tuxdriver.com>
Date: Tue, 26 Jul 2011 12:05:38 -0400

> After the last patch, We are left in a state in which only drivers calling
> ether_setup have IFF_TX_SKB_SHARING set (we assume that drivers touching real
> hardware call ether_setup for their net_devices and don't hold any state in
> their skbs.  There are a handful of drivers that violate this assumption of
> course, and need to be fixed up.  This patch identifies those drivers, and marks
> them as not being able to support the safe transmission of skbs by clearning the
> IFF_TX_SKB_SHARING flag in priv_flags
> 
> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>

Applied.

[PATCH 1/2] net: add IFF_SKB_TX_SHARED flag to priv_flags

[Robert Olsson]

Hello,

Yes seems like good solution to disable this feature individually for "unsupported" drivers
Maybe Dave has some additional comments.

Thanks for fixing this.

Cheers.
						--ro

Signed-off-by: Robert Olsson <robert.olsson@its.uu.se>
	       	      


Neil Horman writes:
 > Pktgen attempts to transmit shared skbs to net devices, which can't be used by
 > some drivers as they keep state information in skbs.  This patch adds a flag
 > marking drivers as being able to handle shared skbs in their tx path.  Drivers
 > are defaulted to being unable to do so, but calling ether_setup enables this
 > flag, as 90% of the drivers calling ether_setup touch real hardware and can
 > handle shared skbs.  A subsequent patch will audit drivers to ensure that the
 > flag is set properly
 > 
 > Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
 > Reported-by: Jiri Pirko <jpirko@redhat.com>
 > CC: Robert Olsson <robert.olsson@its.uu.se>
 > CC: Eric Dumazet <eric.dumazet@gmail.com>
 > CC: Alexey Dobriyan <adobriyan@gmail.com>
 > CC: David S. Miller <davem@davemloft.net>
 > ---
 >  include/linux/if.h |    2 ++
 >  net/core/pktgen.c  |    8 +++++---
 >  net/ethernet/eth.c |    1 +
 >  3 files changed, 8 insertions(+), 3 deletions(-)
 > 
 > diff --git a/include/linux/if.h b/include/linux/if.h
 > index 3bc63e6..03489ca 100644
 > --- a/include/linux/if.h
 > +++ b/include/linux/if.h
 > @@ -76,6 +76,8 @@
 >  #define IFF_BRIDGE_PORT	0x4000		/* device used as bridge port */
 >  #define IFF_OVS_DATAPATH	0x8000	/* device used as Open vSwitch
 >  					 * datapath port */
 > +#define IFF_TX_SKB_SHARING	0x10000	/* The interface supports sharing
 > +					 * skbs on transmit */
 >  
 >  #define IF_GET_IFACE	0x0001		/* for querying only */
 >  #define IF_GET_PROTO	0x0002
 > diff --git a/net/core/pktgen.c b/net/core/pktgen.c
 > index f76079c..e35a6fb 100644
 > --- a/net/core/pktgen.c
 > +++ b/net/core/pktgen.c
 > @@ -1070,7 +1070,9 @@ static ssize_t pktgen_if_write(struct file *file,
 >  		len = num_arg(&user_buffer[i], 10, &value);
 >  		if (len < 0)
 >  			return len;
 > -
 > +		if ((value > 0) &&
 > +		    (!(pkt_dev->odev->priv_flags & IFF_TX_SKB_SHARING)))
 > +			return -ENOTSUPP;
 >  		i += len;
 >  		pkt_dev->clone_skb = value;
 >  
 > @@ -3555,7 +3557,6 @@ static int pktgen_add_device(struct pktgen_thread *t, const char *ifname)
 >  	pkt_dev->min_pkt_size = ETH_ZLEN;
 >  	pkt_dev->max_pkt_size = ETH_ZLEN;
 >  	pkt_dev->nfrags = 0;
 > -	pkt_dev->clone_skb = pg_clone_skb_d;
 >  	pkt_dev->delay = pg_delay_d;
 >  	pkt_dev->count = pg_count_d;
 >  	pkt_dev->sofar = 0;
 > @@ -3563,7 +3564,6 @@ static int pktgen_add_device(struct pktgen_thread *t, const char *ifname)
 >  	pkt_dev->udp_src_max = 9;
 >  	pkt_dev->udp_dst_min = 9;
 >  	pkt_dev->udp_dst_max = 9;
 > -
 >  	pkt_dev->vlan_p = 0;
 >  	pkt_dev->vlan_cfi = 0;
 >  	pkt_dev->vlan_id = 0xffff;
 > @@ -3575,6 +3575,8 @@ static int pktgen_add_device(struct pktgen_thread *t, const char *ifname)
 >  	err = pktgen_setup_dev(pkt_dev, ifname);
 >  	if (err)
 >  		goto out1;
 > +	if (pkt_dev->odev->priv_flags & IFF_TX_SKB_SHARING)
 > +		pkt_dev->clone_skb = pg_clone_skb_d;
 >  
 >  	pkt_dev->entry = proc_create_data(ifname, 0600, pg_proc_dir,
 >  					  &pktgen_if_fops, pkt_dev);
 > diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c
 > index 5cffb63..4866330 100644
 > --- a/net/ethernet/eth.c
 > +++ b/net/ethernet/eth.c
 > @@ -339,6 +339,7 @@ void ether_setup(struct net_device *dev)
 >  	dev->addr_len		= ETH_ALEN;
 >  	dev->tx_queue_len	= 1000;	/* Ethernet wants good queues */
 >  	dev->flags		= IFF_BROADCAST|IFF_MULTICAST;
 > +	dev->priv_flags		= IFF_TX_SKB_SHARING;
 >  
 >  	memset(dev->broadcast, 0xFF, ETH_ALEN);
 >  
 > -- 
 > 1.7.6
 > 
 > --
 > To unsubscribe from this list: send the line "unsubscribe netdev" in
 > the body of a message to majordomo@vger.kernel.org
 > More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 

Re: [PATCH 1/2] net: add IFF_SKB_TX_SHARED flag to priv_flags

[Neil Horman]

On Thu, Jul 28, 2011 at 10:15:57AM +0200, Robert Olsson wrote:
> 
> Hello,
> 
> Yes seems like good solution to disable this feature individually for "unsupported" drivers
> Maybe Dave has some additional comments.
> 
> Thanks for fixing this.
> 
> Cheers.
> 						--ro
> 
> Signed-off-by: Robert Olsson <robert.olsson@its.uu.se>
> 	       	      
> 
Great, thank you all!
Neil

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

[Michael S. Tsirkin]

On Tue, Jul 26, 2011 at 12:05:36PM -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.
> 
> 3) Audit the network drivers calling ether_setup to identify any code doing so
> that can't actualy handle shared skbs and manually disable the new flag.  There
> are about 10 drivers in this category.
> 
> Change notes:
> v3) Fixed Erics note in which I tested the length of the passed in string rather
> than its converted value for beign > 0
> 
> Thoughts/reviews aprpeciated.
> Neil

It might be a good idea to disable vhost-net zerocopy for
such devices as well: these skbs are shared with userspace.
Shirley, what do you think?

-- 
MST

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

[Neil Horman]

On Fri, Jul 29, 2011 at 09:16:42AM +0300, Michael S. Tsirkin wrote:
> On Tue, Jul 26, 2011 at 12:05:36PM -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.
> > 
> > 3) Audit the network drivers calling ether_setup to identify any code doing so
> > that can't actualy handle shared skbs and manually disable the new flag.  There
> > are about 10 drivers in this category.
> > 
> > Change notes:
> > v3) Fixed Erics note in which I tested the length of the passed in string rather
> > than its converted value for beign > 0
> > 
> > Thoughts/reviews aprpeciated.
> > Neil
> 
> It might be a good idea to disable vhost-net zerocopy for
> such devices as well: these skbs are shared with userspace.
> Shirley, what do you think?
> 
I don't think thats a problem, since (IIUC) only skbs with (tx_flags &
SKBTX_DEV_ZEROCOPY) set can do that.  The pktgen skbs originate in the kernel
and never have that flag set).
Neil

P.S. Don't call me Shirley :).  Sorry, its not every day you get to use that
line.

> -- 
> MST
> 

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

[Ben Hutchings]

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.

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.

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

[Neil Horman]

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?
Neil

> 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.
> 
> 

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

[Ben Hutchings]

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.

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

[Ben Hutchings]

On Mon, 2011-08-22 at 17:17 +0100, Ben Hutchings wrote:
[...]
> 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.

We could specify something like:

        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.

Does that look correct?

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.

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

[Neil Horman]

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.

In the additional cases you are concerned with, multiple users of an skb may or
may not run in parallel.  In the parallel case however, to avoid additional
races, one of the following must be true:

1) All n contexts treat the skb as immutible and wind up sending to the same
device and queue, at which point the xmit_lock in the net_device serializes
their operation.

2) Each of the n contexts (or a subset thereof) will modify the skb data, which
requires that they do their own locking between each other, which must encompass
the transmission of the skb in each context (lest they corrupt the skb during
transmission).  Such locking again serializes the transmit peth.

In either case, access to the skb is serialized such that it is only ever
handled by one driver at one time, and during that window drivers may opt to
modify the state of the skb, as long as they dont' expect the sate to remain in
their control after the return from ndo_start_xmit.


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.

Its a bit more problematic for skb_pad, since the possibility exists for
successive calls to skb_pad to iteratively increase the pad until the skb
tailroom is exhausted, causing a transmission error.  That said however, there
are only 6 drivers that use skb_pad (sfx, niu, rt2800, rt61pci, rt37usb, and
claw), and they all only pad if the skb is not already a minimum length.  These
calls should probably be audited and converted to skb_padto anyway.

So, in my view, these aren't really problems.  If someone just does a blind skb
pad in the driver, then we have something of a problem, but one that is easily
fixed if and when the problem should arise.
Neil

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

[Ben Hutchings]

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.

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

[Neil Horman]

On Tue, Aug 23, 2011 at 03:01:24PM +0100, Ben Hutchings wrote:
> 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.
> 
It _is_ generally safe to make idempotent changes to an skb when their shared,
thats what I was explaining in my previous post.  The only restriction we need
to concern ourselves with is the drivers assumption that any modifications
(idempotent or not) will be preserved after the return from ndo_start_xmit.

> > 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.
> 
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)

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.

If you're ok with my language, I'll put a patch together for that.
Neil
 
> 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.
> 
> 

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

[Ben Hutchings]

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.

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

[Neil Horman]

On Tue, Aug 23, 2011 at 04:53:05PM +0100, Ben Hutchings wrote:
> 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.

Exactly, the fact that struct sk_buff holds output interface and queue
information implies that we need to serialize transmit operations in the shared
skb case, so we're safe even if we're talking about multiple contexts of
execution.

> 
> > 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.
> 
This is true, but we're still protected I think, at least in the case of using
skb_pad[to] in the driver.  A driver won't kfree_skb the skb until the DMA is
complete, so it will remained shared (i.e. skb->users > 1, and skb_shared will
consequently return true).  On iterated uses of skb_pad[to] be they in the same
driver or different drivers), any change that re-allocates or modifies the data
area of an skb goes through pskb_expand_head and/or __pskb_pull_tail.  In the
former case we BUG halt, because its already invalid to call pskb_expand_head on
a shared skb, and in the latter case shared skbs will get cloned automatically,
removing the shared state of the skb.

> > If you're ok with my language, I'll put a patch together for that.
> 
> I don't think we're quite there yet.
> 
Do you have other cases you're concerned about?

Neil

> 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.
> 
>