* [PATCH] pktgen: Clone skb to avoid corruption of skbs in ndo_start_xmit methods @ 2011-07-19 19:52 Neil Horman 2011-07-19 20:02 ` Eric Dumazet ` (2 more replies) 0 siblings, 3 replies; 59+ messages in thread From: Neil Horman @ 2011-07-19 19:52 UTC (permalink / raw) To: netdev; +Cc: Neil Horman, Eric Dumazet, Alexey Dobriyan, David S. Miller 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 ^ permalink raw reply related [flat|nested] 59+ messages in thread
* Re: [PATCH] pktgen: Clone skb to avoid corruption of skbs in ndo_start_xmit methods 2011-07-19 19:52 [PATCH] pktgen: Clone skb to avoid corruption of skbs in ndo_start_xmit methods Neil Horman @ 2011-07-19 20:02 ` Eric Dumazet 2011-07-19 20:17 ` Joe Perches 2011-07-19 20:29 ` Jiri Pirko 2011-07-25 19:45 ` [PATCH 0/2] pktgen: Clone skb to avoid corruption of skbs in ndo_start_xmit methods (v2) Neil Horman 2011-07-26 16:05 ` [PATCH 0/2] pktgen: Clone skb to avoid corruption of skbs in ndo_start_xmit methods (v3) Neil Horman 2 siblings, 2 replies; 59+ messages in thread From: Eric Dumazet @ 2011-07-19 20:02 UTC (permalink / raw) To: Neil Horman; +Cc: netdev, Alexey Dobriyan, David S. Miller 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 ^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH] pktgen: Clone skb to avoid corruption of skbs in ndo_start_xmit methods 2011-07-19 20:02 ` Eric Dumazet @ 2011-07-19 20:17 ` Joe Perches 2011-07-19 20:29 ` Jiri Pirko 1 sibling, 0 replies; 59+ messages in thread From: Joe Perches @ 2011-07-19 20:17 UTC (permalink / raw) To: Eric Dumazet, Robert Olsson Cc: Neil Horman, netdev, Alexey Dobriyan, David S. Miller 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 ^ permalink raw reply related [flat|nested] 59+ messages in thread
* Re: [PATCH] pktgen: Clone skb to avoid corruption of skbs in ndo_start_xmit methods 2011-07-19 20:02 ` Eric Dumazet 2011-07-19 20:17 ` Joe Perches @ 2011-07-19 20:29 ` Jiri Pirko 2011-07-19 20:41 ` Eric Dumazet 1 sibling, 1 reply; 59+ messages in thread From: Jiri Pirko @ 2011-07-19 20:29 UTC (permalink / raw) To: Eric Dumazet; +Cc: Neil Horman, netdev, Alexey Dobriyan, David S. Miller 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 ^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH] pktgen: Clone skb to avoid corruption of skbs in ndo_start_xmit methods 2011-07-19 20:29 ` Jiri Pirko @ 2011-07-19 20:41 ` Eric Dumazet 2011-07-19 21:01 ` Ben Greear 2011-07-20 0:19 ` Neil Horman 0 siblings, 2 replies; 59+ messages in thread From: Eric Dumazet @ 2011-07-19 20:41 UTC (permalink / raw) To: Jiri Pirko; +Cc: Neil Horman, netdev, Alexey Dobriyan, David S. Miller 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... ^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH] pktgen: Clone skb to avoid corruption of skbs in ndo_start_xmit methods 2011-07-19 20:41 ` Eric Dumazet @ 2011-07-19 21:01 ` Ben Greear 2011-07-20 0:19 ` Neil Horman 1 sibling, 0 replies; 59+ messages in thread From: Ben Greear @ 2011-07-19 21:01 UTC (permalink / raw) To: Eric Dumazet Cc: Jiri Pirko, Neil Horman, netdev, Alexey Dobriyan, David S. Miller 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 ^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH] pktgen: Clone skb to avoid corruption of skbs in ndo_start_xmit methods 2011-07-19 20:41 ` Eric Dumazet 2011-07-19 21:01 ` Ben Greear @ 2011-07-20 0:19 ` Neil Horman 2011-07-20 0:25 ` Rick Jones 2011-07-20 0:43 ` Eric Dumazet 1 sibling, 2 replies; 59+ messages in thread From: Neil Horman @ 2011-07-20 0:19 UTC (permalink / raw) To: Eric Dumazet; +Cc: Jiri Pirko, netdev, Alexey Dobriyan, David S. Miller 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 > > > > ^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH] pktgen: Clone skb to avoid corruption of skbs in ndo_start_xmit methods 2011-07-20 0:19 ` Neil Horman @ 2011-07-20 0:25 ` Rick Jones 2011-07-20 15:23 ` Loke, Chetan 2011-07-20 0:43 ` Eric Dumazet 1 sibling, 1 reply; 59+ messages in thread From: Rick Jones @ 2011-07-20 0:25 UTC (permalink / raw) To: Neil Horman Cc: Eric Dumazet, Jiri Pirko, netdev, Alexey Dobriyan, David S. Miller 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 ^ permalink raw reply [flat|nested] 59+ messages in thread
* RE: [PATCH] pktgen: Clone skb to avoid corruption of skbs in ndo_start_xmit methods 2011-07-20 0:25 ` Rick Jones @ 2011-07-20 15:23 ` Loke, Chetan 0 siblings, 0 replies; 59+ messages in thread From: Loke, Chetan @ 2011-07-20 15:23 UTC (permalink / raw) To: Rick Jones, Neil Horman Cc: Eric Dumazet, Jiri Pirko, netdev, Alexey Dobriyan, David S. Miller > -----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. ^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH] pktgen: Clone skb to avoid corruption of skbs in ndo_start_xmit methods 2011-07-20 0:19 ` Neil Horman 2011-07-20 0:25 ` Rick Jones @ 2011-07-20 0:43 ` Eric Dumazet 2011-07-20 0:52 ` Ben Greear 2011-07-20 1:59 ` Neil Horman 1 sibling, 2 replies; 59+ messages in thread From: Eric Dumazet @ 2011-07-20 0:43 UTC (permalink / raw) To: Neil Horman; +Cc: Jiri Pirko, netdev, Alexey Dobriyan, David S. Miller 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() ^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH] pktgen: Clone skb to avoid corruption of skbs in ndo_start_xmit methods 2011-07-20 0:43 ` Eric Dumazet @ 2011-07-20 0:52 ` Ben Greear 2011-07-20 2:07 ` Neil Horman 2011-07-20 1:59 ` Neil Horman 1 sibling, 1 reply; 59+ messages in thread From: Ben Greear @ 2011-07-20 0:52 UTC (permalink / raw) To: Eric Dumazet Cc: Neil Horman, Jiri Pirko, netdev, Alexey Dobriyan, David S. Miller 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 ^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH] pktgen: Clone skb to avoid corruption of skbs in ndo_start_xmit methods 2011-07-20 0:52 ` Ben Greear @ 2011-07-20 2:07 ` Neil Horman 2011-07-20 4:19 ` Ben Greear 2011-07-20 4:24 ` Eric Dumazet 0 siblings, 2 replies; 59+ messages in thread From: Neil Horman @ 2011-07-20 2:07 UTC (permalink / raw) To: Ben Greear Cc: Eric Dumazet, Jiri Pirko, netdev, Alexey Dobriyan, David S. Miller 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 ^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH] pktgen: Clone skb to avoid corruption of skbs in ndo_start_xmit methods 2011-07-20 2:07 ` Neil Horman @ 2011-07-20 4:19 ` Ben Greear 2011-07-20 4:24 ` Eric Dumazet 1 sibling, 0 replies; 59+ messages in thread From: Ben Greear @ 2011-07-20 4:19 UTC (permalink / raw) To: Neil Horman Cc: Eric Dumazet, Jiri Pirko, netdev, Alexey Dobriyan, David S. Miller 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 ^ permalink raw reply related [flat|nested] 59+ messages in thread
* Re: [PATCH] pktgen: Clone skb to avoid corruption of skbs in ndo_start_xmit methods 2011-07-20 2:07 ` Neil Horman 2011-07-20 4:19 ` Ben Greear @ 2011-07-20 4:24 ` Eric Dumazet 2011-07-20 15:17 ` Loke, Chetan 2011-07-20 15:18 ` Neil Horman 1 sibling, 2 replies; 59+ messages in thread From: Eric Dumazet @ 2011-07-20 4:24 UTC (permalink / raw) To: Neil Horman Cc: Ben Greear, Jiri Pirko, netdev, Alexey Dobriyan, David S. Miller 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... ^ permalink raw reply [flat|nested] 59+ messages in thread
* RE: [PATCH] pktgen: Clone skb to avoid corruption of skbs in ndo_start_xmit methods 2011-07-20 4:24 ` Eric Dumazet @ 2011-07-20 15:17 ` Loke, Chetan 2011-07-20 15:18 ` Neil Horman 1 sibling, 0 replies; 59+ messages in thread From: Loke, Chetan @ 2011-07-20 15:17 UTC (permalink / raw) To: Eric Dumazet, Neil Horman Cc: Ben Greear, Jiri Pirko, netdev, Alexey Dobriyan, David S. Miller > 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. ^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH] pktgen: Clone skb to avoid corruption of skbs in ndo_start_xmit methods 2011-07-20 4:24 ` Eric Dumazet 2011-07-20 15:17 ` Loke, Chetan @ 2011-07-20 15:18 ` Neil Horman 2011-07-20 15:30 ` Eric Dumazet ` (3 more replies) 1 sibling, 4 replies; 59+ messages in thread From: Neil Horman @ 2011-07-20 15:18 UTC (permalink / raw) To: Eric Dumazet Cc: Ben Greear, Jiri Pirko, netdev, Alexey Dobriyan, David S. Miller, robert.olsson 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; ^ permalink raw reply related [flat|nested] 59+ messages in thread
* Re: [PATCH] pktgen: Clone skb to avoid corruption of skbs in ndo_start_xmit methods 2011-07-20 15:18 ` Neil Horman @ 2011-07-20 15:30 ` Eric Dumazet 2011-07-20 15:39 ` Neil Horman 2011-07-20 15:35 ` Michał Mirosław ` (2 subsequent siblings) 3 siblings, 1 reply; 59+ messages in thread From: Eric Dumazet @ 2011-07-20 15:30 UTC (permalink / raw) To: Neil Horman Cc: Ben Greear, Jiri Pirko, netdev, Alexey Dobriyan, David S. Miller, robert.olsson 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. ^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH] pktgen: Clone skb to avoid corruption of skbs in ndo_start_xmit methods 2011-07-20 15:30 ` Eric Dumazet @ 2011-07-20 15:39 ` Neil Horman 2011-07-20 15:44 ` Eric Dumazet 0 siblings, 1 reply; 59+ messages in thread From: Neil Horman @ 2011-07-20 15:39 UTC (permalink / raw) To: Eric Dumazet Cc: Ben Greear, Jiri Pirko, netdev, Alexey Dobriyan, David S. Miller, robert.olsson 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 > > ^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH] pktgen: Clone skb to avoid corruption of skbs in ndo_start_xmit methods 2011-07-20 15:39 ` Neil Horman @ 2011-07-20 15:44 ` Eric Dumazet 2011-07-20 16:19 ` Neil Horman 0 siblings, 1 reply; 59+ messages in thread From: Eric Dumazet @ 2011-07-20 15:44 UTC (permalink / raw) To: Neil Horman Cc: Ben Greear, Jiri Pirko, netdev, Alexey Dobriyan, David S. Miller, robert.olsson 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 ;) ^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH] pktgen: Clone skb to avoid corruption of skbs in ndo_start_xmit methods 2011-07-20 15:44 ` Eric Dumazet @ 2011-07-20 16:19 ` Neil Horman 0 siblings, 0 replies; 59+ messages in thread From: Neil Horman @ 2011-07-20 16:19 UTC (permalink / raw) To: Eric Dumazet Cc: Ben Greear, Jiri Pirko, netdev, Alexey Dobriyan, David S. Miller, robert.olsson 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 ^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH] pktgen: Clone skb to avoid corruption of skbs in ndo_start_xmit methods 2011-07-20 15:18 ` Neil Horman 2011-07-20 15:30 ` Eric Dumazet @ 2011-07-20 15:35 ` Michał Mirosław 2011-07-20 15:40 ` Neil Horman 2011-07-20 16:37 ` Ben Greear 2011-07-20 16:33 ` Ben Greear 2011-07-21 22:01 ` David Miller 3 siblings, 2 replies; 59+ messages in thread From: Michał Mirosław @ 2011-07-20 15:35 UTC (permalink / raw) To: Neil Horman Cc: Eric Dumazet, Ben Greear, Jiri Pirko, netdev, Alexey Dobriyan, David S. Miller, robert.olsson 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 ^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH] pktgen: Clone skb to avoid corruption of skbs in ndo_start_xmit methods 2011-07-20 15:35 ` Michał Mirosław @ 2011-07-20 15:40 ` Neil Horman 2011-07-20 16:08 ` Michał Mirosław 2011-07-20 16:37 ` Ben Greear 1 sibling, 1 reply; 59+ messages in thread From: Neil Horman @ 2011-07-20 15:40 UTC (permalink / raw) To: Michał Mirosław Cc: Eric Dumazet, Ben Greear, Jiri Pirko, netdev, Alexey Dobriyan, David S. Miller, robert.olsson 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 > ^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH] pktgen: Clone skb to avoid corruption of skbs in ndo_start_xmit methods 2011-07-20 15:40 ` Neil Horman @ 2011-07-20 16:08 ` Michał Mirosław 2011-07-20 16:18 ` Neil Horman 0 siblings, 1 reply; 59+ messages in thread From: Michał Mirosław @ 2011-07-20 16:08 UTC (permalink / raw) To: Neil Horman Cc: Eric Dumazet, Ben Greear, Jiri Pirko, netdev, Alexey Dobriyan, David S. Miller, robert.olsson 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 ^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH] pktgen: Clone skb to avoid corruption of skbs in ndo_start_xmit methods 2011-07-20 16:08 ` Michał Mirosław @ 2011-07-20 16:18 ` Neil Horman 0 siblings, 0 replies; 59+ messages in thread From: Neil Horman @ 2011-07-20 16:18 UTC (permalink / raw) To: Michał Mirosław Cc: Eric Dumazet, Ben Greear, Jiri Pirko, netdev, Alexey Dobriyan, David S. Miller, robert.olsson 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 > ^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH] pktgen: Clone skb to avoid corruption of skbs in ndo_start_xmit methods 2011-07-20 15:35 ` Michał Mirosław 2011-07-20 15:40 ` Neil Horman @ 2011-07-20 16:37 ` Ben Greear 1 sibling, 0 replies; 59+ messages in thread From: Ben Greear @ 2011-07-20 16:37 UTC (permalink / raw) To: Michał Mirosław Cc: Neil Horman, Eric Dumazet, Jiri Pirko, netdev, Alexey Dobriyan, David S. Miller, robert.olsson 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 ^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH] pktgen: Clone skb to avoid corruption of skbs in ndo_start_xmit methods 2011-07-20 15:18 ` Neil Horman 2011-07-20 15:30 ` Eric Dumazet 2011-07-20 15:35 ` Michał Mirosław @ 2011-07-20 16:33 ` Ben Greear 2011-07-20 16:36 ` Neil Horman 2011-07-21 22:01 ` David Miller 3 siblings, 1 reply; 59+ messages in thread From: Ben Greear @ 2011-07-20 16:33 UTC (permalink / raw) To: Neil Horman Cc: Eric Dumazet, Jiri Pirko, netdev, Alexey Dobriyan, David S. Miller, robert.olsson 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 ^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH] pktgen: Clone skb to avoid corruption of skbs in ndo_start_xmit methods 2011-07-20 16:33 ` Ben Greear @ 2011-07-20 16:36 ` Neil Horman 0 siblings, 0 replies; 59+ messages in thread From: Neil Horman @ 2011-07-20 16:36 UTC (permalink / raw) To: Ben Greear Cc: Eric Dumazet, Jiri Pirko, netdev, Alexey Dobriyan, David S. Miller, robert.olsson 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 > > ^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH] pktgen: Clone skb to avoid corruption of skbs in ndo_start_xmit methods 2011-07-20 15:18 ` Neil Horman ` (2 preceding siblings ...) 2011-07-20 16:33 ` Ben Greear @ 2011-07-21 22:01 ` David Miller 2011-07-21 22:14 ` Ben Greear 3 siblings, 1 reply; 59+ messages in thread From: David Miller @ 2011-07-21 22:01 UTC (permalink / raw) To: nhorman; +Cc: eric.dumazet, greearb, jpirko, netdev, adobriyan, robert.olsson 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. ^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH] pktgen: Clone skb to avoid corruption of skbs in ndo_start_xmit methods 2011-07-21 22:01 ` David Miller @ 2011-07-21 22:14 ` Ben Greear 2011-07-21 22:19 ` David Miller 0 siblings, 1 reply; 59+ messages in thread From: Ben Greear @ 2011-07-21 22:14 UTC (permalink / raw) To: David Miller Cc: nhorman, eric.dumazet, jpirko, netdev, adobriyan, robert.olsson 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 ^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH] pktgen: Clone skb to avoid corruption of skbs in ndo_start_xmit methods 2011-07-21 22:14 ` Ben Greear @ 2011-07-21 22:19 ` David Miller 2011-07-21 22:26 ` Ben Greear 2011-07-21 23:50 ` Neil Horman 0 siblings, 2 replies; 59+ messages in thread From: David Miller @ 2011-07-21 22:19 UTC (permalink / raw) To: greearb; +Cc: nhorman, eric.dumazet, jpirko, netdev, adobriyan, robert.olsson 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. ^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH] pktgen: Clone skb to avoid corruption of skbs in ndo_start_xmit methods 2011-07-21 22:19 ` David Miller @ 2011-07-21 22:26 ` Ben Greear 2011-07-21 23:50 ` Neil Horman 1 sibling, 0 replies; 59+ messages in thread From: Ben Greear @ 2011-07-21 22:26 UTC (permalink / raw) To: David Miller Cc: nhorman, eric.dumazet, jpirko, netdev, adobriyan, robert.olsson 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 ^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH] pktgen: Clone skb to avoid corruption of skbs in ndo_start_xmit methods 2011-07-21 22:19 ` David Miller 2011-07-21 22:26 ` Ben Greear @ 2011-07-21 23:50 ` Neil Horman 2011-07-22 0:08 ` David Miller 1 sibling, 1 reply; 59+ messages in thread From: Neil Horman @ 2011-07-21 23:50 UTC (permalink / raw) To: David Miller Cc: greearb, eric.dumazet, jpirko, netdev, adobriyan, robert.olsson 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 ^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH] pktgen: Clone skb to avoid corruption of skbs in ndo_start_xmit methods 2011-07-21 23:50 ` Neil Horman @ 2011-07-22 0:08 ` David Miller 2011-07-22 1:37 ` Neil Horman 0 siblings, 1 reply; 59+ messages in thread From: David Miller @ 2011-07-22 0:08 UTC (permalink / raw) To: nhorman; +Cc: greearb, eric.dumazet, jpirko, netdev, adobriyan, robert.olsson 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. ^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH] pktgen: Clone skb to avoid corruption of skbs in ndo_start_xmit methods 2011-07-22 0:08 ` David Miller @ 2011-07-22 1:37 ` Neil Horman 0 siblings, 0 replies; 59+ messages in thread From: Neil Horman @ 2011-07-22 1:37 UTC (permalink / raw) To: David Miller Cc: greearb, eric.dumazet, jpirko, netdev, adobriyan, robert.olsson 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 ^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH] pktgen: Clone skb to avoid corruption of skbs in ndo_start_xmit methods 2011-07-20 0:43 ` Eric Dumazet 2011-07-20 0:52 ` Ben Greear @ 2011-07-20 1:59 ` Neil Horman 1 sibling, 0 replies; 59+ messages in thread From: Neil Horman @ 2011-07-20 1:59 UTC (permalink / raw) To: Eric Dumazet; +Cc: Jiri Pirko, netdev, Alexey Dobriyan, David S. Miller 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) ^ permalink raw reply [flat|nested] 59+ messages in thread
* [PATCH 0/2] pktgen: Clone skb to avoid corruption of skbs in ndo_start_xmit methods (v2) 2011-07-19 19:52 [PATCH] pktgen: Clone skb to avoid corruption of skbs in ndo_start_xmit methods Neil Horman 2011-07-19 20:02 ` Eric Dumazet @ 2011-07-25 19:45 ` Neil Horman 2011-07-25 19:45 ` [PATCH 1/2] net: add IFF_SKB_TX_SHARED flag to priv_flags Neil Horman 2011-07-25 19:45 ` [PATCH 2/2] net: Audit drivers to identify those needing IFF_TX_SKB_SHARING cleared Neil Horman 2011-07-26 16:05 ` [PATCH 0/2] pktgen: Clone skb to avoid corruption of skbs in ndo_start_xmit methods (v3) Neil Horman 2 siblings, 2 replies; 59+ messages in thread From: Neil Horman @ 2011-07-25 19:45 UTC (permalink / raw) To: netdev 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 ^ permalink raw reply [flat|nested] 59+ messages in thread
* [PATCH 1/2] net: add IFF_SKB_TX_SHARED flag to priv_flags 2011-07-25 19:45 ` [PATCH 0/2] pktgen: Clone skb to avoid corruption of skbs in ndo_start_xmit methods (v2) Neil Horman @ 2011-07-25 19:45 ` Neil Horman 2011-07-25 20:11 ` Eric Dumazet 2011-07-25 19:45 ` [PATCH 2/2] net: Audit drivers to identify those needing IFF_TX_SKB_SHARING cleared Neil Horman 1 sibling, 1 reply; 59+ messages in thread From: Neil Horman @ 2011-07-25 19:45 UTC (permalink / raw) To: netdev Cc: Neil Horman, Robert Olsson, Eric Dumazet, Alexey Dobriyan, David S. Miller 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 ^ permalink raw reply related [flat|nested] 59+ messages in thread
* Re: [PATCH 1/2] net: add IFF_SKB_TX_SHARED flag to priv_flags 2011-07-25 19:45 ` [PATCH 1/2] net: add IFF_SKB_TX_SHARED flag to priv_flags Neil Horman @ 2011-07-25 20:11 ` Eric Dumazet 2011-07-26 14:54 ` Neil Horman 0 siblings, 1 reply; 59+ messages in thread From: Eric Dumazet @ 2011-07-25 20:11 UTC (permalink / raw) To: Neil Horman; +Cc: netdev, Robert Olsson, Alexey Dobriyan, David S. Miller 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; > ^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH 1/2] net: add IFF_SKB_TX_SHARED flag to priv_flags 2011-07-25 20:11 ` Eric Dumazet @ 2011-07-26 14:54 ` Neil Horman 0 siblings, 0 replies; 59+ messages in thread From: Neil Horman @ 2011-07-26 14:54 UTC (permalink / raw) To: Eric Dumazet; +Cc: netdev, Robert Olsson, Alexey Dobriyan, David S. Miller 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 ^ permalink raw reply [flat|nested] 59+ messages in thread
* [PATCH 2/2] net: Audit drivers to identify those needing IFF_TX_SKB_SHARING cleared 2011-07-25 19:45 ` [PATCH 0/2] pktgen: Clone skb to avoid corruption of skbs in ndo_start_xmit methods (v2) Neil Horman 2011-07-25 19:45 ` [PATCH 1/2] net: add IFF_SKB_TX_SHARED flag to priv_flags Neil Horman @ 2011-07-25 19:45 ` Neil Horman 2011-07-26 18:34 ` Krzysztof Halasa 1 sibling, 1 reply; 59+ messages in thread From: Neil Horman @ 2011-07-25 19:45 UTC (permalink / raw) To: netdev Cc: Neil Horman, Karsten Keil, David S. Miller, Jay Vosburgh, Andy Gospodarek, Patrick McHardy, Krzysztof Halasa, John W. Linville, Greg Kroah-Hartman, Marcel Holtmann, Johannes Berg 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 ^ permalink raw reply related [flat|nested] 59+ messages in thread
* Re: [PATCH 2/2] net: Audit drivers to identify those needing IFF_TX_SKB_SHARING cleared 2011-07-25 19:45 ` [PATCH 2/2] net: Audit drivers to identify those needing IFF_TX_SKB_SHARING cleared Neil Horman @ 2011-07-26 18:34 ` Krzysztof Halasa 0 siblings, 0 replies; 59+ messages in thread From: Krzysztof Halasa @ 2011-07-26 18:34 UTC (permalink / raw) To: Neil Horman Cc: netdev, Karsten Keil, David S. Miller, Jay Vosburgh, Andy Gospodarek, Patrick McHardy, John W. Linville, Greg Kroah-Hartman, Marcel Holtmann, Johannes Berg 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 ^ permalink raw reply [flat|nested] 59+ messages in thread
* [PATCH 0/2] pktgen: Clone skb to avoid corruption of skbs in ndo_start_xmit methods (v3) 2011-07-19 19:52 [PATCH] pktgen: Clone skb to avoid corruption of skbs in ndo_start_xmit methods Neil Horman 2011-07-19 20:02 ` Eric Dumazet 2011-07-25 19:45 ` [PATCH 0/2] pktgen: Clone skb to avoid corruption of skbs in ndo_start_xmit methods (v2) Neil Horman @ 2011-07-26 16:05 ` Neil Horman 2011-07-26 16:05 ` [PATCH 1/2] net: add IFF_SKB_TX_SHARED flag to priv_flags Neil Horman ` (3 more replies) 2 siblings, 4 replies; 59+ messages in thread From: Neil Horman @ 2011-07-26 16:05 UTC (permalink / raw) To: netdev 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 ^ permalink raw reply [flat|nested] 59+ messages in thread
* [PATCH 1/2] net: add IFF_SKB_TX_SHARED flag to priv_flags 2011-07-26 16:05 ` [PATCH 0/2] pktgen: Clone skb to avoid corruption of skbs in ndo_start_xmit methods (v3) Neil Horman @ 2011-07-26 16:05 ` Neil Horman 2011-07-28 5:42 ` David Miller 2011-07-28 8:15 ` Robert Olsson 2011-07-26 16:05 ` [PATCH 2/2] net: Audit drivers to identify those needing IFF_TX_SKB_SHARING cleared Neil Horman ` (2 subsequent siblings) 3 siblings, 2 replies; 59+ messages in thread From: Neil Horman @ 2011-07-26 16:05 UTC (permalink / raw) To: netdev Cc: Neil Horman, Robert Olsson, Eric Dumazet, Alexey Dobriyan, David S. Miller 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 ^ permalink raw reply related [flat|nested] 59+ messages in thread
* Re: [PATCH 1/2] net: add IFF_SKB_TX_SHARED flag to priv_flags 2011-07-26 16:05 ` [PATCH 1/2] net: add IFF_SKB_TX_SHARED flag to priv_flags Neil Horman @ 2011-07-28 5:42 ` David Miller 2011-07-28 8:15 ` Robert Olsson 1 sibling, 0 replies; 59+ messages in thread From: David Miller @ 2011-07-28 5:42 UTC (permalink / raw) To: nhorman; +Cc: netdev, robert.olsson, eric.dumazet, adobriyan 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. ^ permalink raw reply [flat|nested] 59+ messages in thread
* [PATCH 1/2] net: add IFF_SKB_TX_SHARED flag to priv_flags 2011-07-26 16:05 ` [PATCH 1/2] net: add IFF_SKB_TX_SHARED flag to priv_flags Neil Horman 2011-07-28 5:42 ` David Miller @ 2011-07-28 8:15 ` Robert Olsson 2011-07-28 10:50 ` Neil Horman 1 sibling, 1 reply; 59+ messages in thread From: Robert Olsson @ 2011-07-28 8:15 UTC (permalink / raw) To: Neil Horman Cc: netdev, Robert Olsson, Eric Dumazet, Alexey Dobriyan, David S. Miller 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 -- ^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH 1/2] net: add IFF_SKB_TX_SHARED flag to priv_flags 2011-07-28 8:15 ` Robert Olsson @ 2011-07-28 10:50 ` Neil Horman 0 siblings, 0 replies; 59+ messages in thread From: Neil Horman @ 2011-07-28 10:50 UTC (permalink / raw) To: Robert Olsson Cc: netdev, Robert Olsson, Eric Dumazet, Alexey Dobriyan, David S. Miller 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 ^ permalink raw reply [flat|nested] 59+ messages in thread
* [PATCH 2/2] net: Audit drivers to identify those needing IFF_TX_SKB_SHARING cleared 2011-07-26 16:05 ` [PATCH 0/2] pktgen: Clone skb to avoid corruption of skbs in ndo_start_xmit methods (v3) Neil Horman 2011-07-26 16:05 ` [PATCH 1/2] net: add IFF_SKB_TX_SHARED flag to priv_flags Neil Horman @ 2011-07-26 16:05 ` Neil Horman 2011-07-28 5:42 ` David Miller 2011-07-29 6:16 ` [PATCH 0/2] pktgen: Clone skb to avoid corruption of skbs in ndo_start_xmit methods (v3) Michael S. Tsirkin 2011-08-17 15:07 ` Ben Hutchings 3 siblings, 1 reply; 59+ messages in thread From: Neil Horman @ 2011-07-26 16:05 UTC (permalink / raw) To: netdev Cc: Neil Horman, Karsten Keil, David S. Miller, Jay Vosburgh, Andy Gospodarek, Patrick McHardy, Krzysztof Halasa, John W. Linville, Greg Kroah-Hartman, Marcel Holtmann, Johannes Berg 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 ^ permalink raw reply related [flat|nested] 59+ messages in thread
* Re: [PATCH 2/2] net: Audit drivers to identify those needing IFF_TX_SKB_SHARING cleared 2011-07-26 16:05 ` [PATCH 2/2] net: Audit drivers to identify those needing IFF_TX_SKB_SHARING cleared Neil Horman @ 2011-07-28 5:42 ` David Miller 0 siblings, 0 replies; 59+ messages in thread From: David Miller @ 2011-07-28 5:42 UTC (permalink / raw) To: nhorman Cc: netdev, isdn, fubar, andy, kaber, khc, linville, gregkh, marcel, johannes 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. ^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH 0/2] pktgen: Clone skb to avoid corruption of skbs in ndo_start_xmit methods (v3) 2011-07-26 16:05 ` [PATCH 0/2] pktgen: Clone skb to avoid corruption of skbs in ndo_start_xmit methods (v3) Neil Horman 2011-07-26 16:05 ` [PATCH 1/2] net: add IFF_SKB_TX_SHARED flag to priv_flags Neil Horman 2011-07-26 16:05 ` [PATCH 2/2] net: Audit drivers to identify those needing IFF_TX_SKB_SHARING cleared Neil Horman @ 2011-07-29 6:16 ` Michael S. Tsirkin 2011-07-29 11:19 ` Neil Horman 2011-08-17 15:07 ` Ben Hutchings 3 siblings, 1 reply; 59+ messages in thread From: Michael S. Tsirkin @ 2011-07-29 6:16 UTC (permalink / raw) To: Neil Horman, mashirle; +Cc: netdev 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 ^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH 0/2] pktgen: Clone skb to avoid corruption of skbs in ndo_start_xmit methods (v3) 2011-07-29 6:16 ` [PATCH 0/2] pktgen: Clone skb to avoid corruption of skbs in ndo_start_xmit methods (v3) Michael S. Tsirkin @ 2011-07-29 11:19 ` Neil Horman 0 siblings, 0 replies; 59+ messages in thread From: Neil Horman @ 2011-07-29 11:19 UTC (permalink / raw) To: Michael S. Tsirkin; +Cc: mashirle, netdev 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 > ^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH 0/2] pktgen: Clone skb to avoid corruption of skbs in ndo_start_xmit methods (v3) 2011-07-26 16:05 ` [PATCH 0/2] pktgen: Clone skb to avoid corruption of skbs in ndo_start_xmit methods (v3) Neil Horman ` (2 preceding siblings ...) 2011-07-29 6:16 ` [PATCH 0/2] pktgen: Clone skb to avoid corruption of skbs in ndo_start_xmit methods (v3) Michael S. Tsirkin @ 2011-08-17 15:07 ` Ben Hutchings 2011-08-22 0:27 ` Neil Horman 3 siblings, 1 reply; 59+ messages in thread From: Ben Hutchings @ 2011-08-17 15:07 UTC (permalink / raw) To: Neil Horman; +Cc: netdev 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. ^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH 0/2] pktgen: Clone skb to avoid corruption of skbs in ndo_start_xmit methods (v3) 2011-08-17 15:07 ` Ben Hutchings @ 2011-08-22 0:27 ` Neil Horman 2011-08-22 16:17 ` Ben Hutchings 0 siblings, 1 reply; 59+ messages in thread From: Neil Horman @ 2011-08-22 0:27 UTC (permalink / raw) To: Ben Hutchings; +Cc: netdev 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. > > ^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH 0/2] pktgen: Clone skb to avoid corruption of skbs in ndo_start_xmit methods (v3) 2011-08-22 0:27 ` Neil Horman @ 2011-08-22 16:17 ` Ben Hutchings 2011-08-22 17:33 ` Ben Hutchings 2011-08-22 18:14 ` Neil Horman 0 siblings, 2 replies; 59+ messages in thread From: Ben Hutchings @ 2011-08-22 16:17 UTC (permalink / raw) To: Neil Horman; +Cc: netdev 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. ^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH 0/2] pktgen: Clone skb to avoid corruption of skbs in ndo_start_xmit methods (v3) 2011-08-22 16:17 ` Ben Hutchings @ 2011-08-22 17:33 ` Ben Hutchings 2011-08-22 18:14 ` Neil Horman 1 sibling, 0 replies; 59+ messages in thread From: Ben Hutchings @ 2011-08-22 17:33 UTC (permalink / raw) To: Neil Horman; +Cc: netdev 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. ^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH 0/2] pktgen: Clone skb to avoid corruption of skbs in ndo_start_xmit methods (v3) 2011-08-22 16:17 ` Ben Hutchings 2011-08-22 17:33 ` Ben Hutchings @ 2011-08-22 18:14 ` Neil Horman 2011-08-23 14:01 ` Ben Hutchings 1 sibling, 1 reply; 59+ messages in thread From: Neil Horman @ 2011-08-22 18:14 UTC (permalink / raw) To: Ben Hutchings; +Cc: netdev 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 ^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH 0/2] pktgen: Clone skb to avoid corruption of skbs in ndo_start_xmit methods (v3) 2011-08-22 18:14 ` Neil Horman @ 2011-08-23 14:01 ` Ben Hutchings 2011-08-23 15:13 ` Neil Horman 0 siblings, 1 reply; 59+ messages in thread From: Ben Hutchings @ 2011-08-23 14:01 UTC (permalink / raw) To: Neil Horman; +Cc: netdev 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. ^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH 0/2] pktgen: Clone skb to avoid corruption of skbs in ndo_start_xmit methods (v3) 2011-08-23 14:01 ` Ben Hutchings @ 2011-08-23 15:13 ` Neil Horman 2011-08-23 15:53 ` Ben Hutchings 0 siblings, 1 reply; 59+ messages in thread From: Neil Horman @ 2011-08-23 15:13 UTC (permalink / raw) To: Ben Hutchings; +Cc: netdev 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. > > ^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH 0/2] pktgen: Clone skb to avoid corruption of skbs in ndo_start_xmit methods (v3) 2011-08-23 15:13 ` Neil Horman @ 2011-08-23 15:53 ` Ben Hutchings 2011-08-23 16:21 ` Neil Horman 0 siblings, 1 reply; 59+ messages in thread From: Ben Hutchings @ 2011-08-23 15:53 UTC (permalink / raw) To: Neil Horman; +Cc: netdev 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. ^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH 0/2] pktgen: Clone skb to avoid corruption of skbs in ndo_start_xmit methods (v3) 2011-08-23 15:53 ` Ben Hutchings @ 2011-08-23 16:21 ` Neil Horman 0 siblings, 0 replies; 59+ messages in thread From: Neil Horman @ 2011-08-23 16:21 UTC (permalink / raw) To: Ben Hutchings; +Cc: netdev 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. > > ^ permalink raw reply [flat|nested] 59+ messages in thread
end of thread, other threads:[~2011-08-23 16:21 UTC | newest] Thread overview: 59+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2011-07-19 19:52 [PATCH] pktgen: Clone skb to avoid corruption of skbs in ndo_start_xmit methods Neil Horman 2011-07-19 20:02 ` Eric Dumazet 2011-07-19 20:17 ` Joe Perches 2011-07-19 20:29 ` Jiri Pirko 2011-07-19 20:41 ` Eric Dumazet 2011-07-19 21:01 ` Ben Greear 2011-07-20 0:19 ` Neil Horman 2011-07-20 0:25 ` Rick Jones 2011-07-20 15:23 ` Loke, Chetan 2011-07-20 0:43 ` Eric Dumazet 2011-07-20 0:52 ` Ben Greear 2011-07-20 2:07 ` Neil Horman 2011-07-20 4:19 ` Ben Greear 2011-07-20 4:24 ` Eric Dumazet 2011-07-20 15:17 ` Loke, Chetan 2011-07-20 15:18 ` Neil Horman 2011-07-20 15:30 ` Eric Dumazet 2011-07-20 15:39 ` Neil Horman 2011-07-20 15:44 ` Eric Dumazet 2011-07-20 16:19 ` Neil Horman 2011-07-20 15:35 ` Michał Mirosław 2011-07-20 15:40 ` Neil Horman 2011-07-20 16:08 ` Michał Mirosław 2011-07-20 16:18 ` Neil Horman 2011-07-20 16:37 ` Ben Greear 2011-07-20 16:33 ` Ben Greear 2011-07-20 16:36 ` Neil Horman 2011-07-21 22:01 ` David Miller 2011-07-21 22:14 ` Ben Greear 2011-07-21 22:19 ` David Miller 2011-07-21 22:26 ` Ben Greear 2011-07-21 23:50 ` Neil Horman 2011-07-22 0:08 ` David Miller 2011-07-22 1:37 ` Neil Horman 2011-07-20 1:59 ` Neil Horman 2011-07-25 19:45 ` [PATCH 0/2] pktgen: Clone skb to avoid corruption of skbs in ndo_start_xmit methods (v2) Neil Horman 2011-07-25 19:45 ` [PATCH 1/2] net: add IFF_SKB_TX_SHARED flag to priv_flags Neil Horman 2011-07-25 20:11 ` Eric Dumazet 2011-07-26 14:54 ` Neil Horman 2011-07-25 19:45 ` [PATCH 2/2] net: Audit drivers to identify those needing IFF_TX_SKB_SHARING cleared Neil Horman 2011-07-26 18:34 ` Krzysztof Halasa 2011-07-26 16:05 ` [PATCH 0/2] pktgen: Clone skb to avoid corruption of skbs in ndo_start_xmit methods (v3) Neil Horman 2011-07-26 16:05 ` [PATCH 1/2] net: add IFF_SKB_TX_SHARED flag to priv_flags Neil Horman 2011-07-28 5:42 ` David Miller 2011-07-28 8:15 ` Robert Olsson 2011-07-28 10:50 ` Neil Horman 2011-07-26 16:05 ` [PATCH 2/2] net: Audit drivers to identify those needing IFF_TX_SKB_SHARING cleared Neil Horman 2011-07-28 5:42 ` David Miller 2011-07-29 6:16 ` [PATCH 0/2] pktgen: Clone skb to avoid corruption of skbs in ndo_start_xmit methods (v3) Michael S. Tsirkin 2011-07-29 11:19 ` Neil Horman 2011-08-17 15:07 ` Ben Hutchings 2011-08-22 0:27 ` Neil Horman 2011-08-22 16:17 ` Ben Hutchings 2011-08-22 17:33 ` Ben Hutchings 2011-08-22 18:14 ` Neil Horman 2011-08-23 14:01 ` Ben Hutchings 2011-08-23 15:13 ` Neil Horman 2011-08-23 15:53 ` Ben Hutchings 2011-08-23 16:21 ` Neil Horman
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.