linux-kernel - Re: [ltt-dev] [RFC git tree] Userspace RCU (urcu) for Linux (repost)

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: <20090213184048.GA7124@Krystal>
Date:	Fri, 13 Feb 2009 13:40:49 -0500
From:	Mathieu Desnoyers <compudj@...stal.dyndns.org>
To:	Linus Torvalds <torvalds@...ux-foundation.org>
Cc:	Nick Piggin <nickpiggin@...oo.com.au>,
	Bryan Wu <cooloney@...nel.org>, linux-kernel@...r.kernel.org,
	ltt-dev@...ts.casi.polymtl.ca,
	uclinux-dist-devel@...ckfin.uclinux.org,
	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
Subject: Re: [ltt-dev] [RFC git tree] Userspace RCU (urcu) for Linux
	(repost)

* Linus Torvalds (torvalds@...ux-foundation.org) wrote:
> 
> 
> On Fri, 13 Feb 2009, Mathieu Desnoyers wrote:
> > 
> > The whole idea behind _LOAD_SHARED() is that it does not translate in
> > any different assembly output than a standard load. So no, it cannot be
> > possibly slower. It has no more side-effect than a simple comment in the
> > code, and that's its purpose : to identify those variables. So if we
> > find a code path doing
> > 
> >   _STORE_SHARED(x, v);
> >   smp_mc();
> >   while (_LOAD_SHARED(z) != val)
> >     cpu_relax();
> > 
> > We can verify very easily the code correctness :
> > 
> > A write cache flush is required after _STORE_SHARED
> > A read cache flush is required before _LOAD_SHARED
> > Read cache flushes are required to happen eventually between
> >   _LOAD_SHARED in the loop.
> 
> That makes no sense.
> 
> First off, you had the comment that LOAD_SHARED() would flush caches, so 
> your argument that it's just a load, nothing else, is in violation with 
> your own statements. And I told you why such a thing is INSANE.
> 

LOAD_SHARED -> cache flush + load
_LOAD_SHARED -> simple load

There is no contradiction here. And I agree that LOAD_SHARED will
generally produce slow code and that it is inappropriate for multiple
accesses between cache-line flushes.

> As to the underscore-version, what can it do? Nothing. It's perfectly fine 
> to have something like this:
> 
> 	while (_LOAD_SHARED(x) && _LOAD_SHARED(y)) {
> 		cpu_relax();
> 	}
> 
> and the thing is, there is no reason to do read-cache flushes between 
> those two _LOAD_SHARED. So warning about it would be incorrect, and all it 
> can do is be purely ugly "documentation" about the fact that it's doing a 
> shared load, because it's not really allowed to warn about the fact that 
> shared loads should have a cache flush in between, because THEY SHOULD 
> NOT.

Doing two _LOAD_SHARED of different variables without cache flush is
perfectly fine, but we could trigger the warning if some code reads
_the same_ variable twice without any flush in between. The only sane
way I can foresee this being correct is if an interrupt (or signal in
userspace) handler is expected to execute the cache flush for us.

> 
> But it is also _ugly_.
> 

Agreed. I'm under the impression the code is yelling at me when I read
it. :-) Probably that the capital lettering is ill-chosen.

> And more importantly - if you see it as a documentation thing, then it's 
> broken in the first place - you're documenting the places that you already 
> know about, and already know are important, rather than finding places 
> that might be buggy. So what does it help us? Nothing.

Given we have to to match the LOADs and the STOREs within the source
would possibly lead to interesting findings, as learning that such STORE
is really expected to be propagated to the other CPU just because we
notice its associated LOAD. So I think having such in-place
documentation might actually help finding bugs.

> 
> You might as well just document the cpu_relax(). Which it implicitly does: 
> it's a barrier in a tight loop.
> 
> In other words, I see no real point. Your [_][LOAD|STORE]_SHARED is ugly 
> and doesn't add value, or adds value (the cache flush) in really really 
> bad ways that aren't even very well-defined. 
> 

I guess we will have to wait until someone really want to port Linux to
a SMP architecture with non-coherent caches before we can measure the
value of such macro-ish documentation. Now is probably way too soon.

Thanks,

Mathieu

> 			Linus
> 

-- 
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/