[PATCH 2/3][NET] gen_estimator: list_empty() check in est_timer() fixed

[PATCH 2/3][NET] gen_estimator: list_empty() check in est_timer() fixed

[Jarek Poplawski]

This patch changes the method of checking for empty list in est_timer():
list_empty() is not recommended for RCU protected lists. Now, it's simply
a variable used for this.

Signed-off-by: Jarek Poplawski <jarkao2@gmail.com>

---

diff -Nurp 2.6.24-rc8-mm1-p1-/net/core/gen_estimator.c 2.6.24-rc8-mm1-p1+/net/core/gen_estimator.c
--- 2.6.24-rc8-mm1-p1-/net/core/gen_estimator.c	2008-01-20 20:58:35.000000000 +0100
+++ 2.6.24-rc8-mm1-p1+/net/core/gen_estimator.c	2008-01-20 21:07:42.000000000 +0100
@@ -106,6 +106,7 @@ static void est_timer(unsigned long arg)
 {
 	int idx = (int)arg;
 	struct gen_estimator *e;
+	int list_not_empty = 0;
 
 	rcu_read_lock();
 	list_for_each_entry_rcu(e, &elist[idx].list, list) {
@@ -118,6 +119,9 @@ static void est_timer(unsigned long arg)
 		if (e->bstats == NULL)
 			goto skip;
 
+		if (list_not_empty == 0)
+			list_not_empty = 1;
+
 		nbytes = e->bstats->bytes;
 		npackets = e->bstats->packets;
 		rate = (nbytes - e->last_bytes)<<(7 - idx);
@@ -134,7 +138,7 @@ skip:
 		spin_unlock(e->stats_lock);
 	}
 
-	if (!list_empty(&elist[idx].list))
+	if (list_not_empty)
 		mod_timer(&elist[idx].timer, jiffies + ((HZ/4) << idx));
 	rcu_read_unlock();
 }

Re: [PATCH 2/3][NET] gen_estimator: list_empty() check in est_timer() fixed

[Stephen Hemminger]

On Mon, 21 Jan 2008 00:49:59 +0100
Jarek Poplawski <jarkao2@gmail.com> wrote:

> This patch changes the method of checking for empty list in est_timer():
> list_empty() is not recommended for RCU protected lists. Now, it's simply
> a variable used for this.
> 
> Signed-off-by: Jarek Poplawski <jarkao2@gmail.com>
> 
> ---
> 
> diff -Nurp 2.6.24-rc8-mm1-p1-/net/core/gen_estimator.c 2.6.24-rc8-mm1-p1+/net/core/gen_estimator.c
> --- 2.6.24-rc8-mm1-p1-/net/core/gen_estimator.c	2008-01-20 20:58:35.000000000 +0100
> +++ 2.6.24-rc8-mm1-p1+/net/core/gen_estimator.c	2008-01-20 21:07:42.000000000 +0100
> @@ -106,6 +106,7 @@ static void est_timer(unsigned long arg)
>  {
>  	int idx = (int)arg;
>  	struct gen_estimator *e;
> +	int list_not_empty = 0;

Using a negative name for what is a boolean value leads
to code that reads like a double negative sentence. Better to choose
a variable name that is direct, can't use list_empty because that
is a macro, so how about "estimator_found".

>  
>  	rcu_read_lock();
>  	list_for_each_entry_rcu(e, &elist[idx].list, list) {
> @@ -118,6 +119,9 @@ static void est_timer(unsigned long arg)
>  		if (e->bstats == NULL)
>  			goto skip;
>  
> +		if (list_not_empty == 0)
> +			list_not_empty = 1;
> +
>  		nbytes = e->bstats->bytes;
>  		npackets = e->bstats->packets;
>  		rate = (nbytes - e->last_bytes)<<(7 - idx);
> @@ -134,7 +138,7 @@ skip:
>  		spin_unlock(e->stats_lock);
>  	}
>  
> -	if (!list_empty(&elist[idx].list))
> +	if (list_not_empty)
>  		mod_timer(&elist[idx].timer, jiffies + ((HZ/4) << idx));
>  	rcu_read_unlock();
>  }
> --
> 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
> 


-- 
Stephen Hemminger <stephen.hemminger@vyatta.com>

Re: [PATCH 2/3][NET] gen_estimator: list_empty() check in est_timer() fixed

[Jarek Poplawski]

On Sun, Jan 20, 2008 at 03:55:44PM -0800, Stephen Hemminger wrote:
> On Mon, 21 Jan 2008 00:49:59 +0100
> Jarek Poplawski <jarkao2@gmail.com> wrote:
> 
> > This patch changes the method of checking for empty list in est_timer():
> > list_empty() is not recommended for RCU protected lists. Now, it's simply
> > a variable used for this.
> > 
> > Signed-off-by: Jarek Poplawski <jarkao2@gmail.com>
> > 
> > ---
> > 
> > diff -Nurp 2.6.24-rc8-mm1-p1-/net/core/gen_estimator.c 2.6.24-rc8-mm1-p1+/net/core/gen_estimator.c
> > --- 2.6.24-rc8-mm1-p1-/net/core/gen_estimator.c	2008-01-20 20:58:35.000000000 +0100
> > +++ 2.6.24-rc8-mm1-p1+/net/core/gen_estimator.c	2008-01-20 21:07:42.000000000 +0100
> > @@ -106,6 +106,7 @@ static void est_timer(unsigned long arg)
> >  {
> >  	int idx = (int)arg;
> >  	struct gen_estimator *e;
> > +	int list_not_empty = 0;
> 
> Using a negative name for what is a boolean value leads
> to code that reads like a double negative sentence. Better to choose
> a variable name that is direct, can't use list_empty because that
> is a macro, so how about "estimator_found".
> 

Hmm, seems right, but since just after sending this patch I started
to doubt this 2/3 patch could really matter here, I'll maybe wait with
this name change for some confirmation yet.

So, since it certainly doesn't matter for 1/3 and 3/3 I withdraw this
2/3 patch for now.

BTW, I've forgotten to mention with patch 1/3 that this checking with
warning on gen_new_estimator() double call should be only temporary,
and after more testing gen_estimator structure could be probably
decreased after removing bstats and rate_est fields.

Thanks,
Jarek P.

Re: [PATCH 2/3][NET] gen_estimator: list_empty() check in est_timer() fixed

[Jarek Poplawski]

On Mon, Jan 21, 2008 at 07:34:55AM +0100, Jarek Poplawski wrote:
...
> BTW, I've forgotten to mention with patch 1/3 that this checking with
> warning on gen_new_estimator() double call should be only temporary,
> and after more testing gen_estimator structure could be probably
> decreased after removing bstats and rate_est fields.

Hmm, let's forget about this again: it's too early in the morning...

Jarek P.

Re: [PATCH 2/3][NET] gen_estimator: list_empty() check in est_timer() fixed

[David Miller]

From: Jarek Poplawski <jarkao2@gmail.com>
Date: Mon, 21 Jan 2008 07:42:40 +0100

> On Mon, Jan 21, 2008 at 07:34:55AM +0100, Jarek Poplawski wrote:
> ...
> > BTW, I've forgotten to mention with patch 1/3 that this checking with
> > warning on gen_new_estimator() double call should be only temporary,
> > and after more testing gen_estimator structure could be probably
> > decreased after removing bstats and rate_est fields.
> 
> Hmm, let's forget about this again: it's too early in the morning...

:-)

FWIW I agree that double-negatives are confusing and we should
avoid them.

Re: [PATCH 2/3][NET] gen_estimator: list_empty() check in est_timer() fixed

[Jarek Poplawski]

On Mon, Jan 21, 2008 at 02:36:32AM -0800, David Miller wrote:
...
> FWIW I agree that double-negatives are confusing and we should
> avoid them.

Right! No more: CHECKSUM_NONE, SOCK_NOSPACE, IFF_NOARP or KERN_NOTICE!

Jarek P.

Re: [PATCH 2/3][NET] gen_estimator: list_empty() check in est_timer() fixed

[David Miller]

From: Jarek Poplawski <jarkao2@gmail.com>
Date: Mon, 21 Jan 2008 12:19:40 +0100

> On Mon, Jan 21, 2008 at 02:36:32AM -0800, David Miller wrote:
> ...
> > FWIW I agree that double-negatives are confusing and we should
> > avoid them.
> 
> Right! No more: CHECKSUM_NONE, SOCK_NOSPACE, IFF_NOARP or KERN_NOTICE!

Life is difficult sometimes, but that is no excuse to further
the pain :-)

Re: [PATCH 2/3][NET] gen_estimator: list_empty() check in est_timer() fixed

[Jarek Poplawski]

On Mon, Jan 21, 2008 at 03:15:53AM -0800, David Miller wrote:
...
> Life is difficult sometimes, but that is no excuse to further
> the pain :-)
 
YES! I've read somewhere about it too!

Jarek P.

Re: [PATCH 2/3][NET] gen_estimator: list_empty() check in est_timer() fixed

[Jarek Poplawski]

On Mon, Jan 21, 2008 at 03:15:53AM -0800, David Miller wrote:
> From: Jarek Poplawski <jarkao2@gmail.com>
> Date: Mon, 21 Jan 2008 12:19:40 +0100
> 
> > On Mon, Jan 21, 2008 at 02:36:32AM -0800, David Miller wrote:
> > ...
> > > FWIW I agree that double-negatives are confusing and we should
> > > avoid them.
> > 
> > Right! No more: CHECKSUM_NONE, SOCK_NOSPACE, IFF_NOARP or KERN_NOTICE!
> 
> Life is difficult sometimes, but that is no excuse to further
> the pain :-)

BTW, maybe somebody else finds this interesting (because you seem to
know this very well), in some languages, like Polish, e.g.: "that is
no excuse" needs double-negative: "to nie jest zadne wytlumaczenie",
so literally: "that not is no excuse"...

Cheers,
Jarek P.