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 09:48:17AM +0200, Timo Teräs wrote:
>> But flow_cache_lookup disables pre-emption until _put is called.
>> So it should work. Would there be a cleaner way?
> 
> Previously the flow cache returned a policy directly which works
> because whenever we modify that policy we'd take the appropriate
> lock.
>
> Your patch changes it so that it now returns an fce.  But nothing
> is guarding the code that modifies fce.  So two CPUs may end up
> modifying the same fce.

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.

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.

> However, it would appear that this race could be harmless, provided
> that you are careful about dereferencing fce->policy and fce->dst.
> 
> IOW, this is not OK
> 
> 	if (fce->policy)
> 		use fce->policy;
> 
> and this should work
> 
> 	policy = fce->policy;
> 	if (policy)
> 		use policy;

Not a race. We need to keep bh's disabled while touching fce
for various reasons.

> Actually on second tought, even this isn't totally safe.  Who
> is taking a reference count on the policy and dst? I see a ref
> count on the fce, but nothing on fce->dst and fce->policy.  Do
> you have an implicit reference on them?

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.

- Timo