David Miller a écrit :
From: Eric Dumazet <[EMAIL PROTECTED]>
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 <[EMAIL PROTECTED]>
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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
