[PATCH] Fix the cpumask rewrite

[PATCH] Fix the cpumask rewrite

[James Bottomley]

This patch:

ChangeSet 1.1828.1.25 2004/06/24 08:51:00 akpm@osdl.org
  [PATCH] cpumask: rewrite cpumask.h - single bitmap based implementation
  
  From: Paul Jackson <pj@sgi.com>
  
  Major rewrite of cpumask to use a single implementation, as a struct-wrapped
  bitmap.
 
Altered the cpumask iterators to add a volatile to the bitmap type. 
This might be correct for x86 and itanium, but it isn't for parisc where
our bitmap operators don't take volatile pointers.  This makes our
compile incredibly noisy in just about every file:

  CC      mm/rmap.o
In file included from include/linux/sched.h:15,
                 from include/linux/mm.h:4,
                 from mm/rmap.c:29:
include/linux/cpumask.h: In function `__cpu_set':
include/linux/cpumask.h:86: warning: passing arg 2 of `set_bit' discards
qualifiers from pointer target type
include/linux/cpumask.h: In function `__cpu_clear':
include/linux/cpumask.h:92: warning: passing arg 2 of `clear_bit'
discards qualifiers from pointer target type

Since whether the bitmap operators are volatile or not is within the
province of the architecture to decide, I think the correct fix is just
to drop the spurious volatiles from cpumask.h

James

===== include/linux/cpumask.h 1.15 vs edited =====
--- 1.15/include/linux/cpumask.h	2004-06-24 13:57:31 -05:00
+++ edited/include/linux/cpumask.h	2004-06-26 11:00:14 -05:00
@@ -81,13 +81,13 @@
 extern cpumask_t _unused_cpumask_arg_;
 
 #define cpu_set(cpu, dst) __cpu_set((cpu), &(dst))
-static inline void __cpu_set(int cpu, volatile cpumask_t *dstp)
+static inline void __cpu_set(int cpu, cpumask_t *dstp)
 {
 	set_bit(cpu, dstp->bits);
 }
 
 #define cpu_clear(cpu, dst) __cpu_clear((cpu), &(dst))
-static inline void __cpu_clear(int cpu, volatile cpumask_t *dstp)
+static inline void __cpu_clear(int cpu, cpumask_t *dstp)
 {
 	clear_bit(cpu, dstp->bits);
 }

Re: [PATCH] Fix the cpumask rewrite

[Linus Torvalds]

On Sat, 26 Jun 2004, James Bottomley wrote:
>  
> This might be correct for x86 and itanium, but it isn't for parisc where
> our bitmap operators don't take volatile pointers.

Why not? The thing is, the bitmap operators are supposed to work on 
volatile data, ie people are literally using them for things like

	while (test_bit(TASKLET_STATE_SCHED, &t->state));

and the thing is supposed to work.

Now, I personally am not a big believer in the "volatile" keyword itself, 
since I believe that anybody who expects the compiler to generate 
different code for volatiles and non-volatiles is pretty much waiting for 
a bug to happen, but there are two cases where I think it's ok:

 - in function prototypes to show that the function can take volatile data 
   (and not complain).

 - as an arch-specific low-level implementation detail to avoid having to 
   use inline assembly just to load a value. Ie a _data_structure_ should 
   never be volatile, but it's ok to use a volatile pointer for a single 
   access.

I believe the bitop functions fall under #1 - the function is clearly
supposed to handle the case of a volatile pointer, and if it is inlined,
the above endless while-loop must not just load the bit once and turn it 
into an endless loop - it needs to re-load the thing every iteration.

> Since whether the bitmap operators are volatile or not is within the
> province of the architecture to decide,

I disagree. Why wouldn't they be volatile?

		Linus

Re: [PATCH] Fix the cpumask rewrite

[Linus Torvalds]

On Sat, 26 Jun 2004, James Bottomley wrote:
>  
> This might be correct for x86 and itanium, but it isn't for parisc where
> our bitmap operators don't take volatile pointers.

Why not? The thing is, the bitmap operators are supposed to work on 
volatile data, ie people are literally using them for things like

	while (test_bit(TASKLET_STATE_SCHED, &t->state));

and the thing is supposed to work.

Now, I personally am not a big believer in the "volatile" keyword itself, 
since I believe that anybody who expects the compiler to generate 
different code for volatiles and non-volatiles is pretty much waiting for 
a bug to happen, but there are two cases where I think it's ok:

 - in function prototypes to show that the function can take volatile data 
   (and not complain).

 - as an arch-specific low-level implementation detail to avoid having to 
   use inline assembly just to load a value. Ie a _data_structure_ should 
   never be volatile, but it's ok to use a volatile pointer for a single 
   access.

I believe the bitop functions fall under #1 - the function is clearly
supposed to handle the case of a volatile pointer, and if it is inlined,
the above endless while-loop must not just load the bit once and turn it 
into an endless loop - it needs to re-load the thing every iteration.

> Since whether the bitmap operators are volatile or not is within the
> province of the architecture to decide,

I disagree. Why wouldn't they be volatile?

		Linus

Re: [PATCH] Fix the cpumask rewrite

[Linus Torvalds]

On Sat, 26 Jun 2004, Linus Torvalds wrote:
> 
> Why not? The thing is, the bitmap operators are supposed to work on 
> volatile data, ie people are literally using them for things like
> 
> 	while (test_bit(TASKLET_STATE_SCHED, &t->state));

Ahh. That one is part of a "do while ()" loop, and grep mis-identifies it. 
So parisc gets it right, because the whole loop is

	do
		yield();
	while (test_bit(TASKLET_STATE_SCHED, &t->state));

so at least the compiler didn't get to optimize it away.

Hopefully we don't have any other busy loops either - even if it is a busy
loop, we should have a "cpu_relax()" or something there just to keep power
levels down, and that should also tell the compiler not to do anything
silly. But the conceptual point stands.

		Linus

Re: [PATCH] Fix the cpumask rewrite

[James Bottomley]

On Sat, 2004-06-26 at 11:32, Linus Torvalds wrote:
> Why not? The thing is, the bitmap operators are supposed to work on 
> volatile data, ie people are literally using them for things like
> 
> 	while (test_bit(TASKLET_STATE_SCHED, &t->state));
> 
> and the thing is supposed to work.

Well, I agree it's supposed to work, what I don't agree about is that
generic code gets to designate critical data as volatile.

> Now, I personally am not a big believer in the "volatile" keyword itself, 
> since I believe that anybody who expects the compiler to generate 
> different code for volatiles and non-volatiles is pretty much waiting for 
> a bug to happen, but there are two cases where I think it's ok:
> 
>  - in function prototypes to show that the function can take volatile data 
>    (and not complain).

This is a real problem for us on parisc though.  By designating the
function prototype as volatile, you force the function implementation
*also* to be volatile.  Unfortunately, in our case, gcc seems to
generate worse code than a pigs arse when it sees volatile data lurking
anywhere in a function.

Our current set bit functions are *coded* to operate on volatile data,
we just don't use the volatile keyword to persuade gcc to generate
better code.

I realise we could hand code in assembly to get around this problem, but
our implementation actually has to use hashed spinlocks for the
volatility, so we'd rather not if we really don't have to.

The kernel has survived this far with no other bit operator data being
volatile.

>  - as an arch-specific low-level implementation detail to avoid having to 
>    use inline assembly just to load a value. Ie a _data_structure_ should 
>    never be volatile, but it's ok to use a volatile pointer for a single 
>    access.

Definitely agree here ... the arch code is really the place we know the
compiler penalty of being volatile.

James

Re: [PATCH] Fix the cpumask rewrite

[Linus Torvalds]

On Sat, 26 Jun 2004, James Bottomley wrote:
>
> On Sat, 2004-06-26 at 11:32, Linus Torvalds wrote:
> > Why not? The thing is, the bitmap operators are supposed to work on 
> > volatile data, ie people are literally using them for things like
> > 
> > 	while (test_bit(TASKLET_STATE_SCHED, &t->state));
> > 
> > and the thing is supposed to work.
> 
> Well, I agree it's supposed to work, what I don't agree about is that
> generic code gets to designate critical data as volatile.

I agree in the sense that I don't think the _data_ should be volatile.

But I think the functions to access various pieces of data should be able 
to take volatiles without warning.

See the difference? Same way "memcpy()" takes a "const" argument for the 
source. That doesn't mean that the source _has_ to be const, it just means 
that it won't complain if it is.

And the same is true of "volatile" for the bitop functions. They are 
volatile not because they require the data to be volatile, but because 
they have at least traditionally been used for various cases, _including_ 
volatile.

Now, we could say that we don't do that any more, and decide that the 
regular bitop functions really cannot be used on volatile stuff. But 
that's a BIG decision. And it's certainly not a decision that parisc 
users should make.

> Our current set bit functions are *coded* to operate on volatile data,
> we just don't use the volatile keyword to persuade gcc to generate
> better code.

Well, at least judging by your "test_bit()", the function literally is 
_not_ coded to work with volatile data.

If the above loop had been a real case, gcc on parisc would have optimized 
it away, and done the wrong thing.

		Linus

Re: [PATCH] Fix the cpumask rewrite

[James Bottomley]

On Sat, 2004-06-26 at 11:54, Linus Torvalds wrote:
> On Sat, 26 Jun 2004, James Bottomley wrote:
> >
> > On Sat, 2004-06-26 at 11:32, Linus Torvalds wrote:
> > > Why not? The thing is, the bitmap operators are supposed to work on 
> > > volatile data, ie people are literally using them for things like
> > > 
> > > 	while (test_bit(TASKLET_STATE_SCHED, &t->state));

I tried this on PA.  Our gcc definitely generates the correct code, even
without the volatile...

By the way, I had to try with a genuine volatile since
tasklet_struct.state isn't actually volatile in linux/interrupt.h

> > > and the thing is supposed to work.
> > 
> > Well, I agree it's supposed to work, what I don't agree about is that
> > generic code gets to designate critical data as volatile.
> 
> I agree in the sense that I don't think the _data_ should be volatile.
> 
> But I think the functions to access various pieces of data should be able 
> to take volatiles without warning.
> 
> See the difference? Same way "memcpy()" takes a "const" argument for the 
> source. That doesn't mean that the source _has_ to be const, it just means 
> that it won't complain if it is.
> 
> And the same is true of "volatile" for the bitop functions. They are 
> volatile not because they require the data to be volatile, but because 
> they have at least traditionally been used for various cases, _including_ 
> volatile.

But in the current kernel, there are no bitops on volatile data in the
parisc tree.  This cpumask is the first such one that we've come
across...

> Now, we could say that we don't do that any more, and decide that the 
> regular bitop functions really cannot be used on volatile stuff. But 
> that's a BIG decision. And it's certainly not a decision that parisc 
> users should make.

But that's my point.  Currently there is no volatile pointer bitops (and
I have bitten the heads off a few people who tried to introduce some),
so there's no existing case to justify being backward compatible with.

> Well, at least judging by your "test_bit()", the function literally is 
> _not_ coded to work with volatile data.

test_bit's a special case.  The lock is to prevent data corruption, not
racing between test_bit and set_bit.

> If the above loop had been a real case, gcc on parisc would have optimized 
> it away, and done the wrong thing.

That's not what our gcc seems to do (both 3.0.4 and 3.3.4)

James

Re: [PATCH] Fix the cpumask rewrite

[Linus Torvalds]

On Sat, 26 Jun 2004, James Bottomley wrote:
> 
> But in the current kernel, there are no bitops on volatile data in the
> parisc tree.  This cpumask is the first such one that we've come
> across...

So?

You're ignoring my point. 

I'm saying that data structures ARE NOT VOLATILE. I personally believe 
that the notion of a "volatile" data structure is complete and utter shit.

But _code_ can act "with volatility". For bitops, it's just another way of 
saying "people may use this function without locking", because it's 
totally valid to use "test_bit()" to actually implement a lock.

So the rule has always been (and this has nothing to do with parisc, and 
parisc DOES NOT GET TO SET THE RULES!) that "test_bit()" acts in a 
volatile manner, and that you traditionally could write

	while (test_and_set_bit(xxx, field))
		while (test_bit(xx, field)) /* Nothing */;

to do a lock.

NOTE! There are no volatile data structures _anywhere_, because as
mentioned, I think that whole notion is a total piece of crap, and is not
even a well-defined part of the C language. But the test_bit() function
has to act as if the data it is passed down can change. To repeat:  
_codepaths_ may have volatility attributes depending on usage of the data.

Now, we generally discouage this kind of hand-written locking code, and we
these days have a "bit_spin_lock()" helper function to do the above (which
also has the appropriate "cpu_relax()" in place), but the point is that
this code has existed, and test_bit() HAS TO FOLLOW THE RULES. Even on
pa-risc.

I repeat: this has NOTHING to do with "volatile data structures". The
kernel does not belive in that notion, and you generally have to use locks
to make sure that all accesses are serialized.

But sometimes non-locked accesses are ok. test_bit() historically is very
much one of those things. A classic example of this is how something like
bh->b_flags is not marked volatile (some fields of it require explicit
locking rules), but it's ok to check for certain bits of it asynchronously
without having locked the bh.

See? 

 - CODE can be volatile ("I must re-load this value").
 - DATA has access rules ("this value should only be changed with write
   lock xxx held").

And in this case, test_bit() has the "I must re-load this value" rule for 
historical reasons. 

AND PA-RISC IS WRONG IF IT DOESN'T FOLLOW THE RULES!

Final note: I might be willing to just change the rules, if people can 
show that no paths that might need the volatile behaviour exist any more. 
They definitely used to exist, though, and that's a BIG decision to make.

		Linus

Re: [PATCH] Fix the cpumask rewrite

[Vojtech Pavlik]

On Sat, Jun 26, 2004 at 11:01:14AM -0700, Linus Torvalds wrote:

> Final note: I might be willing to just change the rules, if people can 
> show that no paths that might need the volatile behaviour exist any more. 
> They definitely used to exist, though, and that's a BIG decision to make.

At least input pretty much relies on the fact that bitops don't need
locking and act as memory barriers.

-- 
Vojtech Pavlik
SuSE Labs, SuSE CR

Re: [PATCH] Fix the cpumask rewrite

[Linus Torvalds]

On Sat, 26 Jun 2004, Vojtech Pavlik wrote:
> 
> At least input pretty much relies on the fact that bitops don't need
> locking and act as memory barriers.

Well, plain test_bit() has always been more relaxed than the others, and
has never implied a memory barrier. Only the "test_and_set/clear()" things
imply memory barriers.

What we _could_ do (without changing any existing rules) is to add a
"__test_bit()" that is the relaxed version that doesn't do any of the
volatile etc. That would match the "__"  versions of the other bit
operations.

Then people who know that they use the bits without any volatility issues 
can use that one, and let the compiler optimize more. 

Hmm?

		Linus

Re: [PATCH] Fix the cpumask rewrite

[James Bottomley]

On Sat, 2004-06-26 at 13:01, Linus Torvalds wrote:
> So?
> 
> You're ignoring my point. 

You're making 3 points, I think

1) Volatility is a property of the code path, not the data, which I
agree with.

2) the bit operators need to operate on volatile data (i.e. need a
volatile in their prototypes) without gcc issuing a qualifier dropped
warning.

This I disagree with because we have no volatile data currently in the
kernel that necessitates this behavour.

3) The parisc bit operator implementations as inline functions need to
have volatile data in their function templates because otherwise gcc
will not implement them correctly and may optimise them away when it
shouldn't.

I disagree with this too...although I'm on shakier ground, and I'd
prefer gcc experts quantify why this happens, but if, on parisc, I look
at the assembly output of your example

        while (test_and_set_bit(xxx, field))
                while (test_bit(xx, field)) /* Nothing */;

I find it to be coded exactly correctly.  Even without using a volatile
pointer in our test_and_set_bit() and test_bit() implementations, the
compiler still does both checks in the loop.

So my contention is that even without the volatile pointers in our
implementation, we still correctly treat this code path as having
volatile (i.e. we test the bits each time around the loop).  All the
addition of the volatile to the cpumask patch does is cause us to emit
spurious warnings.

> And in this case, test_bit() has the "I must re-load this value" rule for 
> historical reasons. 

Right, I agree exactly with this.  However, empirically we do do this,
even without having to declare the data as volatile.

> AND PA-RISC IS WRONG IF IT DOESN'T FOLLOW THE RULES!

I belive we do follow the rules.

> Final note: I might be willing to just change the rules, if people can 
> show that no paths that might need the volatile behaviour exist any more. 
> They definitely used to exist, though, and that's a BIG decision to make.

Actually, I don't want to change the rules, I just want parisc to be
able to implement them without being forced to have a performance
killing volatile in its implementation functions.

James

Re: [PATCH] Fix the cpumask rewrite

[James Bottomley]

On Sat, 2004-06-26 at 13:54, Linus Torvalds wrote:
> On Sat, 26 Jun 2004, Vojtech Pavlik wrote:
> > At least input pretty much relies on the fact that bitops don't need
> > locking and act as memory barriers.
> 
> Well, plain test_bit() has always been more relaxed than the others, and
> has never implied a memory barrier. Only the "test_and_set/clear()" things
> imply memory barriers.
> 
> What we _could_ do (without changing any existing rules) is to add a
> "__test_bit()" that is the relaxed version that doesn't do any of the
> volatile etc. That would match the "__"  versions of the other bit
> operations.
> 
> Then people who know that they use the bits without any volatility issues 
> can use that one, and let the compiler optimize more. 

Well, we can do this, yes.

Our test bit implementation would then become:

static __inline__ int test_bit(int nr, const volatile void *address)
{
	return __test_bit(nr, (const void *)address);
}

That would keep our implementation happy.

James

Re: [PATCH] Fix the cpumask rewrite

[Linus Torvalds]

On Sat, 26 Jun 2004, James Bottomley wrote:
> 
> 1) Volatility is a property of the code path, not the data, which I
> agree with.

Right.

> 2) the bit operators need to operate on volatile data (i.e. need a
> volatile in their prototypes) without gcc issuing a qualifier dropped
> warning.
> 
> This I disagree with because we have no volatile data currently in the
> kernel that necessitates this behavour.

Why do you disagree, since
 - right now PA-RISC is BUGGY because of the lack of volatile. gcc _does_ 
   optimize and combine accesses since it's an inline function _without_
   any volatile specifier on the pointers.
 - the volatile _documents_ the fact that the bitops have volatile 
   behaviour.

> 3) The parisc bit operator implementations as inline functions need to
> have volatile data in their function templates because otherwise gcc
> will not implement them correctly and may optimise them away when it
> shouldn't.
> 
> I disagree with this too...although I'm on shakier ground, and I'd
> prefer gcc experts quantify why this happens, but if, on parisc, I look
> at the assembly output of your example
> 
>         while (test_and_set_bit(xxx, field))
>                 while (test_bit(xx, field)) /* Nothing */;

It seems the pa-risc optimizer for gcc is somehow broken. I just checked 
on x86:

	#define test_bit(x,y) \
	        (!!((1ul << x) & *(y)))

	int test(unsigned long *a)
	{
	        while (test_bit(0, a));
	}

definitely does not re-load the value. I checked with an inline function 
too, same result:

	        testb   $1, (%eax)
	        .p2align 2,,3
	.L2:
	        jne     .L2

ie gcc _does_ optimize this away without the volatile. With the volatile 
it turns into

	        .p2align 2,,3
	.L2:
	        movl    (%edx), %eax
	        testb   $1, %al
	        jne     .L2

so there's a clear difference here.

Now, why gcc on pa-risc misses that optimization, I have no clue. 

> So my contention is that even without the volatile pointers in our
> implementation, we still correctly treat this code path as having
> volatile (i.e. we test the bits each time around the loop).  All the
> addition of the volatile to the cpumask patch does is cause us to emit
> spurious warnings.

That's apparently just you being lucky due to having a broken gcc.

			Linus

Re: [PATCH] Fix the cpumask rewrite

[Vojtech Pavlik]

On Sat, Jun 26, 2004 at 11:54:24AM -0700, Linus Torvalds wrote:

> On Sat, 26 Jun 2004, Vojtech Pavlik wrote:
> > 
> > At least input pretty much relies on the fact that bitops don't need
> > locking and act as memory barriers.
> 
> Well, plain test_bit() has always been more relaxed than the others, and
> has never implied a memory barrier. Only the "test_and_set/clear()" things
> imply memory barriers.

Ouch. I'll have to revisit some code then.

> What we _could_ do (without changing any existing rules) is to add a
> "__test_bit()" that is the relaxed version that doesn't do any of the
> volatile etc. That would match the "__"  versions of the other bit
> operations.
> 
> Then people who know that they use the bits without any volatility issues 
> can use that one, and let the compiler optimize more. 
> 
> Hmm?
 
That makes a lot of sense to me, as we already have the __ variants for
most of the other bitops already.

-- 
Vojtech Pavlik
SuSE Labs, SuSE CR

Re: [PATCH] Fix the cpumask rewrite

[Linus Torvalds]

On Sat, 26 Jun 2004, James Bottomley wrote:
> 
> Our test bit implementation would then become:
> 
> static __inline__ int test_bit(int nr, const volatile void *address)
> {
> 	return __test_bit(nr, (const void *)address);
> }
> 
> That would keep our implementation happy.

You just _want_ to be screwed over whenever your gcc bugs are fixed, don't 
you?

Are you going to complain to the gcc people when they fix their bugs? Or 
are you going to spend months to debug problems that only happen for other 
people, because they happen to have fixed compilers?

There's a real reason why there is a "volatile" there on other 
architectures. 

		Linus

Re: [PATCH] Fix the cpumask rewrite

[James Bottomley]

On Sat, 2004-06-26 at 14:11, Linus Torvalds wrote:
> It seems the pa-risc optimizer for gcc is somehow broken. I just checked 
> on x86:
> 
> 	#define test_bit(x,y) \
> 	        (!!((1ul << x) & *(y)))
> 
> 	int test(unsigned long *a)
> 	{
> 	        while (test_bit(0, a));
> 	}

OK, this one definitely compiles to a non reaload loop on parisc.  I
concede we need the volatile.

James

Re: [PATCH] Fix the cpumask rewrite

[Chris Wedgwood]

On Sat, Jun 26, 2004 at 09:32:15AM -0700, Linus Torvalds wrote:

> Now, I personally am not a big believer in the "volatile" keyword
> itself, since I believe that anybody who expects the compiler to
> generate different code for volatiles and non-volatiles is pretty
> much waiting for a bug to happen

I recently had to change jiffies_64 (include/linux/jiffies.h) to be
volatile as gcc produced code that didn't work as a result of it.

Clearly in some cases gcc does know about volatile and does produce
'the right thing' --- I don't really see why people claim volatile is
a bad thing, there are clearly places where we need this and gcc seems
to do the right thing.


  --cw

Re: [PATCH] Fix the cpumask rewrite

[Linus Torvalds]

On Sat, 26 Jun 2004, Chris Wedgwood wrote:
> 
> I recently had to change jiffies_64 (include/linux/jiffies.h) to be
> volatile as gcc produced code that didn't work as a result of it.

"jiffies" is one of the few things that I accept as volatile, since it's 
basically read-only and accessed directly and has no internal structure 
(ie it's a single word).

However, "jiffies_64" should _not_ be accessed directly. Anything that 
does so is likely buggy.

But for most data structures, the way to control access is either with 
proper locking (at which point they aren't volatile any more) or through 
proper accessor functions (ie "jiffies_64" should generally only be 
accessed with something that understands about low/high word and update 
ordering and re-testing).

And once you do that proper access function, you should put the volatile 
_there_ - since THAT is the place that understands what the access 
requireemnts are, not the compiler.

I repeat: it is the _code_ that knows about volatile rules, not the data
structure.

> Clearly in some cases gcc does know about volatile and does produce
> 'the right thing' --- I don't really see why people claim volatile is
> a bad thing, there are clearly places where we need this and gcc seems
> to do the right thing.

See above. There are basically _no_ places where the compiler can do the 
right thing except by pure luck. "Programming-by-luck" may work, but it's 
not a good idea.

So please tell me where the jiffies64 thing was suddenly correct as a 
"volatile", and I can almost guarantee you that you were WRONG to mark it 
volatile. It may work 99% of the time, but since the only correct way to 
access jiffies_64 is either:

 - knowing that you are the person that updates it (under xtime_lock),
   thus making it non-volatile
 - using "get_jiffies_64()", using the proper seq_begin() etc.

But you're right, on a 64-bit architecture, jiffies_64 falls back to the
"jiffies" case, and there it would be acceptable to make it volatile. Bit 
since it had better be accessed with "get_jiffies_64()" anyway, you're 
much better off putting the volatile THERE, and not cause problems for 
code generation in the places that don't need it.

See? The volatile is (again) wrong on the data structure (jiffies_64), and
you should have added it to the code (get_jiffies_64).

		Linus

Re: [parisc-linux] Re: [PATCH] Fix the cpumask rewrite

[Alan Cox]

On Sad, 2004-06-26 at 23:48, Linus Torvalds wrote:
> "jiffies" is one of the few things that I accept as volatile, since it's 
> basically read-only and accessed directly and has no internal structure 
> (ie it's a single word).

For most uses jiffies should die. If drivers could not access jiffies
except by a (possibly trivial) helper then it would be a huge step
closer to being able to run embedded linux without a continually running
timer.

jiffies_64

[Chris Wedgwood]

On Sat, Jun 26, 2004 at 03:48:34PM -0700, Linus Torvalds wrote:

> So please tell me where the jiffies64 thing was suddenly correct as
> a "volatile", and I can almost guarantee you that you were WRONG to
> mark it volatile.

In this case it's a platform where moving jiffies around is very
painful so I have a patch to make jiffies per-CPU using the local APIC
timer (jiffies becomes a macro to get_local_jiffies() with all the
horrors that come with that).

Because this drifts we use the PIT as a referrence and resynchronize
every few seconds against jiffies_64.

A few places can't actually use get_local_jiffies() early on like the
bogomips calibration code (because the local APIC timer isn't working
yet) so I've got some hacks in there where they use jiffies_64.

> But you're right, on a 64-bit architecture, jiffies_64 falls back to
> the "jiffies" case, and there it would be acceptable to make it
> volatile.

Actually, I don't like the way we use xtime_lock at all for jiffies_64
now.  Things are unclear and fragile in places.  In my tree I have
jiffies_64 protected by a spinlock instead of the seqlock thing (as it
still needs to be locked even though it's incremented only from one
CPU in irq context) and was looking at using cmpxchg8 on platforms
what can to remove the need to lock jiffies_64 completely.

I've wondered if we can't do the same thing for xtime as well,
however I need to make per-CPU xtime so I'll revisit that when I get a
chance.

> See? The volatile is (again) wrong on the data structure (jiffies_64), and
> you should have added it to the code (get_jiffies_64).

get_jiffies_64() in my case reads the per-CPU value and like I said a
few places don't like that.


  --cw

Re: [parisc-linux] Re: [PATCH] Fix the cpumask rewrite

[Chris Wedgwood]

On Sat, Jun 26, 2004 at 11:54:38PM +0100, Alan Cox wrote:

> For most uses jiffies should die. If drivers could not access jiffies
> except by a (possibly trivial) helper then it would be a huge step
> closer to being able to run embedded linux without a continually running
> timer.

I'm all for that, except last I counted there are about 5000 users of
jiffies.  What do you suggest as a replacement API?



  --cw

more (insane) jiffies ranting

[Chris Wedgwood]

Continuing my rant...

On Sat, Jun 26, 2004 at 03:48:34PM -0700, Linus Torvalds wrote:

> But for most data structures, the way to control access is either
> with proper locking (at which point they aren't volatile any more)
> or through proper accessor functions (ie "jiffies_64" should
> generally only be accessed with something that understands about
> low/high word and update ordering and re-testing).

I don't entirely buy this.  Right now x86 code just assumes 32-bit
loads are atomic and does them blindly in lots of places (ie. every
user of jiffies just about).

Without the volatile it seems entirely reasonable gcc will produce
correct, but wrong code here so I would argue 'volatile' is a property
of the data in this case.

> I repeat: it is the _code_ that knows about volatile rules, not the
> data structure.

Except as I mentioned we have exceptions to this right now.

As far as I can tell jiffies is a mess (I'm talking mostly ia32 here):

  jiffies_64 is protected by xtime_lock, this is a seqlock_t which
  is IMO overly complicated and unnecessary, and this lock is shared
  for xtime as well

  jiffies_64 could be done locklessly as far as I can tell anyhow.

  jiffies is linker-magic to jiffies_64 and works because a
  little-endian load at the same address gives you the 32 lower bits.
  I'm not opposed to this, but a comment wouldn't kill anyone.

  we also have wall_jiffies which is 32-bit (unsigned long, ia32) and
  is used get the gettimeofday code to detect lost ticks, having
  this as well as jiffies_64 seems overkill

  we do xtime updates w/o a lock on most platforms

Perhaps I misunderstand the code right now, the need for the
complexity and what-not --- but I don't like it.

It's either because it's too complicated or it's not-clear what is
going on, I don't know which one matches reality most closely,
probably the latter.

I know there are NTP implications of this this code but it feels more
like "it happens to work, please don't touch it" rather than anything
clean and well designed.  I'm pretty sure there are some tricky corner
cases and subtle interactions to worry about, especially when we look
at leap-seconds.

Maybe having all this code moved into a single place would help, I'm
not sure, with platform-provided abstraction as required so older
platforms which can't do atomic 64-bit updates will still behave
sanely.

To be quite honest I'd like to see jiffies die and xtime become a
per-cpu thing (if that can be made to work reliably, I have some
concerns), or at least have this option on a platform by platform
basis.



  --cw

Re: [parisc-linux] Re: [PATCH] Fix the cpumask rewrite

[Matthias Urlichs]

Hi, Chris Wedgwood wrote:

> On Sat, Jun 26, 2004 at 11:54:38PM +0100, Alan Cox wrote:
> 
>> For most uses jiffies should die. If drivers could not access jiffies
>> except by a (possibly trivial) helper then it would be a huge step
>> closer to being able to run embedded linux without a continually running
>> timer.
> 
> I'm all for that, except last I counted there are about 5000 users of
> jiffies.  What do you suggest as a replacement API?

<heretic>

#define jiffies __get_jiffies()

</heretic>

-- 
Matthias Urlichs

Re: [parisc-linux] Re: [PATCH] Fix the cpumask rewrite

[Alan Cox]

On Sul, 2004-06-27 at 01:05, Chris Wedgwood wrote:
> I'm all for that, except last I counted there are about 5000 users of
> jiffies.  What do you suggest as a replacement API?

For a lot of them they shouldn't be polling. For those that do in
most cases something like

	struct timeout_timer t;

	timeout_set(t, 5 * HZ)
	timeout_cancel(t)

	if(timeout_expired(t))

Where timeout_timer is effectively an existing timer of add_timer
style and a single variable. The code to build that kind of timer
on top of add_timer is trivial.

[parisc-linux] Re: more (insane) jiffies ranting

[Linus Torvalds]

On Sat, 26 Jun 2004, Chris Wedgwood wrote:
> 
> On Sat, Jun 26, 2004 at 03:48:34PM -0700, Linus Torvalds wrote:
> 
> > But for most data structures, the way to control access is either
> > with proper locking (at which point they aren't volatile any more)
> > or through proper accessor functions (ie "jiffies_64" should
> > generally only be accessed with something that understands about
> > low/high word and update ordering and re-testing).
> 
> I don't entirely buy this.  Right now x86 code just assumes 32-bit
> loads are atomic and does them blindly in lots of places (ie. every
> user of jiffies just about).
> 
> Without the volatile it seems entirely reasonable gcc will produce
> correct, but wrong code here so I would argue 'volatile' is a property
> of the data in this case.

It's a property of the data _iff_:
 - it is _always_ volatile
 - it is only ever used atomically: this also means that it must be 
   totally independent of _all_ other data structures and have no linkages 
   to anything else.

Snd basically, the above is pretty much never true except possibly for 
real I/O accesses and sometimes things like simple "flags" (ie it's fine 
to use "volatile sigatomic_t flag;" in user programs to have signal 
handlers say "something happened" in a single-threaded environment).

NOTE! The "single-threaded environment" part really is important, and is 
one of the historical reasons for volatile having been more useful than it 
is today. If you are single-threaded and don't have issues like CPU memory 
ordering etc, then you can let the compiler do more of the work, and there 
are a lot of lockless algorithms that you can use that only depend on 
fairly simple semantics for "volatile".

But the fact is, for the kernel none of the above is ever really true. 
A 32-bit-atomic "jiffies" comes the closest, but even there the "always" 
property wasn't true - it wasn't true in the update phase, and we 
literally used to have something like this:

	*((unsigned long *)&jiffies)++;

to update jiffies and still get good code generation (now that we have a
64-bit jiffies and need to do more complex stuff anyway, we don't have
that any more, but you should be able to find it in 2.3.x kernels if I
remember correctly).

And _anything_ that has any data dependencies, "volatile" is totally
useless. Even the (acceptable in single-threaded user-space) "flag" thing 
is not valid usage in the kernel, since for a flag in a multi-threaded 
environment you still need an explicit CPU memory barrier in the code, 
making it impossible for the compiler to do the right thing anyway.

> > I repeat: it is the _code_ that knows about volatile rules, not the
> > data structure.
> 
> Except as I mentioned we have exceptions to this right now.

No we don't. The _only_ accepted exception is the special case of "the low
bits of jiffies", and that's accepted partly because of historical
reasons, and partly because it's fundamentally a data structure we don't
really care that much about. There should be no other ones.

And that special case _literally_ is only for people who don't care that 
much. Anybody who cares about "real time" needs to get xtime_lock and do 
the proper magic to get a real date.

So I don't see your argument. I'm obviously saying that "yes, we have 
_one_ case where we make a data structure volatile", but at the same time, 
that case is very much a "we don't really care too much about precision 
there, and even so people think we should have real accessor functions".

So I stand by the rule: we should make _code_ have the access rules, and
the data itself should never be volatile. And yes, jiffies breaks that
rule, but hey, that's not something I'm proud of.

		Linus
_______________________________________________
parisc-linux mailing list
parisc-linux@lists.parisc-linux.org
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux

Re: more (insane) jiffies ranting

[Linus Torvalds]

On Sat, 26 Jun 2004, Chris Wedgwood wrote:
> 
> On Sat, Jun 26, 2004 at 03:48:34PM -0700, Linus Torvalds wrote:
> 
> > But for most data structures, the way to control access is either
> > with proper locking (at which point they aren't volatile any more)
> > or through proper accessor functions (ie "jiffies_64" should
> > generally only be accessed with something that understands about
> > low/high word and update ordering and re-testing).
> 
> I don't entirely buy this.  Right now x86 code just assumes 32-bit
> loads are atomic and does them blindly in lots of places (ie. every
> user of jiffies just about).
> 
> Without the volatile it seems entirely reasonable gcc will produce
> correct, but wrong code here so I would argue 'volatile' is a property
> of the data in this case.

It's a property of the data _iff_:
 - it is _always_ volatile
 - it is only ever used atomically: this also means that it must be 
   totally independent of _all_ other data structures and have no linkages 
   to anything else.

Snd basically, the above is pretty much never true except possibly for 
real I/O accesses and sometimes things like simple "flags" (ie it's fine 
to use "volatile sigatomic_t flag;" in user programs to have signal 
handlers say "something happened" in a single-threaded environment).

NOTE! The "single-threaded environment" part really is important, and is 
one of the historical reasons for volatile having been more useful than it 
is today. If you are single-threaded and don't have issues like CPU memory 
ordering etc, then you can let the compiler do more of the work, and there 
are a lot of lockless algorithms that you can use that only depend on 
fairly simple semantics for "volatile".

But the fact is, for the kernel none of the above is ever really true. 
A 32-bit-atomic "jiffies" comes the closest, but even there the "always" 
property wasn't true - it wasn't true in the update phase, and we 
literally used to have something like this:

	*((unsigned long *)&jiffies)++;

to update jiffies and still get good code generation (now that we have a
64-bit jiffies and need to do more complex stuff anyway, we don't have
that any more, but you should be able to find it in 2.3.x kernels if I
remember correctly).

And _anything_ that has any data dependencies, "volatile" is totally
useless. Even the (acceptable in single-threaded user-space) "flag" thing 
is not valid usage in the kernel, since for a flag in a multi-threaded 
environment you still need an explicit CPU memory barrier in the code, 
making it impossible for the compiler to do the right thing anyway.

> > I repeat: it is the _code_ that knows about volatile rules, not the
> > data structure.
> 
> Except as I mentioned we have exceptions to this right now.

No we don't. The _only_ accepted exception is the special case of "the low
bits of jiffies", and that's accepted partly because of historical
reasons, and partly because it's fundamentally a data structure we don't
really care that much about. There should be no other ones.

And that special case _literally_ is only for people who don't care that 
much. Anybody who cares about "real time" needs to get xtime_lock and do 
the proper magic to get a real date.

So I don't see your argument. I'm obviously saying that "yes, we have 
_one_ case where we make a data structure volatile", but at the same time, 
that case is very much a "we don't really care too much about precision 
there, and even so people think we should have real accessor functions".

So I stand by the rule: we should make _code_ have the access rules, and
the data itself should never be volatile. And yes, jiffies breaks that
rule, but hey, that's not something I'm proud of.

		Linus

Re: more (insane) jiffies ranting

[Linus Torvalds]

On Sat, 26 Jun 2004, Chris Wedgwood wrote:
> 
> On Sat, Jun 26, 2004 at 03:48:34PM -0700, Linus Torvalds wrote:
> 
> > But for most data structures, the way to control access is either
> > with proper locking (at which point they aren't volatile any more)
> > or through proper accessor functions (ie "jiffies_64" should
> > generally only be accessed with something that understands about
> > low/high word and update ordering and re-testing).
> 
> I don't entirely buy this.  Right now x86 code just assumes 32-bit
> loads are atomic and does them blindly in lots of places (ie. every
> user of jiffies just about).
> 
> Without the volatile it seems entirely reasonable gcc will produce
> correct, but wrong code here so I would argue 'volatile' is a property
> of the data in this case.

It's a property of the data _iff_:
 - it is _always_ volatile
 - it is only ever used atomically: this also means that it must be 
   totally independent of _all_ other data structures and have no linkages 
   to anything else.

Snd basically, the above is pretty much never true except possibly for 
real I/O accesses and sometimes things like simple "flags" (ie it's fine 
to use "volatile sigatomic_t flag;" in user programs to have signal 
handlers say "something happened" in a single-threaded environment).

NOTE! The "single-threaded environment" part really is important, and is 
one of the historical reasons for volatile having been more useful than it 
is today. If you are single-threaded and don't have issues like CPU memory 
ordering etc, then you can let the compiler do more of the work, and there 
are a lot of lockless algorithms that you can use that only depend on 
fairly simple semantics for "volatile".

But the fact is, for the kernel none of the above is ever really true. 
A 32-bit-atomic "jiffies" comes the closest, but even there the "always" 
property wasn't true - it wasn't true in the update phase, and we 
literally used to have something like this:

	*((unsigned long *)&jiffies)++;

to update jiffies and still get good code generation (now that we have a
64-bit jiffies and need to do more complex stuff anyway, we don't have
that any more, but you should be able to find it in 2.3.x kernels if I
remember correctly).

And _anything_ that has any data dependencies, "volatile" is totally
useless. Even the (acceptable in single-threaded user-space) "flag" thing 
is not valid usage in the kernel, since for a flag in a multi-threaded 
environment you still need an explicit CPU memory barrier in the code, 
making it impossible for the compiler to do the right thing anyway.

> > I repeat: it is the _code_ that knows about volatile rules, not the
> > data structure.
> 
> Except as I mentioned we have exceptions to this right now.

No we don't. The _only_ accepted exception is the special case of "the low
bits of jiffies", and that's accepted partly because of historical
reasons, and partly because it's fundamentally a data structure we don't
really care that much about. There should be no other ones.

And that special case _literally_ is only for people who don't care that 
much. Anybody who cares about "real time" needs to get xtime_lock and do 
the proper magic to get a real date.

So I don't see your argument. I'm obviously saying that "yes, we have 
_one_ case where we make a data structure volatile", but at the same time, 
that case is very much a "we don't really care too much about precision 
there, and even so people think we should have real accessor functions".

So I stand by the rule: we should make _code_ have the access rules, and
the data itself should never be volatile. And yes, jiffies breaks that
rule, but hey, that's not something I'm proud of.

		Linus

Re: [parisc-linux] Re: [PATCH] Fix the cpumask rewrite

[Chris Wedgwood]

On Sun, Jun 27, 2004 at 02:00:03PM +0200, Matthias Urlichs wrote:

> <heretic>
>
> #define jiffies __get_jiffies()
>
> </heretic>

Well, I have that but it's only part of the story.

The real issue seems to be that a suitable clean API for drivers to
use rather than intenral knowledge of jiffies is lacking.

As Alan pointed out a suitable API could also make it easier to work
towards a clock-less system for embedded targets.


   --cw

Re: [parisc-linux] Re: [PATCH] Fix the cpumask rewrite

[Matthias Urlichs]

Hi,

Chris Wedgwood:
> On Sun, Jun 27, 2004 at 02:00:03PM +0200, Matthias Urlichs wrote:
> 
> > <heretic>
> > #define jiffies __get_jiffies()
> > </heretic>
> 
> Well, I have that but it's only part of the story.
> 
True.

> As Alan pointed out a suitable API could also make it easier to work
> towards a clock-less system for embedded targets.
> 
Well, drivers do need some way of timing things, else they wouldn't read
jiffies in the first place. So, at minimum, an embedded system would
need a way to trigger a timeout at some specified time in the future.

A simple __get_jiffies() implementation could just set up a 1/HZ-second
timer (and busy-wait for it, and increase its internal jiffies counter)
every tenth call or so. That would probably slow down the whole system
somewhat, but I'd assume it'd mostly work.

-- 
Matthias Urlichs

Re: [parisc-linux] Re: [PATCH] Fix the cpumask rewrite

[Chris Wedgwood]

On Mon, Jun 28, 2004 at 03:24:07AM +0200, Matthias Urlichs wrote:

> A simple __get_jiffies() implementation could just set up a
> 1/HZ-second timer (and busy-wait for it, and increase its internal
> jiffies counter) every tenth call or so. That would probably slow
> down the whole system somewhat, but I'd assume it'd mostly work.

I think the approach is to look at _why_ people need jiffies and
change this to a cleaner less low-level API using accessor
functions/macros.


  --cw

Re: [parisc-linux] Re: [PATCH] Fix the cpumask rewrite

[Matthias Urlichs]

Hi,

Chris Wedgwood:
> I think the approach is to look at _why_ people need jiffies and
> change this to a cleaner less low-level API using accessor
> functions/macros.
> 
That would be the correct long-term approach, true. 

However, going over 5000 jiffies usages and re-doing all of them doesn't
happen overnight, and I do suspect that some people want their embedded
clockless systems to run Linux 2.4 or 2.6, rather than 2.8...

-- 
Matthias Urlichs   |   {M:U} IT Design @ m-u-it.de   |  smurf@smurf.noris.de

Re: [parisc-linux] Re: [PATCH] Fix the cpumask rewrite

[Chris Wedgwood]

On Mon, Jun 28, 2004 at 08:55:08AM +0200, Matthias Urlichs wrote:

> However, going over 5000 jiffies usages and re-doing all of them
> doesn't happen overnight, and I do suspect that some people want
> their embedded clockless systems to run Linux 2.4 or 2.6, rather
> than 2.8...

Such (embedded) hardware uses a very small percentage of the drivers
we have in the tree, changing them as required seems quite sane and
manageable in 2.7.x time-frame.

I also do see why clock-less has to be embedded only, I suspect maybe
the s390 could make use of this too?  (Think of lots of mostly-idle
Linux instances under VM).

And, it's a 'new feature' so if people need to upgrade, then Life is
tough.  How sad.


  --cw

Re: [parisc-linux] Re: [PATCH] Fix the cpumask rewrite

[Matthias Urlichs]

Hi,

Chris Wedgwood:
> On Mon, Jun 28, 2004 at 08:55:08AM +0200, Matthias Urlichs wrote:
> > However, going over 5000 jiffies usages and re-doing all of them
> > doesn't happen overnight, and I do suspect that some people want
> > their embedded clockless systems to run Linux 2.4 or 2.6, rather
> > than 2.8...
> 
> Such (embedded) hardware uses a very small percentage of the drivers
> we have in the tree, changing them as required seems quite sane and
> manageable in 2.7.x time-frame.
> 
... especially since the 2.7 time-frame hasn't even started yet.

> I also do see why clock-less has to be embedded only, I suspect maybe
> the s390 could make use of this too?  (Think of lots of mostly-idle
> Linux instances under VM).
> 
True. The same argument holds for "real" Linux systems, which could
easily sit in HLT for hours instead of twiddling their thumbs $KERNEL_HZ
times per second.

> And, it's a 'new feature' so if people need to upgrade, then Life is
> tough.  How sad.

I'm not contradicting you here. In fact, I seem to have taken the
<heretic> somewhat further than originally intended. :-/

-- 
Matthias Urlichs   |   {M:U} IT Design @ m-u-it.de   |  smurf@smurf.noris.de

Re: [PATCH] Fix the cpumask rewrite

[Jonathan Lundell]

At 11:01 AM -0700 6/26/04, Linus Torvalds wrote:
>I'm saying that data structures ARE NOT VOLATILE. I personally believe
>that the notion of a "volatile" data structure is complete and utter shit.

Perhaps, but surely they exist. I'm thinking specifically of 
memory-mapped hardware registers and data structures that are shared 
with DMA devices. Most recent Ethernet controllers fall into the 
latter category, and in either case write-locking is not an option.

If I can find some way to force my code to reload the data, then 
sure, call the code "volatile" if you like. But the data is simply 
volatile, in the sense that it can (and is expected to) change 
independent of my code paths.
-- 
/Jonathan Lundell.

Re: [PATCH] Fix the cpumask rewrite

[Pavel Machek]

Hi!

> And the same is true of "volatile" for the bitop functions. They are 
> volatile not because they require the data to be volatile, but because 
> they have at least traditionally been used for various cases, _including_ 
> volatile.
> 
> Now, we could say that we don't do that any more, and decide that the 
> regular bitop functions really cannot be used on volatile stuff. But 
> that's a BIG decision. And it's certainly not a decision that parisc 
> users should make.

Heh, with vojtech we introduced locking into input layer
(there was none before)... using test_bit/set_bit.

(I just hope set_bit etc implies memory barrier... or we'll have to do
it once more)

			Pavel
-- 
64 bytes from 195.113.31.123: icmp_seq=28 ttl=51 time=448769.1 ms         

Re: [parisc-linux] Re: [PATCH] Fix the cpumask rewrite

[Pavel Machek]

Hi!

> > I'm all for that, except last I counted there are about 5000 users of
> > jiffies.  What do you suggest as a replacement API?
> 
> For a lot of them they shouldn't be polling. For those that do in
> most cases something like
> 
> 	struct timeout_timer t;
> 
> 	timeout_set(t, 5 * HZ)
> 	timeout_cancel(t)
> 
> 	if(timeout_expired(t))
> 
> Where timeout_timer is effectively an existing timer of add_timer
> style and a single variable. The code to build that kind of timer
> on top of add_timer is trivial.

Plus some get_jiffies api for stuff like printk ratelimit would be needed,
right?
-- 
64 bytes from 195.113.31.123: icmp_seq=28 ttl=51 time=448769.1 ms         

Re: [parisc-linux] Re: [PATCH] Fix the cpumask rewrite

[Alan Cox]

On Iau, 2004-07-01 at 14:11, Pavel Machek wrote:
> Heh, with vojtech we introduced locking into input layer
> (there was none before)... using test_bit/set_bit.
> 
> (I just hope set_bit etc implies memory barrier... or we'll have to do
> it once more)
		
It doesn't - the ppp layer got burned badly by this long ago. set_bit is
not a memory barrier. OTOH you can just add an mb()

With respect to the need for a get_jiffies API I don't think you 
do need it for ratelimit.

	static DECLARE_TIMEOUT_EXPIRED(printk_timer);

	if(timeout_expired(t))
	{
		printk("Blah");
		timeout_set(t, 5 * HZ);
	}

if we followed the way MUTEX and the other defines work.

Re: [parisc-linux] Re: [PATCH] Fix the cpumask rewrite

[Linus Torvalds]

On Thu, 1 Jul 2004, Alan Cox wrote:
>
> On Iau, 2004-07-01 at 14:11, Pavel Machek wrote:
> > Heh, with vojtech we introduced locking into input layer
> > (there was none before)... using test_bit/set_bit.
> > 
> > (I just hope set_bit etc implies memory barrier... or we'll have to do
> > it once more)
> 		
> It doesn't - the ppp layer got burned badly by this long ago. set_bit is
> not a memory barrier. OTOH you can just add an mb()

The "test_and_xxx()" things are memory barriers, but set_bit/clear_bit 
aren't (since ther aren't really supposed to be usable for locking).

It _happens_ to be one on x86, so sadly it works on 99% of the machines 
out there. And to avoid the extra (suprefluous) mb(), there are

	smp_mb__before_clear_bit()
	smp_mb__after_clear_bit()

that only works with "clear_bit()", on the assumption that the way you'd 
do locking is:

	lock:
		while (test_and_set_bit(..)) /* This is a memory barrier */
			while (test_bit(..))
				cpu_relax();

	.. protected region ..

	unlock:
		smp_mb__before_clear_bit();
		clear_bit(..);

but the fact is, the above is broken too, for a totally _unrelated_ 
reason, namely preemption. And then you have the SMP case etc still.

The fact is, you shouldn't use the bitops for locking. You _will_ get it 
wrong. Use a spinlock, and if you _really_ really need just a single bit, 
use

	bit_spin_lock(bitnum,addr)
	..
	bit_spin_unlock(bitnum,addr)

which should get all of this right, and if we ever chaneg the consistency 
rules, we'll make sure the bitlocks still work.

So please..

		Linus

Re: [parisc-linux] Re: [PATCH] Fix the cpumask rewrite

[Jeff Garzik]

Linus Torvalds wrote:
> It _happens_ to be one on x86, so sadly it works on 99% of the machines 
> out there. And to avoid the extra (suprefluous) mb(), there are
> 
> 	smp_mb__before_clear_bit()
> 	smp_mb__after_clear_bit()
> 
> that only works with "clear_bit()", on the assumption that the way you'd 
> do locking is:
> 
> 	lock:
> 		while (test_and_set_bit(..)) /* This is a memory barrier */
> 			while (test_bit(..))
> 				cpu_relax();
> 
> 	.. protected region ..
> 
> 	unlock:
> 		smp_mb__before_clear_bit();
> 		clear_bit(..);


FWIW one of the major uses of bitops currently is e.g. in 
include/linux/netdevice.h, where bitops are used for atomic selection of 
code paths, but not spinning:

static inline void netif_carrier_off(struct net_device *dev)
{
         if (!test_and_set_bit(__LINK_STATE_NOCARRIER, &dev->state))
                 linkwatch_fire_event(dev);
}

Re: [parisc-linux] Re: [PATCH] Fix the cpumask rewrite

[Linus Torvalds]

On Thu, 1 Jul 2004, Jeff Garzik wrote:
> 
> FWIW one of the major uses of bitops currently is e.g. in 
> include/linux/netdevice.h, where bitops are used for atomic selection of 
> code paths, but not spinning:

That's fine. They _are_ defined to be atomic, and as such they are perfect 
for things like that.

		Linus

Re: [parisc-linux] Re: [PATCH] Fix the cpumask rewrite

[Chris Wedgwood]

On Thu, Jul 01, 2004 at 03:33:08PM +0200, Pavel Machek wrote:

> Plus some get_jiffies api for stuff like printk ratelimit would be
> needed, right?

*Some* things willl need special consideration, but most of the users
of jiffies right now could use a more high-level API.

I'm toying with the idea right now of have per-CPU xtime and deriving
jiffies from that in places too.


  --cw