All of lore.kernel.org
 help / color / mirror / Atom feed
From: Kevin Hilman <khilman@ti.com>
To: Trinabh Gupta <trinabh@linux.vnet.ibm.com>
Cc: arjan@linux.intel.com, peterz@infradead.org, lenb@kernel.org,
	venki@google.com, ak@linux.intel.com, len.brown@intel.com,
	davinci-linux-open-source@linux.davincidsp.com,
	linux-sh@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-acpi@vger.kernel.org, linux-pm@lists.linux-foundation.org,
	linux-omap@vger.kernel.org, linux-arm-kernel@lists.infradead.org
Subject: Re: [linux-pm] [RFC PATCH V3 1/4] cpuidle: Move dev->last_residency update to driver	enter routine;
Date: Wed, 20 Apr 2011 17:27:15 +0000	[thread overview]
Message-ID: <87pqog6dfw.fsf@ti.com> (raw)
In-Reply-To: <20110420065523.332.58678.stgit@tringupt.in.ibm.com> (Trinabh Gupta's message of "Wed, 20 Apr 2011 12:25:34 +0530")

Trinabh Gupta <trinabh@linux.vnet.ibm.com> writes:

> Cpuidle subsystem only suggests the state to enter and does not
> guarantee if the suggested state is entered. The actual entered state
> may be different because of software or hardware demotion. Software
> demotion is done by the back-end cpuidle driver and can be accounted
> correctly. Current cpuidle code uses last_state field to capture the
> actual state entered and based on that updates the statistics for the
> state entered.
>
> Ideally the driver enter routine should update the counters,
> and it should return the state actually entered rather than the time
> spent there. 

OK, the return type was changed to return the state index instead of the
time, but since the governors are still relying on dev->last_residency,
drivers are required to update it.

Because of that, why not keep the update of the  time/usage counters
in common code rather than duplicating it (9 times in this patch) into 
all the drivers?

Something like the patch below should suffice.

Kevin


diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index 845d3ef..875d241 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -91,6 +91,11 @@ static void cpuidle_idle_call(void)
 
 	entered_state = target_state->enter(dev, drv, next_state);
 
+	/* Update cpuidle counters */
+	dev->states_usage[entered_state].time += 
+		(unsigned long long)dev->last_residency;
+	dev->states_usage[entered_state].usage++;
+
 	trace_power_end(dev->cpu);
 	trace_cpu_idle(PWR_EVENT_EXIT, dev->cpu);
 

WARNING: multiple messages have this Message-ID (diff)
From: Kevin Hilman <khilman@ti.com>
To: Trinabh Gupta <trinabh@linux.vnet.ibm.com>
Cc: arjan@linux.intel.com, peterz@infradead.org, lenb@kernel.org,
	venki@google.com, ak@linux.intel.com, len.brown@intel.com,
	davinci-linux-open-source@linux.davincidsp.com,
	linux-sh@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-acpi@vger.kernel.org, linux-pm@lists.linux-foundation.org,
	linux-omap@vger.kernel.org, linux-arm-kernel@lists.infradead.org
Subject: Re: [linux-pm] [RFC PATCH V3 1/4] cpuidle: Move dev->last_residency update to driver	enter routine; remove dev->last_state
Date: Wed, 20 Apr 2011 10:27:15 -0700	[thread overview]
Message-ID: <87pqog6dfw.fsf@ti.com> (raw)
In-Reply-To: <20110420065523.332.58678.stgit@tringupt.in.ibm.com> (Trinabh Gupta's message of "Wed, 20 Apr 2011 12:25:34 +0530")

Trinabh Gupta <trinabh@linux.vnet.ibm.com> writes:

> Cpuidle subsystem only suggests the state to enter and does not
> guarantee if the suggested state is entered. The actual entered state
> may be different because of software or hardware demotion. Software
> demotion is done by the back-end cpuidle driver and can be accounted
> correctly. Current cpuidle code uses last_state field to capture the
> actual state entered and based on that updates the statistics for the
> state entered.
>
> Ideally the driver enter routine should update the counters,
> and it should return the state actually entered rather than the time
> spent there. 

OK, the return type was changed to return the state index instead of the
time, but since the governors are still relying on dev->last_residency,
drivers are required to update it.

Because of that, why not keep the update of the  time/usage counters
in common code rather than duplicating it (9 times in this patch) into 
all the drivers?

Something like the patch below should suffice.

Kevin


diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index 845d3ef..875d241 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -91,6 +91,11 @@ static void cpuidle_idle_call(void)
 
 	entered_state = target_state->enter(dev, drv, next_state);
 
+	/* Update cpuidle counters */
+	dev->states_usage[entered_state].time += 
+		(unsigned long long)dev->last_residency;
+	dev->states_usage[entered_state].usage++;
+
 	trace_power_end(dev->cpu);
 	trace_cpu_idle(PWR_EVENT_EXIT, dev->cpu);
 

WARNING: multiple messages have this Message-ID (diff)
From: khilman@ti.com (Kevin Hilman)
To: linux-arm-kernel@lists.infradead.org
Subject: [linux-pm] [RFC PATCH V3 1/4] cpuidle: Move dev->last_residency update to driver	enter routine; remove dev->last_state
Date: Wed, 20 Apr 2011 10:27:15 -0700	[thread overview]
Message-ID: <87pqog6dfw.fsf@ti.com> (raw)
In-Reply-To: <20110420065523.332.58678.stgit@tringupt.in.ibm.com> (Trinabh Gupta's message of "Wed, 20 Apr 2011 12:25:34 +0530")

Trinabh Gupta <trinabh@linux.vnet.ibm.com> writes:

> Cpuidle subsystem only suggests the state to enter and does not
> guarantee if the suggested state is entered. The actual entered state
> may be different because of software or hardware demotion. Software
> demotion is done by the back-end cpuidle driver and can be accounted
> correctly. Current cpuidle code uses last_state field to capture the
> actual state entered and based on that updates the statistics for the
> state entered.
>
> Ideally the driver enter routine should update the counters,
> and it should return the state actually entered rather than the time
> spent there. 

OK, the return type was changed to return the state index instead of the
time, but since the governors are still relying on dev->last_residency,
drivers are required to update it.

Because of that, why not keep the update of the  time/usage counters
in common code rather than duplicating it (9 times in this patch) into 
all the drivers?

Something like the patch below should suffice.

Kevin


diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index 845d3ef..875d241 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -91,6 +91,11 @@ static void cpuidle_idle_call(void)
 
 	entered_state = target_state->enter(dev, drv, next_state);
 
+	/* Update cpuidle counters */
+	dev->states_usage[entered_state].time += 
+		(unsigned long long)dev->last_residency;
+	dev->states_usage[entered_state].usage++;
+
 	trace_power_end(dev->cpu);
 	trace_cpu_idle(PWR_EVENT_EXIT, dev->cpu);
 

  reply	other threads:[~2011-04-20 17:27 UTC|newest]

Thread overview: 46+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2011-04-20  6:55 [RFC PATCH V3 0/4] cpuidle: global registration of idle states with per-cpu statistics Trinabh Gupta
2011-04-20  6:55 ` [RFC PATCH V3 0/4] cpuidle: global registration of idle states with Trinabh Gupta
2011-04-20  6:55 ` [RFC PATCH V3 0/4] cpuidle: global registration of idle states with per-cpu statistics Trinabh Gupta
2011-04-20  6:55 ` Trinabh Gupta
2011-04-20  6:55 ` [RFC PATCH V3 1/4] cpuidle: Move dev->last_residency update to driver enter routine; remove dev->last_state Trinabh Gupta
2011-04-20  6:55   ` [RFC PATCH V3 1/4] cpuidle: Move dev->last_residency update to driver Trinabh Gupta
2011-04-20  6:55   ` [RFC PATCH V3 1/4] cpuidle: Move dev->last_residency update to driver enter routine; remove dev->last_state Trinabh Gupta
2011-04-20 17:27   ` Kevin Hilman [this message]
2011-04-20 17:27     ` [linux-pm] " Kevin Hilman
2011-04-20 17:27     ` Kevin Hilman
2011-04-21  4:42     ` Trinabh Gupta
2011-04-21  4:42     ` [linux-pm] " Trinabh Gupta
2011-04-21  4:54       ` [linux-pm] [RFC PATCH V3 1/4] cpuidle: Move dev->last_residency Trinabh Gupta
2011-04-21  4:42       ` [linux-pm] [RFC PATCH V3 1/4] cpuidle: Move dev->last_residency update to driver enter routine; remove dev->last_state Trinabh Gupta
2011-04-20 17:27   ` Kevin Hilman
2011-04-20  6:55 ` Trinabh Gupta
2011-04-20  6:55 ` Trinabh Gupta
2011-04-20  6:55 ` [RFC PATCH V3 2/4] cpuidle: Remove CPUIDLE_FLAG_IGNORE and dev->prepare() Trinabh Gupta
2011-04-20  6:55 ` Trinabh Gupta
2011-04-20  6:56   ` [RFC PATCH V3 2/4] cpuidle: Remove CPUIDLE_FLAG_IGNORE and Trinabh Gupta
2011-04-20  6:55   ` [RFC PATCH V3 2/4] cpuidle: Remove CPUIDLE_FLAG_IGNORE and dev->prepare() Trinabh Gupta
2011-04-20  6:56 ` [RFC PATCH V3 3/4] Split cpuidle_state structure and move per-cpu statistics fields Trinabh Gupta
2011-04-20  6:56   ` [RFC PATCH V3 3/4] Split cpuidle_state structure and move per-cpu Trinabh Gupta
2011-04-20  6:56   ` [RFC PATCH V3 3/4] Split cpuidle_state structure and move per-cpu statistics fields Trinabh Gupta
2011-04-20  6:56 ` Trinabh Gupta
2011-04-20  6:56 ` [RFC PATCH V3 4/4] cpuidle: Single/Global registration of idle states Trinabh Gupta
2011-04-20  6:56 ` Trinabh Gupta
2011-04-20  6:57   ` Trinabh Gupta
2011-04-20  6:56   ` Trinabh Gupta
2011-04-20 17:33   ` Kevin Hilman
2011-04-20 17:33   ` [linux-pm] " Kevin Hilman
2011-04-20 17:33     ` Kevin Hilman
2011-04-20 17:33     ` Kevin Hilman
2011-04-20 17:33     ` Kevin Hilman
2011-04-21  4:53     ` Trinabh Gupta
2011-04-21  4:53       ` [linux-pm] [RFC PATCH V3 4/4] cpuidle: Single/Global registration Trinabh Gupta
2011-04-21  4:53       ` [linux-pm] [RFC PATCH V3 4/4] cpuidle: Single/Global registration of idle states Trinabh Gupta
2011-04-22 23:06       ` Kevin Hilman
2011-04-22 23:06         ` Kevin Hilman
2011-04-22 23:06         ` Kevin Hilman
2011-04-22 23:06       ` Kevin Hilman
2011-04-21  4:53     ` Trinabh Gupta
2011-04-25 12:00   ` [linux-pm] " Trinabh Gupta
2011-04-25 12:12     ` [linux-pm] [RFC PATCH V3 4/4] cpuidle: Single/Global registration Trinabh Gupta
2011-04-25 12:00     ` [linux-pm] [RFC PATCH V3 4/4] cpuidle: Single/Global registration of idle states Trinabh Gupta
2011-04-25 12:00   ` Trinabh Gupta

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87pqog6dfw.fsf@ti.com \
    --to=khilman@ti.com \
    --cc=ak@linux.intel.com \
    --cc=arjan@linux.intel.com \
    --cc=davinci-linux-open-source@linux.davincidsp.com \
    --cc=len.brown@intel.com \
    --cc=lenb@kernel.org \
    --cc=linux-acpi@vger.kernel.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-omap@vger.kernel.org \
    --cc=linux-pm@lists.linux-foundation.org \
    --cc=linux-sh@vger.kernel.org \
    --cc=peterz@infradead.org \
    --cc=trinabh@linux.vnet.ibm.com \
    --cc=venki@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.