Re: [PATCH v2 3/5] PM: runtime: Propagate last_busy from dependent to dependency

[Rafael J. Wysocki (Intel)]

On Thu, Nov 5, 2026 at 9:21 PM Brian Norris <briannorris@xxxxxxxxxxxx> wrote:
>
> Hi Rafael,
>
> On Thu, Nov 05, 2026 at 12:59:24PM +0100, Rafael J. Wysocki (Intel) wrote:
> > On Wed, Nov 4, 2026 at 8:31 PM Brian Norris <briannorris@xxxxxxxxxxxx> wrote:
> > >
> > > On Wed, Nov 04, 2026 at 02:34:22PM +0100, Rafael J. Wysocki (Intel) wrote:
> > > > I should have responded to the v1 of this, but didn't manage to get to
> > > > it.  Sorry about that.
> > >
> > > Hehe, well that's what v2 is for :)
> > >
> > > Thanks for looking.
> > >
> > > > On Tue, Nov 3, 2026 at 11:23 PM Brian Norris <briannorris@xxxxxxxxxxxx> wrote:
> > > > >
> > > > > When a child device suspends, it does not update the last_busy timestamp
> > > > > for its parent. If that parent configured autosuspend and didn't
> > > > > otherwise maintain its last_busy timestamp, it may now be immediately
> > > > > eligible to suspend. This is probably not expected -- the parent should
> > > > > wait for its autosuspend delay before suspending.
> > > >
> > > > Let's first talk about what autosuspend is for: My understanding is
> > > > that it is there to avoid incurring the runtime PM suspend and resume
> > > > latency (and cost in terms of energy) in the cases when the device is
> > > > known or at least is expected to be needed in the near future.  IOW,
> > > > we want to defer the runtime suspend of the device because if it
> > > > suspended, it would be resumed shortly thereafter.
> > > >
> > > > I'm not sure if that is always the case when a dependent device
> > > > suspends and its reference to the parent/supplier is dropped.
> > >
> > > To be clear: dropping a reference is not the point at which we extend
> > > the timer -- it's simply the point at which we propagate the last_busy
> > > timestamp.
> > >
> > > The naive algorithm is:
> > > Every time a device updates its last_busy, sync that timestamp to all
> > > its dependencies.
> > >
> > > My proposal modifies that naive algorithm so that it avoids additional
> > > graph walks -- it defers that "sync" step until later. (Now that I'm
> > > spelling this out, I feel like I should have included this in the commit
> > > message...)
> >
> > OK, thanks for explaining!
> >
> > You seem to be arguing that the current behavior is counter-intuitive
> > because people would generally expect the parent's (or supplier's)
> > last_busy to be automatically advanced to the last "busy" time of its
> > last active child (or consumer).
>
> More or less. Or, I don't see why "child was recently busy and may be
> busy again soon" should not translate to "everything the child uses
> might be needed again soon".
>
> > That is a bit far-fetched IMV because the child's last "busy" time
> > need not be the same as the parent's last "busy" time, at least in
> > principle (of course, that is debatable, but that debate can be had
> > later).
>
> I don't get this -- I'm very much asking for that debate right now,
> because so far, I disagree :)
>
> > Still, the underlying observation seems to be that if the child
> > suspends and then resumes relatively shortly, it may not make sense to
> > suspend and resume the parent in the meantime, which is fair enough.
> > Of course, advancing the parent's last_busy will only help here if its
> > autosuspend delay is long enough though.
>
> Right. But that's the whole point of this argument -- if all autosuspend
> delays were the same, this wouldn't really matter. But the presumption
> is that the parent wants delay because it's more costly -- it will be
> imposing extra wakeup cost for its children if it's not accounted
> properly.
>
> > So you can argue that if the parent's autosuspend delay is long
> > enough, then it should prevent the parent from suspending across
> > relatively short periods of child suspension.  Again, this is fair
> > enough, so why does it not happen at times?
> >
> > It certainly does happen when the parent actually has to resume
> > because rpm_resume() calls pm_runtime_mark_last_busy() for it then and
> > (given a sufficiently long autosuspend delay) it will stay active if
> > the child resumes a short enough time after suspending.  However, when
> > the child resumes, the parent is still active, so rpm_resume()
> > triggered by the child does not advance its last_busy timestamp, so at
> > one point in the future it will suspend when the child suspends.
> >
> > This arguably is inconsistent, so my approach to addressing the
> > problem at hand would be the attached patch (sorry for the attachment,
> > but GMail is generally not good at handling appended patches).  If I'm
> > not mistaken, it should be sufficient to address the particular issue
> > that triggered commit c537d3457542.
>
> It's only sufficient if the child is not active for "too long". Suppose
> the child stays active for 2s and then suspends briefly; your solution
> does not work.

It doesn't work in the sense that it doesn't do what you want.  But fair enough.

> In commit c537d3457542, I imagine the 0.2s polling example is just an
> example;

I'm not sure about that, but OK, say it's just an example.

> what if that period was 0.1s, with some natural variance up or
> down? Then I could easily see how the child remains active for longer
> than 2s, and then occasionally is inactive for longer than 0.1s,
> allowing both the child and parent to autosuspend.
>
> Or in concrete terms: see the appended test diff, which attempts to
> emulate:
>
>  - 200 cycles of child get/put
>  - delay in between each put -> get cycle
>  - delay of approximately 100ms, with a configurable +/- variation
>  - apply the shorter variation (e.g., 90ms) most of the time, with a
>    longer variation approximately every 2s
>
> So for example, that looks something like:
>
>  cycle 0 to 22:   delay 90ms
>  cycle 23:        delay 110ms
>  cycle 24 to 46:  delay 90ms
>  ...
>
> With your patch:
>
>    0ms variation == PASS (child suspends 200 times, parent suspends 0 times)
>   10ms variation == FAIL (child suspends 21 times, parent suspends 4 times)
>   20ms variation == FAIL (child suspends 7 times, parent suspends 7 times)
>   30ms variation == FAIL (child suspends 6 times, parent suspends 6 times)
>   40ms variation == FAIL (child suspends 5 times, parent suspends 5 times)
>   50ms variation == FAIL (child suspends 4 times, parent suspends 4 times)
>   60ms variation == FAIL (child suspends 3 times, parent suspends 3 times)
>   70ms variation == FAIL (child suspends 2 times, parent suspends 2 times)
>   80ms variation == FAIL (child suspends 1 time, parent suspends 1 time)
>   90ms variation == PASS (test didn't run long enough to fail)
>
> With my patch, all cases pass. (The child suspends in the same pattern,
> but the parent does not suspend in between.)
>
> For comparison, with neither patch:
>
>    0ms variation == FAIL (child suspends 200 times, parent suspends 10 times)
>   10ms variation == FAIL (child suspends 14 times, parent suspends 8 times)
>   20ms variation == FAIL (child suspends 7 times, parent suspends 7 times)
>   30ms variation == FAIL (child suspends 6 times, parent suspends 6 times)
>   40ms variation == FAIL (child suspends 5 times, parent suspends 5 times)
>   50ms variation == FAIL (child suspends 4 times, parent suspends 4 times)
>   60ms variation == FAIL (child suspends 3 times, parent suspends 3 times)
>   70ms variation == FAIL (child suspends 2 times, parent suspends 2 times)
>   80ms variation == FAIL (child suspends 1 time, parent suspends 1 time)
>   90ms variation == PASS (test didn't run long enough to fail)
>
> So, your patch makes an improvement, but it doesn't handle this kind of
> variation very well.

And it is not designed to do so.

Of course, it will not prevent the parent from suspending if the child
does not attempt to resume it for a time longer than its autosuspend
delay.  That's by design and you didn't have to take the time to
demonstrate it.

The question is how often those kinds of patterns occur in practice.
Or IOW, whether or not they occur often enough to justify adding
complexity related specifically to addressing them.

> > Note that I'm not worried about the potential raciness of this new
> > last_busy update with other possible concurrent updates of last_busy
> > because they all use approximations of the current time and if they
> > are close enough to each other, it should not matter who wins the
> > race.
>
> Ack, that's one advantage in your style of solution, because you're only
> handling timestamps "in the moment."
>
> > > With that said:
> > >
> > > > The core calls pm_runtime_mark_last_busy() for resuming devices under
> > > > the assumption that they are resumed for a reason and so it is likely
> > > > that they will be needed in near future, but I'm not sure if that
> > > > assumption can be extended to a child suspend case.
> > >
> > > But that's not what I'm doing: I'm not calling mark_last_busy() -- I'm
> > > just making mark_last_busy() transitive to all dependencies. So any time
> > > we *already* say the child is busy, I also extend that to the parent --
> > > I'm not generating any new timestamps; I only reuse the timestamps we
> > > already selected.
> > >
> > > My view on the logic (using your terms above): when a child calls
> > > mark_last_busy(), it suggests that child might be needed in the near
> > > future. Whenever a child device is needed, so is its parent (barring
> > > ignore_children). Thus, mark_last_busy() should also refresh the
> > > parent's busy timestamp.
> >
> > Of course, it can be argued this way though there are
> > counter-arguments (probably not worth discussing in detail at this
> > point) and there is the question how much of a difference this would
> > make after addressing the rpm_resume() inconsistency mentioned above
> > (which IMV needs to be address regardless because it is not limited to
> > the parent-child or supplier-consumer situations).
>
> I would love to hear your counterarguments.

OK, I'll say more about that tomorrow (it's past 10 PM here, time to
go away from the keyboard).

> I don't believe your solution solves the problem,

That depends on what problem in particular you mean.

It addresses an inconsistency in rpm_resume() and as I said above,
this needs to be done anyway because it is not only related to the
cases when children or consumers are involved.  There are other cases
in which rpm_resume() gets called for an active device and the lack of
last_busy update causes the device to suspend earlier than expected.

> and my solution does while also handling
> the inconsistency you mention.

I'm not sure about that.

For example, say a device has an autosuspend delay of 2 s.  Say it is
initially suspended and user space writes "on" to its "power/control",
so the device resumes.  Now, 1 s later, user space writes "auto" to
the device's "power/control" and the autosuspend delay is respected,
so the device doesn't suspend.  Next, 1.5 s after the first resume,
user space writes "on" to the device's "power/control" and the device
does not resume because it is active (its last_busy is not updated).
Finally, user space writes "auto" to the device's "power/control", say
1 s later, and the device suspends even though it can be expected to
stay active for at least 1 s more.

How is that going to be addressed by propagating last_busy from
children/consumers to parents/suppliers?

Note that I'm not dismissing your proposal, I'm just having doubts
about the need to do this kind of thing in the core code.