Re: [PATCH] Export current_is_keventd() for libphy

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

Re: [PATCH] Export current_is_keventd() for libphy

[Linus Torvalds]

On Wed, 6 Dec 2006, Linus Torvalds wrote:
>
> and remove the "volatile" from all the bitop accessor functions.

It might also be interesting to see if this would change code-size at all. 

There's a number of things that check different bits in the same word 
right now, and they just reload the word unnecessarily and do multiple 
tests. Some of the page flags functions obviously already work around this 
by doing horrible things by hand instead, eg:

                (page->flags & (
                        1 << PG_lru     |
                        1 << PG_private |
                        1 << PG_locked  |
                        1 << PG_active  |
                        1 << PG_reclaim |
                        1 << PG_slab    |
                        1 << PG_swapcache |
                        1 << PG_writeback |
                        1 << PG_reserved |
                        1 << PG_buddy ))

in the free_pages_check() thing. It may make sense there, but we really 
_should_ allow gcc to just do things like this for us, and just use the 
proper functions to test bits.

			Linus

Re: [PATCH] Export current_is_keventd() for libphy

[David Howells]

Linus Torvalds <torvalds@osdl.org> wrote:

>  (a) "volatile" on kernel data is basically always a bug, and you should 
>      use locking.

But what about when you're building a lock?  Actually, I suspect correct usage
of asm constraints and memory barriers trumps volatile anyway even there.

David

Re: [PATCH] Export current_is_keventd() for libphy

[Linus Torvalds]

On Wed, 6 Dec 2006, David Howells wrote:
>
> Linus Torvalds <torvalds@osdl.org> wrote:
> 
> >  (a) "volatile" on kernel data is basically always a bug, and you should 
> >      use locking.
> 
> But what about when you're building a lock?  Actually, I suspect correct usage
> of asm constraints and memory barriers trumps volatile anyway even there.

The word you look for is not "suspect".

You _cannot_ build a lock using "volatile", unless your CPU is strictly 
in-order and has an in-order memory subsystem too (so, for example, while 
all ia64 implementations today are in-order, they do /not/ have an 
in-order memory subsystem). Only then could you do locking with volatile 
and some crazy Peterson's algorithm.

I don't think any such CPU actually exists.

Anyway, we've had this discussion before on linux-kernel, it really boils 
down to that "volatile" is basically never correct with the exception of 
flags that don't have any meaning and that you don't actually _care_ about 
the exact value (the low word of "jiffies" being the canonical example of 
something where "volatile" is actually fine, and where - as long as you 
can load it atomically - "volatile" really does make sense).

		Linus