Re: [PATCH] xfrm: cache bundle lookup results in flow cache

["Timo Teräs" <timo.teras@iki.fi>]

Herbert Xu wrote:
> On Fri, Mar 19, 2010 at 10:37:58AM +0200, Timo Teräs wrote:
>> But I changed that. the flow cache now does *not* call local_bh_enable
>> if it returns something. This is deferred until corresponding _put
>> call. So bh's are disable while we are touching the lookup results.
> 
> I'm sorry but making a function like flow_cache_lookup return with
> BH disabled is just wrong!
> 
>> It'd probably make sense to remove that. And require _lookup to
>> be called with bh disabled so it's more obvious that bh's are
>> disabled when touching the cache entry.
> 
> That would be better but it's still hacky.  Proper reference
> counting like we had before would be my preference.

Well, the cache entry is still referenced only very shortly,
I don't see why keeping bh disabled why doing it is considered
a hack. Refcounting the cache entries is trickier. Though,
it could be used to optimize the update process: we could safely
update it instead of doing now lookup later.

>> Not a race. We need to keep bh's disabled while touching fce
>> for various reasons.
> 
> What are those reasons (apart from this race)?

This. And that the cache is synchronized by flow_cache_flush
executing stuff on other cpu's, ensuring that it's not running
any protected cache accessing code. See below.

> 
>> Noone. When policy and dst is on cache there's no reference.
>> The cache generation id's ensure that the object exists when
>> they are in cache. It might make sense to add references to
>> both objects and do a BUG_ON if the flow cache flusher would
>> need to delete an object. I guess this would be the proper
>> way, since that's how the dst stuff works too.
> 
> The cache genid is not enough:
> 
> CPU1			CPU2
> check genid == OK
> 			update genid
> 			kill policy
> 			kfree on policy
> use policy == BOOM

The sequence goes currently.

CPU1			CPU2
disable_bh
check genid == OK
			update genid
			call cache_flush
			blocks
use policy == OK
and take refcount
enable_bh
cache_flush smpcall executes and ublocks cpu2
			returns from cache_flush
			kill policy
			kfree on policy

- Timo