Re: [PATCH] Export current_is_keventd() for libphy

[Linus Torvalds]

On Wed, 6 Dec 2006, Linus Torvalds wrote:
> 
> Sadly, gcc doesn't do it in this case. Still, I'd rather keep the source 
> clean, and hope that the compiler improves eventually, than to make the 
> code uglier.

Actually, it's our own damn fault.

We've long had our arguments "const volatile" to test_bit(), which 
basically means that gcc can't do the optimization. Damn.

And they need to be "volatile" not because we _want_ the thing to be 
volaile, but because these things are occationally used on volatile 
objects (so if the function doesn't take a volatile pointer, it would 
warn).

That's why so many of these helper functions use "const volatile" 
pointers, which on the face of it would seem strange if you don't realize 
that it's more about a C type issue than about the _actual_ type being 
either "const" or "volatile".

Sad. I guess we could remove the "const volatile" from the _cast_, but the 
thing is, if the underlying object we're actually looking at really _is_ 
volatile, we shouldn't do that. GAAH.

Really sad. I doubt any of the callers actually want the "volatile" access 
at all. Things like <linux/page-flags.h> definitely _don't_ want it.

I suspect we should just face up to the fact that 

 (a) "volatile" on kernel data is basically always a bug, and you should 
     use locking. "volatile" doesn't help anything at all with memory 
     ordering and friends, so it's insane to think it "solves" anything on 
     its own.
 (b) on "iomem" pointers it does make sense, but those need special 
     accessor functions _anyway_, so things like test_bit() wouldn't work 
     on them.
 (c) if you spin on a value changing, you should use "cpu_relax()" or 
     "barrier()" anyway, which will force gcc to re-load any values from 
     memory over the loop.

and remove the "volatile" from all the bitop accessor functions.

Or at least from "test_bit()". It's not like it's synchronous _anyway_ 
(there's no memory barriers etc).

Hmm? Comments? linux-arch added to Cc, just in case people care (this 
particular thing is in <asm-*/bitops.h>, so it _is_ architecture- 
specific, but we should probably all agree on it first)

		Linus
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/