pktgen: introduce 'rx' mode

Introduce 'RX' mode for pktgen which generates the packets using familiar pktgen commands, but feeds them into netif_receive_skb() instead of ndo_start_xmit().

Signed-off-by Alexei Starovoitov <[email protected]>

---
v2->v3: addressed more Eric comments. Thanks!

v1->v2: as suggested by Eric:
- dropped 'clone_skb' flag, now it will return enotsupp
- fix rps/rfs bug by checking skb->users after every netif_receive_skb
- tested with RPS/RFS, taps, veth, physical devs, various tc cls/act

Introduce 'RX' mode for pktgen which generates the packets using familiar pktgen commands, but feeds them into netif_receive_skb() instead of ndo_start_xmit().

It is designed to test netif_receive_skb and ingress qdisc performace only. Make sure to understand how it works before using it for other rx benchmarking.

Signed-off-by Alexei Starovoitov <[email protected]>

---
v2->v3: addressed more Eric comments. Thanks!

v1->v2: as suggested by Eric:
- dropped 'clone_skb' flag, now it will return enotsupp
- fix rps/rfs bug by checking skb->users after every netif_receive_skb
- tested with RPS/RFS, taps, veth, physical devs, various tc cls/act

 net/core/pktgen.c |   49 +++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 47 insertions(+), 2 deletions(-)

net/core/pktgen.c +47 -2

index 508155b283dd..34fd5ece2681 100644
@@ -203,6 +203,7 @@
 #define F_NODE          (1<<15)	/* Node memory alloc*/
 #define F_UDPCSUM       (1<<16)	/* Include UDP checksum */
 #define F_NO_TIMESTAMP  (1<<17)	/* Don't timestamp packets (default TS) */
+#define F_DO_RX		(1<<18)	/* generate packets for RX */
 
 /* Thread control flag bits */
 #define T_STOP        (1<<0)	/* Stop run */

@@ -1081,7 +1082,8 @@ static ssize_t pktgen_if_write(struct file *file,
 		if (len < 0)
 			return len;
 		if ((value > 0) &&
-		    (!(pkt_dev->odev->priv_flags & IFF_TX_SKB_SHARING)))
+		    ((pkt_dev->flags & F_DO_RX) ||
+		     !(pkt_dev->odev->priv_flags & IFF_TX_SKB_SHARING)))
 			return -ENOTSUPP;
 		i += len;
 		pkt_dev->clone_skb = value;

@@ -1089,6 +1091,12 @@ static ssize_t pktgen_if_write(struct file *file,
 		sprintf(pg_result, "OK: clone_skb=%d", pkt_dev->clone_skb);
 		return count;
 	}
+	if (!strcmp(name, "rx")) {
+		pkt_dev->flags |= F_DO_RX;
+
+		sprintf(pg_result, "OK: RX is ON");
+		return count;
+	}
 	if (!strcmp(name, "count")) {
 		len = num_arg(&user_buffer[i], 10, &value);
 		if (len < 0)

@@ -1134,7 +1142,7 @@ static ssize_t pktgen_if_write(struct file *file,
 			return len;
 
 		i += len;
-		if ((value > 1) &&
+		if ((value > 1) && !(pkt_dev->flags & F_DO_RX) &&
 		    (!(pkt_dev->odev->priv_flags & IFF_TX_SKB_SHARING)))
 			return -ENOTSUPP;
 		pkt_dev->burst = value < 1 ? 1 : value;

@@ -3317,6 +3325,7 @@ static void pktgen_xmit(struct pktgen_dev *pkt_dev)
 	unsigned int burst = ACCESS_ONCE(pkt_dev->burst);
 	struct net_device *odev = pkt_dev->odev;
 	struct netdev_queue *txq;
+	struct sk_buff *skb;
 	int ret;
 
 	/* If device is offline, then don't send */

@@ -3349,11 +3358,46 @@ static void pktgen_xmit(struct pktgen_dev *pkt_dev)
 		pkt_dev->last_pkt_size = pkt_dev->skb->len;
 		pkt_dev->allocated_skbs++;
 		pkt_dev->clone_count = 0;	/* reset counter */
+		if (pkt_dev->flags & F_DO_RX)
+			pkt_dev->skb->protocol = eth_type_trans(pkt_dev->skb,
+								pkt_dev->skb->dev);
 	}
 
 	if (pkt_dev->delay && pkt_dev->last_ok)
 		spin(pkt_dev, pkt_dev->next_tx);
 
+	if (pkt_dev->flags & F_DO_RX) {
+		skb = pkt_dev->skb;
+		atomic_add(burst, &skb->users);
+		local_bh_disable();
+		do {
+			ret = netif_receive_skb(skb);
+			if (ret == NET_RX_DROP)
+				pkt_dev->errors++;
+			pkt_dev->sofar++;
+			pkt_dev->seq_num++;
+			if (atomic_read(&skb->users) != burst) {
+				/* skb was queued by rps/rfs or taps,
+				 * so cannot reuse this skb
+				 */
+				atomic_sub(burst - 1, &skb->users);
+				/* get out of the loop and wait
+				 * until skb is consumed
+				 */
+				pkt_dev->last_ok = 1;
+				pkt_dev->clone_skb = 0;
+				break;
+			}
+			/* skb was 'freed' by stack, so clean few
+			 * bits and reuse it
+			 */
+#ifdef CONFIG_NET_CLS_ACT
+			skb->tc_verd = 0; /* reset reclass/redir ttl */
+#endif
+		} while (--burst > 0);
+		goto out;
+	}
+
 	txq = skb_get_tx_queue(odev, pkt_dev->skb);
 
 	local_bh_disable();

@@ -3401,6 +3445,7 @@ xmit_more:
 unlock:
 	HARD_TX_UNLOCK(odev, txq);
 
+out:
 	local_bh_enable();
 
 	/* If pkt_dev->count is zero, then run forever */

First of all I love the idea of modifying pktgen to performance test the RX path.

I'm not sure the simple "rx" flag is a good "name". It likely conflicts with other work where pktgen can receive it own packets, e.g.
https://people.kth.se/~danieltt/pktgen/ or Ben Greer's solution.

Hi Jesper,

On 05/02/2015 10:46 AM, Jesper Dangaard Brouer wrote:
...

Quoted 6 lines

First of all I love the idea of modifying pktgen to performance test the RX path.

I'm not sure the simple "rx" flag is a good "name". It likely conflicts with other work where pktgen can receive it own packets, e.g.
https://people.kth.se/~danieltt/pktgen/ or Ben Greer's solution.

Why do we start caring about out of tree code now? We never have, really. If there is no interest in merging this stuff upstream, then it's always the case that _their code_ needs to adapt iff you want to run on a more recent kernel; the kernel dictates the uapi, not some out of tree module. ;)

I personally just don't like the "RX" flag name.
What about "STACK_RX" or "RX_INJECT"?

Quoted 7 lines · On Thu, 30 Apr 2015 22:12:10 -0700 Alexei Starovoitov <[email protected]> wrote:

Introduce 'RX' mode for pktgen which generates the packets using familiar pktgen commands, but feeds them into netif_receive_skb() instead of ndo_start_xmit().

It is designed to test netif_receive_skb and ingress qdisc performace only. Make sure to understand how it works before using it for other rx benchmarking.

Hi Alexei

First of all I love the idea of modifying pktgen to performance test the RX path.

I'm not sure the simple "rx" flag is a good "name". It likely conflicts with other work where pktgen can receive it own packets, e.g.
https://people.kth.se/~danieltt/pktgen/ or Ben Greer's solution.

In your patch several things are not pktgen "compliant":
1. Flags in pktgen are normally in upper-case "RX"
2. Flags also require a disable "!RX" option
3. You didn't add the flag to list of supported flags
4. You don't output if the flag is enabled
5. You didn't update the documentation (Documentation/networking/pktgen.txt)

Quoted 20 lines · Jesper Dangaard Brouer

Quoted 7 lines · On Thu, 30 Apr 2015 22:12:10 -0700 Alexei Starovoitov <[email protected]> wrote:

Introduce 'RX' mode for pktgen which generates the packets using familiar pktgen commands, but feeds them into netif_receive_skb() instead of ndo_start_xmit().

It is designed to test netif_receive_skb and ingress qdisc performace only. Make sure to understand how it works before using it for other rx benchmarking.

Hi Alexei

First of all I love the idea of modifying pktgen to performance test the RX path.

I'm not sure the simple "rx" flag is a good "name". It likely conflicts with other work where pktgen can receive it own packets, e.g.
https://people.kth.se/~danieltt/pktgen/ or Ben Greer's solution.

Quoted 6 lines

In your patch several things are not pktgen "compliant":
1. Flags in pktgen are normally in upper-case "RX"
2. Flags also require a disable "!RX" option
3. You didn't add the flag to list of supported flags
4. You don't output if the flag is enabled
5. You didn't update the documentation (Documentation/networking/pktgen.txt)

It's actually not a flag, because it cannot be disabled by design. It cannot be flipped back and forth, because it affects what other real flags can be applied. It's a _mode_.
I don't see yet how I can safely switch this mode back into tx while things are running. It would need a whole new mechanism of stopping things and so on. I wanted to start simple.
For 5, yeah, agree, need to update the doc.
As far as name, I don't have preferences. Will 'stack_inject' sound better? I can respin with that name if you like, but disabling it on the fly is a major change. I'd rather do it in small steps.

Okay, I guess it makes sense as a "mode".

Calling it "stack_inject" is fairly descriptive, but I also like Daniel's idea of option "xmit_mode [rx|tx]".

I would not allow it to be reconfigured on the fly, but only when the test/thread is stopped (!pkt_dev->running), allow re-configuring back to "TX" mode. Although, removing (and free) the entire pkt_dev, would implicitly reset it back.

On 5/2/15 9:46 AM, Jesper Dangaard Brouer wrote:

all makes sense.
btw, I'm off the grid till Monday and it sounds like you see concrete spots in the code to hack this stuff in, so if you want to take over, please go ahead :)

Quoted 1 line · On Sat, 02 May 2015 10:07:48 -0700 Alexei Starovoitov <[email protected]> wrote:

On 5/2/15 9:46 AM, Jesper Dangaard Brouer wrote:

[...]

Quoted 5 lines

all makes sense.
btw, I'm off the grid till Monday and it sounds like you see concrete spots in the code to hack this stuff in, so if you want to take over, please go ahead :)

Okay, I'll address my own concerns and post a patch in your name("From:")

As I would like to see this feature in pktgen, as it provides a really easy and quick way to measure/profile changes in the ingress path...

Introduce xmit_mode 'rx_inject' for pktgen which generates the packets using familiar pktgen commands, but feeds them into netif_receive_skb() instead of ndo_start_xmit().

pgset "xmit_mode rx_inject"

Signed-off-by Alexei Starovoitov <[email protected]>

Signed-off-by Jesper Dangaard Brouer <[email protected]>

net/core/pktgen.c +0 -0

@@ -251,13 +255,14 @@ struct pktgen_dev {
  	 * we will do a random selection from within the range.
  	 */
  	__u32 flags;

Quoted 4 lines · Jesper Dangaard Brouer

Introduce xmit_mode 'rx_inject' for pktgen which generates the packets using familiar pktgen commands, but feeds them into netif_receive_skb() instead of ndo_start_xmit().

...

Quoted 1 line

pgset "xmit_mode rx_inject"

I think 'xmit_mode rx_inject' would make native english speaker cringe, since it's saying 'transmit mode is receive' ... but I don't mind :)

Quoted 2 lines

Signed-off-by Alexei Starovoitov <[email protected]>

Signed-off-by Jesper Dangaard Brouer <[email protected]>

...

Quoted 4 lines

net/core/pktgen.c +0 -0

@@ -251,13 +255,14 @@ struct pktgen_dev {
  	 * we will do a random selection from within the range.
  	 */
  	__u32 flags;

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

Quoted 10 lines

net/core/pktgen.c +3 -3

-	int removal_mark;	/* non-zero => the device is marked for
-				 * removal by worker thread */
-
+	int xmit_mode;
  	int min_pkt_size;
  	int max_pkt_size;
  	int pkt_overhead;	/* overhead for MPLS, VLANs, IPSEC etc */
  	int nfrags;
+	int removal_mark;	/* non-zero => the device is marked for
+				 * removal by worker thread */

I'm not sure why you're moving removal_mark field.

Looks good. Thank you for doing this.
Ack. My SOB is already there :)

btw, these patches didn't reach my subscribed to netdev email yet... something is stalling vger.

Quoted 6 lines · On Tue, 05 May 2015 21:33:26 -0700 Alexei Starovoitov <[email protected]> wrote:

Quoted 4 lines · Jesper Dangaard Brouer

Introduce xmit_mode 'rx_inject' for pktgen which generates the packets using familiar pktgen commands, but feeds them into netif_receive_skb() instead of ndo_start_xmit().

...

Quoted 4 lines

Quoted 1 line

pgset "xmit_mode rx_inject"

I think 'xmit_mode rx_inject' would make native english speaker cringe, since it's saying 'transmit mode is receive' ... but I don't mind :)

Yes, I know. Like Daniel suggested, I considered only calling it "rx" but it made me cringe for this exact reason, thus I extended it with "inject". I'm flexible with the name of this...

Quoted 13 lines · Jesper Dangaard Brouer

Quoted 10 lines · On Tue, 05 May 2015 21:33:26 -0700 Alexei Starovoitov <[email protected]> wrote:

Quoted 4 lines · Jesper Dangaard Brouer

Introduce xmit_mode 'rx_inject' for pktgen which generates the packets using familiar pktgen commands, but feeds them into netif_receive_skb() instead of ndo_start_xmit().

...

Quoted 1 line

pgset "xmit_mode rx_inject"

I think 'xmit_mode rx_inject' would make native english speaker cringe, since it's saying 'transmit mode is receive' ... but I don't mind :)

Quoted 3 lines

Yes, I know. Like Daniel suggested, I considered only calling it "rx" but it made me cringe for this exact reason, thus I extended it with "inject". I'm flexible with the name of this...

I don't mind how you name it eventually. ;) 'xmit_mode' I think is good, and rx|tx would be symmetric. I believe you don't like "rx" due to these two out-of-tree pktgen projects you mentioned having rx capabilities. Is that correct?

From my perspective, it would be more
worth however to improve packet sockets and eBPF that could already do the same thing instead of a dedicated possible pktgen receive/ capturing device for such analysis. Anyway, I can also live with a rx_inject.

net/core/pktgen.c +0 -0

index 43bb215..85195b2 100644
@@ -210,6 +210,10 @@

net/core/pktgen.c +1 -0

@@ -3320,6 +3358,7 @@ static void pktgen_xmit(struct pktgen_dev *pkt_dev)
  	unsigned int burst = ACCESS_ONCE(pkt_dev->burst);
  	struct net_device *odev = pkt_dev->odev;
  	struct netdev_queue *txq;
+	struct sk_buff *skb;
  	int ret;
 
  	/* If device is offline, then don't send */

net/core/pktgen.c +32 -1

  	if (pkt_dev->delay && pkt_dev->last_ok)
  		spin(pkt_dev, pkt_dev->next_tx);
 
+	if (pkt_dev->xmit_mode == M_RX_INJECT) {
+		skb = pkt_dev->skb;
+		atomic_add(burst, &skb->users);
+		local_bh_disable();
+		do {
+			ret = netif_receive_skb(skb);
+			if (ret == NET_RX_DROP)
+				pkt_dev->errors++;
+			pkt_dev->sofar++;
+			pkt_dev->seq_num++;
+			if (atomic_read(&skb->users) != burst) {
+				/* skb was queued by rps/rfs or taps,
+				 * so cannot reuse this skb
+				 */
+				atomic_sub(burst - 1, &skb->users);
+				/* get out of the loop and wait
+				 * until skb is consumed
+				 */
+				pkt_dev->last_ok = 1;
+				break;
+			}
+			/* skb was 'freed' by stack, so clean few
+			 * bits and reuse it
+			 */
+#ifdef CONFIG_NET_CLS_ACT
+			skb->tc_verd = 0; /* reset reclass/redir ttl */
+#endif
+		} while (--burst > 0);
+		goto out; /* Skips xmit_mode M_TX */
+	}
+
  	txq = skb_get_tx_queue(odev, pkt_dev->skb);
 
  	local_bh_disable();

@@ -3404,6 +3477,7 @@ xmit_more:
  unlock:
  	HARD_TX_UNLOCK(odev, txq);
 
+out:
  	local_bh_enable();
 
  	/* If pkt_dev->count is zero, then run forever */
 
--

To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

On 05/05/2015 01:30 PM, Jesper Dangaard Brouer wrote:

<snip>

Quoted 5 lines

net/core/pktgen.c +0 -0

index 43bb215..85195b2 100644
@@ -210,6 +210,10 @@

<snip>

Quoted 7 lines

net/core/pktgen.c +1 -0

@@ -3320,6 +3358,7 @@ static void pktgen_xmit(struct pktgen_dev *pkt_dev)
  	unsigned int burst = ACCESS_ONCE(pkt_dev->burst);
  	struct net_device *odev = pkt_dev->odev;
  	struct netdev_queue *txq;
+	struct sk_buff *skb;
  	int ret;
 

net/core/pktgen.c static void pktgen_xmit(struct pktgen_dev *pkt_dev) · line 3398

Quoted 10 lines

net/core/pktgen.c +3 -0

  	/* If device is offline, then don't send */

@@ -3352,11 +3391,45 @@ static void pktgen_xmit(struct pktgen_dev *pkt_dev)
  		pkt_dev->last_pkt_size = pkt_dev->skb->len;
  		pkt_dev->allocated_skbs++;
  		pkt_dev->clone_count = 0;	/* reset counter */
+		if (pkt_dev->xmit_mode == M_RX_INJECT)
+			pkt_dev->skb->protocol = eth_type_trans(pkt_dev->skb,
+								pkt_dev->skb->dev);
  	}
 

I was just wondering. Since M_RX_INJECT is not compatible with clone_skb couldn't the lines added above be moved down into the block below so that you could avoid the additional conditional jump?

Quoted 51 lines

net/core/pktgen.c +32 -1

  	if (pkt_dev->delay && pkt_dev->last_ok)
  		spin(pkt_dev, pkt_dev->next_tx);
 
+	if (pkt_dev->xmit_mode == M_RX_INJECT) {
+		skb = pkt_dev->skb;
+		atomic_add(burst, &skb->users);
+		local_bh_disable();
+		do {
+			ret = netif_receive_skb(skb);
+			if (ret == NET_RX_DROP)
+				pkt_dev->errors++;
+			pkt_dev->sofar++;
+			pkt_dev->seq_num++;
+			if (atomic_read(&skb->users) != burst) {
+				/* skb was queued by rps/rfs or taps,
+				 * so cannot reuse this skb
+				 */
+				atomic_sub(burst - 1, &skb->users);
+				/* get out of the loop and wait
+				 * until skb is consumed
+				 */
+				pkt_dev->last_ok = 1;
+				break;
+			}
+			/* skb was 'freed' by stack, so clean few
+			 * bits and reuse it
+			 */
+#ifdef CONFIG_NET_CLS_ACT
+			skb->tc_verd = 0; /* reset reclass/redir ttl */
+#endif
+		} while (--burst > 0);
+		goto out; /* Skips xmit_mode M_TX */
+	}
+
  	txq = skb_get_tx_queue(odev, pkt_dev->skb);
 
  	local_bh_disable();

@@ -3404,6 +3477,7 @@ xmit_more:
  unlock:
  	HARD_TX_UNLOCK(odev, txq);
 
+out:
  	local_bh_enable();
 
  	/* If pkt_dev->count is zero, then run forever */
 
--

To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 4 lines

I was just wondering. Since M_RX_INJECT is not compatible with clone_skb couldn't the lines added above be moved down into the block below so that you could avoid the additional conditional jump?

Sure, that is possible. I'll let Alexei answer, as it is his code. (And repost if he also thinks so...)

We could call it "stack_inject" instead? ... to take out the confusing "rx" part of an "transmit/xmit" mode that "receives"

From: Alexei Starovoitov <[email protected]>

Introduce xmit_mode 'netif_receive' for pktgen which generates the packets using familiar pktgen commands, but feeds them into netif_receive_skb() instead of ndo_start_xmit().

Default mode is called 'start_xmit'.

Signed-off-by Alexei Starovoitov <[email protected]>

Signed-off-by Jesper Dangaard Brouer <[email protected]>

---
v4->v5:
- Rename xmit_mode's <start_xmit|netif_receive>
- Save one branch when calling eth_type_trans(), noticed by Alex Duyck

Quoted 7 lines · Jesper Dangaard Brouer

From: Alexei Starovoitov <[email protected]>

Introduce xmit_mode 'netif_receive' for pktgen which generates the packets using familiar pktgen commands, but feeds them into netif_receive_skb() instead of ndo_start_xmit().

Default mode is called 'start_xmit'.

...

Quoted 7 lines

Signed-off-by Alexei Starovoitov <[email protected]>

Signed-off-by Jesper Dangaard Brouer <[email protected]>

---
v4->v5:
- Rename xmit_mode's <start_xmit|netif_receive>
- Save one branch when calling eth_type_trans(), noticed by Alex Duyck

looks good to me. Thanks a lot.

btw, I've started to work on a patch on top of this one that allows multiple pktgen threads to submit into the same netdev. I've used it to stress test removal of spin_lock in ingress qdisc. The idea is to add another 'name' parameter to command:
'add_device name dev'
'name' will be used to identify this pktgen thread in /proc and 'dev' used as target net_device.
I think it will be useful for start_xmit testing as well. I wonder why it wasn't done earlier? The queue configuration is already supported.

You mean other than below commit (iow independant of queue mapping)?

commit e6fce5b916cd7f7f79b2b3e53ba74bbfc1d7cf8b

Author: Robert Olsson <[email protected]>
Date:   Thu Aug 7 02:23:01 2008 -0700

pktgen: multiqueue etc.

Quoted 7 lines · Daniel Borkmann

You mean other than below commit (iow independant of queue mapping)?

commit e6fce5b916cd7f7f79b2b3e53ba74bbfc1d7cf8b

Author: Robert Olsson <[email protected]>
Date:   Thu Aug 7 02:23:01 2008 -0700

Quoted 1 line

pktgen: multiqueue etc.

ahh, I felt that I'm missing something ;)
Too bad it's not documented. I couldn't figure that out from sources. Thanks! will give it a shot.

Quoted 3 lines · Alexei Starovoitov

Quoted 2 lines · Jesper Dangaard Brouer

From: Alexei Starovoitov <[email protected]>

Quoted 10 lines

btw, I've started to work on a patch on top of this one that allows multiple pktgen threads to submit into the same netdev. I've used it to stress test removal of spin_lock in ingress qdisc. The idea is to add another 'name' parameter to command:
'add_device name dev'
'name' will be used to identify this pktgen thread in /proc and 'dev' used as target net_device.
I think it will be useful for start_xmit testing as well. I wonder why it wasn't done earlier? The queue configuration is already supported.

You mean other than below commit (iow independant of queue mapping)?

commit e6fce5b916cd7f7f79b2b3e53ba74bbfc1d7cf8b

Author: Robert Olsson <[email protected]>
Date:   Thu Aug 7 02:23:01 2008 -0700

pktgen: multiqueue etc.

Quoted 3 lines · Alexei Starovoitov

Quoted 2 lines · Jesper Dangaard Brouer

From: Alexei Starovoitov <[email protected]>

Quoted 10 lines

btw, I've started to work on a patch on top of this one that allows multiple pktgen threads to submit into the same netdev. I've used it to stress test removal of spin_lock in ingress qdisc. The idea is to add another 'name' parameter to command:
'add_device name dev'
'name' will be used to identify this pktgen thread in /proc and 'dev' used as target net_device.
I think it will be useful for start_xmit testing as well. I wonder why it wasn't done earlier? The queue configuration is already supported.

You mean other than below commit (iow independant of queue mapping)?

commit e6fce5b916cd7f7f79b2b3e53ba74bbfc1d7cf8b

Author: Robert Olsson <[email protected]>
Date:   Thu Aug 7 02:23:01 2008 -0700

pktgen: multiqueue etc.

Quoted 4 lines · On Thu, 07 May 2015 19:11:58 +0200 Daniel Borkmann <[email protected]> wrote:

Quoted 3 lines · Alexei Starovoitov

Quoted 2 lines · Jesper Dangaard Brouer

From: Alexei Starovoitov <[email protected]>

[...snip...]

Quoted 11 lines

Quoted 10 lines

btw, I've started to work on a patch on top of this one that allows multiple pktgen threads to submit into the same netdev. I've used it to stress test removal of spin_lock in ingress qdisc. The idea is to add another 'name' parameter to command:
'add_device name dev'
'name' will be used to identify this pktgen thread in /proc and 'dev' used as target net_device.
I think it will be useful for start_xmit testing as well. I wonder why it wasn't done earlier? The queue configuration is already supported.

Quoted 7 lines

You mean other than below commit (iow independant of queue mapping)?

commit e6fce5b916cd7f7f79b2b3e53ba74bbfc1d7cf8b

Author: Robert Olsson <[email protected]>
Date:   Thu Aug 7 02:23:01 2008 -0700

pktgen: multiqueue etc.

For completeness and others reading this threads...

Pktgen multiqueue is already supported via mentioned commit, which adds the device naming scheme: "add_device dev@number"

And yes, the documentation does not seem to mention this. I've been using it for years now... My scripts[1] take param "-t" for "threads".
[1] https://github.com/netoptimizer/network-testing/tree/master/pktgen

I've added a more plain version of a script, based on yours, below my signature.

The funny thing now is that scaling does not "happen" as we stall on:

   atomic_long_inc(&skb->dev->rx_dropped);

I'm quite puzzled why a single atomic op could "freeze" my system from scaling beyond 24Mpps.

More interesting observations with the mentioned script (now attached).

On my system the scaling stopped a 24Mpps, when I increased the number of threads the collective scaling was stuck at 24Mpps.

Then I simply removed/compiled-out the:
atomic_long_inc(&skb->dev->rx_dropped);

And after that change, the scaling is basically infinite/perfect.

Single thread performance increased from 24.7Mpps to 31.1Mpps, which corresponds perfectly with the cost of an atomic operation on this HW (8.25ns).

Diff to before:
* (1/24700988*10^9)-(1/31170819*10^9) = 8.40292328196 ns

When increasing the threads now, they all basically run at 31Mpps. Tried it upto 12 threads.

I'm quite puzzled why a single atomic op could "freeze" my system from scaling beyond 24Mpps.

Quoted 3 lines

I've added a more plain version of a script, based on yours, below my signature.

Now attached.

Quoted 2 lines

The funny thing now is that scaling does not "happen" as we stall on:

    atomic_long_inc(&skb->dev->rx_dropped);

More interesting observations with the mentioned script (now attached).

On my system the scaling stopped a 24Mpps, when I increased the number of threads the collective scaling was stuck at 24Mpps.

Quoted 5 lines · Jesper Dangaard Brouer

Quoted 3 lines

I've added a more plain version of a script, based on yours, below my signature.

Now attached.

thanks for the script! Really useful.
Could you add it to samples/pktgen/ and remove useless and confusing pktgen.conf-2-1 ?

Quoted 5 lines

Quoted 2 lines

The funny thing now is that scaling does not "happen" as we stall on:

    atomic_long_inc(&skb->dev->rx_dropped);

More interesting observations with the mentioned script (now attached).

Quoted 2 lines

On my system the scaling stopped a 24Mpps, when I increased the number of threads the collective scaling was stuck at 24Mpps.

what was your config to start hitting that drop counter? We can convert it to per_cpu, but I'm not sure it's worth doing. If I send normal ip packets they don't go this path. Only unknown protocol packets suppose to hit it ?

The funny thing now is that scaling does not "happen" as we stall on:

   atomic_long_inc(&skb->dev->rx_dropped);

I guess an ingress action can do better, by stealing the packet before hitting this point.

The following series introduce some pktgen changes

Patch01:
Cleanup my own work when I introduced NO_TIMESTAMP.

Patch02:
Took over patch from Alexei, and addressed my own concerns, as Alexie is too busy with other work, and this will provide an easy tool for measuring ingress path performance, which is a hot topic ATM.

Changes were primarily user interface related. Introduced a separate "xmit_mode" setting, instead of stealing one of the dev flags like Alexei did.