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: <4BB09178.8050501@iki.fi>
Date:	Mon, 29 Mar 2010 14:39:36 +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 02:23:02PM +0300, Timo Teräs wrote:
>>> I don't see the point.  As long as the data paths do not take
>>> the lock changing this doesn't buy us much.  You're still making
>>> that cacheline exclusive.
>> To my understanding declaring an atomic_t, or reading it with
>> atomic_read does not make cache line exclusive. Only the atomic_*
>> writing to it take the cache line. And since this is done exactly
>> once for policy (or it's a bug/warn thingy) it does not impose
>> significant performance issue.
> 
> I was talking about the lock vs. atomic_xchg in xfrm_policy_kill.
> There is practically no difference for that case.
> 
> Yes, on the read side the lock is a completely different beast
> compared to atomic_read, but I don't see how you can safely
> replace
> 
> 	lock
> 	if (!dead)
> 		take ref
> 	unlock
> 
> without making other changes.

Because the lock is not needed to take ref.

You can take ref as long as someone else is also holding a
valid reference.

The flow cache keeps a reference to each object it is holding,
thus we can always take a reference if we find an object there.
This is what is being done in the current code, and can be
still done in the new code.

The only reason my patch had the lock, was for the dead check.
Since it can be checked without locks, the locking can be
just removed.

The dead check is still an improvement: we find outdated cache
entries without doing a full flush and we find them faster than
using a full flush.

The old code would use policy entries with dead flag set, and
happily return and use them until the policy gc had an
opportunity to run.
--
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