[PATCH] ACPICA: don't cond_resched() when irq_disabled or in_atomic
Xiaotian Feng
a sleeping function called from invalid context bug was founded
during suspend/resume stage. this was fixed in commit abe1dfa by
don't cond_resched when irq_disabled. But recent commit 138d156 changes
the behaviour to don't cond_resched when in_atomic. This makes the
sleeping function called from invalid context bug happen again, which
is reported in http://lkml.org/lkml/2009/12/1/371.
The fix is to cond_sched() only when preemptible, which means not in
irq_disabled or in_atomic.
Reported-and-bisected-by: Larry Finger <Larry....@lwfinger.net>
Signed-off-by: Xiaotian Feng <df...@redhat.com>
---
include/acpi/platform/aclinux.h | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/include/acpi/platform/aclinux.h b/include/acpi/platform/aclinux.h
index 9d7febd..5b415ee 100644
--- a/include/acpi/platform/aclinux.h
+++ b/include/acpi/platform/aclinux.h
@@ -152,7 +152,7 @@ static inline void *acpi_os_acquire_object(acpi_cache_t * cache)
#include <linux/hardirq.h>
#define ACPI_PREEMPTION_POINT() \
do { \
- if (!in_atomic_preempt_off()) \
+ if (preemptible()) \
cond_resched(); \
} while (0)
--
1.6.5.2
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Zhang Rui
Zhang Rui
Justin Mattock
looks good
kernel compiled without any issues,
echo mem > /sys/power/state
reported no warning message.
Also if you don't mind add:
Reported-and-bisected-by: Justin P. Mattock <justin...@gmail.com>
Id like to get some kind of credit for this b*tch.
--
Justin P. Mattock
Danny Feng
http://bugzilla.kernel.org/show_bug.cgi?id=14483
> Id like to get some kind of credit for this b*tch.
>
--
Justin P. Mattock
no worries.. I'll run
my system with this change
to see if anything happens.
As for the bug, leave it open
until this makes it's way into
the main kernel, then rafael can
close it
Justin P. Mattock
Xiaotian Feng
Any feedbacks?
Regards
Xiaotian
Pavel Machek
> commit 8bd108d adds preemption point after each opcode parse, then
> a sleeping function called from invalid context bug was founded
> during suspend/resume stage. this was fixed in commit abe1dfa by
> don't cond_resched when irq_disabled. But recent commit 138d156 changes
> the behaviour to don't cond_resched when in_atomic. This makes the
> sleeping function called from invalid context bug happen again, which
> is reported in http://lkml.org/lkml/2009/12/1/371.
>
> The fix is to cond_sched() only when preemptible, which means not in
> irq_disabled or in_atomic.
>
> #include <linux/hardirq.h>
> #define ACPI_PREEMPTION_POINT() \
> do { \
> - if (!in_atomic_preempt_off()) \
> + if (preemptible()) \
> cond_resched(); \
> } while (0)
Note that this is ugly as hell. It means we have two acpi
interpretters in kernel, one for preemptible, one for non-preemptible,
with very different behaviour.
It would be slightly nicer to pass the "preemptible" info explicitely,
as function parameters.
It would be even better not to need that difference.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Xiaotian Feng
a sleeping function called from invalid context bug was founded
during suspend/resume stage. this was fixed in commit abe1dfa by
don't cond_resched when irq_disabled. But recent commit 138d156 changes
the behaviour to don't cond_resched when in_atomic. This makes the
sleeping function called from invalid context bug happen again, which
is reported in http://lkml.org/lkml/2009/12/1/371.
This patch also fixes http://bugzilla.kernel.org/show_bug.cgi?id=14483
Reported-and-bisected-by: Larry Finger <Larry....@lwfinger.net>
Reported-and-bisected-by: Justin P. Mattock <justin...@gmail.com>
Signed-off-by: Xiaotian Feng <df...@redhat.com>
Cc: Len Brown <le...@kernel.org>
Cc: Bob Moore <robert...@intel.com>
Cc: Lin Ming <ming....@intel.com>
Cc: Alexey Starikovskiy <astari...@suse.de>
Cc: Pavel Machek <pa...@ucw.cz>
---
include/acpi/platform/aclinux.h | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/include/acpi/platform/aclinux.h b/include/acpi/platform/aclinux.h
index 9d7febd..0946997 100644
--- a/include/acpi/platform/aclinux.h
+++ b/include/acpi/platform/aclinux.h
@@ -152,7 +152,7 @@ static inline void *acpi_os_acquire_object(acpi_cache_t * cache)
#include <linux/hardirq.h>
#define ACPI_PREEMPTION_POINT() \
do { \
- if (!in_atomic_preempt_off()) \
+ if (!in_atomic_preempt_off() && !irqs_disabled()) \
cond_resched(); \
} while (0)
--
1.6.5.2
--
Alexey Starikovskiy
I think, this is another round of "armor vs. bullet" race... It will hold until
might_sleep() logic changes again.
Please consider using preemptible() -- IMHO this is the check we should perform
in our case of voluntary preemption.
Regards,
Alex.
Xiaotian Feng пишет:
--
Alexey Starikovskiy
Please elaborate... Your comments "ugly as hell" are too often to be
specific...
There is only one use of ACPI_PREEMPTION_POINT(), and it is in the
ACPICA code,
which we all agreed to keep OS independent, thus the need for #define.
Do you see any other way to add preemption point without introducing
Linux-specific
code into ACPICA?
Thanks,
Alex.
Pavel Machek пишет:
--
Pavel Machek
> Hi Pavel,
>
> Please elaborate... Your comments "ugly as hell" are too often to be
> specific...
> There is only one use of ACPI_PREEMPTION_POINT(), and it is in the
> ACPICA code,
> which we all agreed to keep OS independent, thus the need for #define.
> Do you see any other way to add preemption point without introducing
> Linux-specific
> code into ACPICA?
I believe we want linux-specific code in acpica at this point.
(Or maybe... I guess other systems have concept of preemption and not
all actions are permitted from all contexts, so maybe something like
that would be important for them, too?)
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Alexey Starikovskiy
> On Thu 2009-12-10 20:58:45, Alexey Starikovskiy wrote:
>
>> Hi Pavel,
>>
>> Please elaborate... Your comments "ugly as hell" are too often to be
>> specific...
>> There is only one use of ACPI_PREEMPTION_POINT(), and it is in the
>> ACPICA code,
>> which we all agreed to keep OS independent, thus the need for #define.
>> Do you see any other way to add preemption point without introducing
>> Linux-specific
>> code into ACPICA?
>>
>
> I believe we want linux-specific code in acpica at this point.
>
>
loop. This parse loop may be executed from within atomic context and even
with interrupts off. In this case, cond_resched() should not be called
to not make
might_sleep() guards angry.
Please post the code, which will do the above and will not look "ugly as
hell".
I still don't follow your vague comments.
> (Or maybe... I guess other systems have concept of preemption and not
> all actions are permitted from all contexts, so maybe something like
> that would be important for them, too?)
>
None of them cared about it up to this point.
With the macro above we allowed them to follow Linux, but to go or not
is their call.
Regards,
Alex.
Justin P. Mattock
o.k. I went did a pull to update
the kernel, and then changed
aclinux.h to the above post.
I'm am not seeing this warning message
upon wake-up.
but with the acpi merge stuff with
acpi_walk_namespace seems to break nvidia
(nvidia's problem now)
there is also some thing where the machine
takes a good 30 secs or so to wake up
(not sure if this is from the updated patch)
in dmesg I see:
platform microcode: firmware requesting intel-ucode/06-17-0a
firmware microcode: parent mocrocode should not be sleeping.
I'm thinking I need something in /lib/firmare
Justin P. Mattock
Moore, Robert
Bob
>-----Original Message-----
>From: Justin P. Mattock [mailto:justin...@gmail.com]
>Sent: Thursday, December 10, 2009 2:46 PM
>To: Alexey Starikovskiy
>Cc: Pavel Machek; Xiaotian Feng; le...@kernel.org; Lin, Ming M; Moore,
>Robert; linux...@vger.kernel.org; linux-...@vger.kernel.org
>Subject: Re: [PATCH] ACPICA: don't cond_resched() when irq_disabled or
>in_atomic
>
>On 12/10/09 10:37, Alexey Starikovskiy wrote:
>> Pavel Machek О©╫О©╫О©╫О©╫О©╫:
Lin Ming
> Hi Xiaotian,
>
> I think, this is another round of "armor vs. bullet" race... It will hold until
> might_sleep() logic changes again.
>
> Please consider using preemptible() -- IMHO this is the check we should perform
> in our case of voluntary preemption.
preemptible() may not work here because it always returns 0 for
non-preemptible kernel.
#ifdef CONFIG_PREEMPT
# define preemptible() (preempt_count() == 0 && !irqs_disabled())
# define IRQ_EXIT_OFFSET (HARDIRQ_OFFSET-1)
#else
# define preemptible() 0
# define IRQ_EXIT_OFFSET HARDIRQ_OFFSET
#endif
Lin Ming
Alexey Starikovskiy
> On Thu, 2009-12-10 at 20:21 +0800, Alexey Starikovskiy wrote:
>> Hi Xiaotian,
>>
>> I think, this is another round of "armor vs. bullet" race... It will hold until
>> might_sleep() logic changes again.
>>
>> Please consider using preemptible() -- IMHO this is the check we should perform
>> in our case of voluntary preemption.
>
> preemptible() may not work here because it always returns 0 for
> non-preemptible kernel.
The reason we introduced the preemption point in the first place, was unacceptable latency
due to very long AML methods on some machines. We don't need this preemption point for normal
operation, this is exactly what voluntary preemption does -- allows those in hurry to pass by.
If there are none, fine.
>
> #ifdef CONFIG_PREEMPT
> # define preemptible() (preempt_count() == 0 && !irqs_disabled())
> # define IRQ_EXIT_OFFSET (HARDIRQ_OFFSET-1)
> #else
> # define preemptible() 0
> # define IRQ_EXIT_OFFSET HARDIRQ_OFFSET
> #endif
>
Alex.
Pavel Machek
> Lin Ming ??????????:
> > On Thu, 2009-12-10 at 20:21 +0800, Alexey Starikovskiy wrote:
> >> Hi Xiaotian,
> >>
> >> I think, this is another round of "armor vs. bullet" race... It will hold until
> >> might_sleep() logic changes again.
> >>
> >> Please consider using preemptible() -- IMHO this is the check we should perform
> >> in our case of voluntary preemption.
> >
> > preemptible() may not work here because it always returns 0 for
> > non-preemptible kernel.
> Right, and it means that this machine does not care about low latency that much.
> The reason we introduced the preemption point in the first place, was unacceptable latency
> due to very long AML methods on some machines. We don't need this preemption point for normal
> operation, this is exactly what voluntary preemption does -- allows those in hurry to pass by.
> If there are none, fine.
> >
> > #ifdef CONFIG_PREEMPT
> > # define preemptible() (preempt_count() == 0 && !irqs_disabled())
> > # define IRQ_EXIT_OFFSET (HARDIRQ_OFFSET-1)
> > #else
> > # define preemptible() 0
> > # define IRQ_EXIT_OFFSET HARDIRQ_OFFSET
> > #endif
Well, normally we want low latency even for !CONFIG_PREEMPT kernels.
Actually, explicit preemption points are NOPs for CONFIG_PREEMPT
kernels, right?
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Alexey Starikovskiy
Thanks,
Alex.
Pavel Machek
> >> If there are none, fine.
> >>
> >>> #ifdef CONFIG_PREEMPT
> >>> # define preemptible() (preempt_count() == 0 && !irqs_disabled())
> >>> # define IRQ_EXIT_OFFSET (HARDIRQ_OFFSET-1)
> >>> #else
> >>> # define preemptible() 0
> >>> # define IRQ_EXIT_OFFSET HARDIRQ_OFFSET
> >>> #endif
> >>>
> >
> > Well, normally we want low latency even for !CONFIG_PREEMPT kernels.
> >
> > Actually, explicit preemption points are NOPs for CONFIG_PREEMPT
> > kernels, right?
> Right. Do you have code?
I'd prefer to spend my time with patches to areas that actually do
take cleanup patches.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Pavel Machek
> Pavel Machek ??????????:
> > On Thu 2009-12-10 20:58:45, Alexey Starikovskiy wrote:
> >
> >> Hi Pavel,
> >>
> >> Please elaborate... Your comments "ugly as hell" are too often to be
> >> specific...
> >> There is only one use of ACPI_PREEMPTION_POINT(), and it is in the
> >> ACPICA code,
> >> which we all agreed to keep OS independent, thus the need for #define.
> >> Do you see any other way to add preemption point without introducing
> >> Linux-specific
> >> code into ACPICA?
> >>
> >
> > I believe we want linux-specific code in acpica at this point.
> >
> >
> The point there we call cond_resched() in ACPICA is an interpreter parse
> loop. This parse loop may be executed from within atomic context and even
> with interrupts off. In this case, cond_resched() should not be called
> to not make
> might_sleep() guards angry.
Yes, so pass explicit argument to the interpretter, telling it what
kind of context it runs on. Similar to kmalloc's GFP_KERNEL
vs. GFP_ATOMIC.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Xiaotian Feng
>
>>>> If there are none, fine.
>>>>
>>>>> #ifdef CONFIG_PREEMPT
>>>>> # define IRQ_EXIT_OFFSET (HARDIRQ_OFFSET-1)
>>>>> #else
>>>>> # define preemptible() 0
>>>>> # define IRQ_EXIT_OFFSET HARDIRQ_OFFSET
>>>>> #endif
>>>>>
>>>
>>> Well, normally we want low latency even for !CONFIG_PREEMPT kernels.
>>>
>>> Actually, explicit preemption points are NOPs for CONFIG_PREEMPT
>>> kernels, right?
>
>> Right. Do you have code?
>
> I'd prefer to spend my time with patches to areas that actually do
> take cleanup patches.
What's the status of this now? We can still see the sleeping function
call warning or enable irq at resume stage.
If acpi wants low latency even for !CONFIG_PREEMPT kernels, what's wrong
with V2 patch?
We should not set any preemption points in irq or atomic. Since we have
a simple fix, and it did fix bugs, why should
we make things more complex?
> Pavel
Alexey Starikovskiy
> On 12/12/2009 01:34 AM, Pavel Machek wrote:
>>
>>>>> If there are none, fine.
>>>>>
>>>>>> #ifdef CONFIG_PREEMPT
>>>>>> # define preemptible() (preempt_count() == 0&& !irqs_disabled())
>>>>>> # define IRQ_EXIT_OFFSET (HARDIRQ_OFFSET-1)
>>>>>> #else
>>>>>> # define preemptible() 0
>>>>>> # define IRQ_EXIT_OFFSET HARDIRQ_OFFSET
>>>>>> #endif
>>>>>>
>>>>
>>>> Well, normally we want low latency even for !CONFIG_PREEMPT kernels.
>>>>
>>>> Actually, explicit preemption points are NOPs for CONFIG_PREEMPT
>>>> kernels, right?
>>
>>> Right. Do you have code?
>>
>> I'd prefer to spend my time with patches to areas that actually do
>> take cleanup patches.
>
> What's the status of this now? We can still see the sleeping function
> call warning or enable irq at resume stage.
> If acpi wants low latency even for !CONFIG_PREEMPT kernels, what's wrong
> with V2 patch?
>
> We should not set any preemption points in irq or atomic. Since we have
> a simple fix, and it did fix bugs, why should
> we make things more complex?
Consider me ACK your patch.
Thanks,
Alex
Len Brown
> Xiaotian Feng пишет:
> > What's the status of this now? We can still see the sleeping function
> > call warning or enable irq at resume stage.
> > If acpi wants low latency even for !CONFIG_PREEMPT kernels, what's wrong
> > with V2 patch?
> >
> > We should not set any preemption points in irq or atomic. Since we have
> > a simple fix, and it did fix bugs, why should
> > we make things more complex?
> We should not do anything complex here, you are right.
> Consider me ACK your patch.
This patch has been in the acpi-test tree for a while
and I'll push it upstream with the next batch.
thanks,
Len Brown, Intel Open Source Technology Center
Len Brown
> Let me know when you guys have finalized any changes to aclinux.h, and I will update this file in the base ACPICA code.
I think the v2 patch will go upstream.
Not super-critical to have ACPICA sync with Linux's aclinux.h,
since Linux has it already, but good hygine, I guess.
thanks,
-Len Brown, Intel Open Source Technology Center