Date: Tue, 27 Jan 2004 14:43:43 -0500
From: Joe Korty <joe.korty@ccur.com>
To: David Howells <dhowells@redhat.com>
Cc: akpm@osdl.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] volatile may be needed in rwsem
Message-ID: <20040127194343.GA12763@tsunami.ccur.com>
Reply-To: joe.korty@ccur.com
References: <20040127191155.GA12128@tsunami.ccur.com> <23376.1075231180@redhat.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <23376.1075231180@redhat.com>
User-Agent: Mutt/1.4.1i
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1260
Lines: 38

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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/