LinuxLists.cc - [PATCH] ACPICA: don't cond_resched() when irq_disabled or in

[permalink] [raw]

Subject: Re: [PATCH] ACPICA: don't cond_resched() when irq_disabled or in_atomic

On 12/04/2009 02:50 PM, Justin Mattock wrote:
> On Thu, Dec 3, 2009 at 8:26 PM, Xiaotian Feng<[email protected]> wrote:
>> 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.
>>
>> Reported-and-bisected-by: Larry Finger<[email protected]>
>> Signed-off-by: Xiaotian Feng<[email protected]>
>> ---
>> 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 [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at http://www.tux.org/lkml/
>>
>
> 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<[email protected]>
>
Sure, sorry for I had missed thread for

http://bugzilla.kernel.org/show_bug.cgi?id=14483

> Id like to get some kind of credit for this b*tch.
>

2009-12-04 07:27:49

by Justin P. Mattock

[permalink] [raw]

Subject: Re: [PATCH] ACPICA: don't cond_resched() when irq_disabled or in_atomic

On 12/03/09 23:05, Danny Feng wrote:
> On 12/04/2009 02:50 PM, Justin Mattock wrote:
>> On Thu, Dec 3, 2009 at 8:26 PM, Xiaotian Feng<[email protected]> wrote:
>>> 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.
>>>
>>> Reported-and-bisected-by: Larry Finger<[email protected]>
>>> Signed-off-by: Xiaotian Feng<[email protected]>
>>> ---
>>> 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 [email protected]
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>> Please read the FAQ at http://www.tux.org/lkml/
>>>
>>
>> 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<[email protected]>
>>
> Sure, sorry for I had missed thread for
>
> http://bugzilla.kernel.org/show_bug.cgi?id=14483
>
>> Id like to get some kind of credit for this b*tch.
>>
>
>

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

2009-12-09 01:54:32

[permalink] [raw]

Subject: Re: [PATCH] ACPICA: don't cond_resched() when irq_disabled or in_atomic

On 12/04/2009 03:27 PM, Justin P. Mattock wrote:
> On 12/03/09 23:05, Danny Feng wrote:
>> On 12/04/2009 02:50 PM, Justin Mattock wrote:
>>> On Thu, Dec 3, 2009 at 8:26 PM, Xiaotian Feng<[email protected]> wrote:
>>>> 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.
>>>>
>>>> Reported-and-bisected-by: Larry Finger<[email protected]>
>>>> Signed-off-by: Xiaotian Feng<[email protected]>
>>>> ---
>>>> 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 [email protected]
>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>> Please read the FAQ at http://www.tux.org/lkml/
>>>>
>>>
>>> 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<[email protected]>
>>>
>> Sure, sorry for I had missed thread for
>>
>> http://bugzilla.kernel.org/show_bug.cgi?id=14483
>>
>>> Id like to get some kind of credit for this b*tch.
>>>
>>
>>
>
> no worries.. I'll run
> my system with this change
> to see if anything happens.
>

Any feedbacks?

Regards
Xiaotian

> 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
>

2009-12-10 10:34:47

[permalink] [raw]

Subject: Re: [PATCH] ACPICA: don't cond_resched() when irq_disabled or in_atomic

On Fri 2009-12-04 12:26:00, Xiaotian Feng wrote:
> 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.
>
> @@ -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)

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

2009-12-10 11:57:23

[permalink] [raw]

Subject: [PATCH -V2] acpi: don't cond_resched if irq is disabled

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.

This patch also fixes http://bugzilla.kernel.org/show_bug.cgi?id=14483

Reported-and-bisected-by: Larry Finger <[email protected]>
Reported-and-bisected-by: Justin P. Mattock <[email protected]>
Signed-off-by: Xiaotian Feng <[email protected]>
Cc: Len Brown <[email protected]>
Cc: Bob Moore <[email protected]>
Cc: Lin Ming <[email protected]>
Cc: Alexey Starikovskiy <[email protected]>
Cc: Pavel Machek <[email protected]>
---
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

2009-12-10 12:41:08

[permalink] [raw]

Subject: Re: [PATCH -V2] acpi: don't cond_resched if irq is disabled

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.

Regards,
Alex.

Xiaotian Feng пишет:
> 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.
>
> This patch also fixes http://bugzilla.kernel.org/show_bug.cgi?id=14483
>
> Reported-and-bisected-by: Larry Finger <[email protected]>
> Reported-and-bisected-by: Justin P. Mattock <[email protected]>
> Signed-off-by: Xiaotian Feng <[email protected]>
> Cc: Len Brown <[email protected]>
> Cc: Bob Moore <[email protected]>
> Cc: Lin Ming <[email protected]>
> Cc: Alexey Starikovskiy <[email protected]>
> Cc: Pavel Machek <[email protected]>
> ---
> 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)
>

2009-12-10 17:58:44

[permalink] [raw]

Subject: Re: [PATCH] ACPICA: don't cond_resched() when irq_disabled or in_atomic

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?

Thanks,
Alex.

Pavel Machek пишет:
> On Fri 2009-12-04 12:26:00, Xiaotian Feng wrote:
>
>> 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.
>>
>> @@ -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)
>>
>
> 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
>

2009-12-10 18:18:07

[permalink] [raw]

Subject: Re: [PATCH] ACPICA: don't cond_resched() when irq_disabled or in_atomic

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.

(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

2009-12-10 18:37:55

[permalink] [raw]

Subject: Re: [PATCH] ACPICA: don't cond_resched() when irq_disabled or in_atomic

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.

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.

2009-12-10 22:45:45

by Justin P. Mattock

[permalink] [raw]

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 пишет:
>> 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.
>
> 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.
> --
> 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/
>

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

2009-12-10 22:54:52

by Moore, Robert

[permalink] [raw]

Subject: RE: [PATCH] ACPICA: don't cond_resched() when irq_disabled or in_atomic

Let me know when you guys have finalized any changes to aclinux.h, and I will update this file in the base ACPICA code.

Bob

>-----Original Message-----
>From: Justin P. Mattock [mailto:[email protected]]
>Sent: Thursday, December 10, 2009 2:46 PM
>To: Alexey Starikovskiy
>Cc: Pavel Machek; Xiaotian Feng; [email protected]; Lin, Ming M; Moore,
>Robert; [email protected]; [email protected]
>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 ?????:
>>> 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.
>>
>> 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.
>> --
>> 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/
>>
>
>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

2009-12-11 06:00:44

by Lin Ming

[permalink] [raw]

Subject: Re: [PATCH -V2] acpi: don't cond_resched if irq is disabled

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.

#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

>
> Regards,
> Alex.
>
>
> Xiaotian Feng пишет:
> > 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.
> >
> > This patch also fixes http://bugzilla.kernel.org/show_bug.cgi?id=14483
> >
> > Reported-and-bisected-by: Larry Finger <[email protected]>
> > Reported-and-bisected-by: Justin P. Mattock <[email protected]>
> > Signed-off-by: Xiaotian Feng <[email protected]>
> > Cc: Len Brown <[email protected]>
> > Cc: Bob Moore <[email protected]>
> > Cc: Lin Ming <[email protected]>
> > Cc: Alexey Starikovskiy <[email protected]>
> > Cc: Pavel Machek <[email protected]>
> > ---
> > 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)
> >
>

2009-12-11 11:48:25

[permalink] [raw]

Subject: Re: [PATCH -V2] acpi: don't cond_resched if irq is disabled

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
>
Regards,
Alex.

2009-12-11 16:15:57

[permalink] [raw]

Subject: Re: [PATCH -V2] acpi: don't cond_resched if irq is disabled

On Fri 2009-12-11 14:48:21, Alexey Starikovskiy wrote:
> 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

2009-12-11 16:25:28

[permalink] [raw]

Subject: Re: [PATCH -V2] acpi: don't cond_resched if irq is disabled

Pavel Machek пишет:
> On Fri 2009-12-11 14:48:21, Alexey Starikovskiy wrote:
>
>> 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
>
Right. Do you have code?

Thanks,
Alex.

2009-12-11 17:33:38

[permalink] [raw]

Subject: Re: [PATCH] ACPICA: don't cond_resched() when irq_disabled or in_atomic

On Thu 2009-12-10 21:37:59, Alexey Starikovskiy wrote:
> 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

2009-12-11 17:34:30

[permalink] [raw]

Subject: Re: [PATCH -V2] acpi: don't cond_resched if irq is disabled

> >> 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

2009-12-28 06:03:19

[permalink] [raw]

Subject: Re: [PATCH -V2] acpi: don't cond_resched if irq is disabled

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?

> Pavel

2009-12-28 11:12:38