On Wed, 30 Aug 2006 03:21:26 +0400
Alexey Kuznetsov [off-list ref] wrote:

Hello!

quoted

This should not be any more racy than the existing code.

Existing code is not racy.

Race 1: w/o RCU
					Cpu 0: is in neigh_lookup
						gets read_lock()
						finds entry
						++refcount to 2
						updates it
	Cpu 1: is in forced_gc()
		waits at write_lock()
						releases read_lock()
						drops ref count to 1.
		sees ref count is 1
		deletes it

Race 1: w RCU
					Cpu 0: is in __neigh_lookup
						updates it
	Cpu 1: is in forced_gc()
						leaves refcount=1
		sees ref count is 1
		deletes it

Critical place is interpretation of refcnt==1. Current code assumes,
that when refcnt=1 and entry is in hash table, nobody can take this
entry (table is locked). So, it can be unlinked from the table.

See? __neigh_lookup()/__neigh_lookup_errno() _MUST_ return alive
and hashed entry. And will stay hashed until the reference is held.
Or until neigh entry is forced for destruction by device going down,
in this case referring dst entries will die as well.

Why must it be hashed, it could always get zapped just after the update.

If dst cache grabs an entry, which is purged from table because for some
time it had refcnt==1, you got a valid dst entry referring to dead
neighbour entry.

Hmm.. Since this is a slow path, grabbing the write_lock on the neighbour
entry would block the gc from zapping it.

in __neigh_lookup()
		neigh_hold();
		write_lock(&n->lock);
		if (n->dead) {
			write_unlock()
			neigh_release()
			goto rescan;
		}

		write_unlock()





-- 
Stephen Hemminger [off-list ref]