[IPV4] ROUTE: Avoid sparse warnings

[IPV4] ROUTE: Avoid sparse warnings

[Eric Dumazet]

  CHECK   net/ipv4/route.c
net/ipv4/route.c:298:2: warning: context imbalance in 'rt_cache_get_first' - wrong count at exit
net/ipv4/route.c:307:3: warning: context imbalance in 'rt_cache_get_next' - unexpected unlock
net/ipv4/route.c:346:3: warning: context imbalance in 'rt_cache_seq_stop' - unexpected unlock

Signed-off-by: Eric Dumazet <dada1@cosmosbay.com>

diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index 10915bb..fec0571 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -289,11 +289,11 @@ static struct rtable *rt_cache_get_first(struct seq_file *seq)
 	struct rt_cache_iter_state *st = seq->private;
 
 	for (st->bucket = rt_hash_mask; st->bucket >= 0; --st->bucket) {
-		rcu_read_lock_bh();
 		r = rt_hash_table[st->bucket].chain;
 		if (r)
 			break;
 		rcu_read_unlock_bh();
+		rcu_read_lock_bh();
 	}
 	return r;
 }
@@ -305,9 +305,9 @@ static struct rtable *rt_cache_get_next(struct seq_file *seq, struct rtable *r)
 	r = r->u.dst.rt_next;
 	while (!r) {
 		rcu_read_unlock_bh();
+		rcu_read_lock_bh();
 		if (--st->bucket < 0)
 			break;
-		rcu_read_lock_bh();
 		r = rt_hash_table[st->bucket].chain;
 	}
 	return r;
@@ -324,7 +324,9 @@ static struct rtable *rt_cache_get_idx(struct seq_file *seq, loff_t pos)
 }
 
 static void *rt_cache_seq_start(struct seq_file *seq, loff_t *pos)
+	__acquires(RCU_BH)
 {
+	rcu_read_lock_bh();
 	return *pos ? rt_cache_get_idx(seq, *pos - 1) : SEQ_START_TOKEN;
 }
 
@@ -341,9 +343,9 @@ static void *rt_cache_seq_next(struct seq_file *seq, void *v, loff_t *pos)
 }
 
 static void rt_cache_seq_stop(struct seq_file *seq, void *v)
+	__releases(RCU_BH)
 {
-	if (v && v != SEQ_START_TOKEN)
-		rcu_read_unlock_bh();
+	rcu_read_unlock_bh();
 }
 
 static int rt_cache_seq_show(struct seq_file *seq, void *v)

Re: [IPV4] ROUTE: Avoid sparse warnings

[Herbert Xu]

Eric Dumazet <dada1@cosmosbay.com> wrote:
>  CHECK   net/ipv4/route.c
> net/ipv4/route.c:298:2: warning: context imbalance in 'rt_cache_get_first' - wrong count at exit
> net/ipv4/route.c:307:3: warning: context imbalance in 'rt_cache_get_next' - unexpected unlock
> net/ipv4/route.c:346:3: warning: context imbalance in 'rt_cache_seq_stop' - unexpected unlock
> 
> Signed-off-by: Eric Dumazet <dada1@cosmosbay.com>
> 
> diff --git a/net/ipv4/route.c b/net/ipv4/route.c
> index 10915bb..fec0571 100644
> --- a/net/ipv4/route.c
> +++ b/net/ipv4/route.c
> @@ -289,11 +289,11 @@ static struct rtable *rt_cache_get_first(struct seq_file *seq)
>        struct rt_cache_iter_state *st = seq->private;
> 
>        for (st->bucket = rt_hash_mask; st->bucket >= 0; --st->bucket) {
> -               rcu_read_lock_bh();
>                r = rt_hash_table[st->bucket].chain;
>                if (r)
>                        break;
>                rcu_read_unlock_bh();
> +               rcu_read_lock_bh();

If we have to change perfectly working code to silence sparse then
either sparse or we are doing something wrong.

This is not the only spot where we conditionally hold the lock.
There's got to be a better fix than changing all of them to hold
locks unconditionally.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [IPV4] ROUTE: Avoid sparse warnings

[Eric Dumazet]

On Mon, 07 Jan 2008 23:11:53 +1100
Herbert Xu <herbert@gondor.apana.org.au> wrote:

> Eric Dumazet <dada1@cosmosbay.com> wrote:
> >  CHECK   net/ipv4/route.c
> > net/ipv4/route.c:298:2: warning: context imbalance in 'rt_cache_get_first' - wrong count at exit
> > net/ipv4/route.c:307:3: warning: context imbalance in 'rt_cache_get_next' - unexpected unlock
> > net/ipv4/route.c:346:3: warning: context imbalance in 'rt_cache_seq_stop' - unexpected unlock
> > 
> > Signed-off-by: Eric Dumazet <dada1@cosmosbay.com>
> > 
> > diff --git a/net/ipv4/route.c b/net/ipv4/route.c
> > index 10915bb..fec0571 100644
> > --- a/net/ipv4/route.c
> > +++ b/net/ipv4/route.c
> > @@ -289,11 +289,11 @@ static struct rtable *rt_cache_get_first(struct seq_file *seq)
> >        struct rt_cache_iter_state *st = seq->private;
> > 
> >        for (st->bucket = rt_hash_mask; st->bucket >= 0; --st->bucket) {
> > -               rcu_read_lock_bh();
> >                r = rt_hash_table[st->bucket].chain;
> >                if (r)
> >                        break;
> >                rcu_read_unlock_bh();
> > +               rcu_read_lock_bh();
> 
> If we have to change perfectly working code to silence sparse then
> either sparse or we are doing something wrong.
> 
> This is not the only spot where we conditionally hold the lock.
> There's got to be a better fix than changing all of them to hold
> locks unconditionally.

Maybe sparse (or me :) ) is a litle bit dumb :(

You are right other functions conditionally hold some lock(s), but in this
case this is not really necessary / worth the complexity.

AFAIK, this patch reduces complexity and text size.

Re: [IPV4] ROUTE: Avoid sparse warnings

[Herbert Xu]

On Mon, Jan 07, 2008 at 02:56:24PM +0100, Eric Dumazet wrote:
> 
> AFAIK, this patch reduces complexity and text size.

But if we had loads of empty hash buckets couldn't this potentially
increase latency by disabling BH longer than before?

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [IPV4] ROUTE: Avoid sparse warnings

[Eric Dumazet]

Herbert Xu a écrit :
> On Mon, Jan 07, 2008 at 02:56:24PM +0100, Eric Dumazet wrote:
>> AFAIK, this patch reduces complexity and text size.
> 
> But if we had loads of empty hash buckets couldn't this potentially
> increase latency by disabling BH longer than before?

Well, we call rcu_read_unlock_bh()/rcu_read_lock_bh() for each bucket, empty 
or not, before and after patch, so we dont change latency.

Re: [IPV4] ROUTE: Avoid sparse warnings

[Herbert Xu]

On Mon, Jan 07, 2008 at 09:46:45PM +0100, Eric Dumazet wrote:
>
> Well, we call rcu_read_unlock_bh()/rcu_read_lock_bh() for each bucket, 
> empty or not, before and after patch, so we dont change latency.

Oh I see.  Your patch looks good then.  But we still need a solution
in general unless we're to avoid all uses of conditional locking.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [IPV4] ROUTE: Avoid sparse warnings

[David Miller]

From: Eric Dumazet <dada1@cosmosbay.com>
Date: Mon, 7 Jan 2008 12:01:17 +0100

>   CHECK   net/ipv4/route.c
> net/ipv4/route.c:298:2: warning: context imbalance in 'rt_cache_get_first' - wrong count at exit
> net/ipv4/route.c:307:3: warning: context imbalance in 'rt_cache_get_next' - unexpected unlock
> net/ipv4/route.c:346:3: warning: context imbalance in 'rt_cache_seq_stop' - unexpected unlock
> 
> Signed-off-by: Eric Dumazet <dada1@cosmosbay.com>
> 
> diff --git a/net/ipv4/route.c b/net/ipv4/route.c
> index 10915bb..fec0571 100644
> --- a/net/ipv4/route.c
> +++ b/net/ipv4/route.c
> @@ -289,11 +289,11 @@ static struct rtable *rt_cache_get_first(struct seq_file *seq)
>  	struct rt_cache_iter_state *st = seq->private;
>  
>  	for (st->bucket = rt_hash_mask; st->bucket >= 0; --st->bucket) {
> -		rcu_read_lock_bh();
>  		r = rt_hash_table[st->bucket].chain;
>  		if (r)
>  			break;
>  		rcu_read_unlock_bh();
> +		rcu_read_lock_bh();
>  	}
>  	return r;
>  }

Like for Herbert, this patch leaves a bad taste in my mouth.

I don't understand, is it the case that sparse doesn't like
that rt_cache_get_first() returns with rcu_read_lock_bh()
held?  That's kind of rediculious.

Furthermore, these:

	rcu_read_unlock_bh()
	rcu_read_lock_bh()

sequences are at best funny looking.  For other lock types we would
look at this and ask "Does this even accomplish anything reliably?"

The answer here is that it wants the preempt_enable() to run to get
any potential kernel preemptions executed.  It also allows any
pending software interrupts to run.

So this does something reliably only because rcu_read_unlock_bh() has
specific and explicit side effects.

I don't know, to me it just looks awful :-)  I better understood the
original code.

We can continue splitting hairs over this but I'll hold off on this
patch for now. :)

Re: [IPV4] ROUTE: Avoid sparse warnings

[Eric Dumazet]

David Miller a écrit :
> From: Eric Dumazet <dada1@cosmosbay.com>
> Date: Mon, 7 Jan 2008 12:01:17 +0100
> 
>>   CHECK   net/ipv4/route.c
>> net/ipv4/route.c:298:2: warning: context imbalance in 'rt_cache_get_first' - wrong count at exit
>> net/ipv4/route.c:307:3: warning: context imbalance in 'rt_cache_get_next' - unexpected unlock
>> net/ipv4/route.c:346:3: warning: context imbalance in 'rt_cache_seq_stop' - unexpected unlock
>>
>> Signed-off-by: Eric Dumazet <dada1@cosmosbay.com>
>>
>> diff --git a/net/ipv4/route.c b/net/ipv4/route.c
>> index 10915bb..fec0571 100644
>> --- a/net/ipv4/route.c
>> +++ b/net/ipv4/route.c
>> @@ -289,11 +289,11 @@ static struct rtable *rt_cache_get_first(struct seq_file *seq)
>>  	struct rt_cache_iter_state *st = seq->private;
>>  
>>  	for (st->bucket = rt_hash_mask; st->bucket >= 0; --st->bucket) {
>> -		rcu_read_lock_bh();
>>  		r = rt_hash_table[st->bucket].chain;
>>  		if (r)
>>  			break;
>>  		rcu_read_unlock_bh();
>> +		rcu_read_lock_bh();
>>  	}
>>  	return r;
>>  }
> 
> Like for Herbert, this patch leaves a bad taste in my mouth.
> 
> I don't understand, is it the case that sparse doesn't like
> that rt_cache_get_first() returns with rcu_read_lock_bh()
> held?  That's kind of rediculious.

Not exactly.
It warns us because rt_cache_get_first() can returns with RCU_BH *acquired* or 
not. I find this warning very usefull. In rt_cache_get_first() this warning
is a false alarm.

> 
> Furthermore, these:
> 
> 	rcu_read_unlock_bh()
> 	rcu_read_lock_bh()
> 
> sequences are at best funny looking.  For other lock types we would
> look at this and ask "Does this even accomplish anything reliably?"

Well, original code exactly does the same thing.

> 
> The answer here is that it wants the preempt_enable() to run to get
> any potential kernel preemptions executed.  It also allows any
> pending software interrupts to run.
> 
> So this does something reliably only because rcu_read_unlock_bh() has
> specific and explicit side effects.
> 

I will post a patch to introduce a helper function, so that this is clearly 
documented and not relying on side effects. Actual implementation has latency
problems on empty hash tables if CONFIG_PREEMPT=n


> I don't know, to me it just looks awful :-)  I better understood the
> original code.
> 
> We can continue splitting hairs over this but I'll hold off on this
> patch for now. :)

Fair enough

Thanks

Re: [IPV4] ROUTE: Avoid sparse warnings

[David Miller]

From: Eric Dumazet <dada1@cosmosbay.com>
Date: Tue, 08 Jan 2008 08:53:10 +0100

> It warns us because rt_cache_get_first() can returns with RCU_BH
> *acquired* or not.

As Herbert mentioned that's a pretty often used paradigm called
conditional locking.  :-)

Re: [IPV4] ROUTE: Avoid sparse warnings

[Jarek Poplawski]

On 08-01-2008 08:53, Eric Dumazet wrote:
> David Miller a écrit :
...
>> Furthermore, these:
>>
>>     rcu_read_unlock_bh()
>>     rcu_read_lock_bh()
>>
>> sequences are at best funny looking.  For other lock types we would
>> look at this and ask "Does this even accomplish anything reliably?"
> 
> Well, original code exactly does the same thing.
> 
>>
>> The answer here is that it wants the preempt_enable() to run to get
>> any potential kernel preemptions executed.  It also allows any
>> pending software interrupts to run.
>>
>> So this does something reliably only because rcu_read_unlock_bh() has
>> specific and explicit side effects.
>>
> 
> I will post a patch to introduce a helper function, so that this is 
> clearly documented and not relying on side effects. Actual 
> implementation has latency
> problems on empty hash tables if CONFIG_PREEMPT=n
> 
> 
>> I don't know, to me it just looks awful :-)  I better understood the
>> original code.

It seems this patch only made it more visible how it currently works.
I don't know what changes do you plan for this helper function, but
my proposal is to add some counter and break this rcu only after
looping for some time. Alternatively cpu_relax() could be probably
used between these "locks". Without this probably some cache problems
are possible, but you know this better, I guess.

Regards,
Jarek P.