Re: APM driver patch summary

Re: APM driver patch summary

[Thomas Hood]

On Sat, 2001-12-22 at 09:44, Andreas Steinmetz wrote:
> 1. There is now a module parameter apm-idle-threshold which
> allows to override the compiled in idle percentage threshold
> above which BIOS idle calls are done.

Andrej, your patch doesn't work when compiled as a module
because of a name mismatch.

I went in and cleaned the patch up a bit.  Now there is only
one extra parameter, called "idle_threshold", which you can
set to 100 if you want to disable use of APM BIOS idling.

I have combined this tweaked idle patch with the
notification patch and made it available here:
   http://panopticon.csustan.edu/thood/apm.html
Patch is against 2.4.17.

I hope lots of people will test it.  It's working fine for me.

--
Thomas Hood

Re: APM driver patch summary

[Andreas Steinmetz]

Fine with me,
as I always compile APM into the kernel I just didn't see it (mental note to
self: always try module build before submitting).
I'll get the combined patch and test it.

On 23-Dec-2001 Thomas Hood wrote:
> On Sat, 2001-12-22 at 09:44, Andreas Steinmetz wrote:
>> 1. There is now a module parameter apm-idle-threshold which
>> allows to override the compiled in idle percentage threshold
>> above which BIOS idle calls are done.
> 
> Andrej, your patch doesn't work when compiled as a module
> because of a name mismatch.
> 
> I went in and cleaned the patch up a bit.  Now there is only
> one extra parameter, called "idle_threshold", which you can
> set to 100 if you want to disable use of APM BIOS idling.
> 
> I have combined this tweaked idle patch with the
> notification patch and made it available here:
>    http://panopticon.csustan.edu/thood/apm.html
> Patch is against 2.4.17.
> 
> I hope lots of people will test it.  It's working fine for me.
> 
> --
> Thomas Hood
> 
> 
> 

Andreas Steinmetz
D.O.M. Datenverarbeitung GmbH

APM driver patch okay?

[Thomas Hood]

Hi Stephen 

Just a note to say that the patch combining "idling"
and "notification order" changes is looking good so far.
   http://panopticon.csustan.edu/thood/apm.html
(No one has complained, anyway.)  I think it's ready
for more extensive testing.  Should it go into a 2.4.X-preY
kernel?

-- 
Thomas

Re: APM driver patch okay?

[Stephen Rothwell]

Hi Thomas,

On 30 Dec 2001 15:05:54 -0500
Thomas Hood <jdthood@mail.com> wrote:

> Hi Stephen 
> 
> Just a note to say that the patch combining "idling"
> and "notification order" changes is looking good so far.
>    http://panopticon.csustan.edu/thood/apm.html
> (No one has complained, anyway.)  I think it's ready
> for more extensive testing.  Should it go into a 2.4.X-preY
> kernel?

OK, I have a slightly modified version of the patch that I created just
before starting my Christmas break.  The only problem is that it crashes
the kernel hard when I remove the apm module.  I have not had a chance
to get back to it since then, but I am back at work in Wednesday and
it has my highest priority at the moment.

Question:  Has anyone got any empirical evidence that this patch
improves the idling behaviour?  i.e. does it consume less power or
lower the temperature?  I intend to try to measure this on Wednesday,
but a wider set of data is always better.

Thanks again for putting these patches together.
-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

Re: APM driver patch okay?

[Thomas Hood]

On Sun, 2001-12-30 at 20:02, Stephen Rothwell wrote:
> OK, I have a slightly modified version of the patch that I created just
> before starting my Christmas break.  The only problem is that it crashes
> the kernel hard when I remove the apm module.

Erp.  Obviously that wasn't something I tested.

I was just looking at the code to make sure that when the
module is removed it can't leave the machine in the idle
state.  Unfortunately the apm_cpu_idle() function uses
several gotos, continues and breaks, and uses both "t1"
and "t2" for more than one purpose.  Here's a patch that
makes that function a bit easier to read.            // TH

--- linux-2.4.17/arch/i386/kernel/apm.c_1	Sun Dec 23 14:53:12 2001
+++ linux-2.4.17/arch/i386/kernel/apm.c	Sun Dec 30 21:19:29 2001
@@ -757,12 +757,10 @@
 #define HARD_IDLE_TIMEOUT (HZ/3)
 #define IDLE_CALC_LIMIT   (HZ*100)
 #define IDLE_LEAKY_MAX    16
 
 static void (*sys_idle)(void); /* = 0 */
-static unsigned int last_jiffies; /* = 0 */
-static unsigned int last_stime; /* = 0 */
 
 /**
  * apm_cpu_idle		-	cpu idling for APM capable Linux
  *
  * This is the idling function the kernel executes when APM is available. It 
@@ -770,57 +768,62 @@
  * Furthermore it calls the system default idle routine.
  */
 
 static void apm_cpu_idle(void)
 {
-	static int use_apm_idle = 0;
+	static int use_apm_idle; /* = 0 */
+	static unsigned int last_jiffies; /* = 0 */
+	static unsigned int last_stime; /* = 0 */
 	int apm_is_idle = 0;
-	unsigned int t1 = jiffies - last_jiffies;
-	unsigned int t2;
+	unsigned int delta_jiffies;
+	unsigned int leak;
 
-recalc:	if(t1 > IDLE_CALC_LIMIT)
+	delta_jiffies = jiffies - last_jiffies;
+
+recalc:	if(delta_jiffies > IDLE_CALC_LIMIT)
 		goto reset;
 
-	if(t1 > HARD_IDLE_TIMEOUT) {
-		t2 = current->times.tms_stime - last_stime;
-		t2 *= 100;
-		t2 /= t1;
-		if (t2 > idle_threshold)
+	if(delta_jiffies > HARD_IDLE_TIMEOUT) {
+		unsigned int t = current->times.tms_stime - last_stime;
+		t *= 100;
+		t /= delta_jiffies;
+		if (t > idle_threshold)
 			use_apm_idle = 1;
 		else
 reset:			use_apm_idle = 0;
 		last_jiffies = jiffies;
 		last_stime = current->times.tms_stime;
 	}
 
-	t2 = IDLE_LEAKY_MAX;
+	leak = IDLE_LEAKY_MAX;
 
 	while (!current->need_resched) {
 		if(use_apm_idle) {
-			t1 = jiffies;
+			unsigned int jiffies_before = jiffies;
 			switch (apm_do_idle()) {
 			case 0:	apm_is_idle = 1;
-				if (t1 != jiffies) {
-					if (t2) {
-						t2 = IDLE_LEAKY_MAX;
+				if (jiffies_before != jiffies) {
+					if (leak) {
+						leak = IDLE_LEAKY_MAX;
 						continue;
 					}
-				} else if (t2) {
-					t2--;
+				} else if (leak) {
+					leak--;
 					continue;
 				}
 				break;
 			case 1:	apm_is_idle = 1;
 				break;
+			/* case -1: did not idle */
 			}
 		}
 
 		if (sys_idle)
 			sys_idle();
 
-		t1 = jiffies - last_jiffies;
-		if (t1 > HARD_IDLE_TIMEOUT)
+		delta_jiffies = jiffies - last_jiffies;
+		if (delta_jiffies > HARD_IDLE_TIMEOUT)
 			goto recalc;
 	}
 
 	if (apm_is_idle)
 		apm_do_busy();

Re: APM driver patch okay?

[Andreas Steinmetz]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=us-ascii, Size: 1803 bytes --]

Sorry if the code was to unreadable for you.
I did just try to keep the apm code overhead small and I'm no friend of "local
local" variables. If the code is now better readable that's fine with me.

If you remove the apm module as far as I can see the system can't be in apm
idle state as during module removal the system is not idle, so the system idle
task is not called, so apm is in busy state as apm_cpu_idle() is called via the
system idle task function pointer and asserts that apm_do_busy() is called on
function exit when apm_do_idle was called.

As I can't get useful temperature data out of my laptop (CPU temp 5°C) I do have
to rely on not really measurable things like fan behaviour and estimated
battery lifetime. With the patch I'm back to 2.2 behaviour which is better than
unpatched 2.4 behaviour.

Even if the idle patch may not improve things for biosses that do work
correctly I can't see any degradation in this case. For broken biosses it is an
improvement in any case.

Furthermore the patch removes confusion where the apm kernel thread and the
idle task were both racing for idle time with the result that an idle system
seemed 'heavily loaded' by the apm kernel thread. This may even have caused
unexpected behaviour for processes that use system load information for
processing decisions.

I would suggest to submit the patch in the early 2.4.18pre stages for wider
testing after the hard lockup on module removal is solved as there seem to be no
other complaints.

Regarding the hard lockup there could be a problem in apm_exit(). Assuming that
sys_idle could be NULL if apm initialization was not complete a safer way would
be:

if (idle_threshold < 100 && sys_idle) {
        pm_idle = sys_idle;
        sys_idle = NULL;
}


Andreas Steinmetz
D.O.M. Datenverarbeitung GmbH