[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <4BA33FF5.8010104@iki.fi>
Date: Fri, 19 Mar 2010 11:12:21 +0200
From: Timo Teräs <timo.teras@....fi>
To: Herbert Xu <herbert@...dor.apana.org.au>
CC: netdev@...r.kernel.org
Subject: Re: [PATCH] xfrm: cache bundle lookup results in flow cache
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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists