* [PATCH] pktgen: nowait parameter. @ 2014-09-03 4:20 Rusty Russell 2014-09-03 9:09 ` Jesper Dangaard Brouer 2014-09-05 21:26 ` David Miller 0 siblings, 2 replies; 7+ messages in thread From: Rusty Russell @ 2014-09-03 4:20 UTC (permalink / raw) To: netdev; +Cc: Jesper Dangaard Brouer, Mathias Krause While trying to measure speed of virtio_net, I was getting hangs. This is because we skb_orphan() but delay the tx interrupt indefinitely (by number of slots). With nowait, pktgen won't wait for the skb to be released. This introduces an error, but it's ok if count >> ringsize. I updated the documentation, but it needs far more work (it refers to pgset and an examples directory, none of which exist in the kernel tree). Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> diff --git a/Documentation/networking/pktgen.txt b/Documentation/networking/pktgen.txt index 0dffc6e37902..48d86da74a39 100644 --- a/Documentation/networking/pktgen.txt +++ b/Documentation/networking/pktgen.txt @@ -41,10 +41,13 @@ NIC HW layer (which is bad for bufferbloat). One should be careful to conclude, that packets/descriptors in the HW TX ring cause delay. Drivers usually delay cleaning up the ring-buffers (for various performance reasons), thus packets stalling -the TX ring, might just be waiting for cleanup. +the TX ring, might just be waiting for cleanup. Writing the "nowait" +parameter into /proc/net/pktgen/ethX will avoid waiting for cleanup of +the final packets, introducing a slight error (tiny if the count of +packets being sent is much greater than the ring size of the device). -This cleanup issues is specifically the case, for the driver ixgbe -(Intel 82599 chip). This driver (ixgbe) combine TX+RX ring cleanups, +Alternately, some drivers (eg ixgbe for the Intel 82599 chip) can +have their cleanup interval changed. ixgbe combines TX+RX ring cleanups, and the cleanup interval is affected by the ethtool --coalesce setting of parameter "rx-usecs". @@ -303,6 +305,8 @@ flowlen rate ratep +nowait + References: ftp://robur.slu.se/pub/Linux/net-development/pktgen-testing/ ftp://robur.slu.se/pub/Linux/net-development/pktgen-testing/examples/ diff --git a/net/core/pktgen.c b/net/core/pktgen.c index 8b849ddfef2e..adc41f2b3bc7 100644 --- a/net/core/pktgen.c +++ b/net/core/pktgen.c @@ -290,6 +290,11 @@ struct pktgen_dev { * set clone_skb to 1024. */ + bool no_wait; /* + * Don't wait for packet to be freed + * by driver + */ + char dst_min[IP_NAME_SZ]; /* IP, ie 1.2.3.4 */ char dst_max[IP_NAME_SZ]; /* IP, ie 1.2.3.4 */ char src_min[IP_NAME_SZ]; /* IP, ie 1.2.3.4 */ @@ -679,6 +684,9 @@ static int pktgen_if_show(struct seq_file *seq, void *v) seq_puts(seq, "\n"); + if (pkt_dev->no_wait) + seq_puts(seq, " nowait\n"); + /* not really stopped, more like last-running-at */ stopped = pkt_dev->running ? ktime_get() : pkt_dev->stopped_at; idle = pkt_dev->idle_acc; @@ -1711,6 +1719,17 @@ static ssize_t pktgen_if_write(struct file *file, return count; } + if (!strcmp(name, "nowait")) { + len = num_arg(&user_buffer[i], 10, &value); + if (len < 0) + return len; + + i += len; + pkt_dev->no_wait = value; + sprintf(pg_result, "OK: nowait=%u", pkt_dev->no_wait); + return count; + } + sprintf(pkt_dev->result, "No such parameter \"%s\"", name); return -EINVAL; } @@ -3373,7 +3392,8 @@ unlock: /* If pkt_dev->count is zero, then run forever */ if ((pkt_dev->count != 0) && (pkt_dev->sofar >= pkt_dev->count)) { - pktgen_wait_for_skb(pkt_dev); + if (!pkt_dev->no_wait) + pktgen_wait_for_skb(pkt_dev); /* Done with this */ pktgen_stop_device(pkt_dev); @@ -3565,6 +3585,7 @@ static int pktgen_add_device(struct pktgen_thread *t, const char *ifname) pkt_dev->svlan_cfi = 0; pkt_dev->svlan_id = 0xffff; pkt_dev->node = -1; + pkt_dev->no_wait = false; err = pktgen_setup_dev(t->net, pkt_dev, ifname); if (err) ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] pktgen: nowait parameter. 2014-09-03 4:20 [PATCH] pktgen: nowait parameter Rusty Russell @ 2014-09-03 9:09 ` Jesper Dangaard Brouer 2014-09-05 1:49 ` Rusty Russell 2014-09-05 21:26 ` David Miller 1 sibling, 1 reply; 7+ messages in thread From: Jesper Dangaard Brouer @ 2014-09-03 9:09 UTC (permalink / raw) To: Rusty Russell; +Cc: netdev, Mathias Krause, brouer, Robert Olsson On Wed, 03 Sep 2014 13:50:01 +0930 Rusty Russell <rusty@rustcorp.com.au> wrote: > While trying to measure speed of virtio_net, I was getting hangs. > This is because we skb_orphan() but delay the tx interrupt > indefinitely (by number of slots). > > With nowait, pktgen won't wait for the skb to be released. This > introduces an error, but it's ok if count >> ringsize. This pktgen_wait_for_skb() only happens it the exit case, when count packets have been send. I guess its okay to proceed to pktgen_stop_device() which will call kfree_skb(pkt_dev->skb) with refcnt=2, decrementing to refcnt=1, and then we depend on driver to eventually call kfree_skb(). > I updated the documentation, but it needs far more work (it > refers to pgset and an examples directory, none of which exist > in the kernel tree). Yes, the doc is not in such a good shape. I'm not 100% happy with the name "nowait" parameter, as users could easily misunderstand the purpose of this parameter. But I've not come up with a better name, e.g. "exit_nowait" is also not the best. > diff --git a/net/core/pktgen.c b/net/core/pktgen.c > index 8b849ddfef2e..adc41f2b3bc7 100644 > --- a/net/core/pktgen.c > +++ b/net/core/pktgen.c > @@ -290,6 +290,11 @@ struct pktgen_dev { > * set clone_skb to 1024. > */ > > + bool no_wait; /* > + * Don't wait for packet to be freed > + * by driver > + */ > + DaveM prefers multi line comments like: /* Don't wait for packet to be freed * by driver */ > char dst_min[IP_NAME_SZ]; /* IP, ie 1.2.3.4 */ > char dst_max[IP_NAME_SZ]; /* IP, ie 1.2.3.4 */ > char src_min[IP_NAME_SZ]; /* IP, ie 1.2.3.4 */ > @@ -679,6 +684,9 @@ static int pktgen_if_show(struct seq_file *seq, void *v) > > seq_puts(seq, "\n"); > > + if (pkt_dev->no_wait) > + seq_puts(seq, " nowait\n"); > + Shouldn't you put this print statement above the "Flags:" section? > /* not really stopped, more like last-running-at */ > stopped = pkt_dev->running ? ktime_get() : pkt_dev->stopped_at; > idle = pkt_dev->idle_acc; > @@ -1711,6 +1719,17 @@ static ssize_t pktgen_if_write(struct file *file, -- Best regards, Jesper Dangaard Brouer MSc.CS, Sr. Network Kernel Developer at Red Hat Author of http://www.iptv-analyzer.org LinkedIn: http://www.linkedin.com/in/brouer ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] pktgen: nowait parameter. 2014-09-03 9:09 ` Jesper Dangaard Brouer @ 2014-09-05 1:49 ` Rusty Russell 2014-09-05 2:52 ` Jason Wang 0 siblings, 1 reply; 7+ messages in thread From: Rusty Russell @ 2014-09-05 1:49 UTC (permalink / raw) To: Jesper Dangaard Brouer; +Cc: netdev, Mathias Krause, brouer, Robert Olsson Jesper Dangaard Brouer <brouer@redhat.com> writes: > On Wed, 03 Sep 2014 13:50:01 +0930 > Rusty Russell <rusty@rustcorp.com.au> wrote: > >> While trying to measure speed of virtio_net, I was getting hangs. >> This is because we skb_orphan() but delay the tx interrupt >> indefinitely (by number of slots). >> >> With nowait, pktgen won't wait for the skb to be released. This >> introduces an error, but it's ok if count >> ringsize. > > This pktgen_wait_for_skb() only happens it the exit case, when count > packets have been send. I guess its okay to proceed to > pktgen_stop_device() which will call kfree_skb(pkt_dev->skb) with > refcnt=2, decrementing to refcnt=1, and then we depend on driver to > eventually call kfree_skb(). Yes, exactly. >> I updated the documentation, but it needs far more work (it >> refers to pgset and an examples directory, none of which exist >> in the kernel tree). > > Yes, the doc is not in such a good shape. > > I'm not 100% happy with the name "nowait" parameter, as users could > easily misunderstand the purpose of this parameter. But I've not come > up with a better name, e.g. "exit_nowait" is also not the best. Agreed. It could also be a flag, though that doesn't help with the name. > diff --git a/net/core/pktgen.c b/net/core/pktgen.c >> index 8b849ddfef2e..adc41f2b3bc7 100644 >> --- a/net/core/pktgen.c >> +++ b/net/core/pktgen.c >> @@ -290,6 +290,11 @@ struct pktgen_dev { >> * set clone_skb to 1024. >> */ >> >> + bool no_wait; /* >> + * Don't wait for packet to be freed >> + * by driver >> + */ >> + > > DaveM prefers multi line comments like: > > /* Don't wait for packet to be freed > * by driver > */ He does, but the rest of the kernel and the comment immediately above doesn't: int clone_skb; /* * Use multiple SKBs during packet gen. * If this number is greater than 1, then * that many copies of the same packet will be * sent before a new packet is allocated. * If you want to send 1024 identical packets * before creating a new packet, * set clone_skb to 1024. */ >> char dst_min[IP_NAME_SZ]; /* IP, ie 1.2.3.4 */ >> char dst_max[IP_NAME_SZ]; /* IP, ie 1.2.3.4 */ >> char src_min[IP_NAME_SZ]; /* IP, ie 1.2.3.4 */ >> @@ -679,6 +684,9 @@ static int pktgen_if_show(struct seq_file *seq, void *v) >> >> seq_puts(seq, "\n"); >> >> + if (pkt_dev->no_wait) >> + seq_puts(seq, " nowait\n"); >> + > > Shouldn't you put this print statement above the "Flags:" section? Sure. >> /* not really stopped, more like last-running-at */ >> stopped = pkt_dev->running ? ktime_get() : pkt_dev->stopped_at; >> idle = pkt_dev->idle_acc; >> @@ -1711,6 +1719,17 @@ static ssize_t pktgen_if_write(struct file *file, Subject: pktgen: nowait parameter. While trying to measure speed of virtio_net, I was getting hangs. This is because we skb_orphan() but delay the tx interrupt indefinitely (by number of slots). With nowait, pktgen won't wait for the skb to be released. This introduces an error, but it's ok if count >> ringsize. I updated the documentation, but it needs far more work (it refers to pgset and an examples directory, none of which exist in the kernel tree). Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> diff --git a/Documentation/networking/pktgen.txt b/Documentation/networking/pktgen.txt index 0dffc6e37902..dbd993d1e7f8 100644 --- a/Documentation/networking/pktgen.txt +++ b/Documentation/networking/pktgen.txt @@ -41,10 +41,13 @@ NIC HW layer (which is bad for bufferbloat). One should be careful to conclude, that packets/descriptors in the HW TX ring cause delay. Drivers usually delay cleaning up the ring-buffers (for various performance reasons), thus packets stalling -the TX ring, might just be waiting for cleanup. +the TX ring, might just be waiting for cleanup. Writing the "nowait" +parameter into /proc/net/pktgen/ethX will avoid waiting for cleanup of +the final packets, introducing a slight error (tiny if the count of +packets being sent is much greater than the ring size of the device). -This cleanup issues is specifically the case, for the driver ixgbe -(Intel 82599 chip). This driver (ixgbe) combine TX+RX ring cleanups, +Alternately, some drivers (eg ixgbe for the Intel 82599 chip) can +have their cleanup interval changed. ixgbe combines TX+RX ring cleanups, and the cleanup interval is affected by the ethtool --coalesce setting of parameter "rx-usecs". @@ -303,6 +306,8 @@ flowlen rate ratep +nowait + References: ftp://robur.slu.se/pub/Linux/net-development/pktgen-testing/ ftp://robur.slu.se/pub/Linux/net-development/pktgen-testing/examples/ diff --git a/net/core/pktgen.c b/net/core/pktgen.c index 8b849ddfef2e..1589f5ac0509 100644 --- a/net/core/pktgen.c +++ b/net/core/pktgen.c @@ -290,6 +290,8 @@ struct pktgen_dev { * set clone_skb to 1024. */ + bool no_wait; /* Don't wait for packet to be freed by driver */ + char dst_min[IP_NAME_SZ]; /* IP, ie 1.2.3.4 */ char dst_max[IP_NAME_SZ]; /* IP, ie 1.2.3.4 */ char src_min[IP_NAME_SZ]; /* IP, ie 1.2.3.4 */ @@ -615,6 +617,9 @@ static int pktgen_if_show(struct seq_file *seq, void *v) if (pkt_dev->node >= 0) seq_printf(seq, " node: %d\n", pkt_dev->node); + if (pkt_dev->no_wait) + seq_puts(seq, " nowait\n"); + seq_puts(seq, " Flags: "); if (pkt_dev->flags & F_IPV6) @@ -1711,6 +1716,17 @@ static ssize_t pktgen_if_write(struct file *file, return count; } + if (!strcmp(name, "nowait")) { + len = num_arg(&user_buffer[i], 10, &value); + if (len < 0) + return len; + + i += len; + pkt_dev->no_wait = value; + sprintf(pg_result, "OK: nowait=%u", pkt_dev->no_wait); + return count; + } + sprintf(pkt_dev->result, "No such parameter \"%s\"", name); return -EINVAL; } @@ -3373,7 +3389,8 @@ unlock: /* If pkt_dev->count is zero, then run forever */ if ((pkt_dev->count != 0) && (pkt_dev->sofar >= pkt_dev->count)) { - pktgen_wait_for_skb(pkt_dev); + if (!pkt_dev->no_wait) + pktgen_wait_for_skb(pkt_dev); /* Done with this */ pktgen_stop_device(pkt_dev); @@ -3565,6 +3582,7 @@ static int pktgen_add_device(struct pktgen_thread *t, const char *ifname) pkt_dev->svlan_cfi = 0; pkt_dev->svlan_id = 0xffff; pkt_dev->node = -1; + pkt_dev->no_wait = false; err = pktgen_setup_dev(t->net, pkt_dev, ifname); if (err) ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] pktgen: nowait parameter. 2014-09-05 1:49 ` Rusty Russell @ 2014-09-05 2:52 ` Jason Wang 0 siblings, 0 replies; 7+ messages in thread From: Jason Wang @ 2014-09-05 2:52 UTC (permalink / raw) To: Rusty Russell, Jesper Dangaard Brouer Cc: netdev, Mathias Krause, Robert Olsson On 09/05/2014 09:49 AM, Rusty Russell wrote: > Jesper Dangaard Brouer <brouer@redhat.com> writes: >> > On Wed, 03 Sep 2014 13:50:01 +0930 >> > Rusty Russell <rusty@rustcorp.com.au> wrote: >> > >>> >> While trying to measure speed of virtio_net, I was getting hangs. >>> >> This is because we skb_orphan() but delay the tx interrupt >>> >> indefinitely (by number of slots). >>> >> >>> >> With nowait, pktgen won't wait for the skb to be released. This >>> >> introduces an error, but it's ok if count >> ringsize. >> > >> > This pktgen_wait_for_skb() only happens it the exit case, when count >> > packets have been send. I guess its okay to proceed to >> > pktgen_stop_device() which will call kfree_skb(pkt_dev->skb) with >> > refcnt=2, decrementing to refcnt=1, and then we depend on driver to >> > eventually call kfree_skb(). > Yes, exactly. > >>> >> I updated the documentation, but it needs far more work (it >>> >> refers to pgset and an examples directory, none of which exist >>> >> in the kernel tree). >> > >> > Yes, the doc is not in such a good shape. >> > >> > I'm not 100% happy with the name "nowait" parameter, as users could >> > easily misunderstand the purpose of this parameter. But I've not come >> > up with a better name, e.g. "exit_nowait" is also not the best. > Agreed. It could also be a flag, though that doesn't help with the name. > >> > diff --git a/net/core/pktgen.c b/net/core/pktgen.c >>> >> index 8b849ddfef2e..adc41f2b3bc7 100644 >>> >> --- a/net/core/pktgen.c >>> >> +++ b/net/core/pktgen.c >>> >> @@ -290,6 +290,11 @@ struct pktgen_dev { >>> >> * set clone_skb to 1024. >>> >> */ >>> >> >>> >> + bool no_wait; /* >>> >> + * Don't wait for packet to be freed >>> >> + * by driver >>> >> + */ >>> >> + >> > >> > DaveM prefers multi line comments like: >> > >> > /* Don't wait for packet to be freed >> > * by driver >> > */ > He does, but the rest of the kernel and the comment immediately above > doesn't: > > int clone_skb; /* > * Use multiple SKBs during packet gen. > * If this number is greater than 1, then > * that many copies of the same packet will be > * sent before a new packet is allocated. > * If you want to send 1024 identical packets > * before creating a new packet, > * set clone_skb to 1024. > */ > >>> >> char dst_min[IP_NAME_SZ]; /* IP, ie 1.2.3.4 */ >>> >> char dst_max[IP_NAME_SZ]; /* IP, ie 1.2.3.4 */ >>> >> char src_min[IP_NAME_SZ]; /* IP, ie 1.2.3.4 */ >>> >> @@ -679,6 +684,9 @@ static int pktgen_if_show(struct seq_file *seq, void *v) >>> >> >>> >> seq_puts(seq, "\n"); >>> >> >>> >> + if (pkt_dev->no_wait) >>> >> + seq_puts(seq, " nowait\n"); >>> >> + >> > >> > Shouldn't you put this print statement above the "Flags:" section? > Sure. > >>> >> /* not really stopped, more like last-running-at */ >>> >> stopped = pkt_dev->running ? ktime_get() : pkt_dev->stopped_at; >>> >> idle = pkt_dev->idle_acc; >>> >> @@ -1711,6 +1719,17 @@ static ssize_t pktgen_if_write(struct file *file, > Subject: pktgen: nowait parameter. > > While trying to measure speed of virtio_net, I was getting hangs. > This is because we skb_orphan() but delay the tx interrupt > indefinitely (by number of slots). > > With nowait, pktgen won't wait for the skb to be released. This > introduces an error, but it's ok if count >> ringsize. > > I updated the documentation, but it needs far more work (it > refers to pgset and an examples directory, none of which exist > in the kernel tree). > > Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> This depends on user to know the internals of driver (e.g whether the tx completion is delayed by something e.g using skb_orphan()) which may not be a good idea. We may change the virtio-net to use tx interrupt in the future (I'm testing a draft patch to do this). How about something transparent to the user? I post a patch that marking such device with a special flag (https://patchwork.kernel.org/patch/1800711/), but not all like it. Maybe we need a new ndo_tx_polling() for pktgen or someone else to poll the tx completion? Thanks ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] pktgen: nowait parameter. 2014-09-03 4:20 [PATCH] pktgen: nowait parameter Rusty Russell 2014-09-03 9:09 ` Jesper Dangaard Brouer @ 2014-09-05 21:26 ` David Miller 2014-09-10 23:37 ` Rusty Russell 1 sibling, 1 reply; 7+ messages in thread From: David Miller @ 2014-09-05 21:26 UTC (permalink / raw) To: rusty; +Cc: netdev, brouer, minipli From: Rusty Russell <rusty@rustcorp.com.au> Date: Wed, 03 Sep 2014 13:50:01 +0930 > While trying to measure speed of virtio_net, I was getting hangs. > This is because we skb_orphan() but delay the tx interrupt > indefinitely (by number of slots). > > With nowait, pktgen won't wait for the skb to be released. This > introduces an error, but it's ok if count >> ringsize. > > I updated the documentation, but it needs far more work (it > refers to pgset and an examples directory, none of which exist > in the kernel tree). > > Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> Please just make this a flag, like UDPCSUM, NO_TIMESTAMP, et al. Which also means that it should be capitalized. BTW, wrt. holding onto TX frames for unbounded amounts of time, I think this is a bad idea even with skb_orphan(). There are resources from the SKB you are hanging onto which can stall the removal of modules indefinitely, such as netfilter references. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] pktgen: nowait parameter. 2014-09-05 21:26 ` David Miller @ 2014-09-10 23:37 ` Rusty Russell 2014-09-12 21:52 ` David Miller 0 siblings, 1 reply; 7+ messages in thread From: Rusty Russell @ 2014-09-10 23:37 UTC (permalink / raw) To: David Miller; +Cc: netdev, brouer, minipli David Miller <davem@davemloft.net> writes: > From: Rusty Russell <rusty@rustcorp.com.au> > Date: Wed, 03 Sep 2014 13:50:01 +0930 > >> While trying to measure speed of virtio_net, I was getting hangs. >> This is because we skb_orphan() but delay the tx interrupt >> indefinitely (by number of slots). >> >> With nowait, pktgen won't wait for the skb to be released. This >> introduces an error, but it's ok if count >> ringsize. >> >> I updated the documentation, but it needs far more work (it >> refers to pgset and an examples directory, none of which exist >> in the kernel tree). >> >> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> > > Please just make this a flag, like UDPCSUM, NO_TIMESTAMP, et al. > Which also means that it should be capitalized. Agreed, though I prefer Jason's IFF_TX_SKB_FREE_DELAY, which doesn't require intimate knowledge of the driver to get the option correct. > BTW, wrt. holding onto TX frames for unbounded amounts of time, I > think this is a bad idea even with skb_orphan(). There are resources > from the SKB you are hanging onto which can stall the removal of > modules indefinitely, such as netfilter references. We could certainly have a once-a-second timer which did this, but should skb_orphan() do that work instead? Cheers, Rusty. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] pktgen: nowait parameter. 2014-09-10 23:37 ` Rusty Russell @ 2014-09-12 21:52 ` David Miller 0 siblings, 0 replies; 7+ messages in thread From: David Miller @ 2014-09-12 21:52 UTC (permalink / raw) To: rusty; +Cc: netdev, brouer, minipli From: Rusty Russell <rusty@rustcorp.com.au> Date: Thu, 11 Sep 2014 09:07:29 +0930 > David Miller <davem@davemloft.net> writes: >> From: Rusty Russell <rusty@rustcorp.com.au> >> Date: Wed, 03 Sep 2014 13:50:01 +0930 >> >> BTW, wrt. holding onto TX frames for unbounded amounts of time, I >> think this is a bad idea even with skb_orphan(). There are resources >> from the SKB you are hanging onto which can stall the removal of >> modules indefinitely, such as netfilter references. > > We could certainly have a once-a-second timer which did this, but should > skb_orphan() do that work instead? It would definitely improve the situation. I've discussed a few times with Herbert Xu the idea of using hrtimers since we have those, to do something more clever and timely. ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2014-09-12 21:52 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-09-03 4:20 [PATCH] pktgen: nowait parameter Rusty Russell 2014-09-03 9:09 ` Jesper Dangaard Brouer 2014-09-05 1:49 ` Rusty Russell 2014-09-05 2:52 ` Jason Wang 2014-09-05 21:26 ` David Miller 2014-09-10 23:37 ` Rusty Russell 2014-09-12 21:52 ` David Miller
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.