'flags' should be declared volatile as rwsem_down_failed_common() spins
waiting for this to change. Untested.
Against 2.6.1.
diff -Nua 2.6/lib/rwsem-spinlock.c.0 2.6/lib/rwsem-spinlock.c
--- 2.6/lib/rwsem-spinlock.c.0 2004-01-27 14:03:46.000000000 -0500
+++ 2.6/lib/rwsem-spinlock.c 2004-01-27 14:03:38.000000000 -0500
@@ -12,7 +12,7 @@
struct rwsem_waiter {
struct list_head list;
struct task_struct *task;
- unsigned int flags;
+ volatile unsigned int flags;
#define RWSEM_WAITING_FOR_READ 0x00000001
#define RWSEM_WAITING_FOR_WRITE 0x00000002
};
diff -Nua 2.6/lib/rwsem.c.0 2.6/lib/rwsem.c
--- 2.6/lib/rwsem.c.0 2004-01-27 14:03:46.000000000 -0500
+++ 2.6/lib/rwsem.c 2004-01-27 14:03:19.000000000 -0500
@@ -10,7 +10,7 @@
struct rwsem_waiter {
struct list_head list;
struct task_struct *task;
- unsigned int flags;
+ volatile unsigned int flags;
#define RWSEM_WAITING_FOR_READ 0x00000001
#define RWSEM_WAITING_FOR_WRITE 0x00000002
};
> 'flags' should be declared volatile as rwsem_down_failed_common() spins
> waiting for this to change. Untested.
Is it though? Does this fix an error?
The thing is, we make a function call inside of the loop:
/* wait to be given the lock */
for (;;) {
if (!waiter->flags)
break;
schedule();
set_task_state(tsk, TASK_UNINTERRUPTIBLE);
}
Which might preclude that need. I'm not entirely sure, though... it's one of
those compiler black magic things.
I suppose it can't hurt...
David
On Tue, Jan 27, 2004 at 07:19:40PM +0000, David Howells wrote:
>> 'flags' should be declared volatile as rwsem_down_failed_common() spins
>> waiting for this to change. Untested.
>
> Is it though? Does this fix an error?
>
> The thing is, we make a function call inside of the loop:
>
> /* wait to be given the lock */
> for (;;) {
> if (!waiter->flags)
> break;
> schedule();
> set_task_state(tsk, TASK_UNINTERRUPTIBLE);
> }
>
> Which might preclude that need. I'm not entirely sure, though... it's one of
> those compiler black magic things.
>
> I suppose it can't hurt...
>
> David
Hi David,
I misspoke. The potentially failing spin is in __down_write and
__down_read in lib/rwsem-spinlock.c, not in rwsem_down_failed_common.
The problem is is that 'flags' is on the callee's stack and is thus
subject to be optimized out of the loop if the compiler is smart enough
to discover that it is on the stack. Apparently gcc is not yet smart
enough but that doesn't mean it won't be so soon.
Joe
Joe Korty wrote:
> 'flags' should be declared volatile as rwsem_down_failed_common() spins
> waiting for this to change. Untested.
You should use barrier() to prevent the compiler from optimizing reads away,
not volatile.
Here the compiler hopefully considers schedule() as a memory barrier. So
everything should be fine.
cheers
Christian
Joe Korty wrote:
> On Tue, Jan 27, 2004 at 07:19:40PM +0000, David Howells wrote:
>
>>>'flags' should be declared volatile as rwsem_down_failed_common() spins
>>>waiting for this to change. Untested.
>>>
>>Is it though? Does this fix an error?
>>
>>The thing is, we make a function call inside of the loop:
>>
>> /* wait to be given the lock */
>> for (;;) {
>> if (!waiter->flags)
>> break;
>> schedule();
>> set_task_state(tsk, TASK_UNINTERRUPTIBLE);
>> }
>>
>>Which might preclude that need. I'm not entirely sure, though... it's one of
>>those compiler black magic things.
>>
>>I suppose it can't hurt...
>>
>>David
>>
>
> Hi David,
> I misspoke. The potentially failing spin is in __down_write and
> __down_read in lib/rwsem-spinlock.c, not in rwsem_down_failed_common.
>
> The problem is is that 'flags' is on the callee's stack and is thus
> subject to be optimized out of the loop if the compiler is smart enough
> to discover that it is on the stack. Apparently gcc is not yet smart
> enough but that doesn't mean it won't be so soon.
>
It seems to me that the compiler did the right thing and was smart enough,
because after the function did:
list_add_tail(&waiter.list,&sem->wait_list);
it "published" the address of the structure, so the compiler can no longer
assume that no outside function will have access to it.
So even if the compiler was extremely smart, it would have to do the same thing.
If you told no one where your structure is, how could it be modified outside
your function, and how could you expect "waiter.flags" to be modified while
inside the loop anyway (even if it was volatile)?
IMHO the code is correct.
--
Paulo Marques - http://www.grupopie.com
"In a world without walls and fences who needs windows and gates?"