[patch] xmon bitlock fix

[patch] xmon bitlock fix

[Nick Piggin]

(sorry, untested for lack of hardware)
--
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.

Signed-off-by: Nick Piggin <npiggin@suse.de>
Acked-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
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
@@ -92,16 +92,14 @@ xmon_write(void *handle, void *ptr, int 
 {
 	char *p = ptr;
 	int i, c, ct;
-
-#ifdef CONFIG_SMP
-	static unsigned long xmon_write_lock;
+	static DEFINE_SPINLOCK(xmon_write_lock);
 	int lock_wait = 1000000;
 	int locked;
 
-	while ((locked = test_and_set_bit(0, &xmon_write_lock)) != 0)
+	while (!(locked = spin_trylock(&xmon_write_lock))) {
 		if (--lock_wait == 0)
 			break;
-#endif
+	}
 
 	if (!scc_initialized)
 		xmon_init_scc();
@@ -122,10 +120,9 @@ xmon_write(void *handle, void *ptr, int 
 		eieio();
 	}
 
-#ifdef CONFIG_SMP
-	if (!locked)
-		clear_bit(0, &xmon_write_lock);
-#endif
+	if (locked)
+		spin_unlock(&xmon_write_lock);
+
 	return nb;
 }
 

[patch] powerpc: hash lock use lock bitops

[Nick Piggin]

This isn't a bugfix, but may help performance slightly...

--
powerpc 64-bit hash pte lock bit is an actual lock, so it can take advantage
of lock bitops for slightly more optimal memory barriers (can avoid an lwsync
in the trylock).

Signed-off-by: Nick Piggin <npiggin@suse.de>
Acked-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
Index: linux-2.6/arch/powerpc/mm/hash_native_64.c
===================================================================
--- linux-2.6.orig/arch/powerpc/mm/hash_native_64.c
+++ linux-2.6/arch/powerpc/mm/hash_native_64.c
@@ -113,7 +113,7 @@ static inline void native_lock_hpte(stru
 	unsigned long *word = &hptep->v;
 
 	while (1) {
-		if (!test_and_set_bit(HPTE_LOCK_BIT, word))
+		if (!test_and_set_bit_lock(HPTE_LOCK_BIT, word))
 			break;
 		while(test_bit(HPTE_LOCK_BIT, word))
 			cpu_relax();
@@ -124,8 +124,7 @@ static inline void native_unlock_hpte(st
 {
 	unsigned long *word = &hptep->v;
 
-	asm volatile("lwsync":::"memory");
-	clear_bit(HPTE_LOCK_BIT, word);
+	clear_bit_unlock(HPTE_LOCK_BIT, word);
 }
 
 static long native_hpte_insert(unsigned long hpte_group, unsigned long va,

Re: [patch] xmon bitlock fix

[Paul Mackerras]

Nick Piggin writes:

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

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

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.

Paul.

Re: [patch] xmon bitlock fix

[Nick Piggin]

On Tue, Nov 20, 2007 at 04:28:02PM +1100, Paul Mackerras wrote:
> Nick Piggin writes:
> 
> > 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 <npiggin@suse.de>
---
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;
 }

Re: [patch] powerpc: hash lock use lock bitops

[Benjamin Herrenschmidt]

On Tue, 2007-11-20 at 06:09 +0100, Nick Piggin wrote:
> This isn't a bugfix, but may help performance slightly...
> 
> --
> powerpc 64-bit hash pte lock bit is an actual lock, so it can take advantage
> of lock bitops for slightly more optimal memory barriers (can avoid an lwsync
> in the trylock).
> 
> Signed-off-by: Nick Piggin <npiggin@suse.de>
> Acked-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> ---

Looks nice, I'll try it out on a G5 and let you know.

Cheers,
Ben.

Re: [patch] powerpc: hash lock use lock bitops

[Nick Piggin]

On Tue, Nov 20, 2007 at 05:08:24PM +1100, Benjamin Herrenschmidt wrote:
> 
> On Tue, 2007-11-20 at 06:09 +0100, Nick Piggin wrote:
> > This isn't a bugfix, but may help performance slightly...
> > 
> > --
> > powerpc 64-bit hash pte lock bit is an actual lock, so it can take advantage
> > of lock bitops for slightly more optimal memory barriers (can avoid an lwsync
> > in the trylock).
> > 
> > Signed-off-by: Nick Piggin <npiggin@suse.de>
> > Acked-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > ---
> 
> Looks nice, I'll try it out on a G5 and let you know.

Cool, thanks (I don't have mine handy ATM...).

BTW, here is another thing which you might want to think about. Again
untested for temporary lack of hardware.

--

The radix-tree is now RCU safe, so powerpc can avoid the games it was playing
in order to have a lockless readside. Saves an irq disable/enable, a couple of
__get_cpu_var()s, a cacheline, and a memory barrier, in the fastpath. Should
save a cycle or two...

---
Index: linux-2.6/arch/powerpc/kernel/irq.c
===================================================================
--- linux-2.6.orig/arch/powerpc/kernel/irq.c
+++ linux-2.6/arch/powerpc/kernel/irq.c
@@ -406,8 +406,6 @@ void do_softirq(void)
 
 static LIST_HEAD(irq_hosts);
 static DEFINE_SPINLOCK(irq_big_lock);
-static DEFINE_PER_CPU(unsigned int, irq_radix_reader);
-static unsigned int irq_radix_writer;
 struct irq_map_entry irq_map[NR_IRQS];
 static unsigned int irq_virq_count = NR_IRQS;
 static struct irq_host *irq_default_host;
@@ -550,57 +548,6 @@ void irq_set_virq_count(unsigned int cou
 		irq_virq_count = count;
 }
 
-/* radix tree not lockless safe ! we use a brlock-type mecanism
- * for now, until we can use a lockless radix tree
- */
-static void irq_radix_wrlock(unsigned long *flags)
-{
-	unsigned int cpu, ok;
-
-	spin_lock_irqsave(&irq_big_lock, *flags);
-	irq_radix_writer = 1;
-	smp_mb();
-	do {
-		barrier();
-		ok = 1;
-		for_each_possible_cpu(cpu) {
-			if (per_cpu(irq_radix_reader, cpu)) {
-				ok = 0;
-				break;
-			}
-		}
-		if (!ok)
-			cpu_relax();
-	} while(!ok);
-}
-
-static void irq_radix_wrunlock(unsigned long flags)
-{
-	smp_wmb();
-	irq_radix_writer = 0;
-	spin_unlock_irqrestore(&irq_big_lock, flags);
-}
-
-static void irq_radix_rdlock(unsigned long *flags)
-{
-	local_irq_save(*flags);
-	__get_cpu_var(irq_radix_reader) = 1;
-	smp_mb();
-	if (likely(irq_radix_writer == 0))
-		return;
-	__get_cpu_var(irq_radix_reader) = 0;
-	smp_wmb();
-	spin_lock(&irq_big_lock);
-	__get_cpu_var(irq_radix_reader) = 1;
-	spin_unlock(&irq_big_lock);
-}
-
-static void irq_radix_rdunlock(unsigned long flags)
-{
-	__get_cpu_var(irq_radix_reader) = 0;
-	local_irq_restore(flags);
-}
-
 static int irq_setup_virq(struct irq_host *host, unsigned int virq,
 			    irq_hw_number_t hwirq)
 {
@@ -791,9 +738,9 @@ void irq_dispose_mapping(unsigned int vi
 		/* Check if radix tree allocated yet */
 		if (host->revmap_data.tree.gfp_mask == 0)
 			break;
-		irq_radix_wrlock(&flags);
+		spin_lock_irqsave(&irq_big_lock, flags);
 		radix_tree_delete(&host->revmap_data.tree, hwirq);
-		irq_radix_wrunlock(flags);
+		spin_unlock_irqrestore(&irq_big_lock, flags);
 		break;
 	}
 
@@ -861,9 +808,9 @@ unsigned int irq_radix_revmap(struct irq
 		return irq_find_mapping(host, hwirq);
 
 	/* Now try to resolve */
-	irq_radix_rdlock(&flags);
+	rcu_read_lock();
 	ptr = radix_tree_lookup(tree, hwirq);
-	irq_radix_rdunlock(flags);
+	rcu_read_unlock();
 
 	/* Found it, return */
 	if (ptr) {
@@ -874,9 +821,9 @@ unsigned int irq_radix_revmap(struct irq
 	/* If not there, try to insert it */
 	virq = irq_find_mapping(host, hwirq);
 	if (virq != NO_IRQ) {
-		irq_radix_wrlock(&flags);
+		spin_lock_irqsave(&irq_big_lock, flags);
 		radix_tree_insert(tree, hwirq, &irq_map[virq]);
-		irq_radix_wrunlock(flags);
+		spin_unlock_irqrestore(&irq_big_lock, flags);
 	}
 	return virq;
 }
@@ -989,12 +936,12 @@ static int irq_late_init(void)
 	struct irq_host *h;
 	unsigned long flags;
 
-	irq_radix_wrlock(&flags);
+	spin_lock_irqsave(&irq_big_lock, flags);
 	list_for_each_entry(h, &irq_hosts, link) {
 		if (h->revmap_type == IRQ_HOST_MAP_TREE)
 			INIT_RADIX_TREE(&h->revmap_data.tree, GFP_ATOMIC);
 	}
-	irq_radix_wrunlock(flags);
+	spin_unlock_irqrestore(&irq_big_lock, flags);
 
 	return 0;
 }

Re: [patch] powerpc: hash lock use lock bitops

[Benjamin Herrenschmidt]

On Tue, 2007-11-20 at 07:26 +0100, Nick Piggin wrote:
> 
> BTW, here is another thing which you might want to think about. Again
> untested for temporary lack of hardware.
> 
> --
> 
> The radix-tree is now RCU safe, so powerpc can avoid the games it was playing
> in order to have a lockless readside. Saves an irq disable/enable, a couple of
> __get_cpu_var()s, a cacheline, and a memory barrier, in the fastpath. Should
> save a cycle or two...

Yup, I know. I need to work on that for -rt anyway due to other issues...

Thanks !

Ben.