Message-ID: <4BB082B8.9030400@iki.fi>
Date:	Mon, 29 Mar 2010 13:36:40 +0300
From:	Timo Teräs <timo.teras@....fi>
To:	Herbert Xu <herbert@...dor.apana.org.au>
CC:	David Miller <davem@...emloft.net>, netdev@...r.kernel.org
Subject: Re: [PATCH RFC 1/2] flow: virtualize get and entry deletion methods

Herbert Xu wrote:
> On Mon, Mar 29, 2010 at 01:07:50PM +0300, Timo Teräs wrote:
>>> For the flow cache we didn't need this because the policy code
>>> would flush the cache synchronously so we can always grab a ref
>>> count safely as long as BH is still off.
>> The old code could return policy object that was killed. It
> 
> Which is fine.  I'd rather have that than this new behaviour
> which adds a lock.  We don't delete policies all the time, so
> optimising for that case and pessimising the normal case is wrong!
> 
>> The reason for policy GC calling flush was there for two
>> reasons:
>> - purging the stale entries
>> - making sure that refcount of policy won't go to zero after
>>  releasing flow cache's reference (because the flow cache
>>  did only atomic_dec but did not call destructor)
>>
>> Both issues are handled otherwise in the patch. By refreshing
>> stale entries immediately. Or calling virtual destructor when
>> the object gets deleted. Thus the slow flushing is not needed
>> as often now.
> 
> Let's step back one second.  It's best to not accumulate unrelated
> changes in one patch.  So is there a reason why you must remove
> the synchronous flow cache flushing from the policy destruction
> path? If not please move that into a different patch.

Yes, I'm splitting up the patch to more fine grained pieces.

The synchronous flow cache flushing does not have to be removed,
but I consider it an improvement.

>> We can still drop the locking, as ->dead can be made atomic_t.
> 
> No it doesn't need to be atomic, reading an int is always atomic.

The only reason why it needs to be atomic is because of
xfrm_policy_kill() which writes '1' and checks if it was zero
previously. Since the idea is to get rid of the policy lock, we
can turn ->dead flag to atomic_t and use atomic_xchg for that.
Otherwise it would be ok to have it as just regular int.

>> Checking ->dead improves cache's speed to react to policy object
>> changes. And the virtual ->get is especially needed for bundles
>> as they can get stale a lot more often.
> 
> I really see no point to optimising for such an unlikely case
> but as long as you kill the locks I guess I'm not too bothered.

Agreed. But as the lockless check is cheap, why not to have it.
And some system do get policy changes quite a bit. IKE daemon
sometimes is configured to create policies on-demand so this does
have a real use case.

>> Yeah. Just a note that the road map to RCU policies is trivial
>> and fixes some races in locking currently.
> 
> Please do one change at a time.  Let's just focus on the original
> issue of the bundle linked list for now.

Yes. The way to do that is just a bit long. I've already added
some other stuff and split existing stuff in the patch. It's
currently seven patches. I'm still tracking one reference leak,
but I'll send in the new set soonish.

- 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