[PATCH] volatile may be needed in rwsem

[PATCH] volatile may be needed in rwsem

[Joe Korty]

'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
 };

Re: [PATCH] volatile may be needed in rwsem

[David Howells]

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

Re: [PATCH] volatile may be needed in rwsem

[Joe Korty]

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

Re: [PATCH] volatile may be needed in rwsem

[Christian Borntraeger]

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

Re: [PATCH] volatile may be needed in rwsem

[Paulo Marques]

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 - www.grupopie.com
"In a world without walls and fences who needs windows and gates?"