Hi all,
Looks like commit 8234f6734c5d ("PM-runtime: Switch autosuspend
over to using hrtimers") caused a regression on at least
omap5-uevm where 8250 UART rx wake no longer works. I have not
noticed this happening on others so far.
The devices I've tested all are using 8250 with dedicated
wakeirqs configured for the rx pin. I can see the interrupt
increase on omap5-uevm after some one or more keypresses,
but then nothing. It seems that the uart just falls back
asleep right away or something.
Any ideas what might be going wrong?
Regards,
Tony
Hi Tony,
On Tue, 8 Jan 2019 at 00:38, Tony Lindgren <[email protected]> wrote:
>
> Hi all,
>
> Looks like commit 8234f6734c5d ("PM-runtime: Switch autosuspend
> over to using hrtimers") caused a regression on at least
> omap5-uevm where 8250 UART rx wake no longer works. I have not
> noticed this happening on others so far.
>
> The devices I've tested all are using 8250 with dedicated
> wakeirqs configured for the rx pin. I can see the interrupt
> increase on omap5-uevm after some one or more keypresses,
> but then nothing. It seems that the uart just falls back
> asleep right away or something.
>
> Any ideas what might be going wrong?
What is the autosuspend value ? Can it be that the autosuspend is set
to a short value but was finally greater than 10-20ms on arm32. And
now the autosuspend happens before and this has changed the sequence ?
Regards,
Vincent
>
> Regards,
>
> Tony
>
On Tue, 8 Jan 2019 at 16:53, Tony Lindgren <[email protected]> wrote:
>
> * Vincent Guittot <[email protected]> [190108 08:00]:
> > Hi Tony,
> >
> > On Tue, 8 Jan 2019 at 00:38, Tony Lindgren <[email protected]> wrote:
> > >
> > > Hi all,
> > >
> > > Looks like commit 8234f6734c5d ("PM-runtime: Switch autosuspend
> > > over to using hrtimers") caused a regression on at least
> > > omap5-uevm where 8250 UART rx wake no longer works. I have not
> > > noticed this happening on others so far.
> > >
> > > The devices I've tested all are using 8250 with dedicated
> > > wakeirqs configured for the rx pin. I can see the interrupt
> > > increase on omap5-uevm after some one or more keypresses,
> > > but then nothing. It seems that the uart just falls back
> > > asleep right away or something.
> > >
> > > Any ideas what might be going wrong?
> >
> > What is the autosuspend value ? Can it be that the autosuspend is set
> > to a short value but was finally greater than 10-20ms on arm32. And
> > now the autosuspend happens before and this has changed the sequence ?
>
> It's set to 3 seconds. The difference between let's say
> C-A9 pandaboard (that is working) compared to C-A15 omap5-uevm
> is that the C-A15 has arch_timer in use. Other than that things
> should behave more or less the same way.
>
> Hmm so could it be that we now rely on timers that that may
> not be capable of waking up the system from idle states with
> hrtimer?
With nohz and hrtimer enabled, timer relies on hrtimer to generate
the tick so you should use the same interrupt.
>
> Regards,
>
> Tony
>
* Vincent Guittot <[email protected]> [190108 08:00]:
> Hi Tony,
>
> On Tue, 8 Jan 2019 at 00:38, Tony Lindgren <[email protected]> wrote:
> >
> > Hi all,
> >
> > Looks like commit 8234f6734c5d ("PM-runtime: Switch autosuspend
> > over to using hrtimers") caused a regression on at least
> > omap5-uevm where 8250 UART rx wake no longer works. I have not
> > noticed this happening on others so far.
> >
> > The devices I've tested all are using 8250 with dedicated
> > wakeirqs configured for the rx pin. I can see the interrupt
> > increase on omap5-uevm after some one or more keypresses,
> > but then nothing. It seems that the uart just falls back
> > asleep right away or something.
> >
> > Any ideas what might be going wrong?
>
> What is the autosuspend value ? Can it be that the autosuspend is set
> to a short value but was finally greater than 10-20ms on arm32. And
> now the autosuspend happens before and this has changed the sequence ?
It's set to 3 seconds. The difference between let's say
C-A9 pandaboard (that is working) compared to C-A15 omap5-uevm
is that the C-A15 has arch_timer in use. Other than that things
should behave more or less the same way.
Hmm so could it be that we now rely on timers that that may
not be capable of waking up the system from idle states with
hrtimer?
Regards,
Tony
* Vincent Guittot <[email protected]> [190108 16:42]:
> On Tue, 8 Jan 2019 at 16:53, Tony Lindgren <[email protected]> wrote:
> > Hmm so could it be that we now rely on timers that that may
> > not be capable of waking up the system from idle states with
> > hrtimer?
>
> With nohz and hrtimer enabled, timer relies on hrtimer to generate
> the tick so you should use the same interrupt.
OK yeah looks like that part is working just fine.
Adding some printks and debugging over ssh, looks like
omap8250_runtime_resume() gets called just fine based on a wakeirq,
but then omap8250_runtime_suspend() runs immediately instead of
waiting for the three second timeout.
Lowering the autosuspend_delay_ms to 2100 ms makes things work again.
Anything higher than 2200 ms seems to somehow time out immediately
now :)
Regards,
Tony
Le Tuesday 08 Jan 2019 ? 13:37:43 (-0800), Tony Lindgren a ?crit :
> * Vincent Guittot <[email protected]> [190108 16:42]:
> > On Tue, 8 Jan 2019 at 16:53, Tony Lindgren <[email protected]> wrote:
> > > Hmm so could it be that we now rely on timers that that may
> > > not be capable of waking up the system from idle states with
> > > hrtimer?
> >
> > With nohz and hrtimer enabled, timer relies on hrtimer to generate
> > the tick so you should use the same interrupt.
>
> OK yeah looks like that part is working just fine.
>
> Adding some printks and debugging over ssh, looks like
> omap8250_runtime_resume() gets called just fine based on a wakeirq,
> but then omap8250_runtime_suspend() runs immediately instead of
> waiting for the three second timeout.
>
> Lowering the autosuspend_delay_ms to 2100 ms makes things work again.
> Anything higher than 2200 ms seems to somehow time out immediately
> now :)
This is quite close to the max ns of an int on arm 32bits
Could you try the patch below ?
---
drivers/base/power/runtime.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index 7062469..44c5c76 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -141,7 +141,7 @@ u64 pm_runtime_autosuspend_expiration(struct device *dev)
last_busy = READ_ONCE(dev->power.last_busy);
- expires = last_busy + autosuspend_delay * NSEC_PER_MSEC;
+ expires = last_busy + (u64)(autosuspend_delay) * NSEC_PER_MSEC;
if (expires <= now)
expires = 0; /* Already expired. */
--
2.7.4
>
> Regards,
>
> Tony
* Vincent Guittot <[email protected]> [190109 01:42]:
> Le Tuesday 08 Jan 2019 à 13:37:43 (-0800), Tony Lindgren a écrit :
> > Lowering the autosuspend_delay_ms to 2100 ms makes things work again.
> > Anything higher than 2200 ms seems to somehow time out immediately
> > now :)
>
> This is quite close to the max ns of an int on arm 32bits
>
> Could you try the patch below ?
Yup great thanks, that's it:
Tested-by: Tony Lindgren <[email protected]>
> ---
> drivers/base/power/runtime.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> index 7062469..44c5c76 100644
> --- a/drivers/base/power/runtime.c
> +++ b/drivers/base/power/runtime.c
> @@ -141,7 +141,7 @@ u64 pm_runtime_autosuspend_expiration(struct device *dev)
>
> last_busy = READ_ONCE(dev->power.last_busy);
>
> - expires = last_busy + autosuspend_delay * NSEC_PER_MSEC;
> + expires = last_busy + (u64)(autosuspend_delay) * NSEC_PER_MSEC;
> if (expires <= now)
> expires = 0; /* Already expired. */
>
> --
> 2.7.4
>
>
> >
> > Regards,
> >
> > Tony
On Wed, Jan 9, 2019 at 2:51 AM Tony Lindgren <[email protected]> wrote:
>
> * Vincent Guittot <[email protected]> [190109 01:42]:
> > Le Tuesday 08 Jan 2019 à 13:37:43 (-0800), Tony Lindgren a écrit :
> > > Lowering the autosuspend_delay_ms to 2100 ms makes things work again.
> > > Anything higher than 2200 ms seems to somehow time out immediately
> > > now :)
> >
> > This is quite close to the max ns of an int on arm 32bits
> >
> > Could you try the patch below ?
>
> Yup great thanks, that's it:
>
> Tested-by: Tony Lindgren <[email protected]>
Cool. Thanks for getting to the bottom of this!
On Wed, Jan 09, 2019 at 02:42:18AM +0100, Vincent Guittot wrote:
> Le Tuesday 08 Jan 2019 à 13:37:43 (-0800), Tony Lindgren a écrit :
> > * Vincent Guittot <[email protected]> [190108 16:42]:
> > > On Tue, 8 Jan 2019 at 16:53, Tony Lindgren <[email protected]> wrote:
> > > > Hmm so could it be that we now rely on timers that that may
> > > > not be capable of waking up the system from idle states with
> > > > hrtimer?
> > >
> > > With nohz and hrtimer enabled, timer relies on hrtimer to generate
> > > the tick so you should use the same interrupt.
> >
> > OK yeah looks like that part is working just fine.
> >
> > Adding some printks and debugging over ssh, looks like
> > omap8250_runtime_resume() gets called just fine based on a wakeirq,
> > but then omap8250_runtime_suspend() runs immediately instead of
> > waiting for the three second timeout.
> >
> > Lowering the autosuspend_delay_ms to 2100 ms makes things work again.
> > Anything higher than 2200 ms seems to somehow time out immediately
> > now :)
>
> This is quite close to the max ns of an int on arm 32bits
>
> Could you try the patch below ?
>
> ---
> drivers/base/power/runtime.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> index 7062469..44c5c76 100644
> --- a/drivers/base/power/runtime.c
> +++ b/drivers/base/power/runtime.c
> @@ -141,7 +141,7 @@ u64 pm_runtime_autosuspend_expiration(struct device *dev)
>
> last_busy = READ_ONCE(dev->power.last_busy);
>
> - expires = last_busy + autosuspend_delay * NSEC_PER_MSEC;
> + expires = last_busy + (u64)(autosuspend_delay) * NSEC_PER_MSEC;
> if (expires <= now)
> expires = 0; /* Already expired. */
Hmm, comment above function states it returns "the expiration time in jiffies
(adjusted to be nonzero)", so there's probably more to fix...
You can also consider change like this (still does not return jiffies):
diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index 70624695b6d5..c72eaf21a61c 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -129,23 +129,20 @@ static void pm_runtime_cancel_pending(struct device *dev)
u64 pm_runtime_autosuspend_expiration(struct device *dev)
{
int autosuspend_delay;
- u64 last_busy, expires = 0;
- u64 now = ktime_to_ns(ktime_get());
+ ktime_t expires;
if (!dev->power.use_autosuspend)
- goto out;
+ return 0;
autosuspend_delay = READ_ONCE(dev->power.autosuspend_delay);
if (autosuspend_delay < 0)
- goto out;
-
- last_busy = READ_ONCE(dev->power.last_busy);
+ return 0;
- expires = last_busy + autosuspend_delay * NSEC_PER_MSEC;
- if (expires <= now)
- expires = 0; /* Already expired. */
+ expires = ktime_add_ns(ms_to_ktime(autosuspend_delay),
+ READ_ONCE(dev->power.last_busy));
+ if (expires <= ktime_get())
+ return 0; /* Already expired. */
- out:
return expires;
}
EXPORT_SYMBOL_GPL(pm_runtime_autosuspend_expiration);
Regards,
ladis
On Wed, 9 Jan 2019 at 12:17, Ladislav Michl <[email protected]> wrote:
>
> On Wed, Jan 09, 2019 at 02:42:18AM +0100, Vincent Guittot wrote:
> > Le Tuesday 08 Jan 2019 à 13:37:43 (-0800), Tony Lindgren a écrit :
> > > * Vincent Guittot <[email protected]> [190108 16:42]:
> > > > On Tue, 8 Jan 2019 at 16:53, Tony Lindgren <[email protected]> wrote:
> > > > > Hmm so could it be that we now rely on timers that that may
> > > > > not be capable of waking up the system from idle states with
> > > > > hrtimer?
> > > >
> > > > With nohz and hrtimer enabled, timer relies on hrtimer to generate
> > > > the tick so you should use the same interrupt.
> > >
> > > OK yeah looks like that part is working just fine.
> > >
> > > Adding some printks and debugging over ssh, looks like
> > > omap8250_runtime_resume() gets called just fine based on a wakeirq,
> > > but then omap8250_runtime_suspend() runs immediately instead of
> > > waiting for the three second timeout.
> > >
> > > Lowering the autosuspend_delay_ms to 2100 ms makes things work again.
> > > Anything higher than 2200 ms seems to somehow time out immediately
> > > now :)
> >
> > This is quite close to the max ns of an int on arm 32bits
> >
> > Could you try the patch below ?
> >
> > ---
> > drivers/base/power/runtime.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> > index 7062469..44c5c76 100644
> > --- a/drivers/base/power/runtime.c
> > +++ b/drivers/base/power/runtime.c
> > @@ -141,7 +141,7 @@ u64 pm_runtime_autosuspend_expiration(struct device *dev)
> >
> > last_busy = READ_ONCE(dev->power.last_busy);
> >
> > - expires = last_busy + autosuspend_delay * NSEC_PER_MSEC;
> > + expires = last_busy + (u64)(autosuspend_delay) * NSEC_PER_MSEC;
> > if (expires <= now)
> > expires = 0; /* Already expired. */
>
> Hmm, comment above function states it returns "the expiration time in jiffies
> (adjusted to be nonzero)", so there's probably more to fix...
The comment is wrong and should be updated as commit 8234f6734c5d has
moved on hrtimer and expires is now in raw ns unit
>
> You can also consider change like this (still does not return jiffies):
> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> index 70624695b6d5..c72eaf21a61c 100644
> --- a/drivers/base/power/runtime.c
> +++ b/drivers/base/power/runtime.c
> @@ -129,23 +129,20 @@ static void pm_runtime_cancel_pending(struct device *dev)
> u64 pm_runtime_autosuspend_expiration(struct device *dev)
> {
> int autosuspend_delay;
> - u64 last_busy, expires = 0;
> - u64 now = ktime_to_ns(ktime_get());
> + ktime_t expires;
>
> if (!dev->power.use_autosuspend)
> - goto out;
> + return 0;
>
> autosuspend_delay = READ_ONCE(dev->power.autosuspend_delay);
> if (autosuspend_delay < 0)
> - goto out;
> -
> - last_busy = READ_ONCE(dev->power.last_busy);
> + return 0;
>
> - expires = last_busy + autosuspend_delay * NSEC_PER_MSEC;
> - if (expires <= now)
> - expires = 0; /* Already expired. */
> + expires = ktime_add_ns(ms_to_ktime(autosuspend_delay),
> + READ_ONCE(dev->power.last_busy));
> + if (expires <= ktime_get())
> + return 0; /* Already expired. */
>
> - out:
> return expires;
> }
> EXPORT_SYMBOL_GPL(pm_runtime_autosuspend_expiration);
>
> Regards,
> ladis
On Wed, 9 Jan 2019 at 12:58, Ladislav Michl <[email protected]> wrote:
>
> On Wed, Jan 09, 2019 at 12:27:57PM +0100, Vincent Guittot wrote:
> > On Wed, 9 Jan 2019 at 12:17, Ladislav Michl <[email protected]> wrote:
> > >
> > > On Wed, Jan 09, 2019 at 02:42:18AM +0100, Vincent Guittot wrote:
> > > > Le Tuesday 08 Jan 2019 à 13:37:43 (-0800), Tony Lindgren a écrit :
> > > > > * Vincent Guittot <[email protected]> [190108 16:42]:
> > > > > > On Tue, 8 Jan 2019 at 16:53, Tony Lindgren <[email protected]> wrote:
> > > > > > > Hmm so could it be that we now rely on timers that that may
> > > > > > > not be capable of waking up the system from idle states with
> > > > > > > hrtimer?
> > > > > >
> > > > > > With nohz and hrtimer enabled, timer relies on hrtimer to generate
> > > > > > the tick so you should use the same interrupt.
> > > > >
> > > > > OK yeah looks like that part is working just fine.
> > > > >
> > > > > Adding some printks and debugging over ssh, looks like
> > > > > omap8250_runtime_resume() gets called just fine based on a wakeirq,
> > > > > but then omap8250_runtime_suspend() runs immediately instead of
> > > > > waiting for the three second timeout.
> > > > >
> > > > > Lowering the autosuspend_delay_ms to 2100 ms makes things work again.
> > > > > Anything higher than 2200 ms seems to somehow time out immediately
> > > > > now :)
> > > >
> > > > This is quite close to the max ns of an int on arm 32bits
> > > >
> > > > Could you try the patch below ?
> > > >
> > > > ---
> > > > drivers/base/power/runtime.c | 2 +-
> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> > > > index 7062469..44c5c76 100644
> > > > --- a/drivers/base/power/runtime.c
> > > > +++ b/drivers/base/power/runtime.c
> > > > @@ -141,7 +141,7 @@ u64 pm_runtime_autosuspend_expiration(struct device *dev)
> > > >
> > > > last_busy = READ_ONCE(dev->power.last_busy);
> > > >
> > > > - expires = last_busy + autosuspend_delay * NSEC_PER_MSEC;
> > > > + expires = last_busy + (u64)(autosuspend_delay) * NSEC_PER_MSEC;
> > > > if (expires <= now)
> > > > expires = 0; /* Already expired. */
> > >
> > > Hmm, comment above function states it returns "the expiration time in jiffies
> > > (adjusted to be nonzero)", so there's probably more to fix...
> >
> > The comment is wrong and should be updated as commit 8234f6734c5d has
> > moved on hrtimer and expires is now in raw ns unit
>
> Yup. Who'll send a patch? Is it worth optimizing as bellow? I'm fine with doing
You can send a patch for updating the comment.
> both.
Regarding proposal below:
- pm_runtime_autosuspend_expiration() returns u64 and not ktime_t
- use helper ktime_before/after to compare ktime_t value
Using ktime helper function makes the code less readable and the
proposal make the function more difficult to read IMHO when you
compare
expires = last_busy + autosuspend_delay * NSEC_PER_MSEC;
with
expires = ktime_add_ns(ms_to_ktime(autosuspend_delay),
READ_ONCE(dev->power.last_busy));
or when you compare
if (expires <= now)
with
if (ktime_after(now, expires))
And I'm not sure that this will optimize the code at the end
Only the replacement of the "goto out" by return 0 would make sense IMO
Regards,
Vincent
>
> > > You can also consider change like this (still does not return jiffies):
> > > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> > > index 70624695b6d5..c72eaf21a61c 100644
> > > --- a/drivers/base/power/runtime.c
> > > +++ b/drivers/base/power/runtime.c
> > > @@ -129,23 +129,20 @@ static void pm_runtime_cancel_pending(struct device *dev)
> > > u64 pm_runtime_autosuspend_expiration(struct device *dev)
> > > {
> > > int autosuspend_delay;
> > > - u64 last_busy, expires = 0;
> > > - u64 now = ktime_to_ns(ktime_get());
> > > + ktime_t expires;
> > >
> > > if (!dev->power.use_autosuspend)
> > > - goto out;
> > > + return 0;
> > >
> > > autosuspend_delay = READ_ONCE(dev->power.autosuspend_delay);
> > > if (autosuspend_delay < 0)
> > > - goto out;
> > > -
> > > - last_busy = READ_ONCE(dev->power.last_busy);
> > > + return 0;
> > >
> > > - expires = last_busy + autosuspend_delay * NSEC_PER_MSEC;
> > > - if (expires <= now)
> > > - expires = 0; /* Already expired. */
> > > + expires = ktime_add_ns(ms_to_ktime(autosuspend_delay),
> > > + READ_ONCE(dev->power.last_busy));
> > > + if (expires <= ktime_get())
> > > + return 0; /* Already expired. */
> > >
> > > - out:
> > > return expires;
> > > }
> > > EXPORT_SYMBOL_GPL(pm_runtime_autosuspend_expiration);
> > >
> > > Regards,
> > > ladis
Please keep all thread list when replying :-)
On Wed, 9 Jan 2019 at 14:33, Ladislav Michl <[email protected]> wrote:
>
> On Wed, Jan 09, 2019 at 02:24:37PM +0100, Vincent Guittot wrote:
> > On Wed, 9 Jan 2019 at 12:58, Ladislav Michl <[email protected]> wrote:
> > >
> > > On Wed, Jan 09, 2019 at 12:27:57PM +0100, Vincent Guittot wrote:
> > > > On Wed, 9 Jan 2019 at 12:17, Ladislav Michl <[email protected]> wrote:
> > > > >
> > > > > On Wed, Jan 09, 2019 at 02:42:18AM +0100, Vincent Guittot wrote:
> > > > > > Le Tuesday 08 Jan 2019 à 13:37:43 (-0800), Tony Lindgren a écrit :
> > > > > > > * Vincent Guittot <[email protected]> [190108 16:42]:
> > > > > > > > On Tue, 8 Jan 2019 at 16:53, Tony Lindgren <[email protected]> wrote:
> > > > > > > > > Hmm so could it be that we now rely on timers that that may
> > > > > > > > > not be capable of waking up the system from idle states with
> > > > > > > > > hrtimer?
> > > > > > > >
> > > > > > > > With nohz and hrtimer enabled, timer relies on hrtimer to generate
> > > > > > > > the tick so you should use the same interrupt.
> > > > > > >
> > > > > > > OK yeah looks like that part is working just fine.
> > > > > > >
> > > > > > > Adding some printks and debugging over ssh, looks like
> > > > > > > omap8250_runtime_resume() gets called just fine based on a wakeirq,
> > > > > > > but then omap8250_runtime_suspend() runs immediately instead of
> > > > > > > waiting for the three second timeout.
> > > > > > >
> > > > > > > Lowering the autosuspend_delay_ms to 2100 ms makes things work again.
> > > > > > > Anything higher than 2200 ms seems to somehow time out immediately
> > > > > > > now :)
> > > > > >
> > > > > > This is quite close to the max ns of an int on arm 32bits
> > > > > >
> > > > > > Could you try the patch below ?
> > > > > >
> > > > > > ---
> > > > > > drivers/base/power/runtime.c | 2 +-
> > > > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> > > > > > index 7062469..44c5c76 100644
> > > > > > --- a/drivers/base/power/runtime.c
> > > > > > +++ b/drivers/base/power/runtime.c
> > > > > > @@ -141,7 +141,7 @@ u64 pm_runtime_autosuspend_expiration(struct device *dev)
> > > > > >
> > > > > > last_busy = READ_ONCE(dev->power.last_busy);
> > > > > >
> > > > > > - expires = last_busy + autosuspend_delay * NSEC_PER_MSEC;
> > > > > > + expires = last_busy + (u64)(autosuspend_delay) * NSEC_PER_MSEC;
> > > > > > if (expires <= now)
> > > > > > expires = 0; /* Already expired. */
> > > > >
> > > > > Hmm, comment above function states it returns "the expiration time in jiffies
> > > > > (adjusted to be nonzero)", so there's probably more to fix...
> > > >
> > > > The comment is wrong and should be updated as commit 8234f6734c5d has
> > > > moved on hrtimer and expires is now in raw ns unit
> > >
> > > Yup. Who'll send a patch? Is it worth optimizing as bellow? I'm fine with doing
> >
> > You can send a patch for updating the comment.
> >
> > > both.
> >
> > Regarding proposal below:
> > - pm_runtime_autosuspend_expiration() returns u64 and not ktime_t
>
> Well, that's matter of adding ktime_to_ns (which is noop).
>
> > - use helper ktime_before/after to compare ktime_t value
> >
> > Using ktime helper function makes the code less readable and the
>
> That why I avoided it.
But you must use helper function if you use ktime_t
That's one main reason for using u64 instead of ktime_t
>
> > proposal make the function more difficult to read IMHO when you
> > compare
> > expires = last_busy + autosuspend_delay * NSEC_PER_MSEC;
> > with
> > expires = ktime_add_ns(ms_to_ktime(autosuspend_delay),
> > READ_ONCE(dev->power.last_busy));
>
> I agree, but it doea all the magic correctly, so you won't need
> to touch that code is ktime_t changes (I know, unlikely..)
But the current implementation doesn't care of any changes in ktime_t
as it uses raw ns
>
> > or when you compare
> > if (expires <= now)
> > with
> > if (ktime_after(now, expires))
> >
> > And I'm not sure that this will optimize the code at the end
>
> Checked generated code on ARM and x86 and gcc does pretty good job here.
>
> > Only the replacement of the "goto out" by return 0 would make sense IMO
>
> Well, main concern was not to call ktime_get at the beginning of function
> as it is not too cheap.
Doesn't the compiler optimize that to just before the test ?
>
> > Regards,
> > Vincent
> >
> > >
> > > > > You can also consider change like this (still does not return jiffies):
> > > > > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> > > > > index 70624695b6d5..c72eaf21a61c 100644
> > > > > --- a/drivers/base/power/runtime.c
> > > > > +++ b/drivers/base/power/runtime.c
> > > > > @@ -129,23 +129,20 @@ static void pm_runtime_cancel_pending(struct device *dev)
> > > > > u64 pm_runtime_autosuspend_expiration(struct device *dev)
> > > > > {
> > > > > int autosuspend_delay;
> > > > > - u64 last_busy, expires = 0;
> > > > > - u64 now = ktime_to_ns(ktime_get());
> > > > > + ktime_t expires;
> > > > >
> > > > > if (!dev->power.use_autosuspend)
> > > > > - goto out;
> > > > > + return 0;
> > > > >
> > > > > autosuspend_delay = READ_ONCE(dev->power.autosuspend_delay);
> > > > > if (autosuspend_delay < 0)
> > > > > - goto out;
> > > > > -
> > > > > - last_busy = READ_ONCE(dev->power.last_busy);
> > > > > + return 0;
> > > > >
> > > > > - expires = last_busy + autosuspend_delay * NSEC_PER_MSEC;
> > > > > - if (expires <= now)
> > > > > - expires = 0; /* Already expired. */
> > > > > + expires = ktime_add_ns(ms_to_ktime(autosuspend_delay),
> > > > > + READ_ONCE(dev->power.last_busy));
> > > > > + if (expires <= ktime_get())
> > > > > + return 0; /* Already expired. */
> > > > >
> > > > > - out:
> > > > > return expires;
> > > > > }
> > > > > EXPORT_SYMBOL_GPL(pm_runtime_autosuspend_expiration);
> > > > >
> > > > > Regards,
> > > > > ladis
* Rafael J. Wysocki <[email protected]> [190109 09:44]:
> On Wed, Jan 9, 2019 at 2:51 AM Tony Lindgren <[email protected]> wrote:
> >
> > * Vincent Guittot <[email protected]> [190109 01:42]:
> > > Le Tuesday 08 Jan 2019 à 13:37:43 (-0800), Tony Lindgren a écrit :
> > > > Lowering the autosuspend_delay_ms to 2100 ms makes things work again.
> > > > Anything higher than 2200 ms seems to somehow time out immediately
> > > > now :)
> > >
> > > This is quite close to the max ns of an int on arm 32bits
> > >
> > > Could you try the patch below ?
> >
> > Yup great thanks, that's it:
> >
> > Tested-by: Tony Lindgren <[email protected]>
>
> Cool. Thanks for getting to the bottom of this!
No problem.
One more thing I noticed: The 25% slack can get noticeable
for larger values. For things like a 3 second uart console
timeout slack of 750 ms is quite large variation.
Should we have a limit of max 100 ms for the slack?
Regards,
Tony
On Wed, 9 Jan 2019 at 17:07, Ladislav Michl <[email protected]> wrote:
>
> On Wed, Jan 09, 2019 at 03:12:25PM +0100, Vincent Guittot wrote:
> > Please keep all thread list when replying :-)
>
> Ahh, sorry for that...
> [snip]
> > On Wed, 9 Jan 2019 at 14:33, Ladislav Michl <[email protected]> wrote:
> > > I agree, but it doea all the magic correctly, so you won't need
> > > to touch that code is ktime_t changes (I know, unlikely..)
> >
> > But the current implementation doesn't care of any changes in ktime_t
> > as it uses raw ns
>
> Fair enough, so let's go back to:
This one looks good for me
> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> index 70624695b6d5..e61ee9b47a26 100644
> --- a/drivers/base/power/runtime.c
> +++ b/drivers/base/power/runtime.c
> @@ -121,7 +121,7 @@ static void pm_runtime_cancel_pending(struct device *dev)
> * Compute the autosuspend-delay expiration time based on the device's
> * power.last_busy time. If the delay has already expired or is disabled
> * (negative) or the power.use_autosuspend flag isn't set, return 0.
> - * Otherwise return the expiration time in jiffies (adjusted to be nonzero).
> + * Otherwise return the expiration time in nanoseconds (adjusted to be nonzero).
> *
> * This function may be called either with or without dev->power.lock held.
> * Either way it can be racy, since power.last_busy may be updated at any time.
> @@ -129,24 +129,21 @@ static void pm_runtime_cancel_pending(struct device *dev)
> u64 pm_runtime_autosuspend_expiration(struct device *dev)
> {
> int autosuspend_delay;
> - u64 last_busy, expires = 0;
> - u64 now = ktime_to_ns(ktime_get());
> + u64 expires;
>
> if (!dev->power.use_autosuspend)
> - goto out;
> + return 0;
>
> autosuspend_delay = READ_ONCE(dev->power.autosuspend_delay);
> if (autosuspend_delay < 0)
> - goto out;
> -
> - last_busy = READ_ONCE(dev->power.last_busy);
> + return 0;
>
> - expires = last_busy + autosuspend_delay * NSEC_PER_MSEC;
> - if (expires <= now)
> - expires = 0; /* Already expired. */
> + expires = READ_ONCE(dev->power.last_busy);
> + expires += NSEC_PER_MSEC * (u64)autosuspend_delay;
> + if (expires > ktime_to_ns(ktime_get()))
> + return expires; /* Expires in the future */
>
> - out:
> - return expires;
> + return 0;
> }
> EXPORT_SYMBOL_GPL(pm_runtime_autosuspend_expiration);
>
> Which gives for arm:
> 00000480 <pm_runtime_autosuspend_expiration>:
> 480: e92d41f0 push {r4, r5, r6, r7, r8, lr}
> 484: e5d030d1 ldrb r3, [r0, #209] ; 0xd1
> 488: e3130004 tst r3, #4
> 48c: 0a00000c beq 4c4 <pm_runtime_autosuspend_expiration+0x44>
> 490: e59030e4 ldr r3, [r0, #228] ; 0xe4
> 494: e3530000 cmp r3, #0
> 498: ba000009 blt 4c4 <pm_runtime_autosuspend_expiration+0x44>
> 49c: e28050e8 add r5, r0, #232 ; 0xe8
> 4a0: e8950030 ldm r5, {r4, r5}
> 4a4: e59f2030 ldr r2, [pc, #48] ; 4dc <pm_runtime_autosuspend_expiration+0x5c>
> 4a8: e0e54392 smlal r4, r5, r2, r3
> 4ac: ebfffffe bl 0 <ktime_get>
> 4b0: e1550001 cmp r5, r1
> 4b4: 01540000 cmpeq r4, r0
> 4b8: e1a06004 mov r6, r4
> 4bc: e1a07005 mov r7, r5
> 4c0: 8a000001 bhi 4cc <pm_runtime_autosuspend_expiration+0x4c>
> 4c4: e3a06000 mov r6, #0
> 4c8: e3a07000 mov r7, #0
> 4cc: e1a00006 mov r0, r6
> 4d0: e1a01007 mov r1, r7
> 4d4: e8bd41f0 pop {r4, r5, r6, r7, r8, lr}
> 4d8: e12fff1e bx lr
> 4dc: 000f4240 .word 0x000f4240
>
> And x86:
> 0000000000000230 <pm_runtime_autosuspend_expiration.part.0>:
> 230: 8b 87 a4 01 00 00 mov 0x1a4(%rdi),%eax
> 236: 53 push %rbx
> 237: 85 c0 test %eax,%eax
> 239: 78 1e js 259 <pm_runtime_autosuspend_expiration.part.0+0x29>
> 23b: 48 63 d8 movslq %eax,%rbx
> 23e: 48 8b 97 a8 01 00 00 mov 0x1a8(%rdi),%rdx
> 245: 48 69 db 40 42 0f 00 imul $0xf4240,%rbx,%rbx
> 24c: 48 01 d3 add %rdx,%rbx
> 24f: e8 00 00 00 00 callq 254 <pm_runtime_autosuspend_expiration.part.0+0x24>
> 254: 48 39 c3 cmp %rax,%rbx
> 257: 77 02 ja 25b <pm_runtime_autosuspend_expiration.part.0+0x2b>
> 259: 31 db xor %ebx,%ebx
> 25b: 48 89 d8 mov %rbx,%rax
> 25e: 5b pop %rbx
> 25f: c3 retq
>
> 00000000000002a0 <pm_runtime_autosuspend_expiration>:
> 2a0: f6 87 91 01 00 00 04 testb $0x4,0x191(%rdi)
> 2a7: 74 02 je 2ab <pm_runtime_autosuspend_expiration+0xb>
> 2a9: eb 85 jmp 230 <pm_runtime_autosuspend_expiration.part.0>
> 2ab: 31 c0 xor %eax,%eax
> 2ad: c3 retq
> 2ae: 66 90 xchg %ax,%ax
>
>
> [snip]
> > > Well, main concern was not to call ktime_get at the beginning of function
> > > as it is not too cheap.
> >
> > Doesn't the compiler optimize that to just before the test ?
>
> No (compare with above). And it is also almost certain that someone will run
> script and send "...do not needlesly initialize..." patch :)
>
> gcc version 8.2.1 20181130 (OSELAS.Toolchain-2018.12.0 8-20181130)
>
> 00000110 <pm_runtime_autosuspend_expiration>:
> 110: e92d41f0 push {r4, r5, r6, r7, r8, lr}
> 114: e1a06000 mov r6, r0
> 118: ebfffffe bl 0 <ktime_get>
> 11c: e5d630d1 ldrb r3, [r6, #209] ; 0xd1
> 120: e3130004 tst r3, #4
> 124: 0a00000d beq 160 <pm_runtime_autosuspend_expiration+0x50>
> 128: e596c0e4 ldr ip, [r6, #228] ; 0xe4
> 12c: e35c0000 cmp ip, #0
> 130: ba00000a blt 160 <pm_runtime_autosuspend_expiration+0x50>
> 134: e28630e8 add r3, r6, #232 ; 0xe8
> 138: e893000c ldm r3, {r2, r3}
> 13c: e1a05001 mov r5, r1
> 140: e1a04000 mov r4, r0
> 144: e59f002c ldr r0, [pc, #44] ; 178 <pm_runtime_autosuspend_expiration+0x68>
> 148: e0010c90 mul r1, r0, ip
> 14c: e0926001 adds r6, r2, r1
> 150: e0a37fc1 adc r7, r3, r1, asr #31
> 154: e1550007 cmp r5, r7
> 158: 01540006 cmpeq r4, r6
> 15c: 3a000001 bcc 168 <pm_runtime_autosuspend_expiration+0x58>
> 160: e3a06000 mov r6, #0
> 164: e3a07000 mov r7, #0
> 168: e1a00006 mov r0, r6
> 16c: e1a01007 mov r1, r7
> 170: e8bd41f0 pop {r4, r5, r6, r7, r8, lr}
> 174: e12fff1e bx lr
> 178: 000f4240 .word 0x000f4240
* Vincent Guittot <[email protected]> [190109 16:48]:
> On Wed, 9 Jan 2019 at 17:28, Tony Lindgren <[email protected]> wrote:
> >
> > * Rafael J. Wysocki <[email protected]> [190109 09:44]:
> > > On Wed, Jan 9, 2019 at 2:51 AM Tony Lindgren <[email protected]> wrote:
> > > >
> > > > * Vincent Guittot <[email protected]> [190109 01:42]:
> > > > > Le Tuesday 08 Jan 2019 à 13:37:43 (-0800), Tony Lindgren a écrit :
> > > > > > Lowering the autosuspend_delay_ms to 2100 ms makes things work again.
> > > > > > Anything higher than 2200 ms seems to somehow time out immediately
> > > > > > now :)
> > > > >
> > > > > This is quite close to the max ns of an int on arm 32bits
> > > > >
> > > > > Could you try the patch below ?
> > > >
> > > > Yup great thanks, that's it:
> > > >
> > > > Tested-by: Tony Lindgren <[email protected]>
> > >
> > > Cool. Thanks for getting to the bottom of this!
> >
> > No problem.
> >
> > One more thing I noticed: The 25% slack can get noticeable
> > for larger values. For things like a 3 second uart console
> > timeout slack of 750 ms is quite large variation.
> >
> > Should we have a limit of max 100 ms for the slack?
>
> Keep in mind that when jiffies were used, expires was rounded to a
> full second when delay was greater than a second. So you could already
> have difference of up 990ms on arm before this patch
> And i don't take into account the rework of timer infra which add
> another level of variation, something like up to 640 ms more when the
> timer is greater than 2880 ms for arm IIRC
I think it was rounded up earlier.
Don't we get rounded down now also?
Regards,
Tony
On Wed, 9 Jan 2019 at 17:50, Tony Lindgren <[email protected]> wrote:
>
> * Vincent Guittot <[email protected]> [190109 16:48]:
> > On Wed, 9 Jan 2019 at 17:28, Tony Lindgren <[email protected]> wrote:
> > >
> > > * Rafael J. Wysocki <[email protected]> [190109 09:44]:
> > > > On Wed, Jan 9, 2019 at 2:51 AM Tony Lindgren <[email protected]> wrote:
> > > > >
> > > > > * Vincent Guittot <[email protected]> [190109 01:42]:
> > > > > > Le Tuesday 08 Jan 2019 à 13:37:43 (-0800), Tony Lindgren a écrit :
> > > > > > > Lowering the autosuspend_delay_ms to 2100 ms makes things work again.
> > > > > > > Anything higher than 2200 ms seems to somehow time out immediately
> > > > > > > now :)
> > > > > >
> > > > > > This is quite close to the max ns of an int on arm 32bits
> > > > > >
> > > > > > Could you try the patch below ?
> > > > >
> > > > > Yup great thanks, that's it:
> > > > >
> > > > > Tested-by: Tony Lindgren <[email protected]>
> > > >
> > > > Cool. Thanks for getting to the bottom of this!
> > >
> > > No problem.
> > >
> > > One more thing I noticed: The 25% slack can get noticeable
> > > for larger values. For things like a 3 second uart console
> > > timeout slack of 750 ms is quite large variation.
> > >
> > > Should we have a limit of max 100 ms for the slack?
> >
> > Keep in mind that when jiffies were used, expires was rounded to a
> > full second when delay was greater than a second. So you could already
> > have difference of up 990ms on arm before this patch
> > And i don't take into account the rework of timer infra which add
> > another level of variation, something like up to 640 ms more when the
> > timer is greater than 2880 ms for arm IIRC
>
> I think it was rounded up earlier.
>
> Don't we get rounded down now also?
We still round up. In hrtimer we have :
timer->_softexpires = time;
timer->node.expires = ktime_add_safe(time, delta);
so the hrtimer will expire between "time" and "time+delta"
>
> Regards,
>
> Tony
* Vincent Guittot <[email protected]> [190109 16:56]:
> On Wed, 9 Jan 2019 at 17:50, Tony Lindgren <[email protected]> wrote:
> >
> > * Vincent Guittot <[email protected]> [190109 16:48]:
> > > On Wed, 9 Jan 2019 at 17:28, Tony Lindgren <[email protected]> wrote:
> > > >
> > > > * Rafael J. Wysocki <[email protected]> [190109 09:44]:
> > > > > On Wed, Jan 9, 2019 at 2:51 AM Tony Lindgren <[email protected]> wrote:
> > > > > >
> > > > > > * Vincent Guittot <[email protected]> [190109 01:42]:
> > > > > > > Le Tuesday 08 Jan 2019 à 13:37:43 (-0800), Tony Lindgren a écrit :
> > > > > > > > Lowering the autosuspend_delay_ms to 2100 ms makes things work again.
> > > > > > > > Anything higher than 2200 ms seems to somehow time out immediately
> > > > > > > > now :)
> > > > > > >
> > > > > > > This is quite close to the max ns of an int on arm 32bits
> > > > > > >
> > > > > > > Could you try the patch below ?
> > > > > >
> > > > > > Yup great thanks, that's it:
> > > > > >
> > > > > > Tested-by: Tony Lindgren <[email protected]>
> > > > >
> > > > > Cool. Thanks for getting to the bottom of this!
> > > >
> > > > No problem.
> > > >
> > > > One more thing I noticed: The 25% slack can get noticeable
> > > > for larger values. For things like a 3 second uart console
> > > > timeout slack of 750 ms is quite large variation.
> > > >
> > > > Should we have a limit of max 100 ms for the slack?
> > >
> > > Keep in mind that when jiffies were used, expires was rounded to a
> > > full second when delay was greater than a second. So you could already
> > > have difference of up 990ms on arm before this patch
> > > And i don't take into account the rework of timer infra which add
> > > another level of variation, something like up to 640 ms more when the
> > > timer is greater than 2880 ms for arm IIRC
> >
> > I think it was rounded up earlier.
> >
> > Don't we get rounded down now also?
>
> We still round up. In hrtimer we have :
> timer->_softexpires = time;
> timer->node.expires = ktime_add_safe(time, delta);
> so the hrtimer will expire between "time" and "time+delta"
OK thanks for checking it. In that case we should be good to go :)
Tony
On Wed, 9 Jan 2019 at 17:28, Tony Lindgren <[email protected]> wrote:
>
> * Rafael J. Wysocki <[email protected]> [190109 09:44]:
> > On Wed, Jan 9, 2019 at 2:51 AM Tony Lindgren <[email protected]> wrote:
> > >
> > > * Vincent Guittot <[email protected]> [190109 01:42]:
> > > > Le Tuesday 08 Jan 2019 à 13:37:43 (-0800), Tony Lindgren a écrit :
> > > > > Lowering the autosuspend_delay_ms to 2100 ms makes things work again.
> > > > > Anything higher than 2200 ms seems to somehow time out immediately
> > > > > now :)
> > > >
> > > > This is quite close to the max ns of an int on arm 32bits
> > > >
> > > > Could you try the patch below ?
> > >
> > > Yup great thanks, that's it:
> > >
> > > Tested-by: Tony Lindgren <[email protected]>
> >
> > Cool. Thanks for getting to the bottom of this!
>
> No problem.
>
> One more thing I noticed: The 25% slack can get noticeable
> for larger values. For things like a 3 second uart console
> timeout slack of 750 ms is quite large variation.
>
> Should we have a limit of max 100 ms for the slack?
Keep in mind that when jiffies were used, expires was rounded to a
full second when delay was greater than a second. So you could already
have difference of up 990ms on arm before this patch
And i don't take into account the rework of timer infra which add
another level of variation, something like up to 640 ms more when the
timer is greater than 2880 ms for arm IIRC
>
> Regards,
>
> Tony
On Wed, 9 Jan 2019 at 18:26, Ladislav Michl <[email protected]> wrote:
>
> On Wed, Jan 09, 2019 at 05:32:31PM +0100, Vincent Guittot wrote:
> > On Wed, 9 Jan 2019 at 17:07, Ladislav Michl <[email protected]> wrote:
> > >
> > > On Wed, Jan 09, 2019 at 03:12:25PM +0100, Vincent Guittot wrote:
> > > > Please keep all thread list when replying :-)
> > >
> > > Ahh, sorry for that...
> > > [snip]
> > > > On Wed, 9 Jan 2019 at 14:33, Ladislav Michl <[email protected]> wrote:
> > > > > I agree, but it doea all the magic correctly, so you won't need
> > > > > to touch that code is ktime_t changes (I know, unlikely..)
> > > >
> > > > But the current implementation doesn't care of any changes in ktime_t
> > > > as it uses raw ns
> > >
> > > Fair enough, so let's go back to:
> >
> > This one looks good for me
>
> Lets split is for 'comments fix' patch, which was just send and 'the rest'.
> Now, 'the rest' can either be v2 of your "PM/runtime: Fix autosuspend_delay on
> 32bits arch" or will wait for 5.1. You decide :)
I don't really mind.
Rafael,
Do you prefer to only take the fix for u64 casting problem or do you
prefer to also take the optimization below in one single patch ?
>
> > > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> > > index 70624695b6d5..e61ee9b47a26 100644
> > > --- a/drivers/base/power/runtime.c
> > > +++ b/drivers/base/power/runtime.c
> > > @@ -121,7 +121,7 @@ static void pm_runtime_cancel_pending(struct device *dev)
> > > * Compute the autosuspend-delay expiration time based on the device's
> > > * power.last_busy time. If the delay has already expired or is disabled
> > > * (negative) or the power.use_autosuspend flag isn't set, return 0.
> > > - * Otherwise return the expiration time in jiffies (adjusted to be nonzero).
> > > + * Otherwise return the expiration time in nanoseconds (adjusted to be nonzero).
> > > *
> > > * This function may be called either with or without dev->power.lock held.
> > > * Either way it can be racy, since power.last_busy may be updated at any time.
> > > @@ -129,24 +129,21 @@ static void pm_runtime_cancel_pending(struct device *dev)
> > > u64 pm_runtime_autosuspend_expiration(struct device *dev)
> > > {
> > > int autosuspend_delay;
> > > - u64 last_busy, expires = 0;
> > > - u64 now = ktime_to_ns(ktime_get());
> > > + u64 expires;
> > >
> > > if (!dev->power.use_autosuspend)
> > > - goto out;
> > > + return 0;
> > >
> > > autosuspend_delay = READ_ONCE(dev->power.autosuspend_delay);
> > > if (autosuspend_delay < 0)
> > > - goto out;
> > > -
> > > - last_busy = READ_ONCE(dev->power.last_busy);
> > > + return 0;
> > >
> > > - expires = last_busy + autosuspend_delay * NSEC_PER_MSEC;
> > > - if (expires <= now)
> > > - expires = 0; /* Already expired. */
> > > + expires = READ_ONCE(dev->power.last_busy);
> > > + expires += NSEC_PER_MSEC * (u64)autosuspend_delay;
> > > + if (expires > ktime_to_ns(ktime_get()))
> > > + return expires; /* Expires in the future */
> > >
> > > - out:
> > > - return expires;
> > > + return 0;
> > > }
> > > EXPORT_SYMBOL_GPL(pm_runtime_autosuspend_expiration);
> > >
> > > Which gives for arm:
> > > 00000480 <pm_runtime_autosuspend_expiration>:
> > > 480: e92d41f0 push {r4, r5, r6, r7, r8, lr}
> > > 484: e5d030d1 ldrb r3, [r0, #209] ; 0xd1
> > > 488: e3130004 tst r3, #4
> > > 48c: 0a00000c beq 4c4 <pm_runtime_autosuspend_expiration+0x44>
> > > 490: e59030e4 ldr r3, [r0, #228] ; 0xe4
> > > 494: e3530000 cmp r3, #0
> > > 498: ba000009 blt 4c4 <pm_runtime_autosuspend_expiration+0x44>
> > > 49c: e28050e8 add r5, r0, #232 ; 0xe8
> > > 4a0: e8950030 ldm r5, {r4, r5}
> > > 4a4: e59f2030 ldr r2, [pc, #48] ; 4dc <pm_runtime_autosuspend_expiration+0x5c>
> > > 4a8: e0e54392 smlal r4, r5, r2, r3
> > > 4ac: ebfffffe bl 0 <ktime_get>
> > > 4b0: e1550001 cmp r5, r1
> > > 4b4: 01540000 cmpeq r4, r0
> > > 4b8: e1a06004 mov r6, r4
> > > 4bc: e1a07005 mov r7, r5
> > > 4c0: 8a000001 bhi 4cc <pm_runtime_autosuspend_expiration+0x4c>
> > > 4c4: e3a06000 mov r6, #0
> > > 4c8: e3a07000 mov r7, #0
> > > 4cc: e1a00006 mov r0, r6
> > > 4d0: e1a01007 mov r1, r7
> > > 4d4: e8bd41f0 pop {r4, r5, r6, r7, r8, lr}
> > > 4d8: e12fff1e bx lr
> > > 4dc: 000f4240 .word 0x000f4240
> > >
> > > And x86:
> > > 0000000000000230 <pm_runtime_autosuspend_expiration.part.0>:
> > > 230: 8b 87 a4 01 00 00 mov 0x1a4(%rdi),%eax
> > > 236: 53 push %rbx
> > > 237: 85 c0 test %eax,%eax
> > > 239: 78 1e js 259 <pm_runtime_autosuspend_expiration.part.0+0x29>
> > > 23b: 48 63 d8 movslq %eax,%rbx
> > > 23e: 48 8b 97 a8 01 00 00 mov 0x1a8(%rdi),%rdx
> > > 245: 48 69 db 40 42 0f 00 imul $0xf4240,%rbx,%rbx
> > > 24c: 48 01 d3 add %rdx,%rbx
> > > 24f: e8 00 00 00 00 callq 254 <pm_runtime_autosuspend_expiration.part.0+0x24>
> > > 254: 48 39 c3 cmp %rax,%rbx
> > > 257: 77 02 ja 25b <pm_runtime_autosuspend_expiration.part.0+0x2b>
> > > 259: 31 db xor %ebx,%ebx
> > > 25b: 48 89 d8 mov %rbx,%rax
> > > 25e: 5b pop %rbx
> > > 25f: c3 retq
> > >
> > > 00000000000002a0 <pm_runtime_autosuspend_expiration>:
> > > 2a0: f6 87 91 01 00 00 04 testb $0x4,0x191(%rdi)
> > > 2a7: 74 02 je 2ab <pm_runtime_autosuspend_expiration+0xb>
> > > 2a9: eb 85 jmp 230 <pm_runtime_autosuspend_expiration.part.0>
> > > 2ab: 31 c0 xor %eax,%eax
> > > 2ad: c3 retq
> > > 2ae: 66 90 xchg %ax,%ax
> > >
> > >
> > > [snip]
> > > > > Well, main concern was not to call ktime_get at the beginning of function
> > > > > as it is not too cheap.
> > > >
> > > > Doesn't the compiler optimize that to just before the test ?
> > >
> > > No (compare with above). And it is also almost certain that someone will run
> > > script and send "...do not needlesly initialize..." patch :)
> > >
> > > gcc version 8.2.1 20181130 (OSELAS.Toolchain-2018.12.0 8-20181130)
> > >
> > > 00000110 <pm_runtime_autosuspend_expiration>:
> > > 110: e92d41f0 push {r4, r5, r6, r7, r8, lr}
> > > 114: e1a06000 mov r6, r0
> > > 118: ebfffffe bl 0 <ktime_get>
> > > 11c: e5d630d1 ldrb r3, [r6, #209] ; 0xd1
> > > 120: e3130004 tst r3, #4
> > > 124: 0a00000d beq 160 <pm_runtime_autosuspend_expiration+0x50>
> > > 128: e596c0e4 ldr ip, [r6, #228] ; 0xe4
> > > 12c: e35c0000 cmp ip, #0
> > > 130: ba00000a blt 160 <pm_runtime_autosuspend_expiration+0x50>
> > > 134: e28630e8 add r3, r6, #232 ; 0xe8
> > > 138: e893000c ldm r3, {r2, r3}
> > > 13c: e1a05001 mov r5, r1
> > > 140: e1a04000 mov r4, r0
> > > 144: e59f002c ldr r0, [pc, #44] ; 178 <pm_runtime_autosuspend_expiration+0x68>
> > > 148: e0010c90 mul r1, r0, ip
> > > 14c: e0926001 adds r6, r2, r1
> > > 150: e0a37fc1 adc r7, r3, r1, asr #31
> > > 154: e1550007 cmp r5, r7
> > > 158: 01540006 cmpeq r4, r6
> > > 15c: 3a000001 bcc 168 <pm_runtime_autosuspend_expiration+0x58>
> > > 160: e3a06000 mov r6, #0
> > > 164: e3a07000 mov r7, #0
> > > 168: e1a00006 mov r0, r6
> > > 16c: e1a01007 mov r1, r7
> > > 170: e8bd41f0 pop {r4, r5, r6, r7, r8, lr}
> > > 174: e12fff1e bx lr
> > > 178: 000f4240 .word 0x000f4240
On Wed, Jan 09, 2019 at 03:12:25PM +0100, Vincent Guittot wrote:
> Please keep all thread list when replying :-)
Ahh, sorry for that...
[snip]
> On Wed, 9 Jan 2019 at 14:33, Ladislav Michl <[email protected]> wrote:
> > I agree, but it doea all the magic correctly, so you won't need
> > to touch that code is ktime_t changes (I know, unlikely..)
>
> But the current implementation doesn't care of any changes in ktime_t
> as it uses raw ns
Fair enough, so let's go back to:
diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index 70624695b6d5..e61ee9b47a26 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -121,7 +121,7 @@ static void pm_runtime_cancel_pending(struct device *dev)
* Compute the autosuspend-delay expiration time based on the device's
* power.last_busy time. If the delay has already expired or is disabled
* (negative) or the power.use_autosuspend flag isn't set, return 0.
- * Otherwise return the expiration time in jiffies (adjusted to be nonzero).
+ * Otherwise return the expiration time in nanoseconds (adjusted to be nonzero).
*
* This function may be called either with or without dev->power.lock held.
* Either way it can be racy, since power.last_busy may be updated at any time.
@@ -129,24 +129,21 @@ static void pm_runtime_cancel_pending(struct device *dev)
u64 pm_runtime_autosuspend_expiration(struct device *dev)
{
int autosuspend_delay;
- u64 last_busy, expires = 0;
- u64 now = ktime_to_ns(ktime_get());
+ u64 expires;
if (!dev->power.use_autosuspend)
- goto out;
+ return 0;
autosuspend_delay = READ_ONCE(dev->power.autosuspend_delay);
if (autosuspend_delay < 0)
- goto out;
-
- last_busy = READ_ONCE(dev->power.last_busy);
+ return 0;
- expires = last_busy + autosuspend_delay * NSEC_PER_MSEC;
- if (expires <= now)
- expires = 0; /* Already expired. */
+ expires = READ_ONCE(dev->power.last_busy);
+ expires += NSEC_PER_MSEC * (u64)autosuspend_delay;
+ if (expires > ktime_to_ns(ktime_get()))
+ return expires; /* Expires in the future */
- out:
- return expires;
+ return 0;
}
EXPORT_SYMBOL_GPL(pm_runtime_autosuspend_expiration);
Which gives for arm:
00000480 <pm_runtime_autosuspend_expiration>:
480: e92d41f0 push {r4, r5, r6, r7, r8, lr}
484: e5d030d1 ldrb r3, [r0, #209] ; 0xd1
488: e3130004 tst r3, #4
48c: 0a00000c beq 4c4 <pm_runtime_autosuspend_expiration+0x44>
490: e59030e4 ldr r3, [r0, #228] ; 0xe4
494: e3530000 cmp r3, #0
498: ba000009 blt 4c4 <pm_runtime_autosuspend_expiration+0x44>
49c: e28050e8 add r5, r0, #232 ; 0xe8
4a0: e8950030 ldm r5, {r4, r5}
4a4: e59f2030 ldr r2, [pc, #48] ; 4dc <pm_runtime_autosuspend_expiration+0x5c>
4a8: e0e54392 smlal r4, r5, r2, r3
4ac: ebfffffe bl 0 <ktime_get>
4b0: e1550001 cmp r5, r1
4b4: 01540000 cmpeq r4, r0
4b8: e1a06004 mov r6, r4
4bc: e1a07005 mov r7, r5
4c0: 8a000001 bhi 4cc <pm_runtime_autosuspend_expiration+0x4c>
4c4: e3a06000 mov r6, #0
4c8: e3a07000 mov r7, #0
4cc: e1a00006 mov r0, r6
4d0: e1a01007 mov r1, r7
4d4: e8bd41f0 pop {r4, r5, r6, r7, r8, lr}
4d8: e12fff1e bx lr
4dc: 000f4240 .word 0x000f4240
And x86:
0000000000000230 <pm_runtime_autosuspend_expiration.part.0>:
230: 8b 87 a4 01 00 00 mov 0x1a4(%rdi),%eax
236: 53 push %rbx
237: 85 c0 test %eax,%eax
239: 78 1e js 259 <pm_runtime_autosuspend_expiration.part.0+0x29>
23b: 48 63 d8 movslq %eax,%rbx
23e: 48 8b 97 a8 01 00 00 mov 0x1a8(%rdi),%rdx
245: 48 69 db 40 42 0f 00 imul $0xf4240,%rbx,%rbx
24c: 48 01 d3 add %rdx,%rbx
24f: e8 00 00 00 00 callq 254 <pm_runtime_autosuspend_expiration.part.0+0x24>
254: 48 39 c3 cmp %rax,%rbx
257: 77 02 ja 25b <pm_runtime_autosuspend_expiration.part.0+0x2b>
259: 31 db xor %ebx,%ebx
25b: 48 89 d8 mov %rbx,%rax
25e: 5b pop %rbx
25f: c3 retq
00000000000002a0 <pm_runtime_autosuspend_expiration>:
2a0: f6 87 91 01 00 00 04 testb $0x4,0x191(%rdi)
2a7: 74 02 je 2ab <pm_runtime_autosuspend_expiration+0xb>
2a9: eb 85 jmp 230 <pm_runtime_autosuspend_expiration.part.0>
2ab: 31 c0 xor %eax,%eax
2ad: c3 retq
2ae: 66 90 xchg %ax,%ax
[snip]
> > Well, main concern was not to call ktime_get at the beginning of function
> > as it is not too cheap.
>
> Doesn't the compiler optimize that to just before the test ?
No (compare with above). And it is also almost certain that someone will run
script and send "...do not needlesly initialize..." patch :)
gcc version 8.2.1 20181130 (OSELAS.Toolchain-2018.12.0 8-20181130)
00000110 <pm_runtime_autosuspend_expiration>:
110: e92d41f0 push {r4, r5, r6, r7, r8, lr}
114: e1a06000 mov r6, r0
118: ebfffffe bl 0 <ktime_get>
11c: e5d630d1 ldrb r3, [r6, #209] ; 0xd1
120: e3130004 tst r3, #4
124: 0a00000d beq 160 <pm_runtime_autosuspend_expiration+0x50>
128: e596c0e4 ldr ip, [r6, #228] ; 0xe4
12c: e35c0000 cmp ip, #0
130: ba00000a blt 160 <pm_runtime_autosuspend_expiration+0x50>
134: e28630e8 add r3, r6, #232 ; 0xe8
138: e893000c ldm r3, {r2, r3}
13c: e1a05001 mov r5, r1
140: e1a04000 mov r4, r0
144: e59f002c ldr r0, [pc, #44] ; 178 <pm_runtime_autosuspend_expiration+0x68>
148: e0010c90 mul r1, r0, ip
14c: e0926001 adds r6, r2, r1
150: e0a37fc1 adc r7, r3, r1, asr #31
154: e1550007 cmp r5, r7
158: 01540006 cmpeq r4, r6
15c: 3a000001 bcc 168 <pm_runtime_autosuspend_expiration+0x58>
160: e3a06000 mov r6, #0
164: e3a07000 mov r7, #0
168: e1a00006 mov r0, r6
16c: e1a01007 mov r1, r7
170: e8bd41f0 pop {r4, r5, r6, r7, r8, lr}
174: e12fff1e bx lr
178: 000f4240 .word 0x000f4240
On Wed, Jan 09, 2019 at 05:32:31PM +0100, Vincent Guittot wrote:
> On Wed, 9 Jan 2019 at 17:07, Ladislav Michl <[email protected]> wrote:
> >
> > On Wed, Jan 09, 2019 at 03:12:25PM +0100, Vincent Guittot wrote:
> > > Please keep all thread list when replying :-)
> >
> > Ahh, sorry for that...
> > [snip]
> > > On Wed, 9 Jan 2019 at 14:33, Ladislav Michl <[email protected]> wrote:
> > > > I agree, but it doea all the magic correctly, so you won't need
> > > > to touch that code is ktime_t changes (I know, unlikely..)
> > >
> > > But the current implementation doesn't care of any changes in ktime_t
> > > as it uses raw ns
> >
> > Fair enough, so let's go back to:
>
> This one looks good for me
Lets split is for 'comments fix' patch, which was just send and 'the rest'.
Now, 'the rest' can either be v2 of your "PM/runtime: Fix autosuspend_delay on
32bits arch" or will wait for 5.1. You decide :)
> > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> > index 70624695b6d5..e61ee9b47a26 100644
> > --- a/drivers/base/power/runtime.c
> > +++ b/drivers/base/power/runtime.c
> > @@ -121,7 +121,7 @@ static void pm_runtime_cancel_pending(struct device *dev)
> > * Compute the autosuspend-delay expiration time based on the device's
> > * power.last_busy time. If the delay has already expired or is disabled
> > * (negative) or the power.use_autosuspend flag isn't set, return 0.
> > - * Otherwise return the expiration time in jiffies (adjusted to be nonzero).
> > + * Otherwise return the expiration time in nanoseconds (adjusted to be nonzero).
> > *
> > * This function may be called either with or without dev->power.lock held.
> > * Either way it can be racy, since power.last_busy may be updated at any time.
> > @@ -129,24 +129,21 @@ static void pm_runtime_cancel_pending(struct device *dev)
> > u64 pm_runtime_autosuspend_expiration(struct device *dev)
> > {
> > int autosuspend_delay;
> > - u64 last_busy, expires = 0;
> > - u64 now = ktime_to_ns(ktime_get());
> > + u64 expires;
> >
> > if (!dev->power.use_autosuspend)
> > - goto out;
> > + return 0;
> >
> > autosuspend_delay = READ_ONCE(dev->power.autosuspend_delay);
> > if (autosuspend_delay < 0)
> > - goto out;
> > -
> > - last_busy = READ_ONCE(dev->power.last_busy);
> > + return 0;
> >
> > - expires = last_busy + autosuspend_delay * NSEC_PER_MSEC;
> > - if (expires <= now)
> > - expires = 0; /* Already expired. */
> > + expires = READ_ONCE(dev->power.last_busy);
> > + expires += NSEC_PER_MSEC * (u64)autosuspend_delay;
> > + if (expires > ktime_to_ns(ktime_get()))
> > + return expires; /* Expires in the future */
> >
> > - out:
> > - return expires;
> > + return 0;
> > }
> > EXPORT_SYMBOL_GPL(pm_runtime_autosuspend_expiration);
> >
> > Which gives for arm:
> > 00000480 <pm_runtime_autosuspend_expiration>:
> > 480: e92d41f0 push {r4, r5, r6, r7, r8, lr}
> > 484: e5d030d1 ldrb r3, [r0, #209] ; 0xd1
> > 488: e3130004 tst r3, #4
> > 48c: 0a00000c beq 4c4 <pm_runtime_autosuspend_expiration+0x44>
> > 490: e59030e4 ldr r3, [r0, #228] ; 0xe4
> > 494: e3530000 cmp r3, #0
> > 498: ba000009 blt 4c4 <pm_runtime_autosuspend_expiration+0x44>
> > 49c: e28050e8 add r5, r0, #232 ; 0xe8
> > 4a0: e8950030 ldm r5, {r4, r5}
> > 4a4: e59f2030 ldr r2, [pc, #48] ; 4dc <pm_runtime_autosuspend_expiration+0x5c>
> > 4a8: e0e54392 smlal r4, r5, r2, r3
> > 4ac: ebfffffe bl 0 <ktime_get>
> > 4b0: e1550001 cmp r5, r1
> > 4b4: 01540000 cmpeq r4, r0
> > 4b8: e1a06004 mov r6, r4
> > 4bc: e1a07005 mov r7, r5
> > 4c0: 8a000001 bhi 4cc <pm_runtime_autosuspend_expiration+0x4c>
> > 4c4: e3a06000 mov r6, #0
> > 4c8: e3a07000 mov r7, #0
> > 4cc: e1a00006 mov r0, r6
> > 4d0: e1a01007 mov r1, r7
> > 4d4: e8bd41f0 pop {r4, r5, r6, r7, r8, lr}
> > 4d8: e12fff1e bx lr
> > 4dc: 000f4240 .word 0x000f4240
> >
> > And x86:
> > 0000000000000230 <pm_runtime_autosuspend_expiration.part.0>:
> > 230: 8b 87 a4 01 00 00 mov 0x1a4(%rdi),%eax
> > 236: 53 push %rbx
> > 237: 85 c0 test %eax,%eax
> > 239: 78 1e js 259 <pm_runtime_autosuspend_expiration.part.0+0x29>
> > 23b: 48 63 d8 movslq %eax,%rbx
> > 23e: 48 8b 97 a8 01 00 00 mov 0x1a8(%rdi),%rdx
> > 245: 48 69 db 40 42 0f 00 imul $0xf4240,%rbx,%rbx
> > 24c: 48 01 d3 add %rdx,%rbx
> > 24f: e8 00 00 00 00 callq 254 <pm_runtime_autosuspend_expiration.part.0+0x24>
> > 254: 48 39 c3 cmp %rax,%rbx
> > 257: 77 02 ja 25b <pm_runtime_autosuspend_expiration.part.0+0x2b>
> > 259: 31 db xor %ebx,%ebx
> > 25b: 48 89 d8 mov %rbx,%rax
> > 25e: 5b pop %rbx
> > 25f: c3 retq
> >
> > 00000000000002a0 <pm_runtime_autosuspend_expiration>:
> > 2a0: f6 87 91 01 00 00 04 testb $0x4,0x191(%rdi)
> > 2a7: 74 02 je 2ab <pm_runtime_autosuspend_expiration+0xb>
> > 2a9: eb 85 jmp 230 <pm_runtime_autosuspend_expiration.part.0>
> > 2ab: 31 c0 xor %eax,%eax
> > 2ad: c3 retq
> > 2ae: 66 90 xchg %ax,%ax
> >
> >
> > [snip]
> > > > Well, main concern was not to call ktime_get at the beginning of function
> > > > as it is not too cheap.
> > >
> > > Doesn't the compiler optimize that to just before the test ?
> >
> > No (compare with above). And it is also almost certain that someone will run
> > script and send "...do not needlesly initialize..." patch :)
> >
> > gcc version 8.2.1 20181130 (OSELAS.Toolchain-2018.12.0 8-20181130)
> >
> > 00000110 <pm_runtime_autosuspend_expiration>:
> > 110: e92d41f0 push {r4, r5, r6, r7, r8, lr}
> > 114: e1a06000 mov r6, r0
> > 118: ebfffffe bl 0 <ktime_get>
> > 11c: e5d630d1 ldrb r3, [r6, #209] ; 0xd1
> > 120: e3130004 tst r3, #4
> > 124: 0a00000d beq 160 <pm_runtime_autosuspend_expiration+0x50>
> > 128: e596c0e4 ldr ip, [r6, #228] ; 0xe4
> > 12c: e35c0000 cmp ip, #0
> > 130: ba00000a blt 160 <pm_runtime_autosuspend_expiration+0x50>
> > 134: e28630e8 add r3, r6, #232 ; 0xe8
> > 138: e893000c ldm r3, {r2, r3}
> > 13c: e1a05001 mov r5, r1
> > 140: e1a04000 mov r4, r0
> > 144: e59f002c ldr r0, [pc, #44] ; 178 <pm_runtime_autosuspend_expiration+0x68>
> > 148: e0010c90 mul r1, r0, ip
> > 14c: e0926001 adds r6, r2, r1
> > 150: e0a37fc1 adc r7, r3, r1, asr #31
> > 154: e1550007 cmp r5, r7
> > 158: 01540006 cmpeq r4, r6
> > 15c: 3a000001 bcc 168 <pm_runtime_autosuspend_expiration+0x58>
> > 160: e3a06000 mov r6, #0
> > 164: e3a07000 mov r7, #0
> > 168: e1a00006 mov r0, r6
> > 16c: e1a01007 mov r1, r7
> > 170: e8bd41f0 pop {r4, r5, r6, r7, r8, lr}
> > 174: e12fff1e bx lr
> > 178: 000f4240 .word 0x000f4240
On Wed, Jan 9, 2019 at 7:05 PM Vincent Guittot
<[email protected]> wrote:
>
> On Wed, 9 Jan 2019 at 18:26, Ladislav Michl <[email protected]> wrote:
> >
> > On Wed, Jan 09, 2019 at 05:32:31PM +0100, Vincent Guittot wrote:
> > > On Wed, 9 Jan 2019 at 17:07, Ladislav Michl <[email protected]> wrote:
> > > >
> > > > On Wed, Jan 09, 2019 at 03:12:25PM +0100, Vincent Guittot wrote:
> > > > > Please keep all thread list when replying :-)
> > > >
> > > > Ahh, sorry for that...
> > > > [snip]
> > > > > On Wed, 9 Jan 2019 at 14:33, Ladislav Michl <[email protected]> wrote:
> > > > > > I agree, but it doea all the magic correctly, so you won't need
> > > > > > to touch that code is ktime_t changes (I know, unlikely..)
> > > > >
> > > > > But the current implementation doesn't care of any changes in ktime_t
> > > > > as it uses raw ns
> > > >
> > > > Fair enough, so let's go back to:
> > >
> > > This one looks good for me
> >
> > Lets split is for 'comments fix' patch, which was just send and 'the rest'.
> > Now, 'the rest' can either be v2 of your "PM/runtime: Fix autosuspend_delay on
> > 32bits arch" or will wait for 5.1. You decide :)
>
> I don't really mind.
>
> Rafael,
> Do you prefer to only take the fix for u64 casting problem or do you
> prefer to also take the optimization below in one single patch ?
The casting problem is a bug and the optimization is next release
material in my view. Please send the optimization separately.
On Wed, Jan 09, 2019 at 11:06:34PM +0100, Rafael J. Wysocki wrote:
> On Wed, Jan 9, 2019 at 7:05 PM Vincent Guittot
> <[email protected]> wrote:
> >
> > On Wed, 9 Jan 2019 at 18:26, Ladislav Michl <[email protected]> wrote:
> > >
> > > On Wed, Jan 09, 2019 at 05:32:31PM +0100, Vincent Guittot wrote:
> > > > On Wed, 9 Jan 2019 at 17:07, Ladislav Michl <[email protected]> wrote:
> > > > >
> > > > > On Wed, Jan 09, 2019 at 03:12:25PM +0100, Vincent Guittot wrote:
> > > > > > Please keep all thread list when replying :-)
> > > > >
> > > > > Ahh, sorry for that...
> > > > > [snip]
> > > > > > On Wed, 9 Jan 2019 at 14:33, Ladislav Michl <[email protected]> wrote:
> > > > > > > I agree, but it doea all the magic correctly, so you won't need
> > > > > > > to touch that code is ktime_t changes (I know, unlikely..)
> > > > > >
> > > > > > But the current implementation doesn't care of any changes in ktime_t
> > > > > > as it uses raw ns
> > > > >
> > > > > Fair enough, so let's go back to:
> > > >
> > > > This one looks good for me
> > >
> > > Lets split is for 'comments fix' patch, which was just send and 'the rest'.
> > > Now, 'the rest' can either be v2 of your "PM/runtime: Fix autosuspend_delay on
> > > 32bits arch" or will wait for 5.1. You decide :)
> >
> > I don't really mind.
> >
> > Rafael,
> > Do you prefer to only take the fix for u64 casting problem or do you
> > prefer to also take the optimization below in one single patch ?
>
> The casting problem is a bug and the optimization is next release
> material in my view. Please send the optimization separately.
Ok, will send that separately in a few moments...
Btw, u64 casting problem seems to be still present in rpm_suspend while
computing slack for autosuspend delay greater than 25% of 2^31 (2^33) ns.
Not sure if that triggers any real bug.
On Thu, 10 Jan 2019 at 08:46, Ladislav Michl <[email protected]> wrote:
>
> On Wed, Jan 09, 2019 at 11:06:34PM +0100, Rafael J. Wysocki wrote:
> > On Wed, Jan 9, 2019 at 7:05 PM Vincent Guittot
> > <[email protected]> wrote:
> > >
> > > On Wed, 9 Jan 2019 at 18:26, Ladislav Michl <[email protected]> wrote:
> > > >
> > > > On Wed, Jan 09, 2019 at 05:32:31PM +0100, Vincent Guittot wrote:
> > > > > On Wed, 9 Jan 2019 at 17:07, Ladislav Michl <[email protected]> wrote:
> > > > > >
> > > > > > On Wed, Jan 09, 2019 at 03:12:25PM +0100, Vincent Guittot wrote:
> > > > > > > Please keep all thread list when replying :-)
> > > > > >
> > > > > > Ahh, sorry for that...
> > > > > > [snip]
> > > > > > > On Wed, 9 Jan 2019 at 14:33, Ladislav Michl <[email protected]> wrote:
> > > > > > > > I agree, but it doea all the magic correctly, so you won't need
> > > > > > > > to touch that code is ktime_t changes (I know, unlikely..)
> > > > > > >
> > > > > > > But the current implementation doesn't care of any changes in ktime_t
> > > > > > > as it uses raw ns
> > > > > >
> > > > > > Fair enough, so let's go back to:
> > > > >
> > > > > This one looks good for me
> > > >
> > > > Lets split is for 'comments fix' patch, which was just send and 'the rest'.
> > > > Now, 'the rest' can either be v2 of your "PM/runtime: Fix autosuspend_delay on
> > > > 32bits arch" or will wait for 5.1. You decide :)
> > >
> > > I don't really mind.
> > >
> > > Rafael,
> > > Do you prefer to only take the fix for u64 casting problem or do you
> > > prefer to also take the optimization below in one single patch ?
> >
> > The casting problem is a bug and the optimization is next release
> > material in my view. Please send the optimization separately.
>
> Ok, will send that separately in a few moments...
> Btw, u64 casting problem seems to be still present in rpm_suspend while
> computing slack for autosuspend delay greater than 25% of 2^31 (2^33) ns.
Good point. I will add it to the fix as well
> Not sure if that triggers any real bug.
On Thu, Jan 10, 2019 at 08:50:14AM +0100, Vincent Guittot wrote:
> On Thu, 10 Jan 2019 at 08:46, Ladislav Michl <[email protected]> wrote:
> >
> > On Wed, Jan 09, 2019 at 11:06:34PM +0100, Rafael J. Wysocki wrote:
> > > On Wed, Jan 9, 2019 at 7:05 PM Vincent Guittot
> > > <[email protected]> wrote:
> > > >
> > > > On Wed, 9 Jan 2019 at 18:26, Ladislav Michl <[email protected]> wrote:
> > > > >
> > > > > On Wed, Jan 09, 2019 at 05:32:31PM +0100, Vincent Guittot wrote:
> > > > > > On Wed, 9 Jan 2019 at 17:07, Ladislav Michl <[email protected]> wrote:
> > > > > > >
> > > > > > > On Wed, Jan 09, 2019 at 03:12:25PM +0100, Vincent Guittot wrote:
> > > > > > > > Please keep all thread list when replying :-)
> > > > > > >
> > > > > > > Ahh, sorry for that...
> > > > > > > [snip]
> > > > > > > > On Wed, 9 Jan 2019 at 14:33, Ladislav Michl <[email protected]> wrote:
> > > > > > > > > I agree, but it doea all the magic correctly, so you won't need
> > > > > > > > > to touch that code is ktime_t changes (I know, unlikely..)
> > > > > > > >
> > > > > > > > But the current implementation doesn't care of any changes in ktime_t
> > > > > > > > as it uses raw ns
> > > > > > >
> > > > > > > Fair enough, so let's go back to:
> > > > > >
> > > > > > This one looks good for me
> > > > >
> > > > > Lets split is for 'comments fix' patch, which was just send and 'the rest'.
> > > > > Now, 'the rest' can either be v2 of your "PM/runtime: Fix autosuspend_delay on
> > > > > 32bits arch" or will wait for 5.1. You decide :)
> > > >
> > > > I don't really mind.
> > > >
> > > > Rafael,
> > > > Do you prefer to only take the fix for u64 casting problem or do you
> > > > prefer to also take the optimization below in one single patch ?
> > >
> > > The casting problem is a bug and the optimization is next release
> > > material in my view. Please send the optimization separately.
> >
> > Ok, will send that separately in a few moments...
> > Btw, u64 casting problem seems to be still present in rpm_suspend while
> > computing slack for autosuspend delay greater than 25% of 2^31 (2^33) ns.
>
> Good point. I will add it to the fix as well
Another nit then, for (u64)(autosuspend_delay) is (u64)autosuspend_delay
enough :)
I'll wait for your v2 before sending optimization patch.
> > Not sure if that triggers any real bug.