[patch] s390: do not use _local_bh_enable()

[patch] s390: do not use _local_bh_enable()

[Ingo Molnar]

Heiko, what do you think about the patch below - is there perhaps some 
deeper reason to s390's _local_bh_enable() use that i missed?

	Ingo

-------------------------->
Subject: [patch] s390: do not use _local_bh_enable()
From: Ingo Molnar <mingo@elte.hu>

_local_bh_enable() enables bhs without invoke_softirqs(), and can thus 
cause missed softirq processing. The s390 code seems to have two such 
uses of _local_bh_enable().

in cio.c the bh disable/enable pair is superfluous, because 
irq_enter()/exit() disables softirqs anyway.

Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 drivers/s390/char/sclp.c |    2 +-
 drivers/s390/cio/cio.c   |    3 +--
 2 files changed, 2 insertions(+), 3 deletions(-)

Index: linux/drivers/s390/char/sclp.c
===================================================================
--- linux.orig/drivers/s390/char/sclp.c
+++ linux/drivers/s390/char/sclp.c
@@ -407,8 +407,8 @@ sclp_sync_wait(void)
 	}
 	local_irq_disable();
 	__ctl_load(cr0, 0, 0);
-	_local_bh_enable();
 	local_irq_restore(flags);
+	local_bh_enable();
 }
 
 EXPORT_SYMBOL(sclp_sync_wait);
Index: linux/drivers/s390/cio/cio.c
===================================================================
--- linux.orig/drivers/s390/cio/cio.c
+++ linux/drivers/s390/cio/cio.c
@@ -140,7 +140,6 @@ cio_tpi(void)
 	sch = (struct subchannel *)(unsigned long)tpi_info->intparm;
 	if (!sch)
 		return 1;
-	local_bh_disable();
 	irq_enter ();
 	spin_lock(sch->lock);
 	memcpy (&sch->schib.scsw, &irb->scsw, sizeof (struct scsw));
@@ -148,7 +147,7 @@ cio_tpi(void)
 		sch->driver->irq(&sch->dev);
 	spin_unlock(sch->lock);
 	irq_exit ();
-	_local_bh_enable();
+
 	return 1;
 }
 

Re: [patch] s390: do not use _local_bh_enable()

[Heiko Carstens]

On Fri, Feb 23, 2007 at 07:14:59AM +0100, Ingo Molnar wrote:
> 
> Heiko, what do you think about the patch below - is there perhaps some 
> deeper reason to s390's _local_bh_enable() use that i missed?

Yes, both of these usages are quite subtle.

> Index: linux/drivers/s390/char/sclp.c
> ===================================================================
> --- linux.orig/drivers/s390/char/sclp.c
> +++ linux/drivers/s390/char/sclp.c
> @@ -407,8 +407,8 @@ sclp_sync_wait(void)
>  	}
>  	local_irq_disable();
>  	__ctl_load(cr0, 0, 0);
> -	_local_bh_enable();
>  	local_irq_restore(flags);
> +	local_bh_enable();
>  }
> 
>  EXPORT_SYMBOL(sclp_sync_wait);

The code here looks a bit different in the meantime.
See c59d744bd8a0e283daf6726881e4c9aa4bd25261. What we want to do here: the
console driver has requests pending and needs to wait _synchronously_ that
an interrupt arrives. It does that regardless of whatever context we are in:
process/softirq/hardirq/irqs disabled/irqs enabled. That's why we have the
local_irq_save() at the very beginning. Also when we leave this function we
do not want to execute bottom halves, since it could be that interrupts
where disabled before this function was called.
That's why we call _local_bh_enable() instead of local_bh_enable().

> Index: linux/drivers/s390/cio/cio.c
> ===================================================================
> --- linux.orig/drivers/s390/cio/cio.c
> +++ linux/drivers/s390/cio/cio.c
> @@ -140,7 +140,6 @@ cio_tpi(void)
>  	sch = (struct subchannel *)(unsigned long)tpi_info->intparm;
>  	if (!sch)
>  		return 1;
> -	local_bh_disable();
>  	irq_enter ();
>  	spin_lock(sch->lock);
>  	memcpy (&sch->schib.scsw, &irb->scsw, sizeof (struct scsw));
> @@ -148,7 +147,7 @@ cio_tpi(void)
>  		sch->driver->irq(&sch->dev);
>  	spin_unlock(sch->lock);
>  	irq_exit ();
> -	_local_bh_enable();
> +
>  	return 1;
>  }

Same here: this is not really an irq handler but a function that gets called
from different contexts and pretends to be an irq handler. The
local_bh_disable()/_local_bh_enable() pair is just a trick to prevent bottom
halve execution. I think you can blame Martin for this ;)

Probably both of these functions needs some comment I guess... and this one
as well: bf6f6aa46feada857a52cb67d99a7c2fe4a70e87 (our new __udelay
implementation).

-- 
Heiko Carstens
Linux on System z Development

IBM Deutschland Entwicklung GmbH
Vorsitzender des Aufsichtsrats: Johann Weihen
Geschaeftsfuehrung : Herbert Kircher
Sitz der Gesellschaft: Boeblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

Re: [patch] s390: do not use _local_bh_enable()

[Martin Schwidefsky]

On Fri, 2007-02-23 at 10:36 +0100, Heiko Carstens wrote:
> Same here: this is not really an irq handler but a function that gets called
> from different contexts and pretends to be an irq handler. The
> local_bh_disable()/_local_bh_enable() pair is just a trick to prevent bottom
> halve execution. I think you can blame Martin for this ;)

This code is really tricky. There are now three functions (cio_tpi,
sclp_sync_wait and __udelay) where we use a local_bh_disable()
_local_bh_enable() pair while we are disabled for interrupts. cio_tpi
and sclp_sync_wait are used to wait for the interrupt of the console
device to make room in the buffer for a printk out of disabled context.
__udelay() is used in the ETR clock-synchronization code where we are
disabled as well and the only alternative would be looping on a STCK for
about a second.

So basically we want to synchronously receive a specific interrupt. All
other interrupt sources are disabled. We know that the interrupt we wait
for will not cause a softirq to get scheduled. In case of sclp_sync_wait
and __udelay the interrupt is delivered the usual way by using the
asynchronous interrupt handler (I told you it is tricky ;-). That works
as long as only the hardirq part of the interrupt is executed, the
softirq part may not happen -> local_bh_disable. Back in the function
after the interrupt we cannot allow the softirq to happen when we
reenable the bottom-halves since the functions are called disabled and
need to stay disabled -> _local_bh_enable. Since the interrupt we waited
for did not add a softirq no harms is done, no?

-- 
blue skies,              IBM Deutschland Entwicklung GmbH
   Martin                Vorsitzender des Aufsichtsrats: Johann Weihen
                         Geschäftsführung: Herbert Kircher
Martin Schwidefsky       Sitz der Gesellschaft: Böblingen
Linux on zSeries         Registergericht: Amtsgericht Stuttgart,
   Development           HRB 243294

"Reality continues to ruin my life." - Calvin.

Re: [patch] s390: do not use _local_bh_enable()

[Ingo Molnar]

* Martin Schwidefsky <schwidefsky@de.ibm.com> wrote:

> [...] cio_tpi and sclp_sync_wait are used to wait for the interrupt of 
> the console device to make room in the buffer for a printk out of 
> disabled context.

ouch. So you want/need to wait for a specific type of interrupt, in a 
section of code that has all interrupts disabled? Is this the only form 
of communication to the hypervisor, for this particular purpose? It 
seems to me that polling a bit in a buffer shared between the hypervisor 
and the guest OS [combined with cpu_relax()] would fit this scenario 
alot better (and wouldnt cause any such gymnastics to avoid regular 
Linux irq processing) than waiting for an interrupt to be injected by 
the hypervisor. Or is this interrupt-based interface an ABI property and 
the only way to do it?

	Ingo

Re: [patch] s390: do not use _local_bh_enable()

[Martin Schwidefsky]

On Fri, 2007-02-23 at 15:04 +0100, Ingo Molnar wrote:
> > [...] cio_tpi and sclp_sync_wait are used to wait for the interrupt of 
> > the console device to make room in the buffer for a printk out of 
> > disabled context.
> 
> ouch. So you want/need to wait for a specific type of interrupt, in a 
> section of code that has all interrupts disabled? Is this the only form 
> of communication to the hypervisor, for this particular purpose? It 
> seems to me that polling a bit in a buffer shared between the hypervisor 
> and the guest OS [combined with cpu_relax()] would fit this scenario 
> alot better (and wouldnt cause any such gymnastics to avoid regular 
> Linux irq processing) than waiting for an interrupt to be injected by 
> the hypervisor. Or is this interrupt-based interface an ABI property and 
> the only way to do it?

This has nothing to do with the hypervisor. Even on bare metal (that
machine mode exists up to G7, newer machine always run a hypervisor) you
have to deal with your console device. If this is a 3270 green-screen
you have to do asynchronous i/o. To wait for some room in the console
buffer translates to waiting for the interrupt of a ccw device. The
memory of the machine is unchanged until the interrupt is received.
So yes, ouch!

-- 
blue skies,              IBM Deutschland Entwicklung GmbH
   Martin                Vorsitzender des Aufsichtsrats: Johann Weihen
                         Geschäftsführung: Herbert Kircher
Martin Schwidefsky       Sitz der Gesellschaft: Böblingen
Linux on zSeries         Registergericht: Amtsgericht Stuttgart,
   Development           HRB 243294

"Reality continues to ruin my life." - Calvin.