Re: [PATCH] Updated: Dynamic Tick version 050408-1 - C-state measures

Re: [PATCH] Updated: Dynamic Tick version 050408-1 - C-state measures

[Thomas Renninger]

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

Here are some figures (I used your pmstats):

The machine is a Pentium M 2.00 GHz, supporting C0-C4 processor power states.
The machine run at 2.00 GHz all the time.
A lot of modules (pcmcia, usb, ...) where loaded, services that could
produce load where stopped -> processor is mostly idle.

_____________________________________________________________________________________
*Running with 1000Hz:*

_No processor module:_

Average current the last 100 seconds: *2289mA*
(cmp. ftp://ftp.suse.com/pub/people/trenn/dyn_tick_c_states/measures_C4_machine/1000_HZ_No_module_loaded)


_passing bm_history=0xFFFFFFFF (default) to processor module:_

Average current the last 470 seconds: *1986mA* (also measured better values ~1800, does battery level play a role?!?)
(cmp. ftp://ftp.suse.com/pub/people/trenn/dyn_tick_c_states/measures_C4_machine/1000_HZ_bm_history_FFFFFFFF)


_passing bm_history=0xFF to processor module:_

Average current the last 190 seconds: *1757mA*
(cmp. ftp://ftp.suse.com/pub/people/trenn/dyn_tick_c_states/measures_C4_machine/1000_HZ_bm_history_FF)
(Usage count could be bogus, as some invokations could not succeed if bm has currently been active).
_____________________________________________________________________________________

*Running with CONFIG_NO_IDLE_HZ:*
Patched with http://www.muru.com/linux/dyntick/patches/patch-dynamic-tick-2.6.12-rc2-050408-1.gz
(With the c-state patch attached applied)

_No processor module:_

Average current the last 80 seconds: *2262mA*
(cmp. ftp://ftp.suse.com/pub/people/trenn/dyn_tick_c_states/measures_C4_machine/tony_dyn_tick_No_module_loaded)

idle_ms == 40, bm_promote_bs == 30
Average current the last 160 seconds: *1507mA*
(cmp. ftp://ftp.suse.com/pub/people/trenn/dyn_tick_c_states/measures_C4_machine/tony_dyn_tick_processor_idle_40_bm_30)

idle_ms == 100, bm_promote_bs == 30
Average current the last 80 seconds: *1466mA*
(cmp. ftp://ftp.suse.com/pub/people/trenn/dyn_tick_c_states/measures_C4_machine/tony_dyn_tick_processor_idle_100_bm_30)

idle_ms == 40, bm_promote_bs == 50
Average current the last 150 seconds: *1481mA*
(cmp. ftp://ftp.suse.com/pub/people/trenn/dyn_tick_c_states/measures_C4_machine/tony_dyn_tick_processor_idle_40_bm_30)

idle_ms == 40, bm_promote_bs == 10
Average current the last 330 seconds: *1474mA*
(cmp. ftp://ftp.suse.com/pub/people/trenn/dyn_tick_c_states/measures_C4_machine/tony_dyn_tick_processor_idle_40_bm_10)

Hmm, parameters do not influence at all ... (idle_ms should only comes in when switching between idle/not idle).
_____________________________________________________________________________________


The measures are based on the /proc/acpi/battery/*/* info and are not very accurate, but could give an overall picture.

      Thomas

P.S.: Not tested, because I have no x86_64 C3 machine, but the patch should also work reliable with Andi's dyn_tick patch
for x86_64 machines.

Tony: I modified your pmstats to produce an average current value: ftp://ftp.suse.com/pub/people/trenn/dyn_tick_c_states/pmstats

[-- Attachment #2: dynamic_tick_cstate_patch_final.diff --]
[-- Type: text/x-patch, Size: 10275 bytes --]

Patch not enough tested, yet.
Should behave the same if compile with !CONFIG_IDLE_HZ.

If CONFIG_IDLE_HZ is set, the c-state will be evaluated on
three control values (averages of the last 4 measures):

a) idle_ms -> if machine was active for longer than this
   value (avg), the machine is assumed to not be idle
   and C1 will be triggered.

b) bm_promote_ms -> if the avg bus master activity is below
   this threshold, C2 is invoked.

c) sleep_avg (no module param) -> the average sleep time of the
   last four C2 (or higher) invokations.
   If a and b does not apply, a C-state will be searched that has
   the highest latency, but still has a latency below the latest
   C2 (or higher) sleeping time and average sleeping time value.

ToDo:
Test on other machines (only C2 or C0 support).
Discuss and enhance algorithm.
If it is used this way, average calculations could get MMX optimised.

--- linux-2.6.12-rc2.orig/drivers/acpi/processor_idle.c	2005-04-19 15:03:13.000000000 +0200
+++ linux-2.6.12-rc2/drivers/acpi/processor_idle.c	2005-04-19 15:17:56.000000000 +0200
@@ -60,6 +60,22 @@
 static unsigned int nocst = 0;
 module_param(nocst, uint, 0000);
 
+static unsigned int idle_ms = 40;
+module_param(idle_ms, uint, 0644);
+MODULE_PARM_DESC(idle_ms, "Promote to lower state if machine stays shorter than x ms not in idle func (avg) [40].");
+
+static unsigned int bm_promote_ms = 30;
+module_param(bm_promote_ms, uint, 0644);
+MODULE_PARM_DESC(bm_promote_ms, "Promote to lower state if avg bm is less than x ms [30].");
+
+//#define DEBUG 1
+#ifdef DEBUG
+#define myPrintk(string, args...) printk(KERN_INFO ""string, ##args);
+#else
+#define myPrintk(string, args...) {};
+#endif
+
+
 /*
  * bm_history -- bit-mask with a bit per jiffy of bus-master activity
  * 1000 HZ: 0xFFFFFFFF: 32 jiffies = 32ms
@@ -162,6 +178,88 @@
 	return;
 }
 
+u16 calc_average (u64 last_measures){
+	int x;
+	u16 ret = 0;
+	for (x = 0; x < sizeof(u64)*8; x+=16){
+		ret += (last_measures >> x) & (u64)0xFFFF;
+//		myPrintk (KERN_INFO "x: %d - ret: %X - last_measures: %X - result %X\n", x, ret, (u64)last_measures, (last_measures >> x) & (u64)0xFFFF);
+	}
+/* divide by four -> average bm activity for the last 4 times */
+	ret >>= 2;
+	return ret;
+}
+
+/*
+ * check BM Activity
+ * -----------------
+ * Check for bus mastering activity and save history in
+ * pr->power.bm_activity
+ *
+ * return 0  -> no bm activity for long time
+ * return 1  -> we currently have bus master activity 
+ *
+ */
+
+static void check_bm_activity(struct acpi_processor *pr) {
+	u32		bm_status = 0;
+	unsigned long	diff = jiffies - pr->power.bm_check_timestamp;
+	
+#ifndef CONFIG_NO_IDLE_HZ
+	if (diff > 32)
+		diff = 32;
+	while (diff) {
+		/* if we didn't get called, assume there was busmaster activity */
+		diff--;
+		if (diff)
+			pr->power.bm_activity |= 0x1;
+		pr->power.bm_activity <<= 1;
+	}
+	
+	acpi_get_register(ACPI_BITREG_BUS_MASTER_STATUS,
+			  &bm_status, ACPI_MTX_DO_NOT_LOCK);
+	if (bm_status) {
+		pr->power.bm_activity++;
+		acpi_set_register(ACPI_BITREG_BUS_MASTER_STATUS,
+				  1, ACPI_MTX_DO_NOT_LOCK);
+	}
+	/*
+	 * PIIX4 Erratum #18: Note that BM_STS doesn't always reflect
+	 * the true state of bus mastering activity; forcing us to
+	 * manually check the BMIDEA bit of each IDE channel.
+	 */
+	else if (errata.piix4.bmisx) {
+		if ((inb_p(errata.piix4.bmisx + 0x02) & 0x01)
+		    || (inb_p(errata.piix4.bmisx + 0x0A) & 0x01))
+			pr->power.bm_activity++;
+	}
+	pr->power.bm_check_timestamp = jiffies;
+	
+#else
+	if (diff > 0xFFFF)
+		diff = 0xFFFF;
+	acpi_get_register(ACPI_BITREG_BUS_MASTER_STATUS,
+			  &bm_status, ACPI_MTX_DO_NOT_LOCK);
+//	myPrintk (KERN_INFO "bm_status - %d, diff: %lu - bm_avg_ms: %X\n",
+//		  bm_status, diff, pr->power.bm_avg_ms);
+	if (bm_status) {
+		acpi_set_register(ACPI_BITREG_BUS_MASTER_STATUS,
+				  1, ACPI_MTX_DO_NOT_LOCK);
+		/* we had an active bus master -> get timestamp */
+		pr->power.bm_check_timestamp = jiffies;
+		pr->power.bm_last_measures <<= 16;
+	}
+	/* just OR, even if !bm_status, shouldn't matter */
+	pr->power.bm_last_measures |= diff;
+	
+//	myPrintk (KERN_INFO "diff: %lu - Avg: bm_last_measures: %#16X\n", diff, (u64)pr->power.bm_last_measures);
+
+	pr->power.bm_avg_ms = calc_average (pr->power.bm_last_measures);
+	
+	
+//	printk (KERN_INFO "Avg: bm_avg_ms: %lu ms \n", (unsigned long)pr->power.bm_avg_ms);
+#endif
+}
 
 static void acpi_processor_idle (void)
 {
@@ -171,6 +269,12 @@
 	int			sleep_ticks = 0;
 	u32			t1, t2 = 0;
 
+#ifdef CONFIG_NO_IDLE_HZ
+	struct acpi_processor_cx *temp_state = NULL;
+	int		        i = 0;
+	unsigned long	diff = jiffies - pr->power.active_timestamp;
+#endif
+
 	pr = processors[_smp_processor_id()];
 	if (!pr)
 		return;
@@ -191,9 +295,27 @@
 	}
 
 	cx = pr->power.state;
+
+#ifdef CONFIG_NO_IDLE_HZ
+	/* keep track of bm activity */
+	check_bm_activity(pr);
+
+	unsigned long	diff = jiffies - pr->power.active_timestamp;
+	if (diff > 0xFFFF)
+		diff = 0xFFFF;
+	pr->power.active_last_measures <<= 16;
+	pr->power.active_last_measures |= (u16)diff;
+	pr->power.active_avg_ms = calc_average(pr->power.active_last_measures);
+	if (pr->power.active_avg_ms > idle_ms){
+		printk (KERN_INFO "We were not idle for %d ms (4 times avg) -> goto C1\n", pr->power.active_avg_ms);
+		next_state = &pr->power.states[ACPI_STATE_C1];
+		goto end;
+	}
+#endif
 	if (!cx)
 		goto easy_out;
-
+	
+#ifndef CONFIG_NO_IDLE_HZ
 	/*
 	 * Check BM Activity
 	 * -----------------
@@ -201,40 +323,8 @@
 	 * for demotion.
 	 */
 	if (pr->flags.bm_check) {
-		u32		bm_status = 0;
-		unsigned long	diff = jiffies - pr->power.bm_check_timestamp;
-
-		if (diff > 32)
-			diff = 32;
-
-		while (diff) {
-			/* if we didn't get called, assume there was busmaster activity */
-			diff--;
-			if (diff)
-				pr->power.bm_activity |= 0x1;
-			pr->power.bm_activity <<= 1;
-		}
-
-		acpi_get_register(ACPI_BITREG_BUS_MASTER_STATUS,
-			&bm_status, ACPI_MTX_DO_NOT_LOCK);
-		if (bm_status) {
-			pr->power.bm_activity++;
-			acpi_set_register(ACPI_BITREG_BUS_MASTER_STATUS,
-				1, ACPI_MTX_DO_NOT_LOCK);
-		}
-		/*
-		 * PIIX4 Erratum #18: Note that BM_STS doesn't always reflect
-		 * the true state of bus mastering activity; forcing us to
-		 * manually check the BMIDEA bit of each IDE channel.
-		 */
-		else if (errata.piix4.bmisx) {
-			if ((inb_p(errata.piix4.bmisx + 0x02) & 0x01)
-				|| (inb_p(errata.piix4.bmisx + 0x0A) & 0x01))
-				pr->power.bm_activity++;
-		}
-
-		pr->power.bm_check_timestamp = jiffies;
-
+		
+		check_bm_activity(pr);
 		/*
 		 * Apply bus mastering demotion policy.  Automatically demote
 		 * to avoid a faulty transition.  Note that the processor
@@ -253,6 +343,8 @@
 			goto end;
 		}
 	}
+#endif
+	cx->usage++;
 
 	cx->usage++;
 
@@ -278,7 +370,7 @@
 		 *      go to an ISR rather than here.  Need to instrument
 		 *      base interrupt handler.
 		 */
-		sleep_ticks = 0xFFFFFFFF;
+		sleep_ticks = 0xFFFFFFF;
 		break;
 
 	case ACPI_STATE_C2:
@@ -320,8 +412,51 @@
 		return;
 	}
 
+	if (sleep_ticks <= 0)
+		cx->failed++;
+
+#ifdef CONFIG_NO_IDLE_HZ
+		pr->power.sleep_last_measures <<= 16;
+		pr->power.sleep_last_measures |= sleep_ticks;
+		pr->power.sleep_avg_ticks = calc_average(pr->power.sleep_last_measures);
+#endif
+
 	next_state = pr->power.state;
 
+#ifdef CONFIG_NO_IDLE_HZ
+	
+	/* use C1/C2 if we have to much bus master activity */
+	if (bm_promote_ms >= pr->power.bm_avg_ms){
+		myPrintk ("pr->power.bm_avg_ms: %d\n", (int)pr->power.bm_avg_ms);
+		next_state = &pr->power.states[ACPI_STATE_C2];
+		if (next_state == NULL || !next_state->valid){
+			next_state = &pr->power.states[ACPI_STATE_C1];
+			if (next_state == NULL || !next_state->valid)
+				goto easy_out;
+			
+		}
+		goto end;
+	}
+		
+	
+	/* goto sleep state which latency fits best to avg sleep time
+	   of the last 4 sleep cycles */
+	for (i=ACPI_STATE_C2; i < ACPI_PROCESSOR_MAX_POWER; i++) {
+		temp_state = &pr->power.states[i];
+		if (temp_state == NULL || !temp_state->valid)
+			break;
+		myPrintk ("pr->power.sleep_avg_ticks: %d - temp_state->promotion.threshold.ticks: %d"
+			  " - sleep_ticks; %d\n", 
+			  pr->power.sleep_avg_ticks, temp_state->promotion.threshold.ticks, sleep_ticks);
+		if (temp_state->promotion.state && 
+		    sleep_ticks > temp_state->promotion.threshold.ticks &&
+		    pr->power.sleep_avg_ticks > temp_state->promotion.threshold.ticks){
+			next_state = temp_state->promotion.state;
+		}
+		else
+			break;
+	}
+#else		
 	/*
 	 * Promotion?
 	 * ----------
@@ -330,6 +465,7 @@
 	 * mastering activity may prevent promotions.
 	 * Do not promote above max_cstate.
 	 */
+
 	if (cx->promotion.state &&
 	    ((cx->promotion.state - pr->power.states) <= max_cstate)) {
 		if (sleep_ticks > cx->promotion.threshold.ticks) {
@@ -366,6 +502,7 @@
 			}
 		}
 	}
+#endif
 
 end:
 	/*
@@ -384,7 +521,9 @@
 	 */
 	if (next_state != pr->power.state)
 		acpi_processor_power_activate(pr, next_state);
-
+#ifdef CONFIG_NO_IDLE_HZ
+	pr->power.active_timestamp = jiffies;
+#endif
 	return;
 
  easy_out:
@@ -393,6 +532,9 @@
 		pm_idle_save();
 	else
 		safe_halt();
+#ifdef CONFIG_NO_IDLE_HZ
+	pr->power.active_timestamp = jiffies;
+#endif
 	return;
 }
 
@@ -910,6 +1052,9 @@
 		seq_printf(seq, "latency[%03d] usage[%08d]\n",
 			pr->power.states[i].latency,
 			pr->power.states[i].usage);
+		seq_printf(seq, "                         failed[%08d]\n",
+			pr->power.states[i].failed);
+
 	}
 
 end:
--- linux-2.6.12-rc2.orig/include/acpi/processor.h	2005-04-19 15:03:44.000000000 +0200
+++ linux-2.6.12-rc2/include/acpi/processor.h	2005-04-19 15:17:56.000000000 +0200
@@ -48,6 +48,7 @@
 	u32			latency_ticks;
 	u32			power;
 	u32			usage;
+	u32                     failed;
 	struct acpi_processor_cx_policy promotion;
 	struct acpi_processor_cx_policy demotion;
 };
@@ -55,8 +56,16 @@
 struct acpi_processor_power {
 	struct acpi_processor_cx *state;
 	unsigned long		bm_check_timestamp;
-	u32			default_state;
 	u32			bm_activity;
+#ifdef CONFIG_NO_IDLE_HZ
+	u16			bm_avg_ms;
+	u64     		bm_last_measures;
+	u16     		sleep_avg_ticks;
+	u64     		sleep_last_measures;
+	unsigned long		active_timestamp;
+	u16     		active_avg_ms;
+	u64     		active_last_measures;
+#endif
 	int			count;
 	struct acpi_processor_cx states[ACPI_PROCESSOR_MAX_POWER];
 };

Re: [PATCH] Updated: Dynamic Tick version 050408-1 - C-state measures

[Dominik Brodowski]

Hi,

On Tue, Apr 19, 2005 at 04:56:56PM +0200, Thomas Renninger wrote:
> If CONFIG_IDLE_HZ is set, the c-state will be evaluated on
> three control values (averages of the last 4 measures):
> 
> a) idle_ms -> if machine was active for longer than this
>    value (avg), the machine is assumed to not be idle
>    and C1 will be triggered.
> 
> b) bm_promote_ms -> if the avg bus master activity is below
>    this threshold, C2 is invoked.
> 
> c) sleep_avg (no module param) -> the average sleep time of the
>    last four C2 (or higher) invokations.
>    If a and b does not apply, a C-state will be searched that has
>    the highest latency, but still has a latency below the latest
>    C2 (or higher) sleeping time and average sleeping time value.

I think that we don't need this complication:

> +//#define DEBUG 1
> +#ifdef DEBUG
> +#define myPrintk(string, args...) printk(KERN_INFO ""string, ##args);
> +#else
> +#define myPrintk(string, args...) {};
> +#endif

Please don't do that... dprintk() is much more common. Also, then don't
comment dprintk() out below in the patch...

>  	if (pr->flags.bm_check) {
> -		u32		bm_status = 0;
> -		unsigned long	diff = jiffies - pr->power.bm_check_timestamp;
> -
> -		if (diff > 32)
> -			diff = 32;
> -
> -		while (diff) {
> -			/* if we didn't get called, assume there was busmaster activity */
> -			diff--;
> -			if (diff)
> -				pr->power.bm_activity |= 0x1;
> -			pr->power.bm_activity <<= 1;
> -		}

"All" we need to do is to update the "diff". Without dynamic ticks, if the
idle loop didn't get called each jiffy, it was a big hint that there was so
much activity in between, and if there is activity, there is most likely
also bus master activity, or at least more work to do, so interrupt activity
is likely. Therefore we assume there was bm_activity even if there was none.

Now, we do know the jiffy value when we started sleeping. If we use
ticks_elapsed(t1, t2), convert it to jiffies, and do
	diff = jiffies - (pr->power.bm_check_timestamp + last_sleep_jiffies);
it should work. I wrote a quick patch to do that, but it locked up my
notebook, so it is most likely broken; hopefully I'll find some time to debug
it, if somebody does it earlier, that'd be great, though.

Thanks,
	Dominik


Only assume busmaster activity on non-idle ticks if we didn't sleep until
that jiffy. Needed for dyn-idle.

Signed-off-by: Dominik Brodowski <linux@brodo.de>

--- linux/drivers/acpi/processor_idle.c.original	2005-04-10 20:04:12.000000000 +0200
+++ linux/drivers/acpi/processor_idle.c	2005-04-10 20:14:33.000000000 +0200
@@ -120,6 +120,14 @@
 		return ((0xFFFFFFFF - t1) + t2);
 }
 
+static inline u32
+ticks_to_jiffies (u32 pm_ticks)
+{
+	pm_ticks *= 286;
+	pm_ticks = (pm_ticks >> 10);
+	return (pm_ticks / (USEC_PER_SEC / HZ));
+}
+
 
 static void
 acpi_processor_power_activate (
@@ -169,7 +177,7 @@
 	struct acpi_processor_cx *cx = NULL;
 	struct acpi_processor_cx *next_state = NULL;
 	int			sleep_ticks = 0;
-	u32			t1, t2 = 0;
+	u32			t1, t2, td = 0;
 
 	pr = processors[_smp_processor_id()];
 	if (!pr)
@@ -201,11 +209,13 @@
 	 * for demotion.
 	 */
 	if (pr->flags.bm_check) {
-		u32		bm_status = 0;
-		unsigned long	diff = jiffies - pr->power.bm_check_timestamp;
+		u32	bm_status = 0;
+		long	diff = jiffies - pr->power.bm_check_timestamp;
 
 		if (diff > 32)
 			diff = 32;
+		else if (diff < 0)
+			diff = 0;
 
 		while (diff) {
 			/* if we didn't get called, assume there was busmaster activity */
@@ -293,7 +303,9 @@
 		/* Re-enable interrupts */
 		local_irq_enable();
 		/* Compute time (ticks) that we were actually asleep */
-		sleep_ticks = ticks_elapsed(t1, t2) - cx->latency_ticks - C2_OVERHEAD;
+		td = ticks_elapsed(t1, t2);
+		sleep_ticks = td - cx->latency_ticks - C2_OVERHEAD;
+		pr->power.bm_check_timestamp += ticks_to_jiffies(td);
 		break;
 
 	case ACPI_STATE_C3:
@@ -312,7 +324,9 @@
 		/* Re-enable interrupts */
 		local_irq_enable();
 		/* Compute time (ticks) that we were actually asleep */
-		sleep_ticks = ticks_elapsed(t1, t2) - cx->latency_ticks - C3_OVERHEAD;
+		td = ticks_elapsed(t1, t2);
+		sleep_ticks = td - cx->latency_ticks - C3_OVERHEAD;
+		pr->power.bm_check_timestamp += ticks_to_jiffies(td);
 		break;
 
 	default:

Re: [PATCH] Updated: Dynamic Tick version 050408-1 - C-state measures

[Thomas Renninger]

Reducing the CC'd people a bit ...

Dominik Brodowski wrote:
> Hi,
>
> On Tue, Apr 19, 2005 at 04:56:56PM +0200, Thomas Renninger wrote:
>>If CONFIG_IDLE_HZ is set, the c-state will be evaluated on
>>three control values (averages of the last 4 measures):
>>
>>a) idle_ms -> if machine was active for longer than this
>>   value (avg), the machine is assumed to not be idle
>>   and C1 will be triggered.
>>
>>b) bm_promote_ms -> if the avg bus master activity is below
>>   this threshold, C2 is invoked.
>>
>>c) sleep_avg (no module param) -> the average sleep time of the
>>   last four C2 (or higher) invokations.
>>   If a and b does not apply, a C-state will be searched that has
>>   the highest latency, but still has a latency below the latest
>>   C2 (or higher) sleeping time and average sleeping time value.
>
> I think that we don't need this complication:
>
>>+//#define DEBUG 1
>>+#ifdef DEBUG
>>+#define myPrintk(string, args...) printk(KERN_INFO ""string, ##args);
>>+#else
>>+#define myPrintk(string, args...) {};
>>+#endif
>
> Please don't do that... dprintk() is much more common. Also, then don't
> comment dprintk() out below in the patch...
>
Ok, this patch is far from perfect, I am happy that it finally runs that nice on
my machine.

>> 	if (pr->flags.bm_check) {
>>-		u32		bm_status = 0;
>>-		unsigned long	diff = jiffies - pr->power.bm_check_timestamp;
>>-
>>-		if (diff > 32)
>>-			diff = 32;
>>-
>>-		while (diff) {
>>-			/* if we didn't get called, assume there was busmaster activity */
>>-			diff--;
>>-			if (diff)
>>-				pr->power.bm_activity |= 0x1;
>>-			pr->power.bm_activity <<= 1;
>>-		}
>
> "All" we need to do is to update the "diff". Without dynamic ticks, if the
> idle loop didn't get called each jiffy, it was a big hint that there was so
> much activity in between, and if there is activity, there is most likely
> also bus master activity, or at least more work to do, so interrupt activity
> is likely. Therefore we assume there was bm_activity even if there was none.
>
If I understand this right you want at least wait 32 (or whatever value) ms if there was bm activity,
before it is allowed to trigger C3/C4?

I think the problem is (at least I made the experience with this particular machine)
that bm activity comes very often and regularly (each 30-150ms?).

I think the approach to directly adjust the latency to a deeper sleep state if the
average bus master and OS activity is low is very efficient.

Because I don't consider whether there was bm_activity the last ms, I only
consider the average, it seems to happen that I try to trigger
C3/C4 when there is just something copied and some bm active ?!? Therefore, it seems to happen
that triggering C3/C4 fails (sleep_ticks < 0). The value of failures is getting smaller if I increase
the limit for average bm activity before triggering C3/C4 (bm_promote_ms must be smaller than average bm activity),
but it never will reach zero.

The patch is useless if these failures end up in system freezes on other machines...
AFAIK there were a lot of freeze problems with C-states? Don't know, it works here.

The problem with the old approach is, that after (doesn't matter C1-Cx) sleep and dyn_idle_tick,
the chance to wake up because of bm activity is very likely.
You enter idle() again -> there was bm_activity -> C2. Wake up after e.g. 50ms, because
of bm_activity again (bm_sts bit set) -> stay in C2, wake up after 40ms -> bm activity...
You only have the chance to get into deeper states if the sleeps are interrupted by an interrupt, not bm activity.

I also thought about only reprogram timer if C1/C2 was successful x times and no bm activity was detected,
same mechanism as now, then only reprogram timer (dyn tick) for deeper sleep states -> like that, you
still can be sure the last x ms was no bm activity bit set before going to deep sleeps.
But I don't know how to do it.

> Now, we do know the jiffy value when we started sleeping. If we use
> ticks_elapsed(t1, t2), convert it to jiffies, and do
> 	diff = jiffies - (pr->power.bm_check_timestamp + last_sleep_jiffies);
> it should work. I wrote a quick patch to do that, but it locked up my
> notebook, so it is most likely broken; hopefully I'll find some time to debug
> it, if somebody does it earlier, that'd be great, though.
>
> Thanks,
> 	Dominik
>
>
> Only assume busmaster activity on non-idle ticks if we didn't sleep until
> that jiffy. Needed for dyn-idle.
>
> Signed-off-by: Dominik Brodowski <linux@brodo.de>
>
> --- linux/drivers/acpi/processor_idle.c.original	2005-04-10 20:04:12.000000000 +0200
> +++ linux/drivers/acpi/processor_idle.c	2005-04-10 20:14:33.000000000 +0200
> @@ -120,6 +120,14 @@
>  		return ((0xFFFFFFFF - t1) + t2);
>  }
>
> +static inline u32
> +ticks_to_jiffies (u32 pm_ticks)
> +{
> +	pm_ticks *= 286;
> +	pm_ticks = (pm_ticks >> 10);
> +	return (pm_ticks / (USEC_PER_SEC / HZ));
> +}
> +
>
>  static void
>  acpi_processor_power_activate (
> @@ -169,7 +177,7 @@
>  	struct acpi_processor_cx *cx = NULL;
>  	struct acpi_processor_cx *next_state = NULL;
>  	int			sleep_ticks = 0;
> -	u32			t1, t2 = 0;
> +	u32			t1, t2, td = 0;
>
>  	pr = processors[_smp_processor_id()];
>  	if (!pr)
> @@ -201,11 +209,13 @@
>  	 * for demotion.
>  	 */
>  	if (pr->flags.bm_check) {
> -		u32		bm_status = 0;
> -		unsigned long	diff = jiffies - pr->power.bm_check_timestamp;
> +		u32	bm_status = 0;
> +		long	diff = jiffies - pr->power.bm_check_timestamp;
>
>  		if (diff > 32)
>  			diff = 32;
> +		else if (diff < 0)
> +			diff = 0;
>
>  		while (diff) {
>  			/* if we didn't get called, assume there was busmaster activity */
> @@ -293,7 +303,9 @@
>  		/* Re-enable interrupts */
>  		local_irq_enable();
>  		/* Compute time (ticks) that we were actually asleep */
> -		sleep_ticks = ticks_elapsed(t1, t2) - cx->latency_ticks - C2_OVERHEAD;
> +		td = ticks_elapsed(t1, t2);
> +		sleep_ticks = td - cx->latency_ticks - C2_OVERHEAD;
> +		pr->power.bm_check_timestamp += ticks_to_jiffies(td);
>  		break;
>
>  	case ACPI_STATE_C3:
> @@ -312,7 +324,9 @@
>  		/* Re-enable interrupts */
>  		local_irq_enable();
>  		/* Compute time (ticks) that we were actually asleep */
> -		sleep_ticks = ticks_elapsed(t1, t2) - cx->latency_ticks - C3_OVERHEAD;
> +		td = ticks_elapsed(t1, t2);
> +		sleep_ticks = td - cx->latency_ticks - C3_OVERHEAD;
> +		pr->power.bm_check_timestamp += ticks_to_jiffies(td);
>  		break;
>
>  	default:

Hmm, I can give it a shot the next days ...

You could also test whether it was bm activity here that caused the end of sleep (it should be in most cases):

	        acpi_get_register(ACPI_BITREG_BUS_MASTER_RLD, &bm_wakeup, ACPI_MTX_DO_NOT_LOCK);
		if (bm_wakeup){
			printk(KERN_INFO "Woke up from C3 from bus master activity after %d ticks\n", td);
			acpi_set_register(ACPI_BITREG_BUS_MASTER_RLD, 1, ACPI_MTX_DO_NOT_LOCK);
			/* also reset bm_sts bit ?!? */
			pr->power.bm_activity++;
		}
		else{
			printk(KERN_INFO "Did not wake up from C3 from bus master activity\n");
		}
		pr->power.bm_check_timestamp += ticks_to_jiffies(td);

Hmm I wonder what the difference is after waking up and checking for bm_rld or bm_sts bit ...

          Thomas

Re: [PATCH] Updated: Dynamic Tick version 050408-1 - C-state measures

[Dominik Brodowski]

On Tue, Apr 19, 2005 at 11:03:30PM +0200, Thomas Renninger wrote:
> > "All" we need to do is to update the "diff". Without dynamic ticks, if the
> > idle loop didn't get called each jiffy, it was a big hint that there was so
> > much activity in between, and if there is activity, there is most likely
> > also bus master activity, or at least more work to do, so interrupt activity
> > is likely. Therefore we assume there was bm_activity even if there was none.
> >
> If I understand this right you want at least wait 32 (or whatever value) ms if there was bm activity,
> before it is allowed to trigger C3/C4?

That's the theory of operation of the current algorithm. I think that we
should do that small change to the current algorithm which allows us to keep
C3/C4 working with dyn-idle first, and then think of a very small abstraction
layer to test different idle algroithms, and -- possibly -- use different
ones for different usages.

> I think the problem is (at least I made the experience with this particular
> machine) that bm activity comes very often and regularly (each 30-150ms?).
> 
> I think the approach to directly adjust the latency to a deeper sleep state if the
> average bus master and OS activity is low is very efficient.
> 
> Because I don't consider whether there was bm_activity the last ms, I only
> consider the average, it seems to happen that I try to trigger
> C3/C4 when there is just something copied and some bm active ?!?

I don't think that this is perfect behaviour: if the system is idle, and
there is _currently_ bus master activity, the CPU should be put into C1 or
C2 type sleep. If you select C3 and actually enter it, you're risking
DMA issues, AFAICS.

> The patch is useless if these failures end up in system freezes on
> other machines...

I know that my patch is useless in its current form, but I wanted to share
it as a different way of doing things. 

> The problem with the old approach is, that after (doesn't matter C1-Cx)
> sleep and dyn_idle_tick, the chance to wake up because of bm activity is
> very likely.
> You enter idle() again -> there was bm_activity -> C2. Wake up after e.g.
> 50ms, because of bm_activity again (bm_sts bit set) -> stay in C2, wake up
> after 40ms -> bm activity... You only have the chance to get into deeper
> states if the sleeps are interrupted by an interrupt, not bm activity.

That's a side-effect, indeed. However: if there _is_ bus master activity, we
must not enter C3, AFAICS.

	Dominik

Re: [PATCH] Updated: Dynamic Tick version 050408-1 - C-state measures

[Pavel Machek]

Hi!

> > Because I don't consider whether there was bm_activity the last ms, I only
> > consider the average, it seems to happen that I try to trigger
> > C3/C4 when there is just something copied and some bm active ?!?
> 
> I don't think that this is perfect behaviour: if the system is idle, and
> there is _currently_ bus master activity, the CPU should be put into C1 or
> C2 type sleep. If you select C3 and actually enter it, you're risking
> DMA issues, AFAICS.

What kinds of DMA issues? Waiting 32msec or so is only heuristic; it
can go wrong any time. It would be really bad if it corrupted data or
something like that.
									Pavel

-- 
Boycott Kodak -- for their patent abuse against Java.

Re: [PATCH] Updated: Dynamic Tick version 050408-1 - C-state measures

[Dominik Brodowski]

On Wed, Apr 20, 2005 at 01:57:39PM +0200, Pavel Machek wrote:
> Hi!
> 
> > > Because I don't consider whether there was bm_activity the last ms, I only
> > > consider the average, it seems to happen that I try to trigger
> > > C3/C4 when there is just something copied and some bm active ?!?
> > 
> > I don't think that this is perfect behaviour: if the system is idle, and
> > there is _currently_ bus master activity, the CPU should be put into C1 or
> > C2 type sleep. If you select C3 and actually enter it, you're risking
> > DMA issues, AFAICS.
> 
> What kinds of DMA issues? Waiting 32msec or so is only heuristic; it
> can go wrong any time. It would be really bad if it corrupted data or
> something like that.

loop()
   a) bus mastering activity is going on at the very moment
   b) the CPU is entering C3
   c) the CPU is woken out of C3 because of bus mastering activity

the repeated delay between b) and c) might be problematic, as can be seen
by the comment in processor_idle.c:

                 * TBD: A better policy might be to fallback to the demotion
                 *      state (use it for this quantum only) istead of
                 *      demoting -- and rely on duration as our sole demotion
                 *      qualification.  This may, however, introduce DMA
                 *      issues (e.g. floppy DMA transfer overrun/underrun).
                 */

I'm not so worried about floppy DMA but about the ipw2x00 issues here.

	Dominik

Re: [PATCH] Updated: Dynamic Tick version 050408-1 - C-state measures

[Pavel Machek]

Hi!

> > > > Because I don't consider whether there was bm_activity the last ms, I only
> > > > consider the average, it seems to happen that I try to trigger
> > > > C3/C4 when there is just something copied and some bm active ?!?
> > > 
> > > I don't think that this is perfect behaviour: if the system is idle, and
> > > there is _currently_ bus master activity, the CPU should be put into C1 or
> > > C2 type sleep. If you select C3 and actually enter it, you're risking
> > > DMA issues, AFAICS.
> > 
> > What kinds of DMA issues? Waiting 32msec or so is only heuristic; it
> > can go wrong any time. It would be really bad if it corrupted data or
> > something like that.
> 
> loop()
>    a) bus mastering activity is going on at the very moment
>    b) the CPU is entering C3
>    c) the CPU is woken out of C3 because of bus mastering activity
> 
> the repeated delay between b) and c) might be problematic, as can be seen
> by the comment in processor_idle.c:
> 
>                  * TBD: A better policy might be to fallback to the demotion
>                  *      state (use it for this quantum only) istead of
>                  *      demoting -- and rely on duration as our sole demotion
>                  *      qualification.  This may, however, introduce DMA
>                  *      issues (e.g. floppy DMA transfer overrun/underrun).
>                  */
> 
> I'm not so worried about floppy DMA but about the ipw2x00 issues here.

Like "ipw2x00 looses packets" if this happens too often?

								Pavel
-- 
Boycott Kodak -- for their patent abuse against Java.

Re: [PATCH] Updated: Dynamic Tick version 050408-1 - C-state measures

[Dominik Brodowski]

Hi,

On Wed, Apr 20, 2005 at 02:08:46PM +0200, Pavel Machek wrote:
> Like "ipw2x00 looses packets" if this happens too often?

See "PCI latency error if C3 enabled" on http://ipw2100.sf.net -- it causes
network instability, frequent firmware restarts.

	Dominik

Re: [PATCH] Updated: Dynamic Tick version 050408-1 - C-state measures

[Thomas Renninger]

Dominik Brodowski wrote:
> On Tue, Apr 19, 2005 at 11:03:30PM +0200, Thomas Renninger wrote:
>>>"All" we need to do is to update the "diff". Without dynamic ticks, if the
>>>idle loop didn't get called each jiffy, it was a big hint that there was so
>>>much activity in between, and if there is activity, there is most likely
>>>also bus master activity, or at least more work to do, so interrupt activity
>>>is likely. Therefore we assume there was bm_activity even if there was none.
>>>
>>If I understand this right you want at least wait 32 (or whatever value) ms if there was bm activity,
>>before it is allowed to trigger C3/C4?
> 
> That's the theory of operation of the current algorithm. I think that we
> should do that small change to the current algorithm which allows us to keep
> C3/C4 working with dyn-idle first, and then think of a very small abstraction
> layer to test different idle algroithms, and -- possibly -- use different
> ones for different usages.
> 
>>I think the problem is (at least I made the experience with this particular
>>machine) that bm activity comes very often and regularly (each 30-150ms?).
>>
>>I think the approach to directly adjust the latency to a deeper sleep state if the
>>average bus master and OS activity is low is very efficient.
>>
>>Because I don't consider whether there was bm_activity the last ms, I only
>>consider the average, it seems to happen that I try to trigger
>>C3/C4 when there is just something copied and some bm active ?!?
> 
> I don't think that this is perfect behaviour: if the system is idle, and
> there is _currently_ bus master activity, the CPU should be put into C1 or
> C2 type sleep. If you select C3 and actually enter it, you're risking
> DMA issues, AFAICS.
> 
On my system triggering C3/C4 is just ignored (sleep_ticks < 0).
These ignorings (C3/C4 failures) seem to directly depend on how much bm_activity
there actually is.
With the current method (wait at least 30 ms if there was bm activity before
triggering C3/C4) these failures never happened.
As mentioned using bm_promotion_ms you can lower the failures, but never reach zero.
If these failures lead to system freezes on other systems, my next sentence is valid
(I meant my patch).

>>The patch is useless if these failures end up in system freezes on
>>other machines...
> 
> I know that my patch is useless in its current form, but I wanted to share
> it as a different way of doing things. 
> 
>>The problem with the old approach is, that after (doesn't matter C1-Cx)
>>sleep and dyn_idle_tick, the chance to wake up because of bm activity is
>>very likely.
>>You enter idle() again -> there was bm_activity -> C2. Wake up after e.g.
>>50ms, because of bm_activity again (bm_sts bit set) -> stay in C2, wake up
>>after 40ms -> bm activity... You only have the chance to get into deeper
>>states if the sleeps are interrupted by an interrupt, not bm activity.
> 
> That's a side-effect, indeed. However: if there _is_ bus master activity, we
> must not enter C3, AFAICS.
> 

What about a mixed approach: only reprogram timer if you want to go to deeper
sleeping states (C3-Cx) when bm activity comes in place?

It's the only way you can say: the last xy ms there was no bm activity (use bm_history),
now it's safe to sleep and also be efficient: don't sleep forever in C1/C2 -> bm_sts bit
will probably be set afterwards and you need to wait another xy ms in C1/C2
-> endless loop ...

Like that the timer is only disabled where it is really useful, on C3-Cx machines
(or are there other cases?).


    Thomas

Re: [PATCH] Updated: Dynamic Tick version 050408-1 - C-state measures

[Pavel Machek]

Hi!

> The machine is a Pentium M 2.00 GHz, supporting C0-C4 processor power states.
> The machine run at 2.00 GHz all the time.
..
> _passing bm_history=0xFFFFFFFF (default) to processor module:_
> 
> Average current the last 470 seconds: *1986mA* (also measured better
> values ~1800, does battery level play a role?!?)

Probably yes. If voltage changes, 2000mA means different ammount of power.


> (cmp. ftp://ftp.suse.com/pub/people/trenn/dyn_tick_c_states/measures_C4_machine/1000_HZ_bm_history_FFFFFFFF)
> 
> 
> _passing bm_history=0xFF to processor module:_
> 
> Average current the last 190 seconds: *1757mA*
> (cmp. ftp://ftp.suse.com/pub/people/trenn/dyn_tick_c_states/measures_C4_machine/1000_HZ_bm_history_FF)
> (Usage count could be bogus, as some invokations could not succeed
> if bm has currently been active).

Ok.

> idle_ms == 100, bm_promote_bs == 30
> Average current the last 80 seconds: *1466mA*
> (cmp.
> ftp://ftp.suse.com/pub/people/trenn/dyn_tick_c_states/measures_C4_machine/tony_dyn_tick_processor_idle_100_bm_30)

Very nice indeed. That seems like ~5W saved, right? That might give
you one more hour of battery life....
								Pavel

-- 
Boycott Kodak -- for their patent abuse against Java.

Re: [PATCH] Updated: Dynamic Tick version 050408-1 - C-state measures

[Tony Lindgren]

* Pavel Machek <pavel@suse.cz> [050419 14:10]:
> Hi!
> 
> > The machine is a Pentium M 2.00 GHz, supporting C0-C4 processor power states.
> > The machine run at 2.00 GHz all the time.
> ..
> > _passing bm_history=0xFFFFFFFF (default) to processor module:_
> > 
> > Average current the last 470 seconds: *1986mA* (also measured better
> > values ~1800, does battery level play a role?!?)
> 
> Probably yes. If voltage changes, 2000mA means different ammount of power.

Thomas, thanks for doing all the stats and patches to squeeze some
real power savings out of this! :)

We should display both average mA and average Watts with pmstats.
BTW, I've posted Thomas' version of pmstats as pmstats-0.2.gz to
muru.com also.

> > (cmp. ftp://ftp.suse.com/pub/people/trenn/dyn_tick_c_states/measures_C4_machine/1000_HZ_bm_history_FFFFFFFF)
> > 
> > 
> > _passing bm_history=0xFF to processor module:_
> > 
> > Average current the last 190 seconds: *1757mA*
> > (cmp. ftp://ftp.suse.com/pub/people/trenn/dyn_tick_c_states/measures_C4_machine/1000_HZ_bm_history_FF)
> > (Usage count could be bogus, as some invokations could not succeed
> > if bm has currently been active).
> 
> Ok.
> 
> > idle_ms == 100, bm_promote_bs == 30
> > Average current the last 80 seconds: *1466mA*
> > (cmp.
> > ftp://ftp.suse.com/pub/people/trenn/dyn_tick_c_states/measures_C4_machine/tony_dyn_tick_processor_idle_100_bm_30)
> 
> Very nice indeed. That seems like ~5W saved, right? That might give
> you one more hour of battery life....

Depending on your battery capacity. But looking at the average Watts
on the first 8 lines of the two stats above:

1000_HZ_bm_history_FFFFFFFF:
(21.43 + 23.32 + 23.32 + 21.71 + 21.71 + 23.84 + 23.84 + 22.62) / 8
= 22.724W

tony_dyn_tick_processor_idle_100_bm_30:
(16.07 + 16.07 + 16.00 + 16.00 + 16.08 + 16.08 + 16.29 + 16.29) / 8
= 16.11W

And then comparing these two:
22.72 / 16.11 = 1.4103

So according to my calculations this should provide about 1.4 times
longer battery life compared to what you were getting earlier...
That is assuming system is mostly idle, of course.

Tony

Re: [PATCH] Updated: Dynamic Tick version 050408-1 - C-state measures

[Thomas Renninger]

Tony Lindgren wrote:
> * Pavel Machek <pavel@suse.cz> [050419 14:10]:
>>Hi!
>>
>>>The machine is a Pentium M 2.00 GHz, supporting C0-C4 processor power states.
>>>The machine run at 2.00 GHz all the time.
>>..
>>>_passing bm_history=0xFFFFFFFF (default) to processor module:_
>>>
>>>Average current the last 470 seconds: *1986mA* (also measured better
>>>values ~1800, does battery level play a role?!?)
>>Probably yes. If voltage changes, 2000mA means different ammount of power.
> 
> Thomas, thanks for doing all the stats and patches to squeeze some
> real power savings out of this! :)
> 
> We should display both average mA and average Watts with pmstats.
> BTW, I've posted Thomas' version of pmstats as pmstats-0.2.gz to
> muru.com also.
> 
>>>(cmp. ftp://ftp.suse.com/pub/people/trenn/dyn_tick_c_states/measures_C4_machine/1000_HZ_bm_history_FFFFFFFF)
>>>
>>>
>>>_passing bm_history=0xFF to processor module:_
>>>
>>>Average current the last 190 seconds: *1757mA*
>>>(cmp. ftp://ftp.suse.com/pub/people/trenn/dyn_tick_c_states/measures_C4_machine/1000_HZ_bm_history_FF)
>>>(Usage count could be bogus, as some invokations could not succeed
>>>if bm has currently been active).
>>Ok.
>>
>>>idle_ms == 100, bm_promote_bs == 30
>>>Average current the last 80 seconds: *1466mA*
>>>(cmp.
>>>ftp://ftp.suse.com/pub/people/trenn/dyn_tick_c_states/measures_C4_machine/tony_dyn_tick_processor_idle_100_bm_30)
>>Very nice indeed. That seems like ~5W saved, right? That might give
>>you one more hour of battery life....
> 
> Depending on your battery capacity. But looking at the average Watts
> on the first 8 lines of the two stats above:
> 
> 1000_HZ_bm_history_FFFFFFFF:
> (21.43 + 23.32 + 23.32 + 21.71 + 21.71 + 23.84 + 23.84 + 22.62) / 8
> = 22.724W
> 
> tony_dyn_tick_processor_idle_100_bm_30:
> (16.07 + 16.07 + 16.00 + 16.00 + 16.08 + 16.08 + 16.29 + 16.29) / 8
> = 16.11W
> 
> And then comparing these two:
> 22.72 / 16.11 = 1.4103
> 
> So according to my calculations this should provide about 1.4 times
> longer battery life compared to what you were getting earlier...
> That is assuming system is mostly idle, of course.
> 
Be aware that speedstep was off (2.0 GHz). When CPU frequency is controlled
you won't have that much enhancement anymore ...

       Thomas