Re: Possible bug in ACPI

Re: Possible bug in ACPI

[Andrew Morton]

cc added.

On Tue, 19 Sep 2006 23:47:24 +0200
Dalibor Straka <dast@panelnet.cz> wrote:

> Hello,
> 
> I am often running out of memory. It looks like an ACPI code is guilty:
> dast@lili:~$ grep -i acpi /proc/slabinfo 
> Acpi-Operand        3076   3127     64   59    1 : tunables  120   60 8 : slabdata     53     53      0
> Acpi-ParseExt         16     59     64   59    1 : tunables  120   60 8 : slabdata      1      1      0
> Acpi-Parse            76     92     40   92    1 : tunables  120   60 8 : slabdata      1      1      0
> Acpi-State        1644960 1644960     80   48    1 : tunables  120   60 8 : slabdata  34270  34270      0
> Acpi-Namespace      1177   1232     32  112    1 : tunables  120   60 8 : slabdata     11     11      0
> dast@lili:~$ free
> Mem:        899280     892472       6808          0      82212 77936
> -/+ buffers/cache:     732324     166956
> Swap:      2634620     243052    2404568
> dast@lili:~$ uname -a
> Linux lili 2.6.18-rc7 #1 SMP Sun Sep 17 15:01:00 CEST 2006 x86_64
> 
> 
> Actualy the Acpi-State's memory is increasing slowly in minutes:
> Acpi-State         16176  16176     80   48    1 : tunables  120   60 8 : slabdata    337    337      0
> Acpi-State         18816  18816     80   48    1 : tunables  120   60 8 : slabdata    392    392      0
> Acpi-State         19200  19200     80   48    1 : tunables  120   60 8 : slabdata    400    400      0
> Acpi-State         20160  20160     80   48    1 : tunables  120   60 8 : slabdata    420    420      0

Yes, that is a memory leak.

> I am not familiar with kernel sources, but i can do c pretty well.
> BTW: Bios says i have 1024MB, but kernel sees 899MB :-?. The system is
> pure HP nx6325. It happens with all the recent kernels .18-rc* .17.* and
> debian's distribution 2.6.17-1-amd64-k8-smp.
> 
> Please Cc: to me, I read lkml only when I have a good mood.

Re: Possible bug in ACPI

[Andrew Morton]

We appear to have a fatal memory leak in ACPI.  It's a shame this was
known about in the -rc series but not fixed then.

Dalibor, please raise a full and new report at bugzilla.kernel.org.

> On Sat, 2 Dec 2006 21:51:40 +0100 Dalibor Straka <dast@panelnet.cz> wrote:
> On Thu, Nov 09, 2006 at 02:04:16PM -0800, Andrew Morton wrote:
> > 
> > If this bug is still present in 2.6.19-rc5 could you please raise a report
> > at bugzilla.kernel.org?
> > 
> > Thanks.
> > 
> > On Tue, 19 Sep 2006 23:47:24 +0200
> > Dalibor Straka <dast@panelnet.cz> wrote:
> > 
> > > Hello,
> > > 
> > > I am often running out of memory. It looks like an ACPI code is guilty:
> > > dast@lili:~$ grep -i acpi /proc/slabinfo 
> > > Acpi-Operand        3076   3127     64   59    1 : tunables  120   60 8 : slabdata     53     53      0
> > > Acpi-ParseExt         16     59     64   59    1 : tunables  120   60 8 : slabdata      1      1      0
> > > Acpi-Parse            76     92     40   92    1 : tunables  120   60 8 : slabdata      1      1      0
> > > Acpi-State        1644960 1644960     80   48    1 : tunables  120   60 8 : slabdata  34270  34270      0
> > > Acpi-Namespace      1177   1232     32  112    1 : tunables  120   60 8 : slabdata     11     11      0
> > > dast@lili:~$ free
> > > Mem:        899280     892472       6808          0      82212 77936
> > > -/+ buffers/cache:     732324     166956
> > > Swap:      2634620     243052    2404568
> > > dast@lili:~$ uname -a
> > > Linux lili 2.6.18-rc7 #1 SMP Sun Sep 17 15:01:00 CEST 2006 x86_64
> > > 
> > > 
> > > Actualy the Acpi-State's memory is increasing slowly in minutes:
> > > Acpi-State         16176  16176     80   48    1 : tunables  120   60 8 : slabdata    337    337      0
> > > Acpi-State         18816  18816     80   48    1 : tunables  120   60 8 : slabdata    392    392      0
> > > Acpi-State         19200  19200     80   48    1 : tunables  120   60 8 : slabdata    400    400      0
> > > Acpi-State         20160  20160     80   48    1 : tunables  120   60 8 : slabdata    420    420      0
> > > 
> > > I am not familiar with kernel sources, but i can do c pretty well.
> > > BTW: Bios says i have 1024MB, but kernel sees 899MB :-?. The system is
> > > pure HP nx6325. It happens with all the recent kernels .18-rc* .17.* and
> > > debian's distribution 2.6.17-1-amd64-k8-smp.
> > > 
> > > Please Cc: to me, I read lkml only when I have a good mood.
> 
> 
> Hello,
> 
> the bug is present again in 2.6.19 but not in 19-rc5 and not in 19-rc6.
> The only thing changed in drivers/acpi/ is osl.c and procesor_perflib.c.
> --- ./processor_perflib.c       2006-09-20 05:42:06.000000000 +0200
> +++ /usr/src/linux-2.6.19/drivers/acpi/processor_perflib.c
> 2006-12-01 00:34:06.000000000 +0100
> @@ -83,10 +83,8 @@
>                 goto out;
>  
>         ppc = (unsigned int)pr->performance_platform_limit;
> -       if (!ppc)
> -               goto out;
>  
> -       if (ppc > pr->performance->state_count)
> +       if (ppc >= pr->performance->state_count)
>                 goto out;
>  
>         cpufreq_verify_within_limits(policy, 0,
> --- osl.c	2006-12-02 21:40:57.000000000 +0100
> +++ /usr/src/linux-2.6.19-rc6/drivers/acpi/osl.c	2006-11-18 13:38:01.000000000 +0100
> @@ -73,6 +73,7 @@
>  static acpi_osd_handler acpi_irq_handler;
>  static void *acpi_irq_context;
>  static struct workqueue_struct *kacpid_wq;
> +static struct workqueue_struct *kacpi_notify_wq;
>  
>  acpi_status acpi_os_initialize(void)
>  {
> @@ -91,8 +92,9 @@
>  		return AE_NULL_ENTRY;
>  	}
>  	kacpid_wq = create_singlethread_workqueue("kacpid");
> +	kacpi_notify_wq = create_singlethread_workqueue("kacpi_notify");
>  	BUG_ON(!kacpid_wq);
> -
> +	BUG_ON(!kacpi_notify_wq);
>  	return AE_OK;
>  }
>  
> @@ -104,7 +106,7 @@
>  	}
>  
>  	destroy_workqueue(kacpid_wq);
> -	destroy_workqueue(kacpid_notify_wq);
> +	destroy_workqueue(kacpi_notify_wq);
>  
>  	return AE_OK;
>  }
> @@ -567,10 +569,7 @@
>  
>  static void acpi_os_execute_deferred(void *context)
>  {
> -	struct acpi_os_dpc *dpc = NULL;
> -
> -
> -	dpc = (struct acpi_os_dpc *)context;
> +	struct acpi_os_dpc *dpc = (struct acpi_os_dpc *)context;
>  	if (!dpc) {
>  		printk(KERN_ERR PREFIX "Invalid (NULL) context\n");
>  		return;
> @@ -605,14 +604,12 @@
>  	struct acpi_os_dpc *dpc;
>  	struct work_struct *task;
>  
> -	ACPI_FUNCTION_TRACE("os_queue_for_execution");
> -
>  	ACPI_DEBUG_PRINT((ACPI_DB_EXEC,
>  			  "Scheduling function [%p(%p)] for deferred execution.\n",
>  			  function, context));
>  
>  	if (!function)
> -		return_ACPI_STATUS(AE_BAD_PARAMETER);
> +		return AE_BAD_PARAMETER;
>  
>  	/*
>  	 * Allocate/initialize DPC structure.  Note that this memory will be
> @@ -625,26 +622,20 @@
>  	 * from the same memory.
>  	 */
>  
> -	dpc =
> -	    kmalloc(sizeof(struct acpi_os_dpc) + sizeof(struct work_struct),
> -		    GFP_ATOMIC);
> +	dpc = kmalloc(sizeof(struct acpi_os_dpc) +
> +			sizeof(struct work_struct), GFP_ATOMIC);
>  	if (!dpc)
> -		return_ACPI_STATUS(AE_NO_MEMORY);
> -
> +		return AE_NO_MEMORY;
>  	dpc->function = function;
>  	dpc->context = context;
> -
>  	task = (void *)(dpc + 1);
>  	INIT_WORK(task, acpi_os_execute_deferred, (void *)dpc);
> -
> -	if (!queue_work(kacpid_wq, task)) {
> -		ACPI_DEBUG_PRINT((ACPI_DB_ERROR,
> -				  "Call to queue_work() failed.\n"));
> -		kfree(dpc);
> +	if (!queue_work((type == OSL_NOTIFY_HANDLER)?
> +			kacpi_notify_wq : kacpid_wq, task)) {
>  		status = AE_ERROR;
> +		kfree(dpc);
>  	}
> -
> -	return_ACPI_STATUS(status);
> +	return status;
>  }
>  
>  EXPORT_SYMBOL(acpi_os_execute);

Re: Possible bug in ACPI

[Alexey Starikovskiy]

Andrew,
This is a patch reverted by Linus from rc6-git2 because it broke his 
Compaq n620c, it refers to #5534 bug. Basically, kacpid deadlocks on 
some new HP notebooks, and all incoming requests would be queued until 
memory is over if this patch is not applied. On a bright side -- it's 
not a memory leak...
Patch, which works for Linus laptop and "looks acceptable" to Linus is 
the last in #5534 list.

Regards,
    Alex.

Andrew Morton wrote:
> We appear to have a fatal memory leak in ACPI.  It's a shame this was
> known about in the -rc series but not fixed then.
>
> Dalibor, please raise a full and new report at bugzilla.kernel.org.
>
>   
>> On Sat, 2 Dec 2006 21:51:40 +0100 Dalibor Straka <dast@panelnet.cz> wrote:
>> On Thu, Nov 09, 2006 at 02:04:16PM -0800, Andrew Morton wrote:
>>     
>>> If this bug is still present in 2.6.19-rc5 could you please raise a report
>>> at bugzilla.kernel.org?
>>>
>>> Thanks.
>>>
>>> On Tue, 19 Sep 2006 23:47:24 +0200
>>> Dalibor Straka <dast@panelnet.cz> wrote:
>>>
>>>       
>>>> Hello,
>>>>
>>>> I am often running out of memory. It looks like an ACPI code is guilty:
>>>> dast@lili:~$ grep -i acpi /proc/slabinfo 
>>>> Acpi-Operand        3076   3127     64   59    1 : tunables  120   60 8 : slabdata     53     53      0
>>>> Acpi-ParseExt         16     59     64   59    1 : tunables  120   60 8 : slabdata      1      1      0
>>>> Acpi-Parse            76     92     40   92    1 : tunables  120   60 8 : slabdata      1      1      0
>>>> Acpi-State        1644960 1644960     80   48    1 : tunables  120   60 8 : slabdata  34270  34270      0
>>>> Acpi-Namespace      1177   1232     32  112    1 : tunables  120   60 8 : slabdata     11     11      0
>>>> dast@lili:~$ free
>>>> Mem:        899280     892472       6808          0      82212 77936
>>>> -/+ buffers/cache:     732324     166956
>>>> Swap:      2634620     243052    2404568
>>>> dast@lili:~$ uname -a
>>>> Linux lili 2.6.18-rc7 #1 SMP Sun Sep 17 15:01:00 CEST 2006 x86_64
>>>>
>>>>
>>>> Actualy the Acpi-State's memory is increasing slowly in minutes:
>>>> Acpi-State         16176  16176     80   48    1 : tunables  120   60 8 : slabdata    337    337      0
>>>> Acpi-State         18816  18816     80   48    1 : tunables  120   60 8 : slabdata    392    392      0
>>>> Acpi-State         19200  19200     80   48    1 : tunables  120   60 8 : slabdata    400    400      0
>>>> Acpi-State         20160  20160     80   48    1 : tunables  120   60 8 : slabdata    420    420      0
>>>>
>>>> I am not familiar with kernel sources, but i can do c pretty well.
>>>> BTW: Bios says i have 1024MB, but kernel sees 899MB :-?. The system is
>>>> pure HP nx6325. It happens with all the recent kernels .18-rc* .17.* and
>>>> debian's distribution 2.6.17-1-amd64-k8-smp.
>>>>
>>>> Please Cc: to me, I read lkml only when I have a good mood.
>>>>         
>> Hello,
>>
>> the bug is present again in 2.6.19 but not in 19-rc5 and not in 19-rc6.
>> The only thing changed in drivers/acpi/ is osl.c and procesor_perflib.c.
>> --- ./processor_perflib.c       2006-09-20 05:42:06.000000000 +0200
>> +++ /usr/src/linux-2.6.19/drivers/acpi/processor_perflib.c
>> 2006-12-01 00:34:06.000000000 +0100
>> @@ -83,10 +83,8 @@
>>                 goto out;
>>  
>>         ppc = (unsigned int)pr->performance_platform_limit;
>> -       if (!ppc)
>> -               goto out;
>>  
>> -       if (ppc > pr->performance->state_count)
>> +       if (ppc >= pr->performance->state_count)
>>                 goto out;
>>  
>>         cpufreq_verify_within_limits(policy, 0,
>> --- osl.c	2006-12-02 21:40:57.000000000 +0100
>> +++ /usr/src/linux-2.6.19-rc6/drivers/acpi/osl.c	2006-11-18 13:38:01.000000000 +0100
>> @@ -73,6 +73,7 @@
>>  static acpi_osd_handler acpi_irq_handler;
>>  static void *acpi_irq_context;
>>  static struct workqueue_struct *kacpid_wq;
>> +static struct workqueue_struct *kacpi_notify_wq;
>>  
>>  acpi_status acpi_os_initialize(void)
>>  {
>> @@ -91,8 +92,9 @@
>>  		return AE_NULL_ENTRY;
>>  	}
>>  	kacpid_wq = create_singlethread_workqueue("kacpid");
>> +	kacpi_notify_wq = create_singlethread_workqueue("kacpi_notify");
>>  	BUG_ON(!kacpid_wq);
>> -
>> +	BUG_ON(!kacpi_notify_wq);
>>  	return AE_OK;
>>  }
>>  
>> @@ -104,7 +106,7 @@
>>  	}
>>  
>>  	destroy_workqueue(kacpid_wq);
>> -	destroy_workqueue(kacpid_notify_wq);
>> +	destroy_workqueue(kacpi_notify_wq);
>>  
>>  	return AE_OK;
>>  }
>> @@ -567,10 +569,7 @@
>>  
>>  static void acpi_os_execute_deferred(void *context)
>>  {
>> -	struct acpi_os_dpc *dpc = NULL;
>> -
>> -
>> -	dpc = (struct acpi_os_dpc *)context;
>> +	struct acpi_os_dpc *dpc = (struct acpi_os_dpc *)context;
>>  	if (!dpc) {
>>  		printk(KERN_ERR PREFIX "Invalid (NULL) context\n");
>>  		return;
>> @@ -605,14 +604,12 @@
>>  	struct acpi_os_dpc *dpc;
>>  	struct work_struct *task;
>>  
>> -	ACPI_FUNCTION_TRACE("os_queue_for_execution");
>> -
>>  	ACPI_DEBUG_PRINT((ACPI_DB_EXEC,
>>  			  "Scheduling function [%p(%p)] for deferred execution.\n",
>>  			  function, context));
>>  
>>  	if (!function)
>> -		return_ACPI_STATUS(AE_BAD_PARAMETER);
>> +		return AE_BAD_PARAMETER;
>>  
>>  	/*
>>  	 * Allocate/initialize DPC structure.  Note that this memory will be
>> @@ -625,26 +622,20 @@
>>  	 * from the same memory.
>>  	 */
>>  
>> -	dpc =
>> -	    kmalloc(sizeof(struct acpi_os_dpc) + sizeof(struct work_struct),
>> -		    GFP_ATOMIC);
>> +	dpc = kmalloc(sizeof(struct acpi_os_dpc) +
>> +			sizeof(struct work_struct), GFP_ATOMIC);
>>  	if (!dpc)
>> -		return_ACPI_STATUS(AE_NO_MEMORY);
>> -
>> +		return AE_NO_MEMORY;
>>  	dpc->function = function;
>>  	dpc->context = context;
>> -
>>  	task = (void *)(dpc + 1);
>>  	INIT_WORK(task, acpi_os_execute_deferred, (void *)dpc);
>> -
>> -	if (!queue_work(kacpid_wq, task)) {
>> -		ACPI_DEBUG_PRINT((ACPI_DB_ERROR,
>> -				  "Call to queue_work() failed.\n"));
>> -		kfree(dpc);
>> +	if (!queue_work((type == OSL_NOTIFY_HANDLER)?
>> +			kacpi_notify_wq : kacpid_wq, task)) {
>>  		status = AE_ERROR;
>> +		kfree(dpc);
>>  	}
>> -
>> -	return_ACPI_STATUS(status);
>> +	return status;
>>  }
>>  
>>  EXPORT_SYMBOL(acpi_os_execute);
>>     
> -
> 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: Possible bug in ACPI

[Alexey Starikovskiy]

Andrew Morton wrote:
> On Sun, 03 Dec 2006 12:06:54 +0300
> Alexey Starikovskiy <alexey.y.starikovskiy@linux.intel.com> wrote:
>
>   
>> This is a patch reverted by Linus from rc6-git2 because it broke his 
>> Compaq n620c, it refers to #5534 bug. Basically, kacpid deadlocks on 
>> some new HP notebooks, and all incoming requests would be queued until 
>> memory is over if this patch is not applied. On a bright side -- it's 
>> not a memory leak...
>> Patch, which works for Linus laptop and "looks acceptable" to Linus is 
>> the last in #5534 list.
>>     
>
> hm, if you say so.
I forwarded Linus' mail to you...
>   The description in that patch is nowhere near complete
> enough for me to be able to work out what it does.
>
>   
Will update.

> The sys_sched_yield() is particularly incomprehensible and needs good
> commenting.  You are, I hope, aware of the severe problems which yield()
> causes when the system is busy?  The process which calls it will get
> practically no CPU at all.
>
>   
On Linus' machine, as soon as we execute deferred work, it's GPE becomes 
enabled again and BIOS sends us a new event.
So kacpid is always ready to run, while kacpi_notify don't have a chance 
to run. sys_sched_yield() was added to
give kacpi_notify a chance to run.
I was thinking about lowering the priority of kacpid, is it better?
> Minor point: that patch has several unneded (and undesirable) casts of void*:
>
> +static void acpi_os_execute_notify(void *context)
> +{
> +	struct acpi_os_dpc *dpc = (struct acpi_os_dpc *)context;
> 	
> please remove those.
>   

Re: Possible bug in ACPI

[Andrew Morton]

On Sun, 03 Dec 2006 12:06:54 +0300
Alexey Starikovskiy <alexey.y.starikovskiy@linux.intel.com> wrote:

> This is a patch reverted by Linus from rc6-git2 because it broke his 
> Compaq n620c, it refers to #5534 bug. Basically, kacpid deadlocks on 
> some new HP notebooks, and all incoming requests would be queued until 
> memory is over if this patch is not applied. On a bright side -- it's 
> not a memory leak...
> Patch, which works for Linus laptop and "looks acceptable" to Linus is 
> the last in #5534 list.

hm, if you say so.  The description in that patch is nowhere near complete
enough for me to be able to work out what it does.

The sys_sched_yield() is particularly incomprehensible and needs good
commenting.  You are, I hope, aware of the severe problems which yield()
causes when the system is busy?  The process which calls it will get
practically no CPU at all.

Minor point: that patch has several unneded (and undesirable) casts of void*:

+static void acpi_os_execute_notify(void *context)
+{
+	struct acpi_os_dpc *dpc = (struct acpi_os_dpc *)context;
	
please remove those.

Re: Possible bug in ACPI

[Andrew Morton]

On Sun, 03 Dec 2006 12:30:52 +0300
Alexey Starikovskiy <alexey.y.starikovskiy@linux.intel.com> wrote:

> Andrew Morton wrote:
> > On Sun, 03 Dec 2006 12:06:54 +0300
> > Alexey Starikovskiy <alexey.y.starikovskiy@linux.intel.com> wrote:
> >
> >   
> >> This is a patch reverted by Linus from rc6-git2 because it broke his 
> >> Compaq n620c, it refers to #5534 bug. Basically, kacpid deadlocks on 
> >> some new HP notebooks, and all incoming requests would be queued until 
> >> memory is over if this patch is not applied. On a bright side -- it's 
> >> not a memory leak...
> >> Patch, which works for Linus laptop and "looks acceptable" to Linus is 
> >> the last in #5534 list.
> >>     
> >
> > hm, if you say so.
> I forwarded Linus' mail to you...

I didn't receive it.

> >   The description in that patch is nowhere near complete
> > enough for me to be able to work out what it does.
> >
> >   
> Will update.
> 
> > The sys_sched_yield() is particularly incomprehensible and needs good
> > commenting.  You are, I hope, aware of the severe problems which yield()
> > causes when the system is busy?  The process which calls it will get
> > practically no CPU at all.
> >
> >   
> On Linus' machine, as soon as we execute deferred work, it's GPE becomes 
> enabled again and BIOS sends us a new event.
> So kacpid is always ready to run, while kacpi_notify don't have a chance 
> to run. sys_sched_yield() was added to
> give kacpi_notify a chance to run.
> I was thinking about lowering the priority of kacpid, is it better?

It all sounds horridly hacky, but I don't understand the problem well
enough to be able to recommend any solutions.  That's why I was hoping for
a decent description of the patch.

How does it relate to this? http://lkml.org/lkml/2006/8/8/336

> > Minor point: that patch has several unneded (and undesirable) casts of void*:
> >
> > +static void acpi_os_execute_notify(void *context)
> > +{
> > +	struct acpi_os_dpc *dpc = (struct acpi_os_dpc *)context;
> > 	
> > please remove those.
> >   

I'll take that as an "OK" ;)

Re: Possible bug in ACPI

[Alexey Starikovskiy]

[-- Attachment #1: Type: text/plain, Size: 5521 bytes --]

Andrew Morton wrote:
> On Sun, 03 Dec 2006 12:30:52 +0300
> Alexey Starikovskiy <alexey.y.starikovskiy@linux.intel.com> wrote:
>
>   
>> Andrew Morton wrote:
>>     
>>> On Sun, 03 Dec 2006 12:06:54 +0300
>>> Alexey Starikovskiy <alexey.y.starikovskiy@linux.intel.com> wrote:
>>>
>>>   
>>>       
>>>> This is a patch reverted by Linus from rc6-git2 because it broke his 
>>>> Compaq n620c, it refers to #5534 bug. Basically, kacpid deadlocks on 
>>>> some new HP notebooks, and all incoming requests would be queued until 
>>>> memory is over if this patch is not applied. On a bright side -- it's 
>>>> not a memory leak...
>>>> Patch, which works for Linus laptop and "looks acceptable" to Linus is 
>>>> the last in #5534 list.
>>>>     
>>>>         
>>> hm, if you say so.
>>>       
>> I forwarded Linus' mail to you...
>>     
>
> I didn't receive it.
>   
Please see attached.
>   
>>>   The description in that patch is nowhere near complete
>>> enough for me to be able to work out what it does.
>>>
>>>   
>>>       
>> Will update.
>>
>>     
>>> The sys_sched_yield() is particularly incomprehensible and needs good
>>> commenting.  You are, I hope, aware of the severe problems which yield()
>>> causes when the system is busy?  The process which calls it will get
>>> practically no CPU at all.
>>>
>>>   
>>>       
>> On Linus' machine, as soon as we execute deferred work, it's GPE becomes 
>> enabled again and BIOS sends us a new event.
>> So kacpid is always ready to run, while kacpi_notify don't have a chance 
>> to run. sys_sched_yield() was added to
>> give kacpi_notify a chance to run.
>> I was thinking about lowering the priority of kacpid, is it better?
>>     
>
> It all sounds horridly hacky, but I don't understand the problem well
> enough to be able to recommend any solutions.  That's why I was hoping for
> a decent description of the patch.
>
>   
HP nx6125/nx6325/... machines have a _GPE handler with an infinite loop 
sending Notify() events to different ACPI subsystems.
Notify handler in ACPI driver is a C-routine, which may call ACPI 
interpreter again to get access to some ACPI variables (acpi_evaluate_xxx).
On these HP machines such an evaluation changes state of some variable 
and lets the loop above break.
In the current ACPI implementation Notify requests are being deferred to 
the same kacpid workqueue on which the above GPE handler with infinite 
loop is executing. Thus we have a deadlock -- loop will continue to 
spin, sending notify events, and at the same time preventing these 
notify events from being run on a workqueue. All notify events are 
deferred, thus we see increase in memory consumption noticed by author 
of the thread. Also as GPE handling is bloked, machines overheat. 
Eventually by external poll of the same acpi_evaluate, kacpid is 
released and all the queued notify events are free to run, thus 100% cpu 
utilization by kacpid for several seconds or more.

To prevent all these horrors it's needed to not put notify events to 
kacpid workqueue by either executing them immediately or putting them on 
some other thread.
It's dangerous to execute notify events in place, as it will put several 
ACPI interpreter stacks on top of each other (at least 4 in case of 
nx6125), thus causing kernel  stack overflow.
First attempt to create a new thread was done by /Peter Wainwright 
<mailto:prw@ceiriog.eclipse.co.uk>: he created a bunch of threads which 
were stealing work from a kacpid workqueue.
This patch appeared in 2.6.15 kernel shipped with Ubuntu 6.06 LTS.
Second attempt was done by me,  I  created a new  thread for each Notify 
event. This worked OK on HP nx machines, but broke Linus' Compaq n620c, 
by producing threads with a speed what they stopped the machine 
completely. Thus this patch was reverted from 18-rc2 as I remember.
I re-made the patch to create second workqueue just for notify events, 
thus hopping it will not break Linus' machine. Patch was tested on the 
same HP nx machines in #5534 and #7122, but I did not received reply 
from Linus on a test patch sent to him.
Patch went to 19-rc and was rejected with much fanfare again.
There was 4th patch, which inserted schedule_timeout(1) into deferred 
execution of kacpid if we had any notify requests pending, but Linus 
decided that it was too complex (involved either changes to workqueue to 
see if it's empty or atomic inc/dec).
Now you see last variant which adds yield() to every GPE execution.
/
> How does it relate to this? http://lkml.org/lkml/2006/8/8/336
>
>   
In both cases GPE (General Purpose Events) are involved.
There are two types of GPE -- level and edge triggered, they are 
described as _Lxx or _Exx methods under _GPE namespace of ACPI 
interpreter (DSDT). Both should be disabled, then we receive them until 
they are handled by methods above. Level should be cleared after 
execution of handler, while Edge should be cleared immediately. In this 
post DSDT was written with error and edge method was described as level, 
thus requiring changing clearing of level events as soon as edge ones. 
The solution for this problem is changing DSDT to declare method in 
question as _Exx and not _Lxx.
>>> Minor point: that patch has several unneded (and undesirable) casts of void*:
>>>
>>> +static void acpi_os_execute_notify(void *context)
>>> +{
>>> +	struct acpi_os_dpc *dpc = (struct acpi_os_dpc *)context;
>>> 	
>>> please remove those.
>>>   
>>>       
>
> I'll take that as an "OK" ;)
>   
Sorry, yes, OK. :)

Regards,
    Alex.

[-- Attachment #2: Attached Message --]
[-- Type: message/rfc822, Size: 3119 bytes --]

From: Linus Torvalds <torvalds@osdl.org>
To: Alexey Starikovskiy <alexey.y.starikovskiy@linux.intel.com>
Subject: Re: ACPI breakage (Re: 2.6.19-rc6: known regressions (v2))
Date: Mon, 20 Nov 2006 10:07:39 -0800 (PST)
Message-ID: <Pine.LNX.4.64.0611201003540.3692@woody.osdl.org>



On Sun, 19 Nov 2006, Alexey Starikovskiy wrote:
>
> I agree to all your comments with one exception, please see below. Attached is
> the reworked patch against latest git. Please test.

Ok, this one works for me too, and looks much simpler.

> Linus Torvalds wrote:
> > And we might as well do it when we add an entry to the _deferred_ queue, no? 
>   
> acpi_os_execute() is called from interrupt context for insertion into
> _deferred_ queue, so it's not possible to yield in it, no?

Hmm. Yes. Anyway, the new patch looks acceptable, and certainly much 
simpler than trying to count events.

It probably causes tons of new unnecessary scheduling events, but I doubt 
we really care.

That said, what we _really_ want here is a "priority queue" for the 
events, and some way to put an event back on the queue while running it 
(eg ACPI "Sleep" event). But I guess the ACPI interpreter isn't done that 
way (ie you can't just push and pop ACPI state).

		Linus

Re: Possible bug in ACPI

[Alexey Starikovskiy]

Andrew,

Would it be better to replace sys_sched_yield() with cond_resched()?

Thanks in advance,
    Alex.


Alexey Starikovskiy wrote:
> Andrew Morton wrote:
>> On Sun, 03 Dec 2006 12:30:52 +0300
>> Alexey Starikovskiy <alexey.y.starikovskiy@linux.intel.com> wrote:
>>
>>  
>>> Andrew Morton wrote:
>>>    
>>>> On Sun, 03 Dec 2006 12:06:54 +0300
>>>> Alexey Starikovskiy <alexey.y.starikovskiy@linux.intel.com> wrote:
>>>>
>>>>        
>>>>> This is a patch reverted by Linus from rc6-git2 because it broke 
>>>>> his Compaq n620c, it refers to #5534 bug. Basically, kacpid 
>>>>> deadlocks on some new HP notebooks, and all incoming requests 
>>>>> would be queued until memory is over if this patch is not applied. 
>>>>> On a bright side -- it's not a memory leak...
>>>>> Patch, which works for Linus laptop and "looks acceptable" to 
>>>>> Linus is the last in #5534 list.
>>>>>             
>>>> hm, if you say so.
>>>>       
>>> I forwarded Linus' mail to you...
>>>     
>>
>> I didn't receive it.
>>   
> Please see attached.
>>  
>>>>   The description in that patch is nowhere near complete
>>>> enough for me to be able to work out what it does.
>>>>
>>>>         
>>> Will update.
>>>
>>>    
>>>> The sys_sched_yield() is particularly incomprehensible and needs good
>>>> commenting.  You are, I hope, aware of the severe problems which 
>>>> yield()
>>>> causes when the system is busy?  The process which calls it will get
>>>> practically no CPU at all.
>>>>
>>>>         
>>> On Linus' machine, as soon as we execute deferred work, it's GPE 
>>> becomes enabled again and BIOS sends us a new event.
>>> So kacpid is always ready to run, while kacpi_notify don't have a 
>>> chance to run. sys_sched_yield() was added to
>>> give kacpi_notify a chance to run.
>>> I was thinking about lowering the priority of kacpid, is it better?
>>>     
>>
>> It all sounds horridly hacky, but I don't understand the problem well
>> enough to be able to recommend any solutions.  That's why I was 
>> hoping for
>> a decent description of the patch.
>>
>>   
> HP nx6125/nx6325/... machines have a _GPE handler with an infinite 
> loop sending Notify() events to different ACPI subsystems.
> Notify handler in ACPI driver is a C-routine, which may call ACPI 
> interpreter again to get access to some ACPI variables 
> (acpi_evaluate_xxx).
> On these HP machines such an evaluation changes state of some variable 
> and lets the loop above break.
> In the current ACPI implementation Notify requests are being deferred 
> to the same kacpid workqueue on which the above GPE handler with 
> infinite loop is executing. Thus we have a deadlock -- loop will 
> continue to spin, sending notify events, and at the same time 
> preventing these notify events from being run on a workqueue. All 
> notify events are deferred, thus we see increase in memory consumption 
> noticed by author of the thread. Also as GPE handling is bloked, 
> machines overheat. Eventually by external poll of the same 
> acpi_evaluate, kacpid is released and all the queued notify events are 
> free to run, thus 100% cpu utilization by kacpid for several seconds 
> or more.
>
> To prevent all these horrors it's needed to not put notify events to 
> kacpid workqueue by either executing them immediately or putting them 
> on some other thread.
> It's dangerous to execute notify events in place, as it will put 
> several ACPI interpreter stacks on top of each other (at least 4 in 
> case of nx6125), thus causing kernel  stack overflow.
> First attempt to create a new thread was done by /Peter Wainwright 
> <mailto:prw@ceiriog.eclipse.co.uk>: he created a bunch of threads 
> which were stealing work from a kacpid workqueue.
> This patch appeared in 2.6.15 kernel shipped with Ubuntu 6.06 LTS.
> Second attempt was done by me,  I  created a new  thread for each 
> Notify event. This worked OK on HP nx machines, but broke Linus' 
> Compaq n620c, by producing threads with a speed what they stopped the 
> machine completely. Thus this patch was reverted from 18-rc2 as I 
> remember.
> I re-made the patch to create second workqueue just for notify events, 
> thus hopping it will not break Linus' machine. Patch was tested on the 
> same HP nx machines in #5534 and #7122, but I did not received reply 
> from Linus on a test patch sent to him.
> Patch went to 19-rc and was rejected with much fanfare again.
> There was 4th patch, which inserted schedule_timeout(1) into deferred 
> execution of kacpid if we had any notify requests pending, but Linus 
> decided that it was too complex (involved either changes to workqueue 
> to see if it's empty or atomic inc/dec).
> Now you see last variant which adds yield() to every GPE execution.
> /
>> How does it relate to this? http://lkml.org/lkml/2006/8/8/336
>>
>>   
> In both cases GPE (General Purpose Events) are involved.
> There are two types of GPE -- level and edge triggered, they are 
> described as _Lxx or _Exx methods under _GPE namespace of ACPI 
> interpreter (DSDT). Both should be disabled, then we receive them 
> until they are handled by methods above. Level should be cleared after 
> execution of handler, while Edge should be cleared immediately. In 
> this post DSDT was written with error and edge method was described as 
> level, thus requiring changing clearing of level events as soon as 
> edge ones. The solution for this problem is changing DSDT to declare 
> method in question as _Exx and not _Lxx.
>>>> Minor point: that patch has several unneded (and undesirable) casts 
>>>> of void*:
>>>>
>>>> +static void acpi_os_execute_notify(void *context)
>>>> +{
>>>> +    struct acpi_os_dpc *dpc = (struct acpi_os_dpc *)context;
>>>>     
>>>> please remove those.
>>>>         
>>
>> I'll take that as an "OK" ;)
>>   
> Sorry, yes, OK. :)
>
> Regards,
>    Alex.
>
> ------------------------------------------------------------------------
>
> Subject:
> Re: ACPI breakage (Re: 2.6.19-rc6: known regressions (v2))
> From:
> Linus Torvalds <torvalds@osdl.org>
> Date:
> Mon, 20 Nov 2006 10:07:39 -0800 (PST)
> To:
> Alexey Starikovskiy <alexey.y.starikovskiy@linux.intel.com>
>
> To:
> Alexey Starikovskiy <alexey.y.starikovskiy@linux.intel.com>
>
>
> On Sun, 19 Nov 2006, Alexey Starikovskiy wrote:
>   
>> I agree to all your comments with one exception, please see below. Attached is
>> the reworked patch against latest git. Please test.
>>     
>
> Ok, this one works for me too, and looks much simpler.
>
>   
>> Linus Torvalds wrote:
>>     
>>> And we might as well do it when we add an entry to the _deferred_ queue, no? 
>>>       
>>   
>> acpi_os_execute() is called from interrupt context for insertion into
>> _deferred_ queue, so it's not possible to yield in it, no?
>>     
>
> Hmm. Yes. Anyway, the new patch looks acceptable, and certainly much 
> simpler than trying to count events.
>
> It probably causes tons of new unnecessary scheduling events, but I doubt 
> we really care.
>
> That said, what we _really_ want here is a "priority queue" for the 
> events, and some way to put an event back on the queue while running it 
> (eg ACPI "Sleep" event). But I guess the ACPI interpreter isn't done that 
> way (ie you can't just push and pop ACPI state).
>
> 		Linus
>   

Re: Possible bug in ACPI

[Andrew Morton]

On Wed, 06 Dec 2006 18:36:04 +0300
Alexey Starikovskiy <alexey.y.starikovskiy@linux.intel.com> wrote:

> Would it be better to replace sys_sched_yield() with cond_resched()?

I don't know, because I don't understand the dynamics of the proposed
change at all.

Again:

- How does it relate to http://lkml.org/lkml/2006/8/8/336

- Can we please have a *full* description of the problem and of the proposed
  solution?

Re: Possible bug in ACPI

[Alexey Starikovskiy]

Andrew,

Full description was sent to you and linux-acpi mail-list. I received 
copy from mail-list, so if you didn't, you can browse linux-acpi archive 
of Dec 4.
Also patch in #5534 was updated with the full description.
If you know, why my mails don't reach you, please advise...

Thanks,
    Alex.

Andrew Morton wrote:
> On Wed, 06 Dec 2006 18:36:04 +0300
> Alexey Starikovskiy <alexey.y.starikovskiy@linux.intel.com> wrote:
>
>   
>> Would it be better to replace sys_sched_yield() with cond_resched()?
>>     
>
> I don't know, because I don't understand the dynamics of the proposed
> change at all.
>
> Again:
>
> - How does it relate to http://lkml.org/lkml/2006/8/8/336
>
> - Can we please have a *full* description of the problem and of the proposed
>   solution?
> -
> 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: Possible bug in ACPI

[Alexey Starikovskiy]

Andrew Morton wrote:
> On Wed, 06 Dec 2006 18:36:04 +0300
> Alexey Starikovskiy <alexey.y.starikovskiy@linux.intel.com> wrote:
>
>   
>> Would it be better to replace sys_sched_yield() with cond_resched()?
>>     
>
> I don't know, because I don't understand the dynamics of the proposed
> change at all.
>
> Again:
>
> - How does it relate to http://lkml.org/lkml/2006/8/8/336
>
> - Can we please have a *full* description of the problem and of the proposed
>   solution?
>   

Hmm. There was *full* description in the part you just cut off.

Regards,
    Alex.