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

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

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

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.Finger@lwfinger.net>
Signed-off-by: Xiaotian Feng <dfeng@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

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

[Zhang Rui]

CC Alexey.

And this is also the fix for
http://bugzilla.kernel.org/show_bug.cgi?id=14483

thanks,
rui

On Fri, 2009-12-04 at 12:26 +0800, 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.
> 
> Reported-and-bisected-by: Larry Finger <Larry.Finger@lwfinger.net>
> Signed-off-by: Xiaotian Feng <dfeng@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)
>  

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

[Zhang Rui]

On Fri, 2009-12-04 at 13:36 +0800, Zhang Rui wrote:
> CC Alexey.
> 
> And this is also the fix for
> http://bugzilla.kernel.org/show_bug.cgi?id=14483
> 
> thanks,
> rui
> 
> On Fri, 2009-12-04 at 12:26 +0800, 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.
> > 
> > Reported-and-bisected-by: Larry Finger <Larry.Finger@lwfinger.net>
> > Signed-off-by: Xiaotian Feng <dfeng@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)
> >  
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Justin Mattock]

On Thu, Dec 3, 2009 at 8:26 PM, Xiaotian Feng <dfeng@redhat.com> 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 <Larry.Finger@lwfinger.net>
> Signed-off-by: Xiaotian Feng <dfeng@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 majordomo@vger.kernel.org
> 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 <justinmattock@gmail.com>

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

-- 
Justin P. Mattock

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

[Danny Feng]

On 12/04/2009 02:50 PM, Justin Mattock wrote:
> On Thu, Dec 3, 2009 at 8:26 PM, Xiaotian Feng<dfeng@redhat.com>  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<Larry.Finger@lwfinger.net>
>> Signed-off-by: Xiaotian Feng<dfeng@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 majordomo@vger.kernel.org
>> 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<justinmattock@gmail.com>
>
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.
>

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

[Justin P. Mattock]

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<dfeng@redhat.com> 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<Larry.Finger@lwfinger.net>
>>> Signed-off-by: Xiaotian Feng<dfeng@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 majordomo@vger.kernel.org
>>> 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<justinmattock@gmail.com>
>>
> 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

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

[Xiaotian Feng]

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<dfeng@redhat.com> 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<Larry.Finger@lwfinger.net>
>>>> Signed-off-by: Xiaotian Feng<dfeng@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 majordomo@vger.kernel.org
>>>> 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<justinmattock@gmail.com>
>>>
>> 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
>

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

[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
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

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

[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 <Larry.Finger@lwfinger.net>
Reported-and-bisected-by: Justin P. Mattock <justinmattock@gmail.com>
Signed-off-by: Xiaotian Feng <dfeng@redhat.com>
Cc: Len Brown <lenb@kernel.org>
Cc: Bob Moore <robert.moore@intel.com>
Cc: Lin Ming <ming.m.lin@intel.com>
Cc: Alexey Starikovskiy <astarikovskiy@suse.de>
Cc: Pavel Machek <pavel@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

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

[Alexey Starikovskiy]

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 <Larry.Finger@lwfinger.net>
> Reported-and-bisected-by: Justin P. Mattock <justinmattock@gmail.com>
> Signed-off-by: Xiaotian Feng <dfeng@redhat.com>
> Cc: Len Brown <lenb@kernel.org>
> Cc: Bob Moore <robert.moore@intel.com>
> Cc: Lin Ming <ming.m.lin@intel.com>
> Cc: Alexey Starikovskiy <astarikovskiy@suse.de>
> Cc: Pavel Machek <pavel@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)
>  

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

[Alexey Starikovskiy]

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
>   

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.

(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

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

[Alexey Starikovskiy]

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.

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

[Justin P. Mattock]

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 majordomo@vger.kernel.org
> 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

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

[Moore, Robert]

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:justinmattock@gmail.com]
>Sent: Thursday, December 10, 2009 2:46 PM
>To: Alexey Starikovskiy
>Cc: Pavel Machek; Xiaotian Feng; lenb@kernel.org; Lin, Ming M; Moore,
>Robert; linux-acpi@vger.kernel.org; linux-kernel@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 пишет:
>>> 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 majordomo@vger.kernel.org
>> 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

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.

#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 <Larry.Finger@lwfinger.net>
> > Reported-and-bisected-by: Justin P. Mattock <justinmattock@gmail.com>
> > Signed-off-by: Xiaotian Feng <dfeng@redhat.com>
> > Cc: Len Brown <lenb@kernel.org>
> > Cc: Bob Moore <robert.moore@intel.com>
> > Cc: Lin Ming <ming.m.lin@intel.com>
> > Cc: Alexey Starikovskiy <astarikovskiy@suse.de>
> > Cc: Pavel Machek <pavel@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)
> >  
> 

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

[Alexey Starikovskiy]

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.

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
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

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

[Alexey Starikovskiy]

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.

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

[Pavel Machek]

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

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

[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

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

[Xiaotian Feng]

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

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

[Alexey Starikovskiy]

Xiaotian Feng пишет:
> 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?
We should not do anything complex here, you are right.
Consider me ACK your patch.

Thanks,
Alex

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

[Len Brown]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 706 bytes --]

On Mon, 28 Dec 2009, Alexey Starikovskiy wrote:

> 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

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

[Len Brown]

On Thu, 10 Dec 2009, Moore, Robert wrote:

> 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