* [PATCH 0/2] PM-runtime: fix and clean up of time accounting
@ 2019-02-04 16:25 Vincent Guittot
2019-02-04 16:25 ` [PATCH 1/2] PM-runtime: move runtime accounting on ktime_get_mono_fast_ns() Vincent Guittot
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Vincent Guittot @ 2019-02-04 16:25 UTC (permalink / raw)
To: linux-pm, linux-kernel, rjw, ulf.hansson, biju.das
Cc: linux-renesas-soc, Vincent Guittot, geert, thara.gopinath,
linux-arm-kernel
Fix time accounting which has the same lock contraint as for using hrtimer
and update accounting_timestamp only when useful.
Vincent Guittot (2):
PM-runtime: move runtime accounting on ktime_get_mono_fast_ns()
PM-runtime: update time accounting only when enabled
drivers/base/power/runtime.c | 25 ++++++++++++++++++-------
1 file changed, 18 insertions(+), 7 deletions(-)
--
2.7.4
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/2] PM-runtime: move runtime accounting on ktime_get_mono_fast_ns()
2019-02-04 16:25 [PATCH 0/2] PM-runtime: fix and clean up of time accounting Vincent Guittot
@ 2019-02-04 16:25 ` Vincent Guittot
2019-02-05 8:09 ` Ulf Hansson
2019-02-04 16:25 ` [PATCH 2/2] PM-runtime: update time accounting only when enabled Vincent Guittot
2019-02-06 10:27 ` [PATCH 0/2] PM-runtime: fix and clean up of time accounting Rafael J. Wysocki
2 siblings, 1 reply; 7+ messages in thread
From: Vincent Guittot @ 2019-02-04 16:25 UTC (permalink / raw)
To: linux-pm, linux-kernel, rjw, ulf.hansson, biju.das
Cc: linux-renesas-soc, Vincent Guittot, geert, thara.gopinath,
linux-arm-kernel
Similarly to what happened whith autosuspend, a deadlock has been raised
with runtime accounting in the sequence:
change_clocksource
...
write_seqcount_begin
...
timekeeping_update
...
sh_cmt_clocksource_enable
...
rpm_resume
update_pm_runtime_accounting
ktime_get
do
read_seqcount_begin
while read_seqcount_retry
....
write_seqcount_end
Move runtime accounting on ktime_get_mono_fast_ns()
With ktime_get_mono_fast_ns, the timestamp is not guaranteed to be
monotonic across an update of timekeeping and as a result time can goes
backward. Add a test to skip accounting for such situation which should
stay exceptional.
Fixes: a08c2a5a3194 ("PM-runtime: Replace jiffies-based accounting with ktime-based accounting")
Reported-by: Biju Das <biju.das@bp.renesas.com>
Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
---
drivers/base/power/runtime.c | 17 +++++++++++++----
1 file changed, 13 insertions(+), 4 deletions(-)
diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index 1caa394..50740da 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -66,13 +66,22 @@ static int rpm_suspend(struct device *dev, int rpmflags);
*/
void update_pm_runtime_accounting(struct device *dev)
{
- u64 now = ktime_to_ns(ktime_get());
+ u64 now = ktime_get_mono_fast_ns();
+ u64 last = dev->power.accounting_timestamp;
u64 delta;
- delta = now - dev->power.accounting_timestamp;
-
dev->power.accounting_timestamp = now;
+ /*
+ * Because ktime_get_mono_fast_ns() is not monotonic during
+ * timekeeping update, we must ensure that now is after the last saved
+ * timesptamp
+ */
+ if (now < last)
+ return;
+
+ delta = now - last;
+
if (dev->power.disable_depth > 0)
return;
@@ -1312,7 +1321,7 @@ void pm_runtime_enable(struct device *dev)
/* About to enable runtime pm, set accounting_timestamp to now */
if (!dev->power.disable_depth)
- dev->power.accounting_timestamp = ktime_to_ns(ktime_get());
+ dev->power.accounting_timestamp = ktime_get_mono_fast_ns();
} else {
dev_warn(dev, "Unbalanced %s!\n", __func__);
}
--
2.7.4
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/2] PM-runtime: update time accounting only when enabled
2019-02-04 16:25 [PATCH 0/2] PM-runtime: fix and clean up of time accounting Vincent Guittot
2019-02-04 16:25 ` [PATCH 1/2] PM-runtime: move runtime accounting on ktime_get_mono_fast_ns() Vincent Guittot
@ 2019-02-04 16:25 ` Vincent Guittot
2019-02-05 8:09 ` Ulf Hansson
2019-02-06 10:27 ` [PATCH 0/2] PM-runtime: fix and clean up of time accounting Rafael J. Wysocki
2 siblings, 1 reply; 7+ messages in thread
From: Vincent Guittot @ 2019-02-04 16:25 UTC (permalink / raw)
To: linux-pm, linux-kernel, rjw, ulf.hansson, biju.das
Cc: linux-renesas-soc, Vincent Guittot, geert, thara.gopinath,
linux-arm-kernel
Update accounting_timestamp field only when PM runtime is enable
and don't forget to account the last state before disabling it.
Suggested-by: Ulf Hansson <ulf.hansson@linaro.org>
Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
---
drivers/base/power/runtime.c | 18 ++++++++++--------
1 file changed, 10 insertions(+), 8 deletions(-)
diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index 50740da..ed62cca 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -66,11 +66,14 @@ static int rpm_suspend(struct device *dev, int rpmflags);
*/
void update_pm_runtime_accounting(struct device *dev)
{
- u64 now = ktime_get_mono_fast_ns();
- u64 last = dev->power.accounting_timestamp;
- u64 delta;
+ u64 now, last, delta;
- dev->power.accounting_timestamp = now;
+ if (dev->power.disable_depth > 0)
+ return;
+
+ last = dev->power.accounting_timestamp;
+
+ dev->power.accounting_timestamp = now = ktime_get_mono_fast_ns();
/*
* Because ktime_get_mono_fast_ns() is not monotonic during
@@ -82,9 +85,6 @@ void update_pm_runtime_accounting(struct device *dev)
delta = now - last;
- if (dev->power.disable_depth > 0)
- return;
-
if (dev->power.runtime_status == RPM_SUSPENDED)
dev->power.suspended_time += delta;
else
@@ -1298,6 +1298,9 @@ void __pm_runtime_disable(struct device *dev, bool check_resume)
pm_runtime_put_noidle(dev);
}
+ /* Update time accounting before disabling pm runtime */
+ update_pm_runtime_accounting(dev);
+
if (!dev->power.disable_depth++)
__pm_runtime_barrier(dev);
@@ -1521,7 +1524,6 @@ 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 = 0;
INIT_WORK(&dev->power.work, pm_runtime_work);
dev->power.timer_expires = 0;
--
2.7.4
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] PM-runtime: move runtime accounting on ktime_get_mono_fast_ns()
2019-02-04 16:25 ` [PATCH 1/2] PM-runtime: move runtime accounting on ktime_get_mono_fast_ns() Vincent Guittot
@ 2019-02-05 8:09 ` Ulf Hansson
2019-02-05 8:39 ` Vincent Guittot
0 siblings, 1 reply; 7+ messages in thread
From: Ulf Hansson @ 2019-02-05 8:09 UTC (permalink / raw)
To: Vincent Guittot
Cc: Linux PM, Rafael J. Wysocki, Linux Kernel Mailing List,
Thara Gopinath, Biju Das, Linux-Renesas, Geert Uytterhoeven,
Linux ARM
On Mon, 4 Feb 2019 at 17:25, Vincent Guittot <vincent.guittot@linaro.org> wrote:
>
> Similarly to what happened whith autosuspend, a deadlock has been raised
> with runtime accounting in the sequence:
>
> change_clocksource
> ...
> write_seqcount_begin
> ...
> timekeeping_update
> ...
> sh_cmt_clocksource_enable
> ...
> rpm_resume
> update_pm_runtime_accounting
> ktime_get
> do
> read_seqcount_begin
> while read_seqcount_retry
> ....
> write_seqcount_end
>
> Move runtime accounting on ktime_get_mono_fast_ns()
> With ktime_get_mono_fast_ns, the timestamp is not guaranteed to be
> monotonic across an update of timekeeping and as a result time can goes
> backward. Add a test to skip accounting for such situation which should
> stay exceptional.
>
> Fixes: a08c2a5a3194 ("PM-runtime: Replace jiffies-based accounting with ktime-based accounting")
> Reported-by: Biju Das <biju.das@bp.renesas.com>
> Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
Kind regards
Uffe
> ---
> drivers/base/power/runtime.c | 17 +++++++++++++----
> 1 file changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> index 1caa394..50740da 100644
> --- a/drivers/base/power/runtime.c
> +++ b/drivers/base/power/runtime.c
> @@ -66,13 +66,22 @@ static int rpm_suspend(struct device *dev, int rpmflags);
> */
> void update_pm_runtime_accounting(struct device *dev)
> {
> - u64 now = ktime_to_ns(ktime_get());
> + u64 now = ktime_get_mono_fast_ns();
> + u64 last = dev->power.accounting_timestamp;
> u64 delta;
>
> - delta = now - dev->power.accounting_timestamp;
> -
> dev->power.accounting_timestamp = now;
>
> + /*
> + * Because ktime_get_mono_fast_ns() is not monotonic during
> + * timekeeping update, we must ensure that now is after the last saved
> + * timesptamp
> + */
> + if (now < last)
> + return;
> +
> + delta = now - last;
> +
> if (dev->power.disable_depth > 0)
> return;
>
> @@ -1312,7 +1321,7 @@ void pm_runtime_enable(struct device *dev)
>
> /* About to enable runtime pm, set accounting_timestamp to now */
> if (!dev->power.disable_depth)
> - dev->power.accounting_timestamp = ktime_to_ns(ktime_get());
> + dev->power.accounting_timestamp = ktime_get_mono_fast_ns();
> } else {
> dev_warn(dev, "Unbalanced %s!\n", __func__);
> }
> --
> 2.7.4
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] PM-runtime: update time accounting only when enabled
2019-02-04 16:25 ` [PATCH 2/2] PM-runtime: update time accounting only when enabled Vincent Guittot
@ 2019-02-05 8:09 ` Ulf Hansson
0 siblings, 0 replies; 7+ messages in thread
From: Ulf Hansson @ 2019-02-05 8:09 UTC (permalink / raw)
To: Vincent Guittot
Cc: Linux PM, Rafael J. Wysocki, Linux Kernel Mailing List,
Thara Gopinath, Biju Das, Linux-Renesas, Geert Uytterhoeven,
Linux ARM
On Mon, 4 Feb 2019 at 17:26, Vincent Guittot <vincent.guittot@linaro.org> wrote:
>
> Update accounting_timestamp field only when PM runtime is enable
> and don't forget to account the last state before disabling it.
>
> Suggested-by: Ulf Hansson <ulf.hansson@linaro.org>
> Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
Kind regards
Uffe
> ---
> drivers/base/power/runtime.c | 18 ++++++++++--------
> 1 file changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> index 50740da..ed62cca 100644
> --- a/drivers/base/power/runtime.c
> +++ b/drivers/base/power/runtime.c
> @@ -66,11 +66,14 @@ static int rpm_suspend(struct device *dev, int rpmflags);
> */
> void update_pm_runtime_accounting(struct device *dev)
> {
> - u64 now = ktime_get_mono_fast_ns();
> - u64 last = dev->power.accounting_timestamp;
> - u64 delta;
> + u64 now, last, delta;
>
> - dev->power.accounting_timestamp = now;
> + if (dev->power.disable_depth > 0)
> + return;
> +
> + last = dev->power.accounting_timestamp;
> +
> + dev->power.accounting_timestamp = now = ktime_get_mono_fast_ns();
>
> /*
> * Because ktime_get_mono_fast_ns() is not monotonic during
> @@ -82,9 +85,6 @@ void update_pm_runtime_accounting(struct device *dev)
>
> delta = now - last;
>
> - if (dev->power.disable_depth > 0)
> - return;
> -
> if (dev->power.runtime_status == RPM_SUSPENDED)
> dev->power.suspended_time += delta;
> else
> @@ -1298,6 +1298,9 @@ void __pm_runtime_disable(struct device *dev, bool check_resume)
> pm_runtime_put_noidle(dev);
> }
>
> + /* Update time accounting before disabling pm runtime */
> + update_pm_runtime_accounting(dev);
> +
> if (!dev->power.disable_depth++)
> __pm_runtime_barrier(dev);
>
> @@ -1521,7 +1524,6 @@ 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 = 0;
> INIT_WORK(&dev->power.work, pm_runtime_work);
>
> dev->power.timer_expires = 0;
> --
> 2.7.4
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] PM-runtime: move runtime accounting on ktime_get_mono_fast_ns()
2019-02-05 8:09 ` Ulf Hansson
@ 2019-02-05 8:39 ` Vincent Guittot
0 siblings, 0 replies; 7+ messages in thread
From: Vincent Guittot @ 2019-02-05 8:39 UTC (permalink / raw)
To: Ulf Hansson
Cc: Linux PM, Rafael J. Wysocki, Linux Kernel Mailing List,
Thara Gopinath, Biju Das, Linux-Renesas, Geert Uytterhoeven,
Linux ARM
On Tue, 5 Feb 2019 at 09:10, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Mon, 4 Feb 2019 at 17:25, Vincent Guittot <vincent.guittot@linaro.org> wrote:
> >
> > Similarly to what happened whith autosuspend, a deadlock has been raised
> > with runtime accounting in the sequence:
> >
> > change_clocksource
> > ...
> > write_seqcount_begin
> > ...
> > timekeeping_update
> > ...
> > sh_cmt_clocksource_enable
> > ...
> > rpm_resume
> > update_pm_runtime_accounting
> > ktime_get
> > do
> > read_seqcount_begin
> > while read_seqcount_retry
> > ....
> > write_seqcount_end
> >
> > Move runtime accounting on ktime_get_mono_fast_ns()
> > With ktime_get_mono_fast_ns, the timestamp is not guaranteed to be
> > monotonic across an update of timekeeping and as a result time can goes
> > backward. Add a test to skip accounting for such situation which should
> > stay exceptional.
> >
> > Fixes: a08c2a5a3194 ("PM-runtime: Replace jiffies-based accounting with ktime-based accounting")
> > Reported-by: Biju Das <biju.das@bp.renesas.com>
> > Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
>
> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
Thanks
>
> Kind regards
> Uffe
>
>
> > ---
> > drivers/base/power/runtime.c | 17 +++++++++++++----
> > 1 file changed, 13 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> > index 1caa394..50740da 100644
> > --- a/drivers/base/power/runtime.c
> > +++ b/drivers/base/power/runtime.c
> > @@ -66,13 +66,22 @@ static int rpm_suspend(struct device *dev, int rpmflags);
> > */
> > void update_pm_runtime_accounting(struct device *dev)
> > {
> > - u64 now = ktime_to_ns(ktime_get());
> > + u64 now = ktime_get_mono_fast_ns();
> > + u64 last = dev->power.accounting_timestamp;
> > u64 delta;
> >
> > - delta = now - dev->power.accounting_timestamp;
> > -
> > dev->power.accounting_timestamp = now;
> >
> > + /*
> > + * Because ktime_get_mono_fast_ns() is not monotonic during
> > + * timekeeping update, we must ensure that now is after the last saved
> > + * timesptamp
> > + */
> > + if (now < last)
> > + return;
> > +
> > + delta = now - last;
> > +
> > if (dev->power.disable_depth > 0)
> > return;
> >
> > @@ -1312,7 +1321,7 @@ void pm_runtime_enable(struct device *dev)
> >
> > /* About to enable runtime pm, set accounting_timestamp to now */
> > if (!dev->power.disable_depth)
> > - dev->power.accounting_timestamp = ktime_to_ns(ktime_get());
> > + dev->power.accounting_timestamp = ktime_get_mono_fast_ns();
> > } else {
> > dev_warn(dev, "Unbalanced %s!\n", __func__);
> > }
> > --
> > 2.7.4
> >
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 0/2] PM-runtime: fix and clean up of time accounting
2019-02-04 16:25 [PATCH 0/2] PM-runtime: fix and clean up of time accounting Vincent Guittot
2019-02-04 16:25 ` [PATCH 1/2] PM-runtime: move runtime accounting on ktime_get_mono_fast_ns() Vincent Guittot
2019-02-04 16:25 ` [PATCH 2/2] PM-runtime: update time accounting only when enabled Vincent Guittot
@ 2019-02-06 10:27 ` Rafael J. Wysocki
2 siblings, 0 replies; 7+ messages in thread
From: Rafael J. Wysocki @ 2019-02-06 10:27 UTC (permalink / raw)
To: Vincent Guittot
Cc: ulf.hansson, linux-pm, linux-kernel, thara.gopinath, biju.das,
linux-renesas-soc, geert, linux-arm-kernel
On Monday, February 4, 2019 5:25:51 PM CET Vincent Guittot wrote:
> Fix time accounting which has the same lock contraint as for using hrtimer
> and update accounting_timestamp only when useful.
>
> Vincent Guittot (2):
> PM-runtime: move runtime accounting on ktime_get_mono_fast_ns()
> PM-runtime: update time accounting only when enabled
>
> drivers/base/power/runtime.c | 25 ++++++++++++++++++-------
> 1 file changed, 18 insertions(+), 7 deletions(-)
>
>
Both applied, thanks!
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-02-06 10:28 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-04 16:25 [PATCH 0/2] PM-runtime: fix and clean up of time accounting Vincent Guittot
2019-02-04 16:25 ` [PATCH 1/2] PM-runtime: move runtime accounting on ktime_get_mono_fast_ns() Vincent Guittot
2019-02-05 8:09 ` Ulf Hansson
2019-02-05 8:39 ` Vincent Guittot
2019-02-04 16:25 ` [PATCH 2/2] PM-runtime: update time accounting only when enabled Vincent Guittot
2019-02-05 8:09 ` Ulf Hansson
2019-02-06 10:27 ` [PATCH 0/2] PM-runtime: fix and clean up of time accounting Rafael J. Wysocki
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).