[PATCH net-next] pktgen: fix packet generation

[PATCH net-next] pktgen: fix packet generation

[Alexei Starovoitov]

pkt_gen->last_ok was not set properly, so after the first burst
pktgen instead of allocating new packet, will reuse old one, advance
eth_type_trans further, which would mean the stack will be seeing very
short bogus packets.

Fixes: 62f64aed622b ("pktgen: introduce xmit_mode '<start_xmit|netif_receive>'")
Signed-off-by: Alexei Starovoitov <ast@plumgrid.com>
---
This bug slipped through due to all code refactoring and can be seen
after clean reboot. If taps, rps or tx mode was used at least once,
the bug will be hidden.

Note to users: if you don't see ip_rcv() in your perf profile, it means
you were hitting this.
As commit log of 62f64aed622b is saying, the baseline perf profile
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

Jesper, that explains why you were seeing hot:
atomic_long_inc(&skb->dev->rx_dropped);

Only my V1 version was correct in this regard. V2 and above had this
latent bug. Sorry about that.

 net/core/pktgen.c |   11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/net/core/pktgen.c b/net/core/pktgen.c
index 8f2687da058e..62f979984a23 100644
--- a/net/core/pktgen.c
+++ b/net/core/pktgen.c
@@ -1189,6 +1189,16 @@ static ssize_t pktgen_if_write(struct file *file,
 				return -ENOTSUPP;
 
 			pkt_dev->xmit_mode = M_NETIF_RECEIVE;
+
+			/* make sure new packet is allocated every time
+			 * pktgen_xmit() is called
+			 */
+			pkt_dev->last_ok = 1;
+
+			/* override clone_skb if user passed default value
+			 * at module loading time
+			 */
+			pkt_dev->clone_skb = 0;
 		} else {
 			sprintf(pg_result,
 				"xmit_mode -:%s:- unknown\nAvailable modes: %s",
@@ -3415,7 +3425,6 @@ static void pktgen_xmit(struct pktgen_dev *pkt_dev)
 				/* 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
-- 
1.7.9.5

Re: [PATCH net-next] pktgen: fix packet generation

[Jesper Dangaard Brouer]

On Mon, 11 May 2015 15:19:48 -0700
Alexei Starovoitov <ast@plumgrid.com> wrote:

> pkt_gen->last_ok was not set properly, so after the first burst
> pktgen instead of allocating new packet, will reuse old one, advance
> eth_type_trans further, which would mean the stack will be seeing very
> short bogus packets.
> 
> Fixes: 62f64aed622b ("pktgen: introduce xmit_mode '<start_xmit|netif_receive>'")
> Signed-off-by: Alexei Starovoitov <ast@plumgrid.com>
> ---
> This bug slipped through due to all code refactoring and can be seen
> after clean reboot. If taps, rps or tx mode was used at least once,
> the bug will be hidden.
> 
> Note to users: if you don't see ip_rcv() in your perf profile, it means
> you were hitting this.
> As commit log of 62f64aed622b is saying, the baseline perf profile
> 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
> 
> Jesper, that explains why you were seeing hot:
> atomic_long_inc(&skb->dev->rx_dropped);

Acked-by: Jesper Dangaard Brouer <brouer@redhat.com>

Yes, just confirmed that this problem is gone. E.g. the multiqueue
script now scales without hitting this "skb->dev->rx_dropped".

 Device: eth4@0
  21127682pps 10141Mb/sec (10141287360bps) errors: 10000000
 Device: eth4@1
  22170175pps 10641Mb/sec (10641684000bps) errors: 10000000
 Device: eth4@2
  22230977pps 10670Mb/sec (10670868960bps) errors: 10000000
 Device: eth4@3
  22269033pps 10689Mb/sec (10689135840bps) errors: 10000000

I was also a little puzzled that I was not seeing kmem_cache_{alloc/free}
in my previous perf reports, but I just assumed burst was very efficient.

Good this got fixed, as my plan is to use this to profile the memory
allocators fast path for SKB alloc/free. 

Setting "burst = 0" (and flag NO_TIMESTAMP):
 Device: eth4@0
  3938513pps 1890Mb/sec (1890486240bps) errors: 10000000

Thus, performance hit from 22.1Mpps to 3.9Mpps, thus 209 nanosec more
expensive.  20% is the cost of pktgen itself, still I'm surprised that
the hit is this big, as this should hit the most optimal cache-hot case
of SKB alloc/free.

-- 
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 net-next] pktgen: fix packet generation

[Alexei Starovoitov]

On 5/12/15 1:19 AM, Jesper Dangaard Brouer wrote:
> On Mon, 11 May 2015 15:19:48 -0700
> Alexei Starovoitov <ast@plumgrid.com> wrote:
>
>> pkt_gen->last_ok was not set properly, so after the first burst
>> pktgen instead of allocating new packet, will reuse old one, advance
>> eth_type_trans further, which would mean the stack will be seeing very
>> short bogus packets.
>>
>> Fixes: 62f64aed622b ("pktgen: introduce xmit_mode '<start_xmit|netif_receive>'")
>> Signed-off-by: Alexei Starovoitov <ast@plumgrid.com>
>> ---
>> This bug slipped through due to all code refactoring and can be seen
>> after clean reboot. If taps, rps or tx mode was used at least once,
>> the bug will be hidden.
>>
>> Note to users: if you don't see ip_rcv() in your perf profile, it means
>> you were hitting this.
>> As commit log of 62f64aed622b is saying, the baseline perf profile
>> 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
>>
>> Jesper, that explains why you were seeing hot:
>> atomic_long_inc(&skb->dev->rx_dropped);
>
> Acked-by: Jesper Dangaard Brouer <brouer@redhat.com>

thanks!

> Yes, just confirmed that this problem is gone. E.g. the multiqueue
> script now scales without hitting this "skb->dev->rx_dropped".

great.

> Good this got fixed, as my plan is to use this to profile the memory
> allocators fast path for SKB alloc/free.
>
> Setting "burst = 0" (and flag NO_TIMESTAMP):
>   Device: eth4@0
>    3938513pps 1890Mb/sec (1890486240bps) errors: 10000000
>
> Thus, performance hit from 22.1Mpps to 3.9Mpps, thus 209 nanosec more
> expensive.  20% is the cost of pktgen itself, still I'm surprised that
> the hit is this big, as this should hit the most optimal cache-hot case
> of SKB alloc/free.

I tried similar and I think I've seen ip_send_check(iph); called by
pktgen->fill_packet_ipv4 to be quite hot, so burst=1 will be measuring
quite a bit more than just skb alloc/free. I think your skb allocator
micro bench was more accurate.

btw, this multi-core pktgen into netif_receive skb exposed all
spin_locks in tc actions. We need to convert them to rcu.

Re: [PATCH net-next] pktgen: fix packet generation

[David Miller]

From: Alexei Starovoitov <ast@plumgrid.com>
Date: Mon, 11 May 2015 15:19:48 -0700

> pkt_gen->last_ok was not set properly, so after the first burst
> pktgen instead of allocating new packet, will reuse old one, advance
> eth_type_trans further, which would mean the stack will be seeing very
> short bogus packets.
> 
> Fixes: 62f64aed622b ("pktgen: introduce xmit_mode '<start_xmit|netif_receive>'")
> Signed-off-by: Alexei Starovoitov <ast@plumgrid.com>

Applied, thanks for this fix.