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 <[email protected]>
_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 <[email protected]>
---
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;
}
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
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.
* Martin Schwidefsky <[email protected]> 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
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.