Re: [patch] xmon bitlock fix

On Tue, Nov 20, 2007 at 04:28:02PM +1100, Paul Mackerras wrote:

Nick Piggin writes:

quoted

xmon uses a bit lock spinlock but doesn't close the critical section
when releasing it. It doesn't seem like a big deal because it will
eventually break out of the lock anyway, but presumably that's only
in exceptional cases where some error is tolerated, while the lack of a memory
barrier could allow incorrect results during normal functioning operation
as well.

Convert it to use a regular spinlock instead.

I'd rather not.  The idea is that xmon is as independent as possible
of the rest of the kernel, so that it will work even when lots of
kernel data structures are corrupted.  If spinlocks were simple spin
loops then maybe, but they're not these days.

Fair enough, I guess you still want lockdep kernels to work.

 

As for the memory barrier comment, I don't think it is a reason, since
test_and_set_bit acts as a barrier, and in any case the worst thing

test_and_set_bit is a barrier (although heavier than required to acquire
a lock). But clear_bit isn't strong enough... you do have eieio() in
there, making it probably less problem, but that won't get executed if
nb is 0, and it is still doing the xmon_init_scc() outside the loop. I
just prefer to make it a simple lock that doesn't rely on barriers or
the specific structure of the critical section.

that could happen is that the characters from different cpu's outputs
get interleaved (that's one reason why the loop has a timeout, the
other is for robustness).

In any case this is in arch/ppc which is dying code.  I don't think
there are any SMP platforms supported in arch/ppc any more except for
some (fairly rare) PReP platforms, and I hope to get PReP moved over
soon.

 
I was just grepping the tree, and trying to reduce code which might
be fragile, broken, or a poor example.

Finally, why do you say that it doesn't close the critical section?
Possibly the "locked" variable is badly named (it's zero when we get
the lock) but AFAICS the code is actually correct.

Well the unlock itself doesn't close it properly because clear_bit
doesn't order previous loads or stores.

How about this?
---

xmon uses a bit lock spinlock but doesn't close the critical section
when releasing it. It doesn't seem like a big deal because it will
eventually break out of the lock anyway, but presumably that's only
in exceptional cases where some error is tolerated, while the lack of a memory
barrier could allow incorrect results during normal functioning operation
as well.

Signed-off-by: Nick Piggin <redacted>
---
Index: linux-2.6/arch/ppc/xmon/start.c
===================================================================

--- linux-2.6.orig/arch/ppc/xmon/start.c
+++ linux-2.6/arch/ppc/xmon/start.c

@@ -98,7 +98,7 @@ xmon_write(void *handle, void *ptr, int 
 	int lock_wait = 1000000;
 	int locked;
 
-	while ((locked = test_and_set_bit(0, &xmon_write_lock)) != 0)
+	while ((locked = test_and_set_bit_lock(0, &xmon_write_lock)) != 0)
 		if (--lock_wait == 0)
 			break;
 #endif

@@ -124,7 +124,7 @@ xmon_write(void *handle, void *ptr, int 
 
 #ifdef CONFIG_SMP
 	if (!locked)
-		clear_bit(0, &xmon_write_lock);
+		clear_bit_unlock(0, &xmon_write_lock);
 #endif
 	return nb;
 }