On Wed, 30 Aug 2006 01:17:22 +0400
Alexey Kuznetsov <[EMAIL PROTECTED]> wrote:
> Hello!
>
> > atomic_inc_and_test is true iff result is zero, so that won't work.
>
> I meant atomic_inc_not_zero(), as Martin noticed.
>
>
> > But the following should work:
> >
> > hlist_for_each_entry_rcu(n, tmp, &tbl->hash_buckets[hash_val], hlist) {
> > if (dev == n->dev && !memcmp(n->primary_key, pkey, key_len)) {
> > if (unlikely(&atomic_inc_return(&n->refcnt) == 1)) {
> > neigh_release(n);
>
> I do not think it will work. It has exactly the same race condition.
>
> Yes, atomic_inc_not_zero() is expensive. But it looks like it is the cheapest
> variant, which works correctly without more work.
>
> Another variant would be rework use of refcnt. It can be done like rt cache:
> when release of the last reference does not mean anything.
>
> Also, probably, it makes sense to add neigh_lookup_light(), which does
> not take refcnt, but required to call
> neigh_release_light() (which is just rcu_read_unlock_bh()).
Which code paths would that make sense on?
fib_detect_death (ok)
infiniband (ok)
wireless/strip (ok) -- hey, this code is crap it has
a refcount leak already!
arp_req_get (ok)
ndisc (ok)
Perhaps killing the refcount all together, and just changing
everybody to neigh_lookup_rcu(). Nobody holds a long term reference
to the entries.
>
> Alexey
Or using the existing dead flag?
@@ -346,17 +359,24 @@ struct neighbour *neigh_lookup(struct ne
NEIGH_CACHE_STAT_INC(tbl, lookups);
- read_lock_bh(&tbl->lock);
- hlist_for_each_entry(n, tmp, &tbl->hash_buckets[hash_val], hlist) {
+ rcu_read_lock();
+ hlist_for_each_entry_rcu(n, tmp, &tbl->hash_buckets[hash_val], hlist) {
if (dev == n->dev && !memcmp(n->primary_key, pkey, key_len)) {
neigh_hold(n);
+ /* Don't rescuitate dead entries */
+ if (unlikely(n->dead)) {
+ neigh_release(n);
+ continue;
+ }
+
NEIGH_CACHE_STAT_INC(tbl, hits);
goto found;
}
}
n = NULL;
found:
- read_unlock_bh(&tbl->lock);
+ rcu_read_unlock();
return n;
}
-
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
