Re: [PATCH] pktgen: nowait parameter.

[Jesper Dangaard Brouer <brouer@redhat.com>]

On Wed, 03 Sep 2014 13:50:01 +0930
Rusty Russell <rusty@rustcorp.com.au> wrote:

> While trying to measure speed of virtio_net, I was getting hangs.
> This is because we skb_orphan() but delay the tx interrupt
> indefinitely (by number of slots).
> 
> With nowait, pktgen won't wait for the skb to be released.  This
> introduces an error, but it's ok if count >> ringsize.

This pktgen_wait_for_skb() only happens it the exit case, when count
packets have been send.  I guess its okay to proceed to
pktgen_stop_device() which will call kfree_skb(pkt_dev->skb) with
refcnt=2, decrementing to refcnt=1, and then we depend on driver to
eventually call kfree_skb().
 
> I updated the documentation, but it needs far more work (it
> refers to pgset and an examples directory, none of which exist
> in the kernel tree).

Yes, the doc is not in such a good shape.

I'm not 100% happy with the name "nowait" parameter, as users could
easily misunderstand the purpose of this parameter.  But I've not come
up with a better name, e.g. "exit_nowait" is also not the best.


> diff --git a/net/core/pktgen.c b/net/core/pktgen.c
> index 8b849ddfef2e..adc41f2b3bc7 100644
> --- a/net/core/pktgen.c
> +++ b/net/core/pktgen.c
> @@ -290,6 +290,11 @@ struct pktgen_dev {
>  				 * set clone_skb to 1024.
>  				 */
>  
> +	bool no_wait;		/*
> +				 * Don't wait for packet to be freed
> +				 * by driver
> +				 */
> +

DaveM prefers multi line comments like:

 /* Don't wait for packet to be freed
  * by driver
  */


>  	char dst_min[IP_NAME_SZ];	/* IP, ie 1.2.3.4 */
>  	char dst_max[IP_NAME_SZ];	/* IP, ie 1.2.3.4 */
>  	char src_min[IP_NAME_SZ];	/* IP, ie 1.2.3.4 */
> @@ -679,6 +684,9 @@ static int pktgen_if_show(struct seq_file *seq, void *v)
>  
>  	seq_puts(seq, "\n");
>  
> +	if (pkt_dev->no_wait)
> +		seq_puts(seq, "     nowait\n");
> +

Shouldn't you put this print statement above the "Flags:" section?

>  	/* not really stopped, more like last-running-at */
>  	stopped = pkt_dev->running ? ktime_get() : pkt_dev->stopped_at;
>  	idle = pkt_dev->idle_acc;
> @@ -1711,6 +1719,17 @@ static ssize_t pktgen_if_write(struct file *file,

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Sr. Network Kernel Developer at Red Hat
  Author of http://www.iptv-analyzer.org
  LinkedIn: http://www.linkedin.com/in/brouer