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
-
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/