some more questions about __raw_write_trylock() hppa implementation

Hello all,

I noticed that:
static __inline__ int __raw_write_trylock(raw_rwlock_t *rw)

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

                return 0;
        }

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

so depending the counter is null or not the lock is kept or relaxed?

following my smp pb invextigation the pb I encounter seems to occure rarely and so according to above comment "/* this basically never happens */" I place here a panic() in the hope to get a better trace if it start to hang by this place. Unfortunately the comment is wrong and system panicing at boot time ;-(

Looking to some other implementation (powerpc and sh in particular), I don't have the feeling that this fnct should have to keep the lock in any case. Any idea?

Actually, there is a problem with the write locks that I noticed during replying to this mail the first time. Then palinux crashed. How ironic.
Grant and I are fixing that right now ...

Better ideas, anyone?

better? not sure, but one thing I am looking at using is tri-state locks, defined as:

0 == Busy, change in process
&lock == Locked
other == Unlocked

Since that can be done with load&clear and the information already on hand in the registers.

One thing we could do is limit the number of times we retry.

Hmm, I just thought. If we interrupted a writer trying to acquire the lock, the writer was about to successfully acquire the lock. But we interrupted them, so they can't be running. And nobody else can acquire the lock because the attempting writer holds the spinlock. So the reader in interrupt context can be deemed to have successfully acquired the lock. Now ... how do we handle releasing that lock, given that it can't acquire the spinlock ...

Quoted 8 lines · Grant Grundler

Here's the patch I ended up with.
Diff is also parked on gsyprf11:~grundler/diff-2.6.18-rc4-pa4-rwlocks-01

include/asm-parisc/spinlock.h +4 -0

+static  __inline__ int __raw_read_trylock(raw_rwlock_t *rw)
+{
+	if (!__raw_spin_trylock(&rw->lock))
+		return 0;

include/asm-parisc/spinlock.h #define __raw_read_trylock(lock) generic · line 89

Quoted 4 lines

include/asm-parisc/spinlock.h +2 -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);

A similar change has to be made to __raw_write_trylock too:

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

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

Quoted 8 lines

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

Quoted 3 lines

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

Quoted 8 lines

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.

Quoted 3 lines · Grant Grundler

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

I don't think we need retries; we're guaranteed to make forward progress. If we fail to acquire the lock, it's because it's either held for a short duration by a reader, or for a long duration by a writer. If it's a writer, we'll fail due to the counter being negative; if it's a reader, we'll succeed soon. Mmm. Unless, of course, we interrupted a read-locker ... crap. They need to take the lock in an irqsafe way too.

Quoted 2 lines

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.

I don't understand why you think that. Can you explain?

Quoted 2 lines

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

Umm. Sounds like a spectacularly bad idea. If the caller wanted to do that, they would have called write_lock_irqsave() or write_lock_irq().

With the out-of-line spinlocks (and for that matter, write locks), that's not going to matter. The only place that calls __raw_write_lock() is in kernel/spinlock.c, so there's no way for gcc to optimise that away. I can put it in anyway, since it's not going to make a difference.

Quoted 4 lines

Quoted 2 lines

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.

I don't understand why you think that. Can you explain?

Without blocking interrupts, that test is reading a value that's not deterministic. ie we don't when if/when we are interrupting a writer. Failing the read lock is safe even if it's not correct.

Quoted 5 lines

Quoted 2 lines

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

Umm. Sounds like a spectacularly bad idea. If the caller wanted to do that, they would have called write_lock_irqsave() or write_lock_irq().

Well, ok - you're right about caller intentions. But the caller also has no clue about parisc rw_locks and how fsck'd the implementation is. I'm just "speculating out loud" in order to make parisc implementation work better in practice.

Quoted 5 lines

With the out-of-line spinlocks (and for that matter, write locks), that's not going to matter. The only place that calls __raw_write_lock() is in kernel/spinlock.c, so there's no way for gcc to optimise that away. I can put it in anyway, since it's not going to make a difference.

I'm not sure that a good reason to put the mb() in. Would "it's correct" be a better reason?
I'm thinking other people will look at the code when trying to understand parisc.