volatile may be needed in rwsem

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

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

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?"

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