Re: Possible bug in ACPI

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Andrew Morton wrote:
On Sun, 03 Dec 2006 12:30:52 +0300
Alexey Starikovskiy <alexey.y.starikovskiy@xxxxxxxxxxxxxxx> wrote:

Andrew Morton wrote:
On Sun, 03 Dec 2006 12:06:54 +0300
Alexey Starikovskiy <alexey.y.starikovskiy@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxx>: 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.
--- Begin Message ---

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

--- End Message ---

[Index of Archives]     [Linux IBM ACPI]     [Linux Power Management]     [Linux Kernel]     [Linux Laptop]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux