netdev - Re: [PATCH] pktgen: nowait parameter.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20140903110956.0872841f@redhat.com>
Date:	Wed, 3 Sep 2014 11:09:56 +0200
From:	Jesper Dangaard Brouer <brouer@...hat.com>
To:	Rusty Russell <rusty@...tcorp.com.au>
Cc:	"netdev" <netdev@...r.kernel.org>,
	Mathias Krause <minipli@...glemail.com>, brouer@...hat.com,
	Robert Olsson <robert@...julf.net>
Subject: Re: [PATCH] pktgen: nowait parameter.

On Wed, 03 Sep 2014 13:50:01 +0930
Rusty Russell <rusty@...tcorp.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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html