[PATCH] Celleb: improve htab lock

[PATCH] Celleb: improve htab lock

[Ishizaki Kou]

Disabling IRQ is required only in invalidation.

Signed-off-by: Kou Ishizaki <kou.ishizaki@toshiba.co.jp>
---


Index: linux-powerpc-git/arch/powerpc/platforms/celleb/htab.c
diff -u linux-powerpc-git/arch/powerpc/platforms/celleb/htab.c:1.1.1.1 linux-powerpc-git/arch/powerpc/platforms/celleb/htab.c:1.9
--- linux-powerpc-git/arch/powerpc/platforms/celleb/htab.c:1.1.1.1	Thu Feb  8 11:38:20 2007
+++ linux-powerpc-git/arch/powerpc/platforms/celleb/htab.c	Wed Feb  7 11:49:17 2007
@@ -95,7 +95,6 @@
 	unsigned long lpar_rc;
 	unsigned long slot;
 	unsigned long hpte_v, hpte_r;
-	unsigned long flags;
 
 	/* same as iseries */
 	if (vflags & HPTE_V_SECONDARY)
@@ -115,17 +114,17 @@
 	if (rflags & (_PAGE_GUARDED|_PAGE_NO_CACHE))
 		hpte_r &= ~_PAGE_COHERENT;
 
-	spin_lock_irqsave(&beat_htab_lock, flags);
+	spin_lock(&beat_htab_lock);
 	if ((lpar_rc = beat_read_mask(hpte_group)) == 0) {
 		if (!(vflags & HPTE_V_BOLTED))
 			DBG_LOW(" full\n");
-		spin_unlock_irqrestore(&beat_htab_lock, flags);
+		spin_unlock(&beat_htab_lock);
 		return -1;
 	}
 
 	lpar_rc = beat_insert_htab_entry(0, hpte_group, lpar_rc << 48,
 		hpte_v, hpte_r, &slot);
-	spin_unlock_irqrestore(&beat_htab_lock, flags);
+	spin_unlock(&beat_htab_lock);
 
 	/*
 	 * Since we try and ioremap PHBs we don't own, the pte insert
@@ -189,7 +188,6 @@
 {
 	unsigned long lpar_rc;
 	unsigned long dummy0, dummy1, want_v;
-	unsigned long flags;
 
 	want_v = hpte_encode_v(va, psize);
 
@@ -197,17 +195,17 @@
 		"avpnv=%016lx, slot=%016lx, psize: %d, newpp %016lx ... ",
 		want_v & HPTE_V_AVPN, slot, psize, newpp);
 
-	spin_lock_irqsave(&beat_htab_lock, flags);
+	spin_lock(&beat_htab_lock);
 	dummy0 = beat_lpar_hpte_getword0(slot);
 	if ((dummy0 & ~0x7FUL) != (want_v & ~0x7FUL)) {
 		DBG_LOW("not found !\n");
-		spin_unlock_irqrestore(&beat_htab_lock, flags);
+		spin_unlock(&beat_htab_lock);
 		return -1;
 	}
 
 	lpar_rc = beat_write_htab_entry(0, slot, 0, newpp, 0, 7, &dummy0,
 					&dummy1);
-	spin_unlock_irqrestore(&beat_htab_lock, flags);
+	spin_unlock(&beat_htab_lock);
 	if (lpar_rc != 0 || dummy0 == 0) {
 		DBG_LOW("not found !\n");
 		return -1;
@@ -256,18 +254,17 @@
 					  int psize)
 {
 	unsigned long lpar_rc, slot, vsid, va, dummy0, dummy1;
-	unsigned long flags;
 
 	vsid = get_kernel_vsid(ea);
 	va = (vsid << 28) | (ea & 0x0fffffff);
 
-	spin_lock_irqsave(&beat_htab_lock, flags);
+	spin_lock(&beat_htab_lock);
 	slot = beat_lpar_hpte_find(va, psize);
 	BUG_ON(slot == -1);
 
 	lpar_rc = beat_write_htab_entry(0, slot, 0, newpp, 0, 7,
 		&dummy0, &dummy1);
-	spin_unlock_irqrestore(&beat_htab_lock, flags);
+	spin_unlock(&beat_htab_lock);
 
 	BUG_ON(lpar_rc != 0);
 }
@@ -284,18 +281,21 @@
 		slot, va, psize, local);
 	want_v = hpte_encode_v(va, psize);
 
-	spin_lock_irqsave(&beat_htab_lock, flags);
+	local_irq_save(flags);
+	spin_lock(&beat_htab_lock);
 	dummy1 = beat_lpar_hpte_getword0(slot);
 
 	if ((dummy1 & ~0x7FUL) != (want_v & ~0x7FUL)) {
 		DBG_LOW("not found !\n");
-		spin_unlock_irqrestore(&beat_htab_lock, flags);
+		spin_unlock(&beat_htab_lock);
+		local_irq_restore(flags);
 		return;
 	}
 
 	lpar_rc = beat_write_htab_entry(0, slot, 0, 0, HPTE_V_VALID, 0,
 		&dummy1, &dummy2);
-	spin_unlock_irqrestore(&beat_htab_lock, flags);
+	spin_unlock(&beat_htab_lock);
+	local_irq_restore(flags);
 
 	BUG_ON(lpar_rc != 0);
 }

Re: [PATCH] Celleb: improve htab lock

[Arnd Bergmann]

T24gVGh1cnNkYXkgMDggRmVicnVhcnkgMjAwNyAwNzoxOCwgSXNoaXpha2kgS291IHdyb3RlOgo+
IHMpOwo+ICugoKCgoKCgbG9jYWxfaXJxX3NhdmUoZmxhZ3MpOwo+ICugoKCgoKCgc3Bpbl9sb2Nr
KCZiZWF0X2h0YWJfbG9jayk7Cj4goKCgoKCgoKBkdW1teTEgPSBiZWF0X2xwYXJfaHB0ZV9nZXR3
b3JkMChzbG90KTsKPiCgCj4goKCgoKCgoKBpZiAoKGR1bW15MSAmIH4weDdGVUwpICE9ICh3YW50
X3YgJiB+MHg3RlVMKSkgewo+IKCgoKCgoKCgoKCgoKCgoKBEQkdfTE9XKCJub3QgZm91bmQgIVxu
Iik7Cj4gLaCgoKCgoKCgoKCgoKCgoHNwaW5fdW5sb2NrX2lycXJlc3RvcmUoJmJlYXRfaHRhYl9s
b2NrLCBmbGFncyk7Cj4gK6CgoKCgoKCgoKCgoKCgoHNwaW5fdW5sb2NrKCZiZWF0X2h0YWJfbG9j
ayk7Cj4gK6CgoKCgoKCgoKCgoKCgoGxvY2FsX2lycV9yZXN0b3JlKGZsYWdzKTsKPiCgoKCgoKCg
oKCgoKCgoKCgcmV0dXJuOwo+IKCgoKCgoKCgfQo+IKAKPiCgoKCgoKCgoGxwYXJfcmMgPSBiZWF0
X3dyaXRlX2h0YWJfZW50cnkoMCwgc2xvdCwgMCwgMCwgSFBURV9WX1ZBTElELCAwLAo+IKCgoKCg
oKCgoKCgoKCgoKAmZHVtbXkxLCAmZHVtbXkyKTsKPiAtoKCgoKCgoHNwaW5fdW5sb2NrX2lycXJl
c3RvcmUoJmJlYXRfaHRhYl9sb2NrLCBmbGFncyk7Cj4gK6CgoKCgoKBzcGluX3VubG9jaygmYmVh
dF9odGFiX2xvY2spOwo+ICugoKCgoKCgbG9jYWxfaXJxX3Jlc3RvcmUoZmxhZ3MpOwoKVGhlIGZ1
bmN0aW9uIG5vcm1hbGx5IHVzZWQgaGVyZSB3b3VsZCBiZSBzcGluX2xvY2tfaXJxKCkvc3Bpbl91
bmxvY2tfaXJxKCksCndoaWNoIGRvZXMgc3Bpbl97LHVufWxvY2sgYWxvbmcgd2l0aCBsb2NhbF9p
cnFfe3NhdmUscmVzdG9yZX0uCgoJQXJuZCA8PjwK

Re: [PATCH] Celleb: improve htab lock

[Christoph Hellwig]

On Thu, Feb 08, 2007 at 07:51:44AM +0100, Arnd Bergmann wrote:
> On Thursday 08 February 2007 07:18, Ishizaki Kou wrote:
> > s);
> > +???????local_irq_save(flags);
> > +???????spin_lock(&beat_htab_lock);
> > ????????dummy1 = beat_lpar_hpte_getword0(slot);
> > ?
> > ????????if ((dummy1 & ~0x7FUL) != (want_v & ~0x7FUL)) {
> > ????????????????DBG_LOW("not found !\n");
> > -???????????????spin_unlock_irqrestore(&beat_htab_lock, flags);
> > +???????????????spin_unlock(&beat_htab_lock);
> > +???????????????local_irq_restore(flags);
> > ????????????????return;
> > ????????}
> > ?
> > ????????lpar_rc = beat_write_htab_entry(0, slot, 0, 0, HPTE_V_VALID, 0,
> > ????????????????&dummy1, &dummy2);
> > -???????spin_unlock_irqrestore(&beat_htab_lock, flags);
> > +???????spin_unlock(&beat_htab_lock);
> > +???????local_irq_restore(flags);
> 
> The function normally used here would be spin_lock_irq()/spin_unlock_irq(),
> which does spin_{,un}lock along with local_irq_{save,restore}.

But I think the issue here is that we don't want to disable irqs because
we want to protect against IRQ context, but rather because the
hypervisor expects it.  Using the separate routines makes this a little
cleaner.  Then again a comment explaining why we do it in addition to
the separate routines would be even better.  And if this is really the
case doing the disabling inside the spinlock might be more explanative
and reduces the irq disabling time.  Also if we're sure that this is
always called from process context and with irqs enabled we should
be using local_irq_disable/local_irq_enable and avoid saving the
flags word.

Re: [PATCH] Celleb: improve htab lock

[Arnd Bergmann]

On Thursday 08 February 2007 21:28, Christoph Hellwig wrote:
> =A0Also if we're sure that this is
> always called from process context and with irqs enabled we should
> be using local_irq_disable/local_irq_enable and avoid saving the
> flags word.

Right, that's what I meant, I was a little confused here.

	Arnd <><

Re: [PATCH] Celleb: improve htab lock

[Benjamin Herrenschmidt]

On Thu, 2007-02-08 at 21:43 +0100, Arnd Bergmann wrote:
> On Thursday 08 February 2007 21:28, Christoph Hellwig wrote:
> >  Also if we're sure that this is
> > always called from process context and with irqs enabled we should
> > be using local_irq_disable/local_irq_enable and avoid saving the
> > flags word.
> 
> Right, that's what I meant, I was a little confused here.

I prefer saving for now. We can be called whenever there is a
flush_tlb_pending() called for flushing a pending batch and that is in
quite a few places including deep in the context switching code, I
wouldn't make assumption on the irq state on entry.

I do intend at one point to move the local_irq_save/restore up a level
though, so that all the htab functions have the same semantic of being
called with IRQs off. Right now, the invalidate ones are sort-of an
exception.

Ben.

Re: [PATCH] Celleb: improve htab lock

[Akira Iguchi]

Ben-san,

> On Thu, 2007-02-08 at 21:43 +0100, Arnd Bergmann wrote:
> > On Thursday 08 February 2007 21:28, Christoph Hellwig wrote:
> > >  Also if we're sure that this is
> > > always called from process context and with irqs enabled we should
> > > be using local_irq_disable/local_irq_enable and avoid saving the
> > > flags word.
> > 
> > Right, that's what I meant, I was a little confused here.

> I prefer saving for now. We can be called whenever there is a
> flush_tlb_pending() called for flushing a pending batch and that is in
> quite a few places including deep in the context switching code, I
> wouldn't make assumption on the irq state on entry.

We prefer saving flags, too.

> I do intend at one point to move the local_irq_save/restore up a level
> though, so that all the htab functions have the same semantic of being
> called with IRQs off. Right now, the invalidate ones are sort-of an
> exception.

We need to turn IRQs off when calling htab functions if and only
if it is non-atomic, so the "pSeries with LPAR" htab function have
no spin_lock or local_irq_save/restore. We think tweaking EE bit
locally gives a way to decrease times to be IRQs off and on again
(yes, it's smallthing to compare with calling HV),
so we prefer current implementation.

We have a small patch to replace "local_irq_save" and "spin_lock"
pair to "spin_lock_irqsave". It is not necessary to apply, but if
it's possible, please apply.

Cheers,
Akira Iguchi

Re: [PATCH] Celleb: improve htab lock

[Benjamin Herrenschmidt]

> But I think the issue here is that we don't want to disable irqs because
> we want to protect against IRQ context, but rather because the
> hypervisor expects it.

No not really. The problem here is that we get a spinlock re-entrancy if
we don't disable IRQs so it's a typical textbook usage of
spin_lock_irqsave.

The problem is that if you don't disable IRQs, you can take an irq or a
decrementer exception causing some code to hit vmalloc or ioremap space
which might cause a hash fault, thus you may deadlock on the spinlock
via htab_insert.

Ben.

Re: [PATCH] Celleb: improve htab lock

[Benjamin Herrenschmidt]

On Thu, 2007-02-08 at 07:51 +0100, Arnd Bergmann wrote:
> On Thursday 08 February 2007 07:18, Ishizaki Kou wrote:
> > s);
> > +       local_irq_save(flags);
> > +       spin_lock(&beat_htab_lock);
> >         dummy1 = beat_lpar_hpte_getword0(slot);
> >  
> >         if ((dummy1 & ~0x7FUL) != (want_v & ~0x7FUL)) {
> >                 DBG_LOW("not found !\n");
> > -               spin_unlock_irqrestore(&beat_htab_lock, flags);
> > +               spin_unlock(&beat_htab_lock);
> > +               local_irq_restore(flags);
> >                 return;
> >         }
> >  
> >         lpar_rc = beat_write_htab_entry(0, slot, 0, 0, HPTE_V_VALID, 0,
> >                 &dummy1, &dummy2);
> > -       spin_unlock_irqrestore(&beat_htab_lock, flags);
> > +       spin_unlock(&beat_htab_lock);
> > +       local_irq_restore(flags);
> 
> The function normally used here would be spin_lock_irq()/spin_unlock_irq(),
> which does spin_{,un}lock along with local_irq_{save,restore}.

Not lock_irq/unlock_irq, that doesn't save/restore but disables/enables
(that is doesn't save the previous state).

I'd rather have him use _irqsave/irqrestore, that is, keep the current
code in that function.

Ben.

Re: [PATCH] Celleb: improve htab lock

[Benjamin Herrenschmidt]

> We need to turn IRQs off when calling htab functions if and only
> if it is non-atomic, so the "pSeries with LPAR" htab function have
> no spin_lock or local_irq_save/restore. We think tweaking EE bit
> locally gives a way to decrease times to be IRQs off and on again
> (yes, it's smallthing to compare with calling HV),
> so we prefer current implementation.

Well, it might be indeed worth keeping it that way to avoid long
latencies with EE off with a big batch... I'll keep it that way for now,
though I suppose I should add some documentation on the expected caller
context in machdep.h next to the function pointers.

Ben.

[PATCH] Celleb: improve htab lock

[Akira Iguchi]

Disabling IRQ is required only in invalidation. Replace
"spin_lock_irqsave" to "spin_lock" in other ops.

Signed-off-by: Kou Ishizaki <kou.ishizaki@toshiba.co.jp>
Signed-off-by: Akira Iguchi <akira2.iguchi@toshiba.co.jp>
---

Index: linux-powerpc-git/arch/powerpc/platforms/celleb/htab.c
diff -u linux-powerpc-git/arch/powerpc/platforms/celleb/htab.c:1.1.1.1 linux-powerpc-git/arch/powerpc/platforms/celleb/htab.c:1.10
--- linux-powerpc-git/arch/powerpc/platforms/celleb/htab.c:1.1.1.1	Thu Feb  8 11:38:20 2007
+++ linux-powerpc-git/arch/powerpc/platforms/celleb/htab.c	Fri Feb  9 13:44:18 2007
@@ -95,7 +95,6 @@
 	unsigned long lpar_rc;
 	unsigned long slot;
 	unsigned long hpte_v, hpte_r;
-	unsigned long flags;
 
 	/* same as iseries */
 	if (vflags & HPTE_V_SECONDARY)
@@ -115,17 +114,17 @@
 	if (rflags & (_PAGE_GUARDED|_PAGE_NO_CACHE))
 		hpte_r &= ~_PAGE_COHERENT;
 
-	spin_lock_irqsave(&beat_htab_lock, flags);
+	spin_lock(&beat_htab_lock);
 	if ((lpar_rc = beat_read_mask(hpte_group)) == 0) {
 		if (!(vflags & HPTE_V_BOLTED))
 			DBG_LOW(" full\n");
-		spin_unlock_irqrestore(&beat_htab_lock, flags);
+		spin_unlock(&beat_htab_lock);
 		return -1;
 	}
 
 	lpar_rc = beat_insert_htab_entry(0, hpte_group, lpar_rc << 48,
 		hpte_v, hpte_r, &slot);
-	spin_unlock_irqrestore(&beat_htab_lock, flags);
+	spin_unlock(&beat_htab_lock);
 
 	/*
 	 * Since we try and ioremap PHBs we don't own, the pte insert
@@ -189,7 +188,6 @@
 {
 	unsigned long lpar_rc;
 	unsigned long dummy0, dummy1, want_v;
-	unsigned long flags;
 
 	want_v = hpte_encode_v(va, psize);
 
@@ -197,17 +195,17 @@
 		"avpnv=%016lx, slot=%016lx, psize: %d, newpp %016lx ... ",
 		want_v & HPTE_V_AVPN, slot, psize, newpp);
 
-	spin_lock_irqsave(&beat_htab_lock, flags);
+	spin_lock(&beat_htab_lock);
 	dummy0 = beat_lpar_hpte_getword0(slot);
 	if ((dummy0 & ~0x7FUL) != (want_v & ~0x7FUL)) {
 		DBG_LOW("not found !\n");
-		spin_unlock_irqrestore(&beat_htab_lock, flags);
+		spin_unlock(&beat_htab_lock);
 		return -1;
 	}
 
 	lpar_rc = beat_write_htab_entry(0, slot, 0, newpp, 0, 7, &dummy0,
 					&dummy1);
-	spin_unlock_irqrestore(&beat_htab_lock, flags);
+	spin_unlock(&beat_htab_lock);
 	if (lpar_rc != 0 || dummy0 == 0) {
 		DBG_LOW("not found !\n");
 		return -1;
@@ -256,18 +254,17 @@
 					  int psize)
 {
 	unsigned long lpar_rc, slot, vsid, va, dummy0, dummy1;
-	unsigned long flags;
 
 	vsid = get_kernel_vsid(ea);
 	va = (vsid << 28) | (ea & 0x0fffffff);
 
-	spin_lock_irqsave(&beat_htab_lock, flags);
+	spin_lock(&beat_htab_lock);
 	slot = beat_lpar_hpte_find(va, psize);
 	BUG_ON(slot == -1);
 
 	lpar_rc = beat_write_htab_entry(0, slot, 0, newpp, 0, 7,
 		&dummy0, &dummy1);
-	spin_unlock_irqrestore(&beat_htab_lock, flags);
+	spin_unlock(&beat_htab_lock);
 
 	BUG_ON(lpar_rc != 0);
 }

Re: [PATCH] Celleb: improve htab lock

[Arnd Bergmann]

On Friday 09 February 2007 08:53, Akira Iguchi wrote:
> Disabling IRQ is required only in invalidation. Replace
> "spin_lock_irqsave" to "spin_lock" in other ops.
> 
> Signed-off-by: Kou Ishizaki <kou.ishizaki@toshiba.co.jp>
> Signed-off-by: Akira Iguchi <akira2.iguchi@toshiba.co.jp>

Acked-by: Arnd Bergmann <arnd.bergmann@de.ibm.com>