RE: [PATCH 5/5] processor: jiffies-based bm_check, bugfixes

RE: [PATCH 5/5] processor: jiffies-based bm_check, bugfixes

[Pallipadi, Venkatesh]

We have another patch to fix this bm_activity history, by doing periodic
bm checks. I will post it here as soon as I rebase the patch with new
processor_idle.c.

Thanks,
Venki 

>-----Original Message-----
>From: acpi-devel-admin-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org 
>[mailto:acpi-devel-admin-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org] On Behalf Of 
>Dominik Brodowski
>Sent: Thursday, December 23, 2004 6:09 AM
>To: acpi-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org; Brown, Len
>Subject: [ACPI] [PATCH 5/5] processor: jiffies-based bm_check, bugfixes
>
>Make the bm_activity depend on "jiffies", instead of numbers
>of the check being called. This means bus mastering activity
>is assumed if bm_check isn't called; and multiple calls during
>one jiffy will be |='ed.
>
>Also, two fixups where promotion and demotion were mixed up.
>
>Signed-off-by: Dominik Brodowski <linux-JhLEnvuH02M@public.gmane.org>
>---
>
> drivers/acpi/processor_idle.c |   19 ++++++++++++++++---
> include/acpi/processor.h      |    1 +
> 2 files changed, 17 insertions(+), 3 deletions(-)
>
>Index: linux-2.6.10-rc3+bk-acpi/drivers/acpi/processor_idle.c
>===================================================================
>--- 
>linux-2.6.10-rc3+bk-acpi.orig/drivers/acpi/processor_idle.c	
>2004-12-23 14:25:32.385908579 +0100
>+++ linux-2.6.10-rc3+bk-acpi/drivers/acpi/processor_idle.c	
>2004-12-23 14:39:58.360081349 +0100
>@@ -193,8 +193,18 @@
> 	 */
> 	if (pr->flags.bm_check) {
> 		u32		bm_status = 0;
>+		unsigned long	diff = jiffies - 
>pr->power.bm_check_timestamp;
> 
>-		pr->power.bm_activity <<= 1;
>+		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);
>@@ -213,6 +223,9 @@
> 				|| (inb_p(errata.piix4.bmisx + 
>0x0A) & 0x01))
> 				pr->power.bm_activity++;
> 		}
>+
>+		pr->power.bm_check_timestamp = jiffies;
>+
> 		/*
> 		 * Apply bus mastering demotion policy.  
>Automatically demote
> 		 * to avoid a faulty transition.  Note that the 
>processor
>@@ -439,13 +452,13 @@
> 
> 		if (higher) {
> 			cx->promotion.state  = higher;
>-			cx->demotion.threshold.ticks = 
>cx->latency_ticks;
>+			cx->promotion.threshold.ticks = 
>cx->latency_ticks;
> 			if (cx->type >= ACPI_STATE_C2)
> 				cx->promotion.threshold.count = 4;
> 			else
> 				cx->promotion.threshold.count = 10;
> 			if (higher->type == ACPI_STATE_C3)
>-				cx->demotion.threshold.bm = 0x0F;
>+				cx->promotion.threshold.bm = 0x0F;
> 		}
> 
> 		higher = cx;
>Index: linux-2.6.10-rc3+bk-acpi/include/acpi/processor.h
>===================================================================
>--- linux-2.6.10-rc3+bk-acpi.orig/include/acpi/processor.h	
>2004-12-23 13:21:12.000000000 +0100
>+++ linux-2.6.10-rc3+bk-acpi/include/acpi/processor.h	
>2004-12-23 14:41:47.558845116 +0100
>@@ -54,6 +54,7 @@
> 
> struct acpi_processor_power {
> 	struct acpi_processor_cx *state;
>+	unsigned long		bm_check_timestamp;
> 	u32			default_state;
> 	u32			bm_activity;
> 	int			count;
>
>
>-------------------------------------------------------
>SF email is sponsored by - The IT Product Guide
>Read honest & candid reviews on hundreds of IT Products from 
>real users.
>Discover which products truly live up to the hype. Start reading now. 
>http://productguide.itmanagersjournal.com/
>_______________________________________________
>Acpi-devel mailing list
>Acpi-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
>https://lists.sourceforge.net/lists/listinfo/acpi-devel
>


-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real users.
Discover which products truly live up to the hype. Start reading now.
http://productguide.itmanagersjournal.com/

Re: [PATCH 5/5] processor: jiffies-based bm_check, bugfixes

[Dominik Brodowski]

On Thu, Dec 23, 2004 at 10:31:50AM -0800, Pallipadi, Venkatesh wrote:
> 
> We have another patch to fix this bm_activity history, by doing periodic
> bm checks.

periodic means overhead even if the system isn't idle. So I'm not convinced
this is the better idea.

Thanks,
	Dominik


-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real users.
Discover which products truly live up to the hype. Start reading now. 
http://productguide.itmanagersjournal.com/

Re: [PATCH 5/5] processor: jiffies-based bm_check, bugfixes

[Venkatesh Pallipadi]

The patch below helps us maintain a proper bm_activity history. We monitor
the bm_activity every 10mS. Using the current 32 bit bm_activity we can store
the history of upto 320mS.

As Dominik pointed out, it has a disadvantage of adding some overhead, even
when CPU is not idle. But, the advantage is that it has a good bm_activity 
history for 320 mS, which can be useful in future, to have more fancier C-state
policies.

Thanks,
Venki

Signed-off-by: Venkatesh Pallipadi <venkatesh.pallipadi-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

--- linux-2.6.10-rc3-cst/drivers/acpi/processor_idle.c.org	2004-12-23 12:48:01.000000000 -0800
+++ linux-2.6.10-rc3-cst/drivers/acpi/processor_idle.c	2004-12-23 15:59:35.000000000 -0800
@@ -36,6 +36,9 @@
 #include <linux/dmi.h>
 #include <linux/moduleparam.h>
 
+#include <linux/workqueue.h>
+#include <linux/compiler.h>
+
 #include <asm/io.h>
 #include <asm/uaccess.h>
 
@@ -108,6 +111,121 @@ ticks_elapsed (
 }
 
 
+static void do_bm_check_timer(void *data);
+static DECLARE_MUTEX	(bm_check_sem);
+static DECLARE_WORK	(bm_check_work, do_bm_check_timer, NULL);
+
+#define BM_CHECK_START 	1
+#define BM_CHECK_STOP 	2
+
+#define BM_CHECK_RATE 	((HZ < 100) ? (1) : (HZ/100))
+
+u32
+acpi_get_bm_status (u32 clear_status)
+{
+	u32		bm_status = 0;
+
+	down(&bm_check_sem);
+	/*
+	 * Check for bus mastering activity
+	 */
+	acpi_get_register(ACPI_BITREG_BUS_MASTER_STATUS, 
+		&bm_status, ACPI_MTX_DO_NOT_LOCK);
+	if (bm_status && clear_status) {
+		/* Clear BM status bit */
+		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))
+			bm_status++;
+	}
+
+	up(&bm_check_sem);
+	return (bm_status);
+}
+
+void
+acpi_bm_activity_update (unsigned long cpu, unsigned long num_bits)
+{
+	struct acpi_processor	*pr = NULL;
+
+	pr = processors[cpu];
+	if (!pr)
+		return;
+
+	if (unlikely(num_bits > 32))
+		num_bits = 32;
+
+	/*
+	 * Check BM Activity
+	 * -----------------
+	 * Check for bus mastering activity (if required), record, and check
+	 * for demotion.
+	 */
+	if (pr->flags.bm_check) {
+		u32 status;
+		int i;
+		status = acpi_get_bm_status(1);
+		for (i = 0; i < num_bits; i++) {
+			pr->power.bm_activity <<= 1;
+			pr->power.bm_activity |= (status & 0x1);
+		}
+	}
+}		
+
+static void 
+do_bm_check_timer(void *data)
+{
+	static unsigned long last_jiffies;
+	unsigned long this_jiffies;
+	unsigned long i, num_bits;
+
+	/*
+	 * Number of bits in the history that we are going to update.
+	 * We want one bit per 10mS. So, if we are not called late, 
+	 * update that many bits in history.
+	 */
+	this_jiffies = jiffies;
+	num_bits = (this_jiffies/BM_CHECK_RATE) - (last_jiffies/BM_CHECK_RATE);
+
+	/*
+	 * It should be enough if we do this check on one CPU.
+	 * Do it only on current CPU for now.
+	 */
+	acpi_bm_activity_update(smp_processor_id(), num_bits);
+
+	last_jiffies = this_jiffies;
+	schedule_delayed_work(&bm_check_work, BM_CHECK_RATE);
+}
+
+static void
+setup_periodic_bm_status_check(int opt)
+{
+	static int bm_status_check_count;
+
+	if (opt == BM_CHECK_START)
+		bm_status_check_count++;
+	else if (opt == BM_CHECK_STOP)
+		bm_status_check_count--;
+
+	if (bm_status_check_count == 1) {
+		INIT_WORK(&bm_check_work, do_bm_check_timer, NULL);
+		schedule_work(&bm_check_work);
+	} else if (bm_status_check_count == 0) {
+		cancel_delayed_work(&bm_check_work);
+	}
+	return;
+}
+ 
+
 static void
 acpi_processor_power_activate (
 	struct acpi_processor	*pr,
@@ -188,27 +306,9 @@ static void acpi_processor_idle (void)
 	 * for demotion.
 	 */
 	if (pr->flags.bm_check) {
-		u32		bm_status = 0;
+		unsigned long bm_activity;
 
-		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++;
-		}
+		bm_activity = pr->power.bm_activity | acpi_get_bm_status(0);
 		/*
 		 * Apply bus mastering demotion policy.  Automatically demote
 		 * to avoid a faulty transition.  Note that the processor
@@ -216,12 +316,12 @@ static void acpi_processor_idle (void)
 		 * funciton) but should upon the next.
 		 *
 		 * TBD: A better policy might be to fallback to the demotion
-		 *      state (use it for this quantum only) istead of
+		 *      state (use it for this quantum only) instead of
 		 *      demoting -- and rely on duration as our sole demotion
 		 *      qualification.  This may, however, introduce DMA
 		 *      issues (e.g. floppy DMA transfer overrun/underrun).
 		 */
-		if (pr->power.bm_activity & cx->demotion.threshold.bm) {
+		if (bm_activity & cx->demotion.threshold.bm) {
 			local_irq_enable();
 			next_state = cx->demotion.state;
 			goto end;
@@ -420,7 +520,7 @@ acpi_processor_set_power_policy (
 			cx->demotion.threshold.ticks = cx->latency_ticks;
 			cx->demotion.threshold.count = 1;
 			if (cx->type == ACPI_STATE_C3)
-				cx->demotion.threshold.bm = 0x0F;
+				cx->demotion.threshold.bm = 0x07;
 		}
 
 		/* from C3 we always demote to C2, even if there are multiple
@@ -443,7 +543,7 @@ acpi_processor_set_power_policy (
 			else
 				cx->promotion.threshold.count = 10;
 			if (higher->type == ACPI_STATE_C3)
-				cx->demotion.threshold.bm = 0x0F;
+				cx->demotion.threshold.bm = 0x07;
 		}
 
 		higher = cx;
@@ -710,6 +810,7 @@ static void acpi_processor_power_verify_
 	cx->valid = 1;
 	cx->latency_ticks = US_TO_PM_TIMER_TICKS(cx->latency);
 	pr->flags.bm_check = 1;
+	setup_periodic_bm_status_check(BM_CHECK_START);
 
 	return_VOID;
 }
@@ -985,5 +1086,8 @@ int acpi_processor_power_exit(struct acp
 		synchronize_kernel();
 	}
 
+	if (pr->flags.bm_check == 1)
+		setup_periodic_bm_status_check(BM_CHECK_STOP);
+
 	return_VALUE(0);
 }


-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real users.
Discover which products truly live up to the hype. Start reading now. 
http://productguide.itmanagersjournal.com/

Re: [PATCH 5/5] processor: jiffies-based bm_check, bugfixes

[Dominik Brodowski]

On Thu, Dec 23, 2004 at 01:41:18PM -0800, Venkatesh Pallipadi wrote:
> 
> 
> The patch below helps us maintain a proper bm_activity history. We monitor
> the bm_activity every 10mS. Using the current 32 bit bm_activity we can store
> the history of upto 320mS.

While this information may be useful, the side effects are very problematic:
advancing the kernel to be "tickless" won't save much any more: the idle
code itself will make sure the system is woken up every 10ms. And that in
addition to the overhead if the CPU isn't idle...

> @@ -710,6 +810,7 @@ static void acpi_processor_power_verify_
>  	cx->valid = 1;
>  	cx->latency_ticks = US_TO_PM_TIMER_TICKS(cx->latency);
>  	pr->flags.bm_check = 1;
> +	setup_periodic_bm_status_check(BM_CHECK_START);

This can be called multiple times, though the BM_CHECK_STOP in _power_exit()

> @@ -985,5 +1086,8 @@ int acpi_processor_power_exit(struct acp
>  		synchronize_kernel();
>  	}
>  
> +	if (pr->flags.bm_check == 1)
> +		setup_periodic_bm_status_check(BM_CHECK_STOP);
> +

will only be called one time per processor. This means the bm_check handler
isn't unregistered but still called, which can cause oopses.

Merry Christmas,

	Dominik


-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real users.
Discover which products truly live up to the hype. Start reading now. 
http://productguide.itmanagersjournal.com/

[PATCH 5/5] processor: jiffies-based bm_check, bugfixes

[Dominik Brodowski]

Make the bm_activity depend on "jiffies", instead of numbers
of the check being called. This means bus mastering activity
is assumed if bm_check isn't called; and multiple calls during
one jiffy will be |='ed.

Also, two fixups where promotion and demotion were mixed up.

Signed-off-by: Dominik Brodowski <linux-JhLEnvuH02M@public.gmane.org>
---

 drivers/acpi/processor_idle.c |   19 ++++++++++++++++---
 include/acpi/processor.h      |    1 +
 2 files changed, 17 insertions(+), 3 deletions(-)

Index: linux-2.6.10-rc3+bk-acpi/drivers/acpi/processor_idle.c
===================================================================
--- linux-2.6.10-rc3+bk-acpi.orig/drivers/acpi/processor_idle.c	2004-12-23 14:25:32.385908579 +0100
+++ linux-2.6.10-rc3+bk-acpi/drivers/acpi/processor_idle.c	2004-12-23 14:39:58.360081349 +0100
@@ -193,8 +193,18 @@
 	 */
 	if (pr->flags.bm_check) {
 		u32		bm_status = 0;
+		unsigned long	diff = jiffies - pr->power.bm_check_timestamp;
 
-		pr->power.bm_activity <<= 1;
+		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);
@@ -213,6 +223,9 @@
 				|| (inb_p(errata.piix4.bmisx + 0x0A) & 0x01))
 				pr->power.bm_activity++;
 		}
+
+		pr->power.bm_check_timestamp = jiffies;
+
 		/*
 		 * Apply bus mastering demotion policy.  Automatically demote
 		 * to avoid a faulty transition.  Note that the processor
@@ -439,13 +452,13 @@
 
 		if (higher) {
 			cx->promotion.state  = higher;
-			cx->demotion.threshold.ticks = cx->latency_ticks;
+			cx->promotion.threshold.ticks = cx->latency_ticks;
 			if (cx->type >= ACPI_STATE_C2)
 				cx->promotion.threshold.count = 4;
 			else
 				cx->promotion.threshold.count = 10;
 			if (higher->type == ACPI_STATE_C3)
-				cx->demotion.threshold.bm = 0x0F;
+				cx->promotion.threshold.bm = 0x0F;
 		}
 
 		higher = cx;
Index: linux-2.6.10-rc3+bk-acpi/include/acpi/processor.h
===================================================================
--- linux-2.6.10-rc3+bk-acpi.orig/include/acpi/processor.h	2004-12-23 13:21:12.000000000 +0100
+++ linux-2.6.10-rc3+bk-acpi/include/acpi/processor.h	2004-12-23 14:41:47.558845116 +0100
@@ -54,6 +54,7 @@
 
 struct acpi_processor_power {
 	struct acpi_processor_cx *state;
+	unsigned long		bm_check_timestamp;
 	u32			default_state;
 	u32			bm_activity;
 	int			count;


-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real users.
Discover which products truly live up to the hype. Start reading now. 
http://productguide.itmanagersjournal.com/

Re: [PATCH 5/5] processor: jiffies-based bm_check, bugfixes

[Len Brown]

Applied the promotion/demotion bugfix part.

Didn't apply the jiffies part yet.

thanks,
-Len



On Thu, 2004-12-23 at 09:08, Dominik Brodowski wrote:
> Make the bm_activity depend on "jiffies", instead of numbers
> of the check being called. This means bus mastering activity
> is assumed if bm_check isn't called; and multiple calls during
> one jiffy will be |='ed.
> 
> Also, two fixups where promotion and demotion were mixed up.
> 
> Signed-off-by: Dominik Brodowski <linux-JhLEnvuH02M@public.gmane.org>
> ---
> 
>  drivers/acpi/processor_idle.c |   19 ++++++++++++++++---
>  include/acpi/processor.h      |    1 +
>  2 files changed, 17 insertions(+), 3 deletions(-)
> 
> Index: linux-2.6.10-rc3+bk-acpi/drivers/acpi/processor_idle.c
> ===================================================================
> --- linux-2.6.10-rc3+bk-acpi.orig/drivers/acpi/processor_idle.c
> 2004-12-23 14:25:32.385908579 +0100
> +++ linux-2.6.10-rc3+bk-acpi/drivers/acpi/processor_idle.c     
> 2004-12-23 14:39:58.360081349 +0100
> @@ -193,8 +193,18 @@
>          */
>         if (pr->flags.bm_check) {
>                 u32             bm_status = 0;
> +               unsigned long   diff = jiffies -
> pr->power.bm_check_timestamp;
> 
> -               pr->power.bm_activity <<= 1;
> +               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);
> @@ -213,6 +223,9 @@
>                                 || (inb_p(errata.piix4.bmisx + 0x0A) &
> 0x01))
>                                 pr->power.bm_activity++;
>                 }
> +
> +               pr->power.bm_check_timestamp = jiffies;
> +
>                 /*
>                  * Apply bus mastering demotion policy.  Automatically
> demote
>                  * to avoid a faulty transition.  Note that the
> processor
> @@ -439,13 +452,13 @@
> 
>                 if (higher) {
>                         cx->promotion.state  = higher;
> -                       cx->demotion.threshold.ticks =
> cx->latency_ticks;
> +                       cx->promotion.threshold.ticks =
> cx->latency_ticks;
>                         if (cx->type >= ACPI_STATE_C2)
>                                 cx->promotion.threshold.count = 4;
>                         else
>                                 cx->promotion.threshold.count = 10;
>                         if (higher->type == ACPI_STATE_C3)
> -                               cx->demotion.threshold.bm = 0x0F;
> +                               cx->promotion.threshold.bm = 0x0F;
>                 }
> 
>                 higher = cx;
> Index: linux-2.6.10-rc3+bk-acpi/include/acpi/processor.h
> ===================================================================
> --- linux-2.6.10-rc3+bk-acpi.orig/include/acpi/processor.h     
> 2004-12-23 13:21:12.000000000 +0100
> +++ linux-2.6.10-rc3+bk-acpi/include/acpi/processor.h   2004-12-23
> 14:41:47.558845116 +0100
> @@ -54,6 +54,7 @@
> 
>  struct acpi_processor_power {
>         struct acpi_processor_cx *state;
> +       unsigned long           bm_check_timestamp;
>         u32                     default_state;
>         u32                     bm_activity;
>         int                     count;
> 
> 



-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real users.
Discover which products truly live up to the hype. Start reading now. 
http://productguide.itmanagersjournal.com/