[v6,1/2] PM-runtime: update accounting_timestamp only when enable

Message ID 1548167070-421-2-git-send-email-vincent.guittot@linaro.org
State New
Headers show
Series
  • Move pm_runtime accounted time to raw nsec
Related show

Commit Message

Vincent Guittot Jan. 22, 2019, 2:24 p.m.
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(-)

-- 
2.7.4

Comments

Vincent Guittot Jan. 23, 2019, 8:50 a.m. | #1
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

> >
Ulf Hansson Jan. 23, 2019, 9:35 a.m. | #2
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

Patch

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;