Re: [Linux-fbdev-devel] Re: [ACPI] inappropriate use of in_atomic()
On Fri, 2005-03-11 at 16:13 -0800, Andrew Morton wrote: > (where'd my cc go?) > > Benjamin Herrenschmidt <[EMAIL PROTECTED]> wrote: > > > > On Fri, 2005-03-11 at 01:46 -0800, Andrew Morton wrote: > > > Jan Kasprzak <[EMAIL PROTECTED]> wrote: > > > > > > > > This may be the cause of > > > > > > > > http://bugme.osdl.org/show_bug.cgi?id=4150 > > > > > > Looks that way, yes. > > > > Note that it would be interesting to fix that (I mean the reliability of > > is_atomic() or an alternative). I agree it's quite bad to rely on that > > in practice, but there are a few corner cases where it's useful (like > > oops handling in fbdev's etc...) > > > > That would require that we increment current->something on every > spin/read/write_lock and decrement it in unlock, even with !CONFIG_PREEMPT. > > iirc, Anton added an option to do that to the ppc64 build, decoupled from > CONFIG_PREEMPT (which ppc64 doesn't support). ppc64 _does_ support PREEMPT nowadays :) > But it's an appreciable amount of overhead. -- Benjamin Herrenschmidt <[EMAIL PROTECTED]> - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Linux-fbdev-devel] Re: [ACPI] inappropriate use of in_atomic()
(where'd my cc go?) Benjamin Herrenschmidt <[EMAIL PROTECTED]> wrote: > > On Fri, 2005-03-11 at 01:46 -0800, Andrew Morton wrote: > > Jan Kasprzak <[EMAIL PROTECTED]> wrote: > > > > > > This may be the cause of > > > > > > http://bugme.osdl.org/show_bug.cgi?id=4150 > > > > Looks that way, yes. > > Note that it would be interesting to fix that (I mean the reliability of > is_atomic() or an alternative). I agree it's quite bad to rely on that > in practice, but there are a few corner cases where it's useful (like > oops handling in fbdev's etc...) > That would require that we increment current->something on every spin/read/write_lock and decrement it in unlock, even with !CONFIG_PREEMPT. iirc, Anton added an option to do that to the ppc64 build, decoupled from CONFIG_PREEMPT (which ppc64 doesn't support). But it's an appreciable amount of overhead. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: inappropriate use of in_atomic()
On Thu, 2005-03-10 at 20:53 -0800, Roland Dreier wrote: > > Consequently the use of in_atomic() in the below files is probably > > deadlocky if CONFIG_PREEMPT=n: > ... > > drivers/infiniband/core/mad.c > > Thanks for pointing this out. I'll get you a patch in the next day or > two. As you can probably tell, the code is just trying to decide > whether to use GFP_ATOMIC or GFP_KERNEL to allocate a couple of > things, depending on what context we're being called from. So at > worst we can just change to GFP_ATOMIC unconditionally. normally you are supposed to know what context your allocating function is called in... if you don't know that often is a sign of a misdesign... - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Linux-fbdev-devel] Re: [ACPI] inappropriate use of in_atomic()
On Fri, 2005-03-11 at 01:46 -0800, Andrew Morton wrote: > Jan Kasprzak <[EMAIL PROTECTED]> wrote: > > > > This may be the cause of > > > > http://bugme.osdl.org/show_bug.cgi?id=4150 > > Looks that way, yes. Note that it would be interesting to fix that (I mean the reliability of is_atomic() or an alternative). I agree it's quite bad to rely on that in practice, but there are a few corner cases where it's useful (like oops handling in fbdev's etc...) Ben. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [ACPI] inappropriate use of in_atomic()
Jan Kasprzak <[EMAIL PROTECTED]> wrote: > > This may be the cause of > > http://bugme.osdl.org/show_bug.cgi?id=4150 Looks that way, yes. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [ACPI] inappropriate use of in_atomic()
Andrew Morton wrote: : : in_atomic() is not a reliable indication of whether it is currently safe : to call schedule(). : : This is because the lockdepth beancounting which in_atomic() uses is only : accumulated if CONFIG_PREEMPT=y. in_atomic() will return false inside : spinlocks if CONFIG_PREEMPT=n. : : Consequently the use of in_atomic() in the below files is probably : deadlocky if CONFIG_PREEMPT=n: [...] : drivers/acpi/osl.c [...] This may be the cause of http://bugme.osdl.org/show_bug.cgi?id=4150 - I have recently verified that the problem described in bug #4150 disappears when CONFIG_PREEMPT=y is used. -Yenya -- | Jan "Yenya" Kasprzak | | GPG: ID 1024/D3498839 Fingerprint 0D99A7FB206605D7 8B35FCDE05B18A5E | | http://www.fi.muni.cz/~kas/ Czech Linux Homepage: http://www.linux.cz/ | > Whatever the Java applications and desktop dances may lead to, Unix will < > still be pushing the packets around for a quite a while. --Rob Pike < - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [ACPI] inappropriate use of in_atomic()
Andrew Morton wrote:
:
: in_atomic() is not a reliable indication of whether it is currently safe
: to call schedule().
:
: This is because the lockdepth beancounting which in_atomic() uses is only
: accumulated if CONFIG_PREEMPT=y. in_atomic() will return false inside
: spinlocks if CONFIG_PREEMPT=n.
:
: Consequently the use of in_atomic() in the below files is probably
: deadlocky if CONFIG_PREEMPT=n:
[...]
: drivers/acpi/osl.c
[...]
This may be the cause of
http://bugme.osdl.org/show_bug.cgi?id=4150
- I have recently verified that the problem described in bug #4150 disappears
when CONFIG_PREEMPT=y is used.
-Yenya
--
| Jan Yenya Kasprzak kas at {fi.muni.cz - work | yenya.net - private} |
| GPG: ID 1024/D3498839 Fingerprint 0D99A7FB206605D7 8B35FCDE05B18A5E |
| http://www.fi.muni.cz/~kas/ Czech Linux Homepage: http://www.linux.cz/ |
Whatever the Java applications and desktop dances may lead to, Unix will
still be pushing the packets around for a quite a while. --Rob Pike
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [ACPI] inappropriate use of in_atomic()
Jan Kasprzak [EMAIL PROTECTED] wrote: This may be the cause of http://bugme.osdl.org/show_bug.cgi?id=4150 Looks that way, yes. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Linux-fbdev-devel] Re: [ACPI] inappropriate use of in_atomic()
On Fri, 2005-03-11 at 01:46 -0800, Andrew Morton wrote: Jan Kasprzak [EMAIL PROTECTED] wrote: This may be the cause of http://bugme.osdl.org/show_bug.cgi?id=4150 Looks that way, yes. Note that it would be interesting to fix that (I mean the reliability of is_atomic() or an alternative). I agree it's quite bad to rely on that in practice, but there are a few corner cases where it's useful (like oops handling in fbdev's etc...) Ben. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: inappropriate use of in_atomic()
On Thu, 2005-03-10 at 20:53 -0800, Roland Dreier wrote: Consequently the use of in_atomic() in the below files is probably deadlocky if CONFIG_PREEMPT=n: ... drivers/infiniband/core/mad.c Thanks for pointing this out. I'll get you a patch in the next day or two. As you can probably tell, the code is just trying to decide whether to use GFP_ATOMIC or GFP_KERNEL to allocate a couple of things, depending on what context we're being called from. So at worst we can just change to GFP_ATOMIC unconditionally. normally you are supposed to know what context your allocating function is called in... if you don't know that often is a sign of a misdesign... - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Linux-fbdev-devel] Re: [ACPI] inappropriate use of in_atomic()
(where'd my cc go?) Benjamin Herrenschmidt [EMAIL PROTECTED] wrote: On Fri, 2005-03-11 at 01:46 -0800, Andrew Morton wrote: Jan Kasprzak [EMAIL PROTECTED] wrote: This may be the cause of http://bugme.osdl.org/show_bug.cgi?id=4150 Looks that way, yes. Note that it would be interesting to fix that (I mean the reliability of is_atomic() or an alternative). I agree it's quite bad to rely on that in practice, but there are a few corner cases where it's useful (like oops handling in fbdev's etc...) That would require that we increment current-something on every spin/read/write_lock and decrement it in unlock, even with !CONFIG_PREEMPT. iirc, Anton added an option to do that to the ppc64 build, decoupled from CONFIG_PREEMPT (which ppc64 doesn't support). But it's an appreciable amount of overhead. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Linux-fbdev-devel] Re: [ACPI] inappropriate use of in_atomic()
On Fri, 2005-03-11 at 16:13 -0800, Andrew Morton wrote: (where'd my cc go?) Benjamin Herrenschmidt [EMAIL PROTECTED] wrote: On Fri, 2005-03-11 at 01:46 -0800, Andrew Morton wrote: Jan Kasprzak [EMAIL PROTECTED] wrote: This may be the cause of http://bugme.osdl.org/show_bug.cgi?id=4150 Looks that way, yes. Note that it would be interesting to fix that (I mean the reliability of is_atomic() or an alternative). I agree it's quite bad to rely on that in practice, but there are a few corner cases where it's useful (like oops handling in fbdev's etc...) That would require that we increment current-something on every spin/read/write_lock and decrement it in unlock, even with !CONFIG_PREEMPT. iirc, Anton added an option to do that to the ppc64 build, decoupled from CONFIG_PREEMPT (which ppc64 doesn't support). ppc64 _does_ support PREEMPT nowadays :) But it's an appreciable amount of overhead. -- Benjamin Herrenschmidt [EMAIL PROTECTED] - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: inappropriate use of in_atomic()
Hi Andrew,
On Thu, 10 Mar 2005 20:40:06 -0800 Andrew Morton <[EMAIL PROTECTED]> wrote:
>
> in_atomic() is not a reliable indication of whether it is currently safe
> to call schedule().
>
> arch/ppc64/kernel/viopath.c
in_atomic() in viopath.c was just used to determine if we had initialised
enough to be able to wait in a semaphore (i.e. schedule). Thus it can be
replaced now with checking system_state for SYSTEM_RUNNING.
Signed-off-by: Stephen Rothwell <[EMAIL PROTECTED]>
Test booted on iSeries (which is the only place it is used).
--
Cheers,
Stephen Rothwell[EMAIL PROTECTED]
http://www.canb.auug.org.au/~sfr/
diff -ruNp linus/arch/ppc64/kernel/viopath.c
linus-in_atomic/arch/ppc64/kernel/viopath.c
--- linus/arch/ppc64/kernel/viopath.c 2005-01-22 06:09:01.0 +1100
+++ linus-in_atomic/arch/ppc64/kernel/viopath.c 2005-03-11 17:19:45.0
+1100
@@ -79,7 +79,7 @@ static void handleMonitorEvent(struct Hv
/*
* We use this structure to handle asynchronous responses. The caller
* blocks on the semaphore and the handler posts the semaphore. However,
- * if in_atomic() is true in the caller, then wait_atomic is used ...
+ * if system_state is not SYSTEM_RUNNING, then wait_atomic is used ...
*/
struct doneAllocParms_t {
struct semaphore *sem;
@@ -465,7 +465,7 @@ static int allocateEvents(HvLpIndex remo
DECLARE_MUTEX_LOCKED(Semaphore);
atomic_t wait_atomic;
- if (in_atomic()) {
+ if (system_state != SYSTEM_RUNNING) {
parms.used_wait_atomic = 1;
atomic_set(_atomic, 1);
parms.wait_atomic = _atomic;
@@ -475,7 +475,7 @@ static int allocateEvents(HvLpIndex remo
}
mf_allocate_lp_events(remoteLp, HvLpEvent_Type_VirtualIo, 250, /* It
would be nice to put a real number here! */
numEvents, _donealloc, );
- if (in_atomic()) {
+ if (system_state != SYSTEM_RUNNING) {
while (atomic_read(_atomic))
mb();
} else
pgp8HryQkIgDo.pgp
Description: PGP signature
Re: inappropriate use of in_atomic()
> Consequently the use of in_atomic() in the below files is probably > deadlocky if CONFIG_PREEMPT=n: ... > drivers/infiniband/core/mad.c Thanks for pointing this out. I'll get you a patch in the next day or two. As you can probably tell, the code is just trying to decide whether to use GFP_ATOMIC or GFP_KERNEL to allocate a couple of things, depending on what context we're being called from. So at worst we can just change to GFP_ATOMIC unconditionally. I'll check into whether we can do something cleverer, but just going the GFP_ATOMIC route won't be horrible. Thanks, Roland - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
inappropriate use of in_atomic()
in_atomic() is not a reliable indication of whether it is currently safe to call schedule(). This is because the lockdepth beancounting which in_atomic() uses is only accumulated if CONFIG_PREEMPT=y. in_atomic() will return false inside spinlocks if CONFIG_PREEMPT=n. Consequently the use of in_atomic() in the below files is probably deadlocky if CONFIG_PREEMPT=n: arch/ppc64/kernel/viopath.c drivers/net/irda/sir_kthread.c drivers/net/wireless/airo.c drivers/video/amba-clcd.c drivers/acpi/osl.c drivers/ieee1394/ieee1394_transactions.c drivers/infiniband/core/mad.c Note that the same beancounting is used for the "scheduling while atomic" warning, so if the code calls schedule with locks held, we won't get a warning. Both are tied to CONFIG_PREEMPT=y. The kernel provides no reliable runtime way of detecting whether or not it is safe to call schedule(). Can we please find ways to change the above code to not use in_atomic()? Then we can whack #ifndef MODULE around its definition to reduce reoccurrences. Will probably rename it to something more scary as well. Thanks. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
inappropriate use of in_atomic()
in_atomic() is not a reliable indication of whether it is currently safe to call schedule(). This is because the lockdepth beancounting which in_atomic() uses is only accumulated if CONFIG_PREEMPT=y. in_atomic() will return false inside spinlocks if CONFIG_PREEMPT=n. Consequently the use of in_atomic() in the below files is probably deadlocky if CONFIG_PREEMPT=n: arch/ppc64/kernel/viopath.c drivers/net/irda/sir_kthread.c drivers/net/wireless/airo.c drivers/video/amba-clcd.c drivers/acpi/osl.c drivers/ieee1394/ieee1394_transactions.c drivers/infiniband/core/mad.c Note that the same beancounting is used for the scheduling while atomic warning, so if the code calls schedule with locks held, we won't get a warning. Both are tied to CONFIG_PREEMPT=y. The kernel provides no reliable runtime way of detecting whether or not it is safe to call schedule(). Can we please find ways to change the above code to not use in_atomic()? Then we can whack #ifndef MODULE around its definition to reduce reoccurrences. Will probably rename it to something more scary as well. Thanks. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: inappropriate use of in_atomic()
Consequently the use of in_atomic() in the below files is probably deadlocky if CONFIG_PREEMPT=n: ... drivers/infiniband/core/mad.c Thanks for pointing this out. I'll get you a patch in the next day or two. As you can probably tell, the code is just trying to decide whether to use GFP_ATOMIC or GFP_KERNEL to allocate a couple of things, depending on what context we're being called from. So at worst we can just change to GFP_ATOMIC unconditionally. I'll check into whether we can do something cleverer, but just going the GFP_ATOMIC route won't be horrible. Thanks, Roland - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: inappropriate use of in_atomic()
Hi Andrew,
On Thu, 10 Mar 2005 20:40:06 -0800 Andrew Morton [EMAIL PROTECTED] wrote:
in_atomic() is not a reliable indication of whether it is currently safe
to call schedule().
arch/ppc64/kernel/viopath.c
in_atomic() in viopath.c was just used to determine if we had initialised
enough to be able to wait in a semaphore (i.e. schedule). Thus it can be
replaced now with checking system_state for SYSTEM_RUNNING.
Signed-off-by: Stephen Rothwell [EMAIL PROTECTED]
Test booted on iSeries (which is the only place it is used).
--
Cheers,
Stephen Rothwell[EMAIL PROTECTED]
http://www.canb.auug.org.au/~sfr/
diff -ruNp linus/arch/ppc64/kernel/viopath.c
linus-in_atomic/arch/ppc64/kernel/viopath.c
--- linus/arch/ppc64/kernel/viopath.c 2005-01-22 06:09:01.0 +1100
+++ linus-in_atomic/arch/ppc64/kernel/viopath.c 2005-03-11 17:19:45.0
+1100
@@ -79,7 +79,7 @@ static void handleMonitorEvent(struct Hv
/*
* We use this structure to handle asynchronous responses. The caller
* blocks on the semaphore and the handler posts the semaphore. However,
- * if in_atomic() is true in the caller, then wait_atomic is used ...
+ * if system_state is not SYSTEM_RUNNING, then wait_atomic is used ...
*/
struct doneAllocParms_t {
struct semaphore *sem;
@@ -465,7 +465,7 @@ static int allocateEvents(HvLpIndex remo
DECLARE_MUTEX_LOCKED(Semaphore);
atomic_t wait_atomic;
- if (in_atomic()) {
+ if (system_state != SYSTEM_RUNNING) {
parms.used_wait_atomic = 1;
atomic_set(wait_atomic, 1);
parms.wait_atomic = wait_atomic;
@@ -475,7 +475,7 @@ static int allocateEvents(HvLpIndex remo
}
mf_allocate_lp_events(remoteLp, HvLpEvent_Type_VirtualIo, 250, /* It
would be nice to put a real number here! */
numEvents, viopath_donealloc, parms);
- if (in_atomic()) {
+ if (system_state != SYSTEM_RUNNING) {
while (atomic_read(wait_atomic))
mb();
} else
pgp8HryQkIgDo.pgp
Description: PGP signature
