[PATCH v3 net-next] pktgen: introduce 'rx' mode

[PATCH v3 net-next] pktgen: introduce 'rx' mode

[Alexei Starovoitov]

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.

Sample script 'pktgen.sh':
\#!/bin/bash
function pgset() {
  local result

  echo $1 > $PGDEV

  result=`cat $PGDEV | fgrep "Result: OK:"`
  if [ "$result" = "" ]; then
    cat $PGDEV | fgrep Result:
  fi
}

ETH=$1

PGDEV=/proc/net/pktgen/kpktgend_0
pgset "rem_device_all"
pgset "add_device $ETH"

PGDEV=/proc/net/pktgen/$ETH
pgset "rx"
pgset "pkt_size 60"
pgset "dst 99.1.1.2"
pgset "dst_mac 90:e2:ba:6e:a8:e5"
pgset "count 10000000"
pgset "burst 32"

PGDEV=/proc/net/pktgen/pgctrl
echo "Running... ctrl^C to stop"
pgset "start"
echo "Done"
cat /proc/net/pktgen/$ETH

Usage:
$ sudo ./pktgen.sh eth2
...
Result: OK: 232376(c232372+d3) usec, 10000000 (60byte,0frags)
  43033682pps 20656Mb/sec (20656167360bps) errors: 10000000

Raw netif_receive_skb speed should be ~43 million packet
per second on 3.7Ghz x86 and 'perf report' should look like:
  37.69%  kpktgend_0   [kernel.vmlinux]  [k] __netif_receive_skb_core
  25.81%  kpktgend_0   [kernel.vmlinux]  [k] kfree_skb
   7.22%  kpktgend_0   [kernel.vmlinux]  [k] ip_rcv
   5.68%  kpktgend_0   [pktgen]          [k] pktgen_thread_worker

if fib_table_lookup is seen on top, it means skb was processed
by the stack. To benchmark netif_receive_skb only make sure
that 'dst_mac' of your pktgen script is different from
receiving device mac and it will be dropped by ip_rcv

Signed-off-by: Alexei Starovoitov <ast@plumgrid.com>
---
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(-)

diff --git a/net/core/pktgen.c b/net/core/pktgen.c
index 508155b283dd..34fd5ece2681 100644
--- a/net/core/pktgen.c
+++ b/net/core/pktgen.c
@@ -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 */
-- 
1.7.9.5

Re: [PATCH v3 net-next] pktgen: introduce 'rx' mode

[Eric Dumazet]

On Thu, 2015-04-30 at 22:12 -0700, Alexei Starovoitov 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().

> Signed-off-by: Alexei Starovoitov <ast@plumgrid.com>
> ---
> 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

Acked-by: Eric Dumazet <edumazet@google.com>

Thanks !

Re: [PATCH v3 net-next] pktgen: introduce 'rx' mode

[Jesper Dangaard Brouer]

On Thu, 30 Apr 2015 22:12:10 -0700
Alexei Starovoitov <ast@plumgrid.com> 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)

Cc.ed Robert and Ben, and left the patch below for their review.


> Signed-off-by: Alexei Starovoitov <ast@plumgrid.com>
> ---
> 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(-)
> 
> diff --git a/net/core/pktgen.c b/net/core/pktgen.c
> index 508155b283dd..34fd5ece2681 100644
> --- a/net/core/pktgen.c
> +++ b/net/core/pktgen.c
> @@ -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 */



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

Re: [PATCH v3 net-next] pktgen: introduce 'rx' mode

[Daniel Borkmann]

Hi Jesper,

On 05/02/2015 10:46 AM, Jesper Dangaard Brouer wrote:
...
> 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. ;)

Cheers,
Daniel

Re: [PATCH v3 net-next] pktgen: introduce 'rx' mode

[Jesper Dangaard Brouer]

On Sat, 02 May 2015 11:44:45 +0200
Daniel Borkmann <daniel@iogearbox.net> wrote:

> Hi Jesper,
> 
> On 05/02/2015 10:46 AM, Jesper Dangaard Brouer wrote:
> ...
> > 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. ;)

Sure, out-of-tree code should not control our choices.

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

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

Re: [PATCH v3 net-next] pktgen: introduce 'rx' mode

[Daniel Borkmann]

On 05/02/2015 11:54 AM, Jesper Dangaard Brouer wrote:
...
> I personally just don't like the "RX" flag name.
> What about "STACK_RX" or "RX_INJECT"?

I'm not sure if it actually qualifies as a flag from user space
point of view, besides that it's used as an implementation detail.

You could have an option "xmit_mode [rx|tx]" (where obviously tx
is the default), perhaps that makes it clearer that you xmit into
the kernel's rx path.

Cheers,
Daniel

Re: [PATCH v3 net-next] pktgen: introduce 'rx' mode

[Alexei Starovoitov]

On 5/2/15 1:46 AM, Jesper Dangaard Brouer wrote:
> On Thu, 30 Apr 2015 22:12:10 -0700
> Alexei Starovoitov <ast@plumgrid.com> 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)

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.

Re: [PATCH v3 net-next] pktgen: introduce 'rx' mode

[Jesper Dangaard Brouer]

On Sat, 02 May 2015 09:01:27 -0700
Alexei Starovoitov <ast@plumgrid.com> wrote:

> On 5/2/15 1:46 AM, Jesper Dangaard Brouer wrote:
> > On Thu, 30 Apr 2015 22:12:10 -0700
> > Alexei Starovoitov <ast@plumgrid.com> 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)
> 
> 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.

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

Re: [PATCH v3 net-next] pktgen: introduce 'rx' mode

[Alexei Starovoitov]

On 5/2/15 9:46 AM, Jesper Dangaard Brouer wrote:
> 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]".

sure. that's fine too.

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

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

Re: [PATCH v3 net-next] pktgen: introduce 'rx' mode

[Jesper Dangaard Brouer]

On Sat, 02 May 2015 10:07:48 -0700 Alexei Starovoitov <ast@plumgrid.com> wrote:
> 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 :)

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

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

Re: [PATCH v3 net-next] pktgen: introduce 'rx' mode

[Alexei Starovoitov]

On 5/5/15 11:15 AM, Jesper Dangaard Brouer wrote:
>
> On Sat, 02 May 2015 10:07:48 -0700 Alexei Starovoitov <ast@plumgrid.com> wrote:
>> 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 :)
>
> Okay, I'll address my own concerns and post a patch in your name("From:")

lol :) I seriously don't care about commit count.
Thank you for taking over.
I'm swamped with tracing stuff at the moment.

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

same here. my motivation for this patch is to have a common tool
to measure ingress speed, so we can have concrete numbers to talk about.
Also, as you mentioned, there is always a danger of 'zooming in'.
Any type of benchmarking shouldn't be blindly followed. At the same time
all numbers are useful. They are different datapoints that make
a complete picture.

[PATCH 0/2] pktgen changes

[Jesper Dangaard Brouer]

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.

---

Alexei Starovoitov (1):
      pktgen: introduce xmit_mode 'rx_inject'

Jesper Dangaard Brouer (1):
      pktgen: adjust flag NO_TIMESTAMP to be more pktgen compliant


 Documentation/networking/pktgen.txt |    9 ++++
 net/core/pktgen.c                   |   87 +++++++++++++++++++++++++++++++++--
 2 files changed, 91 insertions(+), 5 deletions(-)

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

[PATCH 1/2] pktgen: adjust flag NO_TIMESTAMP to be more pktgen compliant

[Jesper Dangaard Brouer]

Allow flag NO_TIMESTAMP to turn timestamping on again, like other flags,
with a negation of the flag like !NO_TIMESTAMP.

Also document the option flag NO_TIMESTAMP.

Fixes: afb84b626184 ("pktgen: add flag NO_TIMESTAMP to disable timestamping")
Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
---

 Documentation/networking/pktgen.txt |    2 ++
 net/core/pktgen.c                   |    3 +++
 2 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/Documentation/networking/pktgen.txt b/Documentation/networking/pktgen.txt
index 0344f1d..6199ee6 100644
--- a/Documentation/networking/pktgen.txt
+++ b/Documentation/networking/pktgen.txt
@@ -145,6 +145,7 @@ Examples:
                               UDPCSUM,
                               IPSEC # IPsec encapsulation (needs CONFIG_XFRM)
                               NODE_ALLOC # node specific memory allocation
+                              NO_TIMESTAMP # disable timestamping
 
  pgset spi SPI_VALUE     Set specific SA used to transform packet.
 
@@ -287,6 +288,7 @@ flag
   UDPCSUM
   IPSEC
   NODE_ALLOC
+  NO_TIMESTAMP
 
 dst_min
 dst_max
diff --git a/net/core/pktgen.c b/net/core/pktgen.c
index 508155b..43bb215 100644
--- a/net/core/pktgen.c
+++ b/net/core/pktgen.c
@@ -1267,6 +1267,9 @@ static ssize_t pktgen_if_write(struct file *file,
 		else if (strcmp(f, "NO_TIMESTAMP") == 0)
 			pkt_dev->flags |= F_NO_TIMESTAMP;
 
+		else if (strcmp(f, "!NO_TIMESTAMP") == 0)
+			pkt_dev->flags &= ~F_NO_TIMESTAMP;
+
 		else {
 			sprintf(pg_result,
 				"Flag -:%s:- unknown\nAvailable flags, (prepend ! to un-set flag):\n%s",

[PATCH 2/2] pktgen: introduce xmit_mode 'rx_inject'

[Jesper Dangaard Brouer]

From: Alexei Starovoitov <ast@plumgrid.com>

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

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.

Sample script 'pktgen.sh':
\#!/bin/bash
function pgset() {
  local result

  echo $1 > $PGDEV

  result=`cat $PGDEV | fgrep "Result: OK:"`
  if [ "$result" = "" ]; then
    cat $PGDEV | fgrep Result:
  fi
}

ETH=$1

PGDEV=/proc/net/pktgen/kpktgend_0
pgset "rem_device_all"
pgset "add_device $ETH"

PGDEV=/proc/net/pktgen/$ETH
pgset "xmit_mode rx_inject"
pgset "pkt_size 60"
pgset "dst 99.1.1.2"
pgset "dst_mac 90:e2:ba:6e:a8:e5"
pgset "count 10000000"
pgset "burst 32"

PGDEV=/proc/net/pktgen/pgctrl
echo "Running... ctrl^C to stop"
pgset "start"
echo "Done"
cat /proc/net/pktgen/$ETH

Usage:
$ sudo ./pktgen.sh eth2
...
Result: OK: 232376(c232372+d3) usec, 10000000 (60byte,0frags)
  43033682pps 20656Mb/sec (20656167360bps) errors: 10000000

Raw netif_receive_skb speed should be ~43 million packet
per second on 3.7Ghz x86 and 'perf report' should look like:
  37.69%  kpktgend_0   [kernel.vmlinux]  [k] __netif_receive_skb_core
  25.81%  kpktgend_0   [kernel.vmlinux]  [k] kfree_skb
   7.22%  kpktgend_0   [kernel.vmlinux]  [k] ip_rcv
   5.68%  kpktgend_0   [pktgen]          [k] pktgen_thread_worker

If fib_table_lookup is seen on top, it means skb was processed
by the stack. To benchmark netif_receive_skb only make sure
that 'dst_mac' of your pktgen script is different from
receiving device mac and it will be dropped by ip_rcv

Signed-off-by: Alexei Starovoitov <ast@plumgrid.com>
Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>

---
v3->v4: Jesper address his own concerns
- User interface intro xmit_mode
- Mention xmit_mode in doc
- Make sure skb_clone cannot be used together with this mode

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

 Documentation/networking/pktgen.txt |    7 +++
 net/core/pktgen.c                   |   84 +++++++++++++++++++++++++++++++++--
 2 files changed, 86 insertions(+), 5 deletions(-)

diff --git a/Documentation/networking/pktgen.txt b/Documentation/networking/pktgen.txt
index 6199ee6..bfb5f2c 100644
--- a/Documentation/networking/pktgen.txt
+++ b/Documentation/networking/pktgen.txt
@@ -193,6 +193,10 @@ Examples:
  pgset "rate 300M"        set rate to 300 Mb/s
  pgset "ratep 1000000"    set rate to 1Mpps
 
+ pgset "xmit_mode rx_inject"	RX inject packets into stack netif_receive_skb()
+				Works with "burst" but not with "clone_skb".
+				Default xmit_mode is "tx".
+
 Sample scripts
 ==============
 
@@ -310,6 +314,9 @@ flowlen
 rate
 ratep
 
+xmit_mode
+
+
 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 43bb215..85195b2 100644
--- a/net/core/pktgen.c
+++ b/net/core/pktgen.c
@@ -210,6 +210,10 @@
 #define T_REMDEVALL   (1<<2)	/* Remove all devs */
 #define T_REMDEV      (1<<3)	/* Remove one dev */
 
+/* Xmit modes */
+#define M_TX		0	/* Default normal TX */
+#define M_RX_INJECT	1	/* RX inject packet into stack */
+
 /* If lock -- protects updating of if_list */
 #define   if_lock(t)           spin_lock(&(t->if_lock));
 #define   if_unlock(t)           spin_unlock(&(t->if_lock));
@@ -251,13 +255,14 @@ struct pktgen_dev {
 	 * we will do a random selection from within the range.
 	 */
 	__u32 flags;
-	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 */
+
 	struct page *page;
 	u64 delay;		/* nano-seconds */
 
@@ -620,6 +625,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->xmit_mode == M_RX_INJECT)
+		seq_puts(seq, "     xmit_mode: rx_inject\n");
+
 	seq_puts(seq, "     Flags: ");
 
 	if (pkt_dev->flags & F_IPV6)
@@ -1081,7 +1089,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->xmit_mode == M_RX_INJECT) ||
+		     !(pkt_dev->odev->priv_flags & IFF_TX_SKB_SHARING)))
 			return -ENOTSUPP;
 		i += len;
 		pkt_dev->clone_skb = value;
@@ -1134,7 +1143,7 @@ static ssize_t pktgen_if_write(struct file *file,
 			return len;
 
 		i += len;
-		if ((value > 1) &&
+		if ((value > 1) && (pkt_dev->xmit_mode == M_TX) &&
 		    (!(pkt_dev->odev->priv_flags & IFF_TX_SKB_SHARING)))
 			return -ENOTSUPP;
 		pkt_dev->burst = value < 1 ? 1 : value;
@@ -1160,6 +1169,35 @@ static ssize_t pktgen_if_write(struct file *file,
 			sprintf(pg_result, "ERROR: node not possible");
 		return count;
 	}
+	if (!strcmp(name, "xmit_mode")) {
+		char f[32];
+
+		memset(f, 0, 32);
+		len = strn_len(&user_buffer[i], sizeof(f) - 1);
+		if (len < 0)
+			return len;
+
+		if (copy_from_user(f, &user_buffer[i], len))
+			return -EFAULT;
+		i += len;
+
+		if (strcmp(f, "tx") == 0) {
+			pkt_dev->xmit_mode = M_TX;
+		} else if (strcmp(f, "rx_inject") == 0) {
+			/* clone_skb set earlier, not supported in this mode */
+			if (pkt_dev->clone_skb > 0)
+				return -ENOTSUPP;
+
+			pkt_dev->xmit_mode = M_RX_INJECT;
+		} else {
+			sprintf(pg_result,
+				"xmit_mode -:%s:- unknown\nAvailable modes: %s",
+				f, "tx, rx_inject\n");
+			return count;
+		}
+		sprintf(pg_result, "OK: xmit_mode=%s", f);
+		return count;
+	}
 	if (!strcmp(name, "flag")) {
 		char f[32];
 		memset(f, 0, 32);
@@ -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 */
@@ -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);
 	}
 
 	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 */

Re: [PATCH 2/2] pktgen: introduce xmit_mode 'rx_inject'

[Alexei Starovoitov]

On 5/5/15 1:30 PM, Jesper Dangaard Brouer wrote:
>
> 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"

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

>
> Signed-off-by: Alexei Starovoitov <ast@plumgrid.com>
> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
...
> @@ -251,13 +255,14 @@ struct pktgen_dev {
>   	 * we will do a random selection from within the range.
>   	 */
>   	__u32 flags;
> -	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.

Re: [PATCH 2/2] pktgen: introduce xmit_mode 'rx_inject'

[Jesper Dangaard Brouer]

On Tue, 05 May 2015 21:33:26 -0700
Alexei Starovoitov <ast@plumgrid.com> wrote:

> On 5/5/15 1:30 PM, Jesper Dangaard Brouer wrote:
> >
> > 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"
> 
> 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...


> > Signed-off-by: Alexei Starovoitov <ast@plumgrid.com>
> > Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
> ...
> > @@ -251,13 +255,14 @@ struct pktgen_dev {
> >   	 * we will do a random selection from within the range.
> >   	 */
> >   	__u32 flags;
> > -	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.

Because I wanted to place 'xmit_more' on a read-only/mostly cache-line,
although it likely does not matter too much, I just wanted to avoid any
funny cache coherency protocol interactions.


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

Hmm, that is strange. I think I see them. And they are on patchwork too.

https://patchwork.ozlabs.org/patch/468378/
https://patchwork.ozlabs.org/patch/468390/

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

Re: [PATCH 2/2] pktgen: introduce xmit_mode 'rx_inject'

[Daniel Borkmann]

On 05/06/2015 07:24 AM, Jesper Dangaard Brouer wrote:
> On Tue, 05 May 2015 21:33:26 -0700
> Alexei Starovoitov <ast@plumgrid.com> wrote:
>> On 5/5/15 1:30 PM, Jesper Dangaard Brouer wrote:
>>>
>>> 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"
>>
>> 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...

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.

Cheers,
Daniel

Re: [PATCH 2/2] pktgen: introduce xmit_mode 'rx_inject'

[Jesper Dangaard Brouer]

On Wed, 06 May 2015 12:17:02 +0200
Daniel Borkmann <daniel@iogearbox.net> wrote:

> On 05/06/2015 07:24 AM, Jesper Dangaard Brouer wrote:
> > On Tue, 05 May 2015 21:33:26 -0700
> > Alexei Starovoitov <ast@plumgrid.com> wrote:
> >> On 5/5/15 1:30 PM, Jesper Dangaard Brouer wrote:
> >>>
> >>> 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"
> >>
> >> 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...
> 
> 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? 

Not correct, I really don't care about the two out-of-tree pktgen projects.

My main concern is to avoid polluting the pktgen "user-interface" (more
than it already is), with a bare option like "rx" which is in no-way
descriptive of its functionality.

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

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

If someone have time, I would like to see better tools that allow us to
measure different parts of the stack.  For now, this is what we got.
And this feature provide an easy and quick way to measure the ingress
code-path, which have gotten a lot of discussion lately... which after
this patch can easily be resolved by measuring instead of hand-waving ;-)

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

Re: [PATCH 2/2] pktgen: introduce xmit_mode 'rx_inject'

[Alexander Duyck]

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

<snip>

> diff --git a/net/core/pktgen.c b/net/core/pktgen.c
> index 43bb215..85195b2 100644
> --- a/net/core/pktgen.c
> +++ b/net/core/pktgen.c
> @@ -210,6 +210,10 @@

<snip>

> @@ -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 */
> @@ -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?

>   	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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH 2/2] pktgen: introduce xmit_mode 'rx_inject'

[Jesper Dangaard Brouer]

On Tue, 05 May 2015 22:32:34 -0700
Alexander Duyck <alexander.duyck@gmail.com> wrote:

> On 05/05/2015 01:30 PM, Jesper Dangaard Brouer wrote:
> 
> <snip>
> 
> > diff --git a/net/core/pktgen.c b/net/core/pktgen.c
> > index 43bb215..85195b2 100644
> > --- a/net/core/pktgen.c
> > +++ b/net/core/pktgen.c
> > @@ -210,6 +210,10 @@
> 
> <snip>
> 
> > @@ -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 */
> > @@ -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?

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


> >   	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 majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >
> 



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

Re: [PATCH 2/2] pktgen: introduce xmit_mode 'rx_inject'

[Alexei Starovoitov]

On 5/6/15 1:44 AM, Jesper Dangaard Brouer wrote:
>>
>> 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...)

I think this optimization makes sense. Cleans up the code a little too.

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

I would vote for: 'mode netif_receive' + 'mode start_xmit'
then if out-of-tree guys would want to upstream their stuff something
like 'mode parse_and_consume' can fit.

[PATCH v5 0/2] pktgen changes

[Jesper Dangaard Brouer]

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.

---

Alexei Starovoitov (1):
      pktgen: introduce xmit_mode '<start_xmit|netif_receive>'

Jesper Dangaard Brouer (1):
      pktgen: adjust flag NO_TIMESTAMP to be more pktgen compliant


 Documentation/networking/pktgen.txt |    9 ++++
 net/core/pktgen.c                   |   85 +++++++++++++++++++++++++++++++++--
 2 files changed, 89 insertions(+), 5 deletions(-)

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

[PATCH v5 1/2] pktgen: adjust flag NO_TIMESTAMP to be more pktgen compliant

[Jesper Dangaard Brouer]

Allow flag NO_TIMESTAMP to turn timestamping on again, like other flags,
with a negation of the flag like !NO_TIMESTAMP.

Also document the option flag NO_TIMESTAMP.

Fixes: afb84b626184 ("pktgen: add flag NO_TIMESTAMP to disable timestamping")
Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
---

 Documentation/networking/pktgen.txt |    2 ++
 net/core/pktgen.c                   |    3 +++
 2 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/Documentation/networking/pktgen.txt b/Documentation/networking/pktgen.txt
index 0344f1d..6199ee6 100644
--- a/Documentation/networking/pktgen.txt
+++ b/Documentation/networking/pktgen.txt
@@ -145,6 +145,7 @@ Examples:
                               UDPCSUM,
                               IPSEC # IPsec encapsulation (needs CONFIG_XFRM)
                               NODE_ALLOC # node specific memory allocation
+                              NO_TIMESTAMP # disable timestamping
 
  pgset spi SPI_VALUE     Set specific SA used to transform packet.
 
@@ -287,6 +288,7 @@ flag
   UDPCSUM
   IPSEC
   NODE_ALLOC
+  NO_TIMESTAMP
 
 dst_min
 dst_max
diff --git a/net/core/pktgen.c b/net/core/pktgen.c
index 508155b..43bb215 100644
--- a/net/core/pktgen.c
+++ b/net/core/pktgen.c
@@ -1267,6 +1267,9 @@ static ssize_t pktgen_if_write(struct file *file,
 		else if (strcmp(f, "NO_TIMESTAMP") == 0)
 			pkt_dev->flags |= F_NO_TIMESTAMP;
 
+		else if (strcmp(f, "!NO_TIMESTAMP") == 0)
+			pkt_dev->flags &= ~F_NO_TIMESTAMP;
+
 		else {
 			sprintf(pg_result,
 				"Flag -:%s:- unknown\nAvailable flags, (prepend ! to un-set flag):\n%s",

[PATCH v5 2/2] pktgen: introduce xmit_mode '<start_xmit|netif_receive>'

[Jesper Dangaard Brouer]

From: Alexei Starovoitov <ast@plumgrid.com>

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

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.

Sample script 'pktgen.sh':
\#!/bin/bash
function pgset() {
  local result

  echo $1 > $PGDEV

  result=`cat $PGDEV | fgrep "Result: OK:"`
  if [ "$result" = "" ]; then
    cat $PGDEV | fgrep Result:
  fi
}

[ -z "$1" ] && echo "Usage: $0 DEV" && exit 1
ETH=$1

PGDEV=/proc/net/pktgen/kpktgend_0
pgset "rem_device_all"
pgset "add_device $ETH"

PGDEV=/proc/net/pktgen/$ETH
pgset "xmit_mode netif_receive"
pgset "pkt_size 60"
pgset "dst 198.18.0.1"
pgset "dst_mac 90:e2:ba:ff:ff:ff"
pgset "count 10000000"
pgset "burst 32"

PGDEV=/proc/net/pktgen/pgctrl
echo "Running... ctrl^C to stop"
pgset "start"
echo "Done"
cat /proc/net/pktgen/$ETH

Usage:
$ sudo ./pktgen.sh eth2
...
Result: OK: 232376(c232372+d3) usec, 10000000 (60byte,0frags)
  43033682pps 20656Mb/sec (20656167360bps) errors: 10000000

Raw netif_receive_skb speed should be ~43 million packet
per second on 3.7Ghz x86 and 'perf report' should look like:
  37.69%  kpktgend_0   [kernel.vmlinux]  [k] __netif_receive_skb_core
  25.81%  kpktgend_0   [kernel.vmlinux]  [k] kfree_skb
   7.22%  kpktgend_0   [kernel.vmlinux]  [k] ip_rcv
   5.68%  kpktgend_0   [pktgen]          [k] pktgen_thread_worker

If fib_table_lookup is seen on top, it means skb was processed
by the stack. To benchmark netif_receive_skb only make sure
that 'dst_mac' of your pktgen script is different from
receiving device mac and it will be dropped by ip_rcv

Signed-off-by: Alexei Starovoitov <ast@plumgrid.com>
Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>

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

v3->v4: Jesper address his own concerns
- User interface intro xmit_mode
- Mention xmit_mode in doc
- Make sure skb_clone cannot be used together with this mode

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

---

 Documentation/networking/pktgen.txt |    7 +++
 net/core/pktgen.c                   |   82 +++++++++++++++++++++++++++++++++--
 2 files changed, 84 insertions(+), 5 deletions(-)

diff --git a/Documentation/networking/pktgen.txt b/Documentation/networking/pktgen.txt
index 6199ee6..747facc 100644
--- a/Documentation/networking/pktgen.txt
+++ b/Documentation/networking/pktgen.txt
@@ -193,6 +193,10 @@ Examples:
  pgset "rate 300M"        set rate to 300 Mb/s
  pgset "ratep 1000000"    set rate to 1Mpps
 
+ pgset "xmit_mode netif_receive"  RX inject into stack netif_receive_skb()
+				  Works with "burst" but not with "clone_skb".
+				  Default xmit_mode is "start_xmit".
+
 Sample scripts
 ==============
 
@@ -310,6 +314,9 @@ flowlen
 rate
 ratep
 
+xmit_mode <start_xmit|netif_receive>
+
+
 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 43bb215..8f2687d 100644
--- a/net/core/pktgen.c
+++ b/net/core/pktgen.c
@@ -210,6 +210,10 @@
 #define T_REMDEVALL   (1<<2)	/* Remove all devs */
 #define T_REMDEV      (1<<3)	/* Remove one dev */
 
+/* Xmit modes */
+#define M_START_XMIT		0	/* Default normal TX */
+#define M_NETIF_RECEIVE 	1	/* Inject packets into stack */
+
 /* If lock -- protects updating of if_list */
 #define   if_lock(t)           spin_lock(&(t->if_lock));
 #define   if_unlock(t)           spin_unlock(&(t->if_lock));
@@ -251,13 +255,14 @@ struct pktgen_dev {
 	 * we will do a random selection from within the range.
 	 */
 	__u32 flags;
-	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 */
+
 	struct page *page;
 	u64 delay;		/* nano-seconds */
 
@@ -620,6 +625,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->xmit_mode == M_NETIF_RECEIVE)
+		seq_puts(seq, "     xmit_mode: netif_receive\n");
+
 	seq_puts(seq, "     Flags: ");
 
 	if (pkt_dev->flags & F_IPV6)
@@ -1081,7 +1089,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->xmit_mode == M_NETIF_RECEIVE) ||
+		     !(pkt_dev->odev->priv_flags & IFF_TX_SKB_SHARING)))
 			return -ENOTSUPP;
 		i += len;
 		pkt_dev->clone_skb = value;
@@ -1134,7 +1143,7 @@ static ssize_t pktgen_if_write(struct file *file,
 			return len;
 
 		i += len;
-		if ((value > 1) &&
+		if ((value > 1) && (pkt_dev->xmit_mode == M_START_XMIT) &&
 		    (!(pkt_dev->odev->priv_flags & IFF_TX_SKB_SHARING)))
 			return -ENOTSUPP;
 		pkt_dev->burst = value < 1 ? 1 : value;
@@ -1160,6 +1169,35 @@ static ssize_t pktgen_if_write(struct file *file,
 			sprintf(pg_result, "ERROR: node not possible");
 		return count;
 	}
+	if (!strcmp(name, "xmit_mode")) {
+		char f[32];
+
+		memset(f, 0, 32);
+		len = strn_len(&user_buffer[i], sizeof(f) - 1);
+		if (len < 0)
+			return len;
+
+		if (copy_from_user(f, &user_buffer[i], len))
+			return -EFAULT;
+		i += len;
+
+		if (strcmp(f, "start_xmit") == 0) {
+			pkt_dev->xmit_mode = M_START_XMIT;
+		} else if (strcmp(f, "netif_receive") == 0) {
+			/* clone_skb set earlier, not supported in this mode */
+			if (pkt_dev->clone_skb > 0)
+				return -ENOTSUPP;
+
+			pkt_dev->xmit_mode = M_NETIF_RECEIVE;
+		} else {
+			sprintf(pg_result,
+				"xmit_mode -:%s:- unknown\nAvailable modes: %s",
+				f, "start_xmit, netif_receive\n");
+			return count;
+		}
+		sprintf(pg_result, "OK: xmit_mode=%s", f);
+		return count;
+	}
 	if (!strcmp(name, "flag")) {
 		char f[32];
 		memset(f, 0, 32);
@@ -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 */
@@ -3357,6 +3396,38 @@ static void pktgen_xmit(struct pktgen_dev *pkt_dev)
 	if (pkt_dev->delay && pkt_dev->last_ok)
 		spin(pkt_dev, pkt_dev->next_tx);
 
+	if (pkt_dev->xmit_mode == M_NETIF_RECEIVE) {
+		skb = pkt_dev->skb;
+		skb->protocol = eth_type_trans(skb, skb->dev);
+		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_START_XMIT */
+	}
+
 	txq = skb_get_tx_queue(odev, pkt_dev->skb);
 
 	local_bh_disable();
@@ -3404,6 +3475,7 @@ xmit_more:
 unlock:
 	HARD_TX_UNLOCK(odev, txq);
 
+out:
 	local_bh_enable();
 
 	/* If pkt_dev->count is zero, then run forever */

Re: [PATCH v5 2/2] pktgen: introduce xmit_mode '<start_xmit|netif_receive>'

[Alexei Starovoitov]

On 5/7/15 7:35 AM, Jesper Dangaard Brouer wrote:
> From: Alexei Starovoitov <ast@plumgrid.com>
>
> 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 <ast@plumgrid.com>
> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
>
> ---
> 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.

Re: [PATCH v5 2/2] pktgen: introduce xmit_mode '<start_xmit|netif_receive>'

[Daniel Borkmann]

On 05/07/2015 06:28 PM, Alexei Starovoitov wrote:
> On 5/7/15 7:35 AM, Jesper Dangaard Brouer wrote:
>> From: Alexei Starovoitov <ast@plumgrid.com>
>>
>> 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 <ast@plumgrid.com>
>> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
>>
>> ---
>> 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 <robert.olsson@its.uu.se>
Date:   Thu Aug 7 02:23:01 2008 -0700

     pktgen: multiqueue etc.

Re: [PATCH v5 2/2] pktgen: introduce xmit_mode '<start_xmit|netif_receive>'

[Alexei Starovoitov]

On 5/7/15 10:11 AM, Daniel Borkmann wrote:
>
> You mean other than below commit (iow independant of queue mapping)?
>
> commit e6fce5b916cd7f7f79b2b3e53ba74bbfc1d7cf8b
> Author: Robert Olsson <robert.olsson@its.uu.se>
> Date:   Thu Aug 7 02:23:01 2008 -0700
>
>      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.

Re: [PATCH v5 2/2] pktgen: introduce xmit_mode '<start_xmit|netif_receive>'

[Daniel Borkmann]

On 05/07/2015 07:16 PM, Alexei Starovoitov wrote:
> On 5/7/15 10:11 AM, Daniel Borkmann wrote:
>>
>> You mean other than below commit (iow independant of queue mapping)?
>>
>> commit e6fce5b916cd7f7f79b2b3e53ba74bbfc1d7cf8b
>> Author: Robert Olsson <robert.olsson@its.uu.se>
>> Date:   Thu Aug 7 02:23:01 2008 -0700
>>
>>      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.

Ok, in any case, I think it would be good if there's some
documentation for that as well. :)

Multiqueue pktgen (was: [PATCH v5 2/2] pktgen: introduce xmit_mode '<start_xmit|netif_receive>')

[Jesper Dangaard Brouer]

On Thu, 07 May 2015 19:11:58 +0200 Daniel Borkmann <daniel@iogearbox.net> wrote:

> On 05/07/2015 06:28 PM, Alexei Starovoitov wrote:
> > On 5/7/15 7:35 AM, Jesper Dangaard Brouer wrote:
> >> From: Alexei Starovoitov <ast@plumgrid.com>
> >>
[...snip...]

> > 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 <robert.olsson@its.uu.se>
> 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".

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

[1] https://github.com/netoptimizer/network-testing/tree/master/pktgen
- - 
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

multiqueue pktgen script:


#!/bin/bash
function pgset() {
    local result

    echo $1 > $PGDEV

    result=`cat $PGDEV | fgrep "Result: OK:"`
    if [ "$result" = "" ]; then
        cat $PGDEV | fgrep Result:
    fi
}

[ -z "$2" ] && echo "Usage: $0 DEV num_threads" && exit 1
ETH=$1
NUM_THREADS=$2
let "NUM_THREADS -= 1"
echo "Number of threads to start: $2 (0 to $NUM_THREADS)"

# General cleanup everything since last run
PGDEV=/proc/net/pktgen/pgctrl
pgset "reset"

# Add devices to threads
#  Notice the naming scheme ETH@NUM
for NUM in `seq 0 $NUM_THREADS`; do
    PGDEV=/proc/net/pktgen/kpktgend_${NUM}
    pgset "rem_device_all"
    pgset "add_device ${ETH}@${NUM}"
done

# Config each device
for NUM in `seq 0 $NUM_THREADS`; do
    PGDEV=/proc/net/pktgen/${ETH}@${NUM}
    pgset "flag QUEUE_MAP_CPU"
    pgset "xmit_mode netif_receive"
    pgset "pkt_size 60"
    pgset "dst 198.18.0.42"
    pgset "dst_mac 90:e2:ba:ff:ff:ff"
    pgset "count 10000000"
    pgset "burst 32"
done

PGDEV=/proc/net/pktgen/pgctrl
echo "Running... ctrl^C to stop"
pgset "start"
echo "Done"

for NUM in `seq 0 $NUM_THREADS`; do
    echo "Device: ${ETH}@${NUM}"
    cat /proc/net/pktgen/${ETH}@${NUM} | grep -A2 "Result:"
done

Re: [PATCH v5 2/2] pktgen: introduce xmit_mode '<start_xmit|netif_receive>'

[Jesper Dangaard Brouer]

On Thu, 07 May 2015 19:11:58 +0200 Daniel Borkmann <daniel@iogearbox.net> wrote:

> On 05/07/2015 06:28 PM, Alexei Starovoitov wrote:  
> > On 5/7/15 7:35 AM, Jesper Dangaard Brouer wrote:  
> >> From: Alexei Starovoitov <ast@plumgrid.com>
> >>  
[...snip...]

> > 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 <robert.olsson@its.uu.se>
> 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".

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

[1] https://github.com/netoptimizer/network-testing/tree/master/pktgen
- - 
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

multiqueue pktgen script:


#!/bin/bash
function pgset() {
    local result

    echo $1 > $PGDEV

    result=`cat $PGDEV | fgrep "Result: OK:"`
    if [ "$result" = "" ]; then
        cat $PGDEV | fgrep Result:
    fi
}

[ -z "$2" ] && echo "Usage: $0 DEV num_threads" && exit 1
ETH=$1
NUM_THREADS=$2
let "NUM_THREADS -= 1"
echo "Number of threads to start: $2 (0 to $NUM_THREADS)"

# General cleanup everything since last run
PGDEV=/proc/net/pktgen/pgctrl
pgset "reset"

# Add devices to threads
#  Notice the naming scheme ETH@NUM
for NUM in `seq 0 $NUM_THREADS`; do
    PGDEV=/proc/net/pktgen/kpktgend_${NUM}
    pgset "rem_device_all"
    pgset "add_device ${ETH}@${NUM}"
done

# Config each device
for NUM in `seq 0 $NUM_THREADS`; do
    PGDEV=/proc/net/pktgen/${ETH}@${NUM}
    pgset "flag QUEUE_MAP_CPU"
    pgset "xmit_mode netif_receive"
    pgset "pkt_size 60"
    pgset "dst 198.18.0.42"
    pgset "dst_mac 90:e2:ba:ff:ff:ff"
    pgset "count 10000000"
    pgset "burst 32"
done

PGDEV=/proc/net/pktgen/pgctrl
echo "Running... ctrl^C to stop"
pgset "start"
echo "Done"

for NUM in `seq 0 $NUM_THREADS`; do
    echo "Device: ${ETH}@${NUM}"
    cat /proc/net/pktgen/${ETH}@${NUM} | grep -A2 "Result:"
done

Multiqueue pktgen and ingress path (Was: [PATCH v5 2/2] pktgen: introduce xmit_mode '<start_xmit|netif_receive>')

[Jesper Dangaard Brouer]

[-- Attachment #1: Type: text/plain, Size: 2772 bytes --]

On Fri, 8 May 2015 17:39:00 +0200
Jesper Dangaard Brouer <brouer@redhat.com> wrote:

> On Thu, 07 May 2015 19:11:58 +0200 Daniel Borkmann <daniel@iogearbox.net> wrote:
> 
> > On 05/07/2015 06:28 PM, Alexei Starovoitov wrote:  
> > > On 5/7/15 7:35 AM, Jesper Dangaard Brouer wrote:  
> > >> From: Alexei Starovoitov <ast@plumgrid.com>
> > >>  
> [...snip...]
> 
> > > 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 <robert.olsson@its.uu.se>
> > 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.

Now attached.

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

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.

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

[-- Attachment #2: pktgen_multiqueue01_xmit_mode.sh --]
[-- Type: application/x-shellscript, Size: 1217 bytes --]

Re: Multiqueue pktgen and ingress path (Was: [PATCH v5 2/2] pktgen: introduce xmit_mode '<start_xmit|netif_receive>')

[Eric Dumazet]

On Fri, 2015-05-08 at 17:49 +0200, Jesper Dangaard Brouer wrote:

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

Law of physics, since this cache line has to constantly fly among your
cpus.

Re: Multiqueue pktgen and ingress path (Was: [PATCH v5 2/2] pktgen: introduce xmit_mode '<start_xmit|netif_receive>')

[Alexander Duyck]

On 05/08/2015 08:49 AM, Jesper Dangaard Brouer wrote:
> 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.

The atomic access likely acts as a serializing event, and on top of that 
it would increase in time needed to be completed as you add more 
threads.  I am guessing the 8ns is probably the cost for a single 
threaded setup where the memory location is available in L2 or L1 
cache.  If it is in L3 cache that would make it more expensive.  If it 
is currently in use by another CPU then that would make it even more 
expensive.  If it is in use on another socket then we are probably 
looking at something in the high 10s if not 100s of nanoseconds.  Once 
you hit the point where the time for the atomic transaction multiplied 
by the number of threads is equal to the time it takes for any one 
thread to complete the operation you have hit the upper limit and 
everything after that is just wasted cycles spinning while waiting for 
cache line access.

So for example if you had 2 threads on the same socket you are looking 
at an L3 cache access which takes about 30 cycles.  That 30 cycles would 
likely be in addition to the 8ns you were already seeing for single 
thread performance, and I don't know if it includes the cache flush 
needed by the remote L1/L2 where the cache line currently resides.  I'd 
be interested in seeing what the 2 socket data looked like as I suspect 
you would take an even heavier hit for that.

- Alex

Re: Multiqueue pktgen and ingress path (Was: [PATCH v5 2/2] pktgen: introduce xmit_mode '<start_xmit|netif_receive>')

[Alexei Starovoitov]

On 5/8/15 8:49 AM, Jesper Dangaard Brouer wrote:
>>
>> 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 ?

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

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 ?

Re: Multiqueue pktgen and ingress path (Was: [PATCH v5 2/2] pktgen: introduce xmit_mode '<start_xmit|netif_receive>')

[Alexander Duyck]

On 05/08/2015 10:00 AM, Alexei Starovoitov wrote:
> On 5/8/15 8:49 AM, Jesper Dangaard Brouer wrote:
>>>
>>> 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 ?
>
>>> 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.
>
> 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 ?

I'm assuming it just has to be a packet that isn't claimed by any 
sockets or interfaces registered on top of the device.  After all it 
isn't as if pktgen sends an unknown protocol so I would assume just 
enabling promiscuous mode on an interface without a bridge or raw socket 
would probably be enough to trigger this.  The overhead itself would 
show up in __netif_receive_skb_core in perf since the atomic_inc would 
be inlined.

I would think a common case where something like this might be seen 
would be if you registered enough macvlan interfaces to force a device 
to switch into promiscuous mode and then received traffic that wasn't 
meant for any of the other interfaces.

- Alex

Re: [PATCH v5 2/2] pktgen: introduce xmit_mode '<start_xmit|netif_receive>'

[Eric Dumazet]

On Fri, 2015-05-08 at 17:39 +0200, Jesper Dangaard Brouer wrote:

> The funny thing now is that scaling does not "happen" as we stall on:
>    atomic_long_inc(&skb->dev->rx_dropped);

Note we already have percpu device refcnt.

We could extend this to contain a per cpu rx_dropped, but I guess only
these tests hit this path, so I simply used one atomic_long_t here.

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

Re: [PATCH v5 2/2] pktgen: introduce xmit_mode '<start_xmit|netif_receive>'

[Alexei Starovoitov]

On 5/8/15 8:57 AM, Eric Dumazet wrote:
>
> I guess an ingress action can do better, by stealing the packet before
> hitting this point.

unfortunately not yet :(
We have spin_lock(&gact->tcf_lock); in tcf_gact,
so simple 'action drop' doesn't scale.
Shouldn't be hard to fix though.

Re: [PATCH v5 0/2] pktgen changes

[David Miller]

From: Jesper Dangaard Brouer <brouer@redhat.com>
Date: Thu, 07 May 2015 16:34:29 +0200

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

Series applied, thanks Jesper.