Re: [parisc-linux] some more questions about __raw_write_trylock() hppa implementation

[Matthew Wilcox <matthew@wil.cx>]

On Thu, Aug 31, 2006 at 12:06:37AM -0600, Grant Grundler wrote:
> Here's the patch I ended up with.
> Diff is also parked on gsyprf11:~grundler/diff-2.6.18-rc4-pa4-rwlocks-01
> 
> +static  __inline__ int __raw_read_trylock(raw_rwlock_t *rw)
> +{
> +	if (!__raw_spin_trylock(&rw->lock))
> +		return 0;
>  
> +	rw->counter++;
>  	__raw_spin_unlock(&rw->lock);
> +	return 1;
>  }

This has the false failure problem.  ie if another CPU is doing a
read_lock() at the same time, it will fail to acquire the lock, even
though it would succeed if it tried again.  I don't know if we have any
code which depends upon that not failing, but it does seem a bit
suboptimal.

Fixing this is somewhat Hard.  My original suggestion was this:

retry:
	if (__raw_spin_trylock(&rw->lock)) {
		rw->counter++;
		__raw_spin_unlock(&rw->lock);
		return 1;
	} else if (rw->counter < 0) {
		return 0;
	} else {
		goto retry;
	}

But this leads to deadlock if called from interrupt context and the CPU
had got to:

static  __inline__ void __raw_write_lock(raw_rwlock_t *rw)
{
retry:
        __raw_spin_lock(&rw->lock);

<<<- here

and hadn't set counter to -1.  Now, we can fix that:

static  __inline__ void __raw_write_lock(raw_rwlock_t *rw)
{
	unsigned long flags;
retry:
	local_irq_save(flags);
        __raw_spin_lock(&rw->lock);

        if (rw->counter != 0) {
                /* this basically never happens */
                __raw_spin_unlock(&rw->lock);
		local_irq_restore(flags);

                while (rw->counter != 0)
                        cpu_relax();

                goto retry;
        }

        /* got it.  now leave without unlocking */
        rw->counter = -1; /* remember we are locked */
	local_irq_restore(flags);
}

A similar change has to be made to __raw_write_trylock too:

static  __inline__ int __raw_write_trylock(raw_rwlock_t *rw)
{
	unsigned long flags;
	int result = 0;
	local_irq_save(flags);
	if (__raw_spin_trylock(&rw->lock)) {
		if (rw->counter == 0) {
			rw->counter = -1;
			result = 1;
		} else {
			__raw_spin_unlock(&rw->lock);
		}
	}
	local_irq_restore(flags);
	return result;
}

Better ideas, anyone?

(And perhaps more importantly, anyone spot any holes in my reasoning?
I'm not too happy disabling interrupts *after* disabling preemption.
And of course, we disable interrupts twice when called with
write_lock_irq.)
_______________________________________________
parisc-linux mailing list
parisc-linux@lists.parisc-linux.org
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux