Re: [PATCH] volatile may be needed in rwsem

[Paulo Marques <pmarques@grupopie.com>]

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