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

[Grant Grundler]

On Thu, Aug 31, 2006 at 06:31:35AM -0600, Matthew Wilcox wrote:
> 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.

trylock() variants are expected to fail some of the time.
But I agree readers should never fail because of another reader.
I guess we have to implement some number of retries (less than 5?).

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

Drop the "if (rw->counter < 0)" test and we won't have a deadlock.
But your next idea on fixing that sounds good to me for other reasons.

> 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);
...

I'm thinking we want to block interrupts here anyway to make sure
the writer gets done and releases the spinlock.

I'm also wondering if the writer code paths need to include "mb()"
to prevent the compiler and/or other back-end optimizers from
re-organizing the instruction stream and "leaking" other code
before "counter = -1" is set. James Bottomley already
fixed our regular spinlocks for this problem once before.

> A similar change has to be made to __raw_write_trylock too:

*nod*

> Better ideas, anyone?
> (And perhaps more importantly, anyone spot any holes in my reasoning?

Nope and nope.

> I'm not too happy disabling interrupts *after* disabling preemption.
> And of course, we disable interrupts twice when called with
> write_lock_irq.)

yup.

thanks,
grant