netdev - Re: [PATCH RFC 1/2] flow: virtualize get and entry deletion methods

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20100329102639.GA23414@gondor.apana.org.au>
Date:	Mon, 29 Mar 2010 18:26:39 +0800
From:	Herbert Xu <herbert@...dor.apana.org.au>
To:	Timo Teräs <timo.teras@....fi>
Cc:	David Miller <davem@...emloft.net>, netdev@...r.kernel.org
Subject: Re: [PATCH RFC 1/2] flow: virtualize get and entry deletion methods

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.

> 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.

> 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.

> 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.

Thanks,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@...dor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
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