pktgen: nowait parameter.

PATCH In review Rusty Russell 4 participants 7 messages Sep 3, 2014 – Sep 12, 2014

Subsystems (3)

Networking Network drivers Documentation

lore

Share & tools

lore

Rusty Russell

author

2014-09-03 04:30 UTC #0 lore raw

PATCH 1/1 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 <[email protected]>

Documentation/networking/pktgen.txt +8 -3

index 0dffc6e37902..48d86da74a39 100644
@@ -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/

net/core/pktgen.c +22 -1

index 8b849ddfef2e..adc41f2b3bc7 100644
@@ -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)

Jesper Dangaard Brouer

2014-09-03 09:10 UTC #1 lore raw

Quoted 6 lines · On Wed, 03 Sep 2014 13:50:01 +0930 Rusty Russell <[email protected]> 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().

Quoted 3 lines

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.

net/core/pktgen.c struct pktgen_dev { · line 297

Quoted 13 lines

net/core/pktgen.c +5 -0

index 8b849ddfef2e..adc41f2b3bc7 100644
@@ -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
*/

net/core/pktgen.c static int pktgen_if_show(struct seq_file *seq, void *v) · line 689

Quoted 10 lines

net/core/pktgen.c +3 -0

 	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?

Quoted 4 lines

net/core/pktgen.c +0 -0

 	/* 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,

Signature · 5 lines

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

Rusty Russell

author

2014-09-05 02:34 UTC #2 lore raw

Quoted 7 lines · Jesper Dangaard Brouer

Quoted 4 lines · On Wed, 03 Sep 2014 13:50:01 +0930 Rusty Russell <[email protected]> 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).

Quoted 8 lines

Quoted 2 lines
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.

Quoted 9 lines

Quoted 3 lines
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.

Quoted 16 lines

net/core/pktgen.c +0 -0
net/core/pktgen.c struct pktgen_dev { · line 297
Quoted 12 lines
index 8b849ddfef2e..adc41f2b3bc7 100644
net/core/pktgen.c +5 -0
@@ -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:

Quoted 3 lines

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

Quoted 5 lines

net/core/pktgen.c +0 -0

 	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)

Quoted 7 lines

net/core/pktgen.c static int pktgen_if_show(struct seq_file *seq, void *v) · line 689
Quoted 5 lines
net/core/pktgen.c +3 -0
 	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.

Quoted 4 lines

net/core/pktgen.c static ssize_t pktgen_if_write(struct file *file,

Quoted 4 lines

net/core/pktgen.c +0 -0

 	/* 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 <[email protected]>

Documentation/networking/pktgen.txt +8 -3

index 0dffc6e37902..dbd993d1e7f8 100644
@@ -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/

net/core/pktgen.c +19 -1

index 8b849ddfef2e..1589f5ac0509 100644
@@ -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)

Jason Wang

2014-09-05 02:52 UTC #3 lore raw

Quoted 85 lines · Rusty Russell

Quoted 7 lines · Jesper Dangaard Brouer
Quoted 7 lines
Quoted 4 lines · On Wed, 03 Sep 2014 13:50:01 +0930 Rusty Russell <[email protected]> wrote:
Quoted 4 lines
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).
Quoted 8 lines
Quoted 8 lines
Quoted 2 lines
Quoted 2 lines
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.
Quoted 9 lines
Quoted 9 lines
Quoted 3 lines
Quoted 3 lines
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.
Quoted 16 lines
Quoted 16 lines
net/core/pktgen.c +0 -0
Quoted 7 lines
Quoted 7 lines
index 8b849ddfef2e..adc41f2b3bc7 100644
net/core/pktgen.c +0 -0
@@ -290,6 +290,11 @@ struct pktgen_dev {
 				 * set clone_skb to 1024.
 				 */
 
Quoted 5 lines
net/core/pktgen.c struct pktgen_dev { · line 297
Quoted 5 lines
net/core/pktgen.c +5 -0
+	bool no_wait;		/*
+				 * Don't wait for packet to be freed
+				 * by driver
+				 */
+
DaveM prefers multi line comments like:
Quoted 3 lines
Quoted 3 lines
/* 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.
*/
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
net/core/pktgen.c +0 -0
 	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)
 
Quoted 7 lines
Quoted 7 lines
Quoted 5 lines
net/core/pktgen.c static int pktgen_if_show(struct seq_file *seq, void *v) · line 622
Quoted 5 lines
net/core/pktgen.c +3 -0
 	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.
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
net/core/pktgen.c static ssize_t pktgen_if_write(struct file *file,
Quoted 4 lines
net/core/pktgen.c +0 -0
 	/* 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).

Quoted 8 lines

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 <[email protected]>

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

David Miller

maintainer

2014-09-05 21:26 UTC #4 lore raw

In reply to Rusty Russell

From: Rusty Russell <[email protected]>
Date: Wed, 03 Sep 2014 13:50:01 +0930

Quoted 4 lines

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

Quoted 8 lines

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 <[email protected]>

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.

Rusty Russell

author

2014-09-10 23:46 UTC #5 lore raw

Quoted 10 lines · David Miller

From: Rusty Russell <[email protected]>
Date: Wed, 03 Sep 2014 13:50:01 +0930
Quoted 7 lines
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.

Quoted 8 lines

Quoted 5 lines
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 <[email protected]>
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.

Quoted 4 lines

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.

David Miller

maintainer

2014-09-12 21:52 UTC #6 lore raw

From: Rusty Russell <[email protected]>
Date: Thu, 11 Sep 2014 09:07:29 +0930

Quoted 11 lines

Quoted 7 lines · David Miller
From: Rusty Russell <[email protected]>
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.