* [PATCH v6 0/3] Move pm_runtime accounted time to raw nsec
@ 2019-01-22 14:24 Vincent Guittot
2019-01-22 14:24 ` [PATCH v6 1/2] PM-runtime: update accounting_timestamp only when enable Vincent Guittot
2019-01-22 14:24 ` [PATCH 2/2] PM-runtime: Replace jiffies based accounting with ktime-based accounting Vincent Guittot
0 siblings, 2 replies; 9+ messages in thread
From: Vincent Guittot @ 2019-01-22 14:24 UTC (permalink / raw)
To: linux-pm, linux-kernel, rjw, thara.gopinath, linux
Cc: ulf.hansson, Vincent Guittot
Move pm_runtime accounted time to raw nsec. The subject of the patchset
has changed as the 1st patch of the previous versions has been queued by
Rafael.
Patch 1 set accounting_timestamp to 0 in pm_runtime_init and update it when enable. So
we remove ordering constraint between timekeeping_init and pm_runtime_init
Patch 2 moves time accounting on raw ns. This patch initially used
ktime instead of raw ns but it was easier to move i915 driver on raw ns
than on ktime.
Changes since v5:
- removed patches already queued.
- set accounting_timestamp to 0 in pm_runtime_init and update it when enable
Changes since v4:
-Update commit message
Changes since v3:
- Rebase on v4.20-rc7 without patch that has been queued by Rafael
- Simplify the new interface pm_runtime_suspended_time()
Changes since v2:
- remove patch1 that has been queued by rafael
- add new interface in pm_runtime to get accounted time
- reorder patchset to prevent compilation error
Changes since v1:
- updated commit message of patch 1
- Added patches 2 & 3 to move runtime_pm accounting on raw ns
Thara Gopinath (1):
PM-runtime: Replace jiffies based accounting with ktime-based
accounting
Vincent Guittot (1):
PM-runtime: update accounting_timestamp only when enable
drivers/base/power/runtime.c | 21 +++++++++++++--------
drivers/base/power/sysfs.c | 11 ++++++++---
include/linux/pm.h | 6 +++---
3 files changed, 24 insertions(+), 14 deletions(-)
--
2.7.4
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v6 1/2] PM-runtime: update accounting_timestamp only when enable
2019-01-22 14:24 [PATCH v6 0/3] Move pm_runtime accounted time to raw nsec Vincent Guittot
@ 2019-01-22 14:24 ` Vincent Guittot
2019-01-22 15:13 ` Rafael J. Wysocki
2019-01-23 8:13 ` Ulf Hansson
2019-01-22 14:24 ` [PATCH 2/2] PM-runtime: Replace jiffies based accounting with ktime-based accounting Vincent Guittot
1 sibling, 2 replies; 9+ messages in thread
From: Vincent Guittot @ 2019-01-22 14:24 UTC (permalink / raw)
To: linux-pm, linux-kernel, rjw, thara.gopinath, linux
Cc: ulf.hansson, Vincent Guittot
Initializing accounting_timestamp to something different from 0 during
pm_runtime_init() doesn't make sense and put useless ordering constraint between
timekeeping_init() and pm_runtime_init().
PM runtime should start accounting time only when it is enable and discard
the period when disabled.
Set accounting_timestamp to now when enabling PM runtime.
Suggested-by: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
---
drivers/base/power/runtime.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index fb5e2b6..7df1d05 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -1306,6 +1306,10 @@ void pm_runtime_enable(struct device *dev)
spin_lock_irqsave(&dev->power.lock, flags);
+ /* About to enable runtime pm, set accounting_timestamp to now */
+ if (dev->power.disable_depth == 1)
+ dev->power.accounting_timestamp = jiffies;
+
if (dev->power.disable_depth > 0)
dev->power.disable_depth--;
else
@@ -1506,7 +1510,7 @@ void pm_runtime_init(struct device *dev)
dev->power.request_pending = false;
dev->power.request = RPM_REQ_NONE;
dev->power.deferred_resume = false;
- dev->power.accounting_timestamp = jiffies;
+ dev->power.accounting_timestamp = 0;
INIT_WORK(&dev->power.work, pm_runtime_work);
dev->power.timer_expires = 0;
--
2.7.4
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/2] PM-runtime: Replace jiffies based accounting with ktime-based accounting
2019-01-22 14:24 [PATCH v6 0/3] Move pm_runtime accounted time to raw nsec Vincent Guittot
2019-01-22 14:24 ` [PATCH v6 1/2] PM-runtime: update accounting_timestamp only when enable Vincent Guittot
@ 2019-01-22 14:24 ` Vincent Guittot
2019-01-22 14:27 ` Vincent Guittot
1 sibling, 1 reply; 9+ messages in thread
From: Vincent Guittot @ 2019-01-22 14:24 UTC (permalink / raw)
To: linux-pm, linux-kernel, rjw, thara.gopinath, linux
Cc: ulf.hansson, Vincent Guittot
From: Thara Gopinath <thara.gopinath@linaro.org>
This patch replaces jiffies based accounting for runtime_active_time
and runtime_suspended_time with ktime-based accounting. This makes the
runtime debug counters inline with genpd and other PM subsytems which
use ktime-based accounting.
Timekeeping is initialized before driver_init(). It's only at that time
that PM runtime can be enabled.
Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org>
[switch from ktime to raw nsec]
Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
---
drivers/base/power/runtime.c | 17 +++++++++--------
drivers/base/power/sysfs.c | 11 ++++++++---
include/linux/pm.h | 6 +++---
3 files changed, 20 insertions(+), 14 deletions(-)
diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index 7df1d05..e49b3a8 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -66,8 +66,8 @@ static int rpm_suspend(struct device *dev, int rpmflags);
*/
void update_pm_runtime_accounting(struct device *dev)
{
- unsigned long now = jiffies;
- unsigned long delta;
+ u64 now = ktime_to_ns(ktime_get());
+ u64 delta;
delta = now - dev->power.accounting_timestamp;
@@ -77,9 +77,9 @@ void update_pm_runtime_accounting(struct device *dev)
return;
if (dev->power.runtime_status == RPM_SUSPENDED)
- dev->power.suspended_jiffies += delta;
+ dev->power.suspended_time += delta;
else
- dev->power.active_jiffies += delta;
+ dev->power.active_time += delta;
}
static void __update_runtime_status(struct device *dev, enum rpm_status status)
@@ -90,16 +90,17 @@ static void __update_runtime_status(struct device *dev, enum rpm_status status)
u64 pm_runtime_suspended_time(struct device *dev)
{
- unsigned long flags, time;
+ u64 time;
+ unsigned long flags;
spin_lock_irqsave(&dev->power.lock, flags);
update_pm_runtime_accounting(dev);
- time = dev->power.suspended_jiffies;
+ time = dev->power.suspended_time;
spin_unlock_irqrestore(&dev->power.lock, flags);
- return jiffies_to_nsecs(time);
+ return time;
}
EXPORT_SYMBOL_GPL(pm_runtime_suspended_time);
@@ -1308,7 +1309,7 @@ void pm_runtime_enable(struct device *dev)
/* About to enable runtime pm, set accounting_timestamp to now */
if (dev->power.disable_depth == 1)
- dev->power.accounting_timestamp = jiffies;
+ dev->power.accounting_timestamp = ktime_to_ns(ktime_get());
if (dev->power.disable_depth > 0)
dev->power.disable_depth--;
diff --git a/drivers/base/power/sysfs.c b/drivers/base/power/sysfs.c
index d713738..96c8a22 100644
--- a/drivers/base/power/sysfs.c
+++ b/drivers/base/power/sysfs.c
@@ -125,9 +125,12 @@ static ssize_t runtime_active_time_show(struct device *dev,
struct device_attribute *attr, char *buf)
{
int ret;
+ u64 tmp;
spin_lock_irq(&dev->power.lock);
update_pm_runtime_accounting(dev);
- ret = sprintf(buf, "%i\n", jiffies_to_msecs(dev->power.active_jiffies));
+ tmp = dev->power.active_time;
+ do_div(tmp, NSEC_PER_MSEC);
+ ret = sprintf(buf, "%llu\n", tmp);
spin_unlock_irq(&dev->power.lock);
return ret;
}
@@ -138,10 +141,12 @@ static ssize_t runtime_suspended_time_show(struct device *dev,
struct device_attribute *attr, char *buf)
{
int ret;
+ u64 tmp;
spin_lock_irq(&dev->power.lock);
update_pm_runtime_accounting(dev);
- ret = sprintf(buf, "%i\n",
- jiffies_to_msecs(dev->power.suspended_jiffies));
+ tmp = dev->power.suspended_time;
+ do_div(tmp, NSEC_PER_MSEC);
+ ret = sprintf(buf, "%llu\n", tmp);
spin_unlock_irq(&dev->power.lock);
return ret;
}
diff --git a/include/linux/pm.h b/include/linux/pm.h
index 0bd9de1..3d2cbf9 100644
--- a/include/linux/pm.h
+++ b/include/linux/pm.h
@@ -633,9 +633,9 @@ struct dev_pm_info {
int runtime_error;
int autosuspend_delay;
u64 last_busy;
- unsigned long active_jiffies;
- unsigned long suspended_jiffies;
- unsigned long accounting_timestamp;
+ u64 active_time;
+ u64 suspended_time;
+ u64 accounting_timestamp;
#endif
struct pm_subsys_data *subsys_data; /* Owned by the subsystem. */
void (*set_latency_tolerance)(struct device *, s32);
--
2.7.4
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] PM-runtime: Replace jiffies based accounting with ktime-based accounting
2019-01-22 14:24 ` [PATCH 2/2] PM-runtime: Replace jiffies based accounting with ktime-based accounting Vincent Guittot
@ 2019-01-22 14:27 ` Vincent Guittot
0 siblings, 0 replies; 9+ messages in thread
From: Vincent Guittot @ 2019-01-22 14:27 UTC (permalink / raw)
To: open list:THERMAL, linux-kernel, Rafael J. Wysocki,
Thara Gopinath, Guenter Roeck
Cc: Ulf Hansson
On Tue, 22 Jan 2019 at 15:24, Vincent Guittot
<vincent.guittot@linaro.org> wrote:
>
> From: Thara Gopinath <thara.gopinath@linaro.org>
>
> This patch replaces jiffies based accounting for runtime_active_time
> and runtime_suspended_time with ktime-based accounting. This makes the
> runtime debug counters inline with genpd and other PM subsytems which
> use ktime-based accounting.
>
> Timekeeping is initialized before driver_init(). It's only at that time
> that PM runtime can be enabled.
>
> Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org>
> [switch from ktime to raw nsec]
> Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
Hi Ulf,
I haven't added you reviewed-by because of the changes in this new version.
Hi Guenter,
Could you test this version ?
Thanks,
Vincent
> ---
> drivers/base/power/runtime.c | 17 +++++++++--------
> drivers/base/power/sysfs.c | 11 ++++++++---
> include/linux/pm.h | 6 +++---
> 3 files changed, 20 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> index 7df1d05..e49b3a8 100644
> --- a/drivers/base/power/runtime.c
> +++ b/drivers/base/power/runtime.c
> @@ -66,8 +66,8 @@ static int rpm_suspend(struct device *dev, int rpmflags);
> */
> void update_pm_runtime_accounting(struct device *dev)
> {
> - unsigned long now = jiffies;
> - unsigned long delta;
> + u64 now = ktime_to_ns(ktime_get());
> + u64 delta;
>
> delta = now - dev->power.accounting_timestamp;
>
> @@ -77,9 +77,9 @@ void update_pm_runtime_accounting(struct device *dev)
> return;
>
> if (dev->power.runtime_status == RPM_SUSPENDED)
> - dev->power.suspended_jiffies += delta;
> + dev->power.suspended_time += delta;
> else
> - dev->power.active_jiffies += delta;
> + dev->power.active_time += delta;
> }
>
> static void __update_runtime_status(struct device *dev, enum rpm_status status)
> @@ -90,16 +90,17 @@ static void __update_runtime_status(struct device *dev, enum rpm_status status)
>
> u64 pm_runtime_suspended_time(struct device *dev)
> {
> - unsigned long flags, time;
> + u64 time;
> + unsigned long flags;
>
> spin_lock_irqsave(&dev->power.lock, flags);
>
> update_pm_runtime_accounting(dev);
> - time = dev->power.suspended_jiffies;
> + time = dev->power.suspended_time;
>
> spin_unlock_irqrestore(&dev->power.lock, flags);
>
> - return jiffies_to_nsecs(time);
> + return time;
> }
> EXPORT_SYMBOL_GPL(pm_runtime_suspended_time);
>
> @@ -1308,7 +1309,7 @@ void pm_runtime_enable(struct device *dev)
>
> /* About to enable runtime pm, set accounting_timestamp to now */
> if (dev->power.disable_depth == 1)
> - dev->power.accounting_timestamp = jiffies;
> + dev->power.accounting_timestamp = ktime_to_ns(ktime_get());
>
> if (dev->power.disable_depth > 0)
> dev->power.disable_depth--;
> diff --git a/drivers/base/power/sysfs.c b/drivers/base/power/sysfs.c
> index d713738..96c8a22 100644
> --- a/drivers/base/power/sysfs.c
> +++ b/drivers/base/power/sysfs.c
> @@ -125,9 +125,12 @@ static ssize_t runtime_active_time_show(struct device *dev,
> struct device_attribute *attr, char *buf)
> {
> int ret;
> + u64 tmp;
> spin_lock_irq(&dev->power.lock);
> update_pm_runtime_accounting(dev);
> - ret = sprintf(buf, "%i\n", jiffies_to_msecs(dev->power.active_jiffies));
> + tmp = dev->power.active_time;
> + do_div(tmp, NSEC_PER_MSEC);
> + ret = sprintf(buf, "%llu\n", tmp);
> spin_unlock_irq(&dev->power.lock);
> return ret;
> }
> @@ -138,10 +141,12 @@ static ssize_t runtime_suspended_time_show(struct device *dev,
> struct device_attribute *attr, char *buf)
> {
> int ret;
> + u64 tmp;
> spin_lock_irq(&dev->power.lock);
> update_pm_runtime_accounting(dev);
> - ret = sprintf(buf, "%i\n",
> - jiffies_to_msecs(dev->power.suspended_jiffies));
> + tmp = dev->power.suspended_time;
> + do_div(tmp, NSEC_PER_MSEC);
> + ret = sprintf(buf, "%llu\n", tmp);
> spin_unlock_irq(&dev->power.lock);
> return ret;
> }
> diff --git a/include/linux/pm.h b/include/linux/pm.h
> index 0bd9de1..3d2cbf9 100644
> --- a/include/linux/pm.h
> +++ b/include/linux/pm.h
> @@ -633,9 +633,9 @@ struct dev_pm_info {
> int runtime_error;
> int autosuspend_delay;
> u64 last_busy;
> - unsigned long active_jiffies;
> - unsigned long suspended_jiffies;
> - unsigned long accounting_timestamp;
> + u64 active_time;
> + u64 suspended_time;
> + u64 accounting_timestamp;
> #endif
> struct pm_subsys_data *subsys_data; /* Owned by the subsystem. */
> void (*set_latency_tolerance)(struct device *, s32);
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v6 1/2] PM-runtime: update accounting_timestamp only when enable
2019-01-22 14:24 ` [PATCH v6 1/2] PM-runtime: update accounting_timestamp only when enable Vincent Guittot
@ 2019-01-22 15:13 ` Rafael J. Wysocki
2019-01-22 16:01 ` Vincent Guittot
2019-01-23 8:13 ` Ulf Hansson
1 sibling, 1 reply; 9+ messages in thread
From: Rafael J. Wysocki @ 2019-01-22 15:13 UTC (permalink / raw)
To: Vincent Guittot
Cc: Linux PM, Linux Kernel Mailing List, Rafael J. Wysocki,
Thara Gopinath, Guenter Roeck, Ulf Hansson
On Tue, Jan 22, 2019 at 3:24 PM Vincent Guittot
<vincent.guittot@linaro.org> wrote:
>
> Initializing accounting_timestamp to something different from 0 during
> pm_runtime_init() doesn't make sense and put useless ordering constraint between
> timekeeping_init() and pm_runtime_init().
> PM runtime should start accounting time only when it is enable and discard
> the period when disabled.
> Set accounting_timestamp to now when enabling PM runtime.
>
> Suggested-by: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
> ---
> drivers/base/power/runtime.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> index fb5e2b6..7df1d05 100644
> --- a/drivers/base/power/runtime.c
> +++ b/drivers/base/power/runtime.c
> @@ -1306,6 +1306,10 @@ void pm_runtime_enable(struct device *dev)
>
> spin_lock_irqsave(&dev->power.lock, flags);
>
> + /* About to enable runtime pm, set accounting_timestamp to now */
> + if (dev->power.disable_depth == 1)
> + dev->power.accounting_timestamp = jiffies;
I would do this in the dev->power.disable_depth > 0 branch below.
Just check if disable_depth is zero after decrementing it and update
accounting_timestamp if so.
> +
> if (dev->power.disable_depth > 0)
> dev->power.disable_depth--;
> else
> @@ -1506,7 +1510,7 @@ void pm_runtime_init(struct device *dev)
> dev->power.request_pending = false;
> dev->power.request = RPM_REQ_NONE;
> dev->power.deferred_resume = false;
> - dev->power.accounting_timestamp = jiffies;
> + dev->power.accounting_timestamp = 0;
> INIT_WORK(&dev->power.work, pm_runtime_work);
>
> dev->power.timer_expires = 0;
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v6 1/2] PM-runtime: update accounting_timestamp only when enable
2019-01-22 15:13 ` Rafael J. Wysocki
@ 2019-01-22 16:01 ` Vincent Guittot
0 siblings, 0 replies; 9+ messages in thread
From: Vincent Guittot @ 2019-01-22 16:01 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Linux PM, Linux Kernel Mailing List, Rafael J. Wysocki,
Thara Gopinath, Guenter Roeck, Ulf Hansson
On Tue, 22 Jan 2019 at 16:14, Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Tue, Jan 22, 2019 at 3:24 PM Vincent Guittot
> <vincent.guittot@linaro.org> wrote:
> >
> > Initializing accounting_timestamp to something different from 0 during
> > pm_runtime_init() doesn't make sense and put useless ordering constraint between
> > timekeeping_init() and pm_runtime_init().
> > PM runtime should start accounting time only when it is enable and discard
> > the period when disabled.
> > Set accounting_timestamp to now when enabling PM runtime.
> >
> > Suggested-by: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> > Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
> > ---
> > drivers/base/power/runtime.c | 6 +++++-
> > 1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> > index fb5e2b6..7df1d05 100644
> > --- a/drivers/base/power/runtime.c
> > +++ b/drivers/base/power/runtime.c
> > @@ -1306,6 +1306,10 @@ void pm_runtime_enable(struct device *dev)
> >
> > spin_lock_irqsave(&dev->power.lock, flags);
> >
> > + /* About to enable runtime pm, set accounting_timestamp to now */
> > + if (dev->power.disable_depth == 1)
> > + dev->power.accounting_timestamp = jiffies;
>
> I would do this in the dev->power.disable_depth > 0 branch below.
> Just check if disable_depth is zero after decrementing it and update
> accounting_timestamp if so.
My goal was to prevent nested "if" which is sometime difficult to read.
I'm going to change that and send a new version
Vincent
>
> > +
> > if (dev->power.disable_depth > 0)
> > dev->power.disable_depth--;
> > else
> > @@ -1506,7 +1510,7 @@ void pm_runtime_init(struct device *dev)
> > dev->power.request_pending = false;
> > dev->power.request = RPM_REQ_NONE;
> > dev->power.deferred_resume = false;
> > - dev->power.accounting_timestamp = jiffies;
> > + dev->power.accounting_timestamp = 0;
> > INIT_WORK(&dev->power.work, pm_runtime_work);
> >
> > dev->power.timer_expires = 0;
> > --
> > 2.7.4
> >
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v6 1/2] PM-runtime: update accounting_timestamp only when enable
2019-01-22 14:24 ` [PATCH v6 1/2] PM-runtime: update accounting_timestamp only when enable Vincent Guittot
2019-01-22 15:13 ` Rafael J. Wysocki
@ 2019-01-23 8:13 ` Ulf Hansson
2019-01-23 8:50 ` Vincent Guittot
1 sibling, 1 reply; 9+ messages in thread
From: Ulf Hansson @ 2019-01-23 8:13 UTC (permalink / raw)
To: Vincent Guittot
Cc: Linux PM, Linux Kernel Mailing List, Rafael J. Wysocki,
Thara Gopinath, Guenter Roeck
On Tue, 22 Jan 2019 at 15:24, Vincent Guittot
<vincent.guittot@linaro.org> wrote:
>
> Initializing accounting_timestamp to something different from 0 during
> pm_runtime_init() doesn't make sense and put useless ordering constraint between
> timekeeping_init() and pm_runtime_init().
> PM runtime should start accounting time only when it is enable and discard
> the period when disabled.
> Set accounting_timestamp to now when enabling PM runtime.
Hmm, my first impression is that this sounds like a reasonable thing
to do. However, there are couple of more things to consider.
1) update_pm_runtime_accounting() is setting a new value of
dev->power.accounting_timestamp, no matter of whether runtime PM has
been enabled. That's seems wrong to me, at least from the perspective
of what we are trying to change here. So you probably needs to fix
that too.
2) I don't think you explicitly need to set
dev->power.accounting_timestamp to zero at pm_runtime_init(). Just
leave it uninitialized, as we are anyways going to initialize it
before we make use of it.
3) How will this change affect accounting while being system
suspended? As you know, the PM core disables and re-enables runtime PM
during a system suspend/resume sequence. It looks to me, that you
actually need to call update_pm_runtime_accounting() from
pm_runtime_enable|disable(), or something along those lines, as to get
the accounting correct, no?
>
> Suggested-by: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
> ---
> drivers/base/power/runtime.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> index fb5e2b6..7df1d05 100644
> --- a/drivers/base/power/runtime.c
> +++ b/drivers/base/power/runtime.c
> @@ -1306,6 +1306,10 @@ void pm_runtime_enable(struct device *dev)
>
> spin_lock_irqsave(&dev->power.lock, flags);
>
> + /* About to enable runtime pm, set accounting_timestamp to now */
> + if (dev->power.disable_depth == 1)
> + dev->power.accounting_timestamp = jiffies;
> +
> if (dev->power.disable_depth > 0)
> dev->power.disable_depth--;
> else
> @@ -1506,7 +1510,7 @@ void pm_runtime_init(struct device *dev)
> dev->power.request_pending = false;
> dev->power.request = RPM_REQ_NONE;
> dev->power.deferred_resume = false;
> - dev->power.accounting_timestamp = jiffies;
> + dev->power.accounting_timestamp = 0;
> INIT_WORK(&dev->power.work, pm_runtime_work);
>
> dev->power.timer_expires = 0;
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v6 1/2] PM-runtime: update accounting_timestamp only when enable
2019-01-23 8:13 ` Ulf Hansson
@ 2019-01-23 8:50 ` Vincent Guittot
2019-01-23 9:35 ` Ulf Hansson
0 siblings, 1 reply; 9+ messages in thread
From: Vincent Guittot @ 2019-01-23 8:50 UTC (permalink / raw)
To: Ulf Hansson
Cc: Linux PM, Linux Kernel Mailing List, Rafael J. Wysocki,
Thara Gopinath, Guenter Roeck
On Wed, 23 Jan 2019 at 09:14, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Tue, 22 Jan 2019 at 15:24, Vincent Guittot
> <vincent.guittot@linaro.org> wrote:
> >
> > Initializing accounting_timestamp to something different from 0 during
> > pm_runtime_init() doesn't make sense and put useless ordering constraint between
> > timekeeping_init() and pm_runtime_init().
> > PM runtime should start accounting time only when it is enable and discard
> > the period when disabled.
> > Set accounting_timestamp to now when enabling PM runtime.
>
> Hmm, my first impression is that this sounds like a reasonable thing
> to do. However, there are couple of more things to consider.
>
> 1) update_pm_runtime_accounting() is setting a new value of
> dev->power.accounting_timestamp, no matter of whether runtime PM has
> been enabled. That's seems wrong to me, at least from the perspective
> of what we are trying to change here. So you probably needs to fix
> that too.
note that whatever is done before enabling pm runtime, this will be
overwritten during the enablement.
I can add a clean up patch but this behavior is already there with jiffies.
Or do you think that update_pm_runtime_accounting can be called before
timekeeping_init() ?
>
> 2) I don't think you explicitly need to set
> dev->power.accounting_timestamp to zero at pm_runtime_init(). Just
> leave it uninitialized, as we are anyways going to initialize it
> before we make use of it.
yes probably
>
> 3) How will this change affect accounting while being system
> suspended? As you know, the PM core disables and re-enables runtime PM
So the time in system suspended will not be accounted
> during a system suspend/resume sequence. It looks to me, that you
> actually need to call update_pm_runtime_accounting() from
> pm_runtime_enable|disable(), or something along those lines, as to get
> the accounting correct, no?
I thought that everything was already done for jiffies too. As soon as
pm runtime is disable, we can't really account this time to
suspended_time or active_time because we don't have control anymore
I think that we don't need any additional changes for
pm_runtime_enable because we just init the accounting_timestamp and
there is no active or suspend runtime to account yet
For pm_runtime_disable(), I thought that everything was already there
otherwise it means that we already didn't account the time correctly.
I will have a deeper look at that
>
> >
> > Suggested-by: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> > Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
> > ---
> > drivers/base/power/runtime.c | 6 +++++-
> > 1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> > index fb5e2b6..7df1d05 100644
> > --- a/drivers/base/power/runtime.c
> > +++ b/drivers/base/power/runtime.c
> > @@ -1306,6 +1306,10 @@ void pm_runtime_enable(struct device *dev)
> >
> > spin_lock_irqsave(&dev->power.lock, flags);
> >
> > + /* About to enable runtime pm, set accounting_timestamp to now */
> > + if (dev->power.disable_depth == 1)
> > + dev->power.accounting_timestamp = jiffies;
> > +
> > if (dev->power.disable_depth > 0)
> > dev->power.disable_depth--;
> > else
> > @@ -1506,7 +1510,7 @@ void pm_runtime_init(struct device *dev)
> > dev->power.request_pending = false;
> > dev->power.request = RPM_REQ_NONE;
> > dev->power.deferred_resume = false;
> > - dev->power.accounting_timestamp = jiffies;
> > + dev->power.accounting_timestamp = 0;
> > INIT_WORK(&dev->power.work, pm_runtime_work);
> >
> > dev->power.timer_expires = 0;
> > --
> > 2.7.4
> >
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v6 1/2] PM-runtime: update accounting_timestamp only when enable
2019-01-23 8:50 ` Vincent Guittot
@ 2019-01-23 9:35 ` Ulf Hansson
0 siblings, 0 replies; 9+ messages in thread
From: Ulf Hansson @ 2019-01-23 9:35 UTC (permalink / raw)
To: Vincent Guittot
Cc: Linux PM, Linux Kernel Mailing List, Rafael J. Wysocki,
Thara Gopinath, Guenter Roeck
On Wed, 23 Jan 2019 at 09:50, Vincent Guittot
<vincent.guittot@linaro.org> wrote:
>
> On Wed, 23 Jan 2019 at 09:14, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >
> > On Tue, 22 Jan 2019 at 15:24, Vincent Guittot
> > <vincent.guittot@linaro.org> wrote:
> > >
> > > Initializing accounting_timestamp to something different from 0 during
> > > pm_runtime_init() doesn't make sense and put useless ordering constraint between
> > > timekeeping_init() and pm_runtime_init().
> > > PM runtime should start accounting time only when it is enable and discard
> > > the period when disabled.
> > > Set accounting_timestamp to now when enabling PM runtime.
> >
> > Hmm, my first impression is that this sounds like a reasonable thing
> > to do. However, there are couple of more things to consider.
> >
> > 1) update_pm_runtime_accounting() is setting a new value of
> > dev->power.accounting_timestamp, no matter of whether runtime PM has
> > been enabled. That's seems wrong to me, at least from the perspective
> > of what we are trying to change here. So you probably needs to fix
> > that too.
>
> note that whatever is done before enabling pm runtime, this will be
> overwritten during the enablement.
Right. After this patch, but the original code seems wrong, doesn't it?
> I can add a clean up patch but this behavior is already there with jiffies.
That would be great, to get this sorted out.
>
> Or do you think that update_pm_runtime_accounting can be called before
> timekeeping_init() ?
No.
>
> >
> > 2) I don't think you explicitly need to set
> > dev->power.accounting_timestamp to zero at pm_runtime_init(). Just
> > leave it uninitialized, as we are anyways going to initialize it
> > before we make use of it.
>
> yes probably
>
> >
> > 3) How will this change affect accounting while being system
> > suspended? As you know, the PM core disables and re-enables runtime PM
>
> So the time in system suspended will not be accounted
Right. But, I am not sure the accounting is correctly done.
>
> > during a system suspend/resume sequence. It looks to me, that you
> > actually need to call update_pm_runtime_accounting() from
> > pm_runtime_enable|disable(), or something along those lines, as to get
> > the accounting correct, no?
>
> I thought that everything was already done for jiffies too. As soon as
> pm runtime is disable, we can't really account this time to
> suspended_time or active_time because we don't have control anymore
Correct.
However, my point is that during system suspend, sooner or later we
end up disabling runtime PM. Up until that point, we should account
for, but it seems like we don't do that correctly. As instead, we just
keep the old accounting timestamp during system suspend (until someone
changes the runtime PM status, but there is now guarantee that
happens).
>
> I think that we don't need any additional changes for
> pm_runtime_enable because we just init the accounting_timestamp and
> there is no active or suspend runtime to account yet
Right, after a second thought, this makes sense.
> For pm_runtime_disable(), I thought that everything was already there
> otherwise it means that we already didn't account the time correctly.
> I will have a deeper look at that
Great!
I think the accounting is simply based upon that the "time" is stopped
during system suspend and thus also the accounting.
Although, as the header of $subject patch indicates, we want to move
to a well defined situation. Therefore it sounds to me that we need to
care about accounting while disabling runtime PM.
Anyway, let's see what you find out from your deeper look. :-)
[...]
Kind regards
Uffe
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-01-23 9:35 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-22 14:24 [PATCH v6 0/3] Move pm_runtime accounted time to raw nsec Vincent Guittot
2019-01-22 14:24 ` [PATCH v6 1/2] PM-runtime: update accounting_timestamp only when enable Vincent Guittot
2019-01-22 15:13 ` Rafael J. Wysocki
2019-01-22 16:01 ` Vincent Guittot
2019-01-23 8:13 ` Ulf Hansson
2019-01-23 8:50 ` Vincent Guittot
2019-01-23 9:35 ` Ulf Hansson
2019-01-22 14:24 ` [PATCH 2/2] PM-runtime: Replace jiffies based accounting with ktime-based accounting Vincent Guittot
2019-01-22 14:27 ` Vincent Guittot
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).