mbox series

[v1,0/5] PM: sleep: Improvements of async suspend and resume of devices

Message ID 13709135.uLZWGnKmhe@rjwysocki.net (mailing list archive)
Headers show
Series PM: sleep: Improvements of async suspend and resume of devices | expand

Message

Rafael J. Wysocki Feb. 25, 2025, 4:38 p.m. UTC
Hi Everyone,

Initially, this was an attempt to address the problems described by
Saravana related to spawning async work for any async device upfront
in the resume path:

https://lore.kernel.org/linux-pm/20241114220921.2529905-1-saravanak@google.com/

but then I realized that it could be extended to the suspend path and
used for speeding it up, which it really does.

Overall, the idea is that instead of starting an async work item for every
async device upfront, which is not very efficient because the majority of
those devices will not be able to make progress due to dependencies anyway,
the async handling is only started upfront for the devices that are likely
to be able to make progress.  That is, devices without parents in the resume
path and leaf devices (ie. devices without children or consumers) in the
suspend path (the underlying observation here is that devices without parents
are likely to have no suppliers too whereas devices without children that
have consumers are not unheard of).  This allows to reduce the amount of
processing that needs to be done to start with.

Then, after processing every device ("async" or "sync"), "async" processing
is started for some devices that have been "unblocked" by it, which are its
children in the resume path or its parent and its suppliers in the suspend
path.  This allows asynchronous handling to start as soon as it makes sense
without delaying the "async" devices unnecessarily.

Fortunately, the additional plumbing needed to implement this is not
particularly complicated.

The first two patches in the series are preparatory.

Patch [3/5] deals with the resume path for all device resume phases.

Patch [4/5] optimizes the "suspend" phase which has the most visible effect (on
the systems in my office the speedup is in the 100 ms range which is around 20%
of the total device resume time).

Patch [5/5] extend this to the "suspend late" and "suspend noirq" phases.

Thanks!

Comments

Saravana Kannan Feb. 27, 2025, 3:44 p.m. UTC | #1
On Tue, Feb 25, 2025 at 8:46 AM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> Hi Everyone,
>
> Initially, this was an attempt to address the problems described by
> Saravana related to spawning async work for any async device upfront
> in the resume path:
>
> https://lore.kernel.org/linux-pm/20241114220921.2529905-1-saravanak@google.com/
>
> but then I realized that it could be extended to the suspend path and
> used for speeding it up, which it really does.

Btw, maybe I didn't  word it correctly, but my patch series was meant
to speed up the non-async case too.

I was going to get around sending a v2 of my series, but was caught up
with some other work. But I'm okay if you want to finish up my effort
-- less work for me and I can focus on the other aspects of suspend :)

Maybe add a Suggested-by: to the patches?

I definitely want to review the series, but very busy this week with
some other work. I'll get to this next week for sure.

Thanks,
Saravana

>
> Overall, the idea is that instead of starting an async work item for every
> async device upfront, which is not very efficient because the majority of
> those devices will not be able to make progress due to dependencies anyway,
> the async handling is only started upfront for the devices that are likely
> to be able to make progress.  That is, devices without parents in the resume
> path and leaf devices (ie. devices without children or consumers) in the
> suspend path (the underlying observation here is that devices without parents
> are likely to have no suppliers too whereas devices without children that
> have consumers are not unheard of).  This allows to reduce the amount of
> processing that needs to be done to start with.
>
> Then, after processing every device ("async" or "sync"), "async" processing
> is started for some devices that have been "unblocked" by it, which are its
> children in the resume path or its parent and its suppliers in the suspend
> path.  This allows asynchronous handling to start as soon as it makes sense
> without delaying the "async" devices unnecessarily.
>
> Fortunately, the additional plumbing needed to implement this is not
> particularly complicated.
>
> The first two patches in the series are preparatory.
>
> Patch [3/5] deals with the resume path for all device resume phases.
>
> Patch [4/5] optimizes the "suspend" phase which has the most visible effect (on
> the systems in my office the speedup is in the 100 ms range which is around 20%
> of the total device resume time).
>
> Patch [5/5] extend this to the "suspend late" and "suspend noirq" phases.
>
> Thanks!
>
>
>
Rafael J. Wysocki Feb. 27, 2025, 4:23 p.m. UTC | #2
On Thu, Feb 27, 2025 at 4:45 PM Saravana Kannan <saravanak@google.com> wrote:
>
> On Tue, Feb 25, 2025 at 8:46 AM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >
> > Hi Everyone,
> >
> > Initially, this was an attempt to address the problems described by
> > Saravana related to spawning async work for any async device upfront
> > in the resume path:
> >
> > https://lore.kernel.org/linux-pm/20241114220921.2529905-1-saravanak@google.com/
> >
> > but then I realized that it could be extended to the suspend path and
> > used for speeding it up, which it really does.
>
> Btw, maybe I didn't  word it correctly, but my patch series was meant
> to speed up the non-async case too.

If "the non-async case" means the case with "async" suspend/resume
disabled entirely, I don't think that the ordering in which devices
are processed can be changed just because there are no known
dependencies.

> I was going to get around sending a v2 of my series, but was caught up
> with some other work. But I'm okay if you want to finish up my effort
> -- less work for me and I can focus on the other aspects of suspend :)
>
> Maybe add a Suggested-by: to the patches?

Yeah, I can do that.

> I definitely want to review the series, but very busy this week with
> some other work. I'll get to this next week for sure.

That should be fine.

> > Overall, the idea is that instead of starting an async work item for every
> > async device upfront, which is not very efficient because the majority of
> > those devices will not be able to make progress due to dependencies anyway,
> > the async handling is only started upfront for the devices that are likely
> > to be able to make progress.  That is, devices without parents in the resume
> > path and leaf devices (ie. devices without children or consumers) in the
> > suspend path (the underlying observation here is that devices without parents
> > are likely to have no suppliers too whereas devices without children that
> > have consumers are not unheard of).  This allows to reduce the amount of
> > processing that needs to be done to start with.
> >
> > Then, after processing every device ("async" or "sync"), "async" processing
> > is started for some devices that have been "unblocked" by it, which are its
> > children in the resume path or its parent and its suppliers in the suspend
> > path.  This allows asynchronous handling to start as soon as it makes sense
> > without delaying the "async" devices unnecessarily.
> >
> > Fortunately, the additional plumbing needed to implement this is not
> > particularly complicated.
> >
> > The first two patches in the series are preparatory.
> >
> > Patch [3/5] deals with the resume path for all device resume phases.
> >
> > Patch [4/5] optimizes the "suspend" phase which has the most visible effect (on
> > the systems in my office the speedup is in the 100 ms range which is around 20%
> > of the total device resume time).
> >
> > Patch [5/5] extend this to the "suspend late" and "suspend noirq" phases.
> >
> > Thanks!
Ulf Hansson March 3, 2025, 12:06 p.m. UTC | #3
On Tue, 25 Feb 2025 at 17:46, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> Hi Everyone,
>
> Initially, this was an attempt to address the problems described by
> Saravana related to spawning async work for any async device upfront
> in the resume path:
>
> https://lore.kernel.org/linux-pm/20241114220921.2529905-1-saravanak@google.com/
>
> but then I realized that it could be extended to the suspend path and
> used for speeding it up, which it really does.
>
> Overall, the idea is that instead of starting an async work item for every
> async device upfront, which is not very efficient because the majority of
> those devices will not be able to make progress due to dependencies anyway,
> the async handling is only started upfront for the devices that are likely
> to be able to make progress.  That is, devices without parents in the resume
> path and leaf devices (ie. devices without children or consumers) in the
> suspend path (the underlying observation here is that devices without parents
> are likely to have no suppliers too whereas devices without children that
> have consumers are not unheard of).  This allows to reduce the amount of
> processing that needs to be done to start with.
>
> Then, after processing every device ("async" or "sync"), "async" processing
> is started for some devices that have been "unblocked" by it, which are its
> children in the resume path or its parent and its suppliers in the suspend
> path.  This allows asynchronous handling to start as soon as it makes sense
> without delaying the "async" devices unnecessarily.
>
> Fortunately, the additional plumbing needed to implement this is not
> particularly complicated.

Thanks for the detailed description! Overall, the approach makes
perfect sense to me too!

I am certainly interested to hear Saravana's thoughts around this too.

>
> The first two patches in the series are preparatory.

For these two, feel free to add:

Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>

>
> Patch [3/5] deals with the resume path for all device resume phases.
>
> Patch [4/5] optimizes the "suspend" phase which has the most visible effect (on
> the systems in my office the speedup is in the 100 ms range which is around 20%
> of the total device resume time).
>
> Patch [5/5] extend this to the "suspend late" and "suspend noirq" phases.

I will try to have a closer look at patch 3->5 later in the week.

Kind regards
Uffe
Rafael J. Wysocki March 3, 2025, 12:08 p.m. UTC | #4
On Mon, Mar 3, 2025 at 1:07 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Tue, 25 Feb 2025 at 17:46, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >
> > Hi Everyone,
> >
> > Initially, this was an attempt to address the problems described by
> > Saravana related to spawning async work for any async device upfront
> > in the resume path:
> >
> > https://lore.kernel.org/linux-pm/20241114220921.2529905-1-saravanak@google.com/
> >
> > but then I realized that it could be extended to the suspend path and
> > used for speeding it up, which it really does.
> >
> > Overall, the idea is that instead of starting an async work item for every
> > async device upfront, which is not very efficient because the majority of
> > those devices will not be able to make progress due to dependencies anyway,
> > the async handling is only started upfront for the devices that are likely
> > to be able to make progress.  That is, devices without parents in the resume
> > path and leaf devices (ie. devices without children or consumers) in the
> > suspend path (the underlying observation here is that devices without parents
> > are likely to have no suppliers too whereas devices without children that
> > have consumers are not unheard of).  This allows to reduce the amount of
> > processing that needs to be done to start with.
> >
> > Then, after processing every device ("async" or "sync"), "async" processing
> > is started for some devices that have been "unblocked" by it, which are its
> > children in the resume path or its parent and its suppliers in the suspend
> > path.  This allows asynchronous handling to start as soon as it makes sense
> > without delaying the "async" devices unnecessarily.
> >
> > Fortunately, the additional plumbing needed to implement this is not
> > particularly complicated.
>
> Thanks for the detailed description! Overall, the approach makes
> perfect sense to me too!
>
> I am certainly interested to hear Saravana's thoughts around this too.
>
> >
> > The first two patches in the series are preparatory.
>
> For these two, feel free to add:
>
> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
>
> >
> > Patch [3/5] deals with the resume path for all device resume phases.
> >
> > Patch [4/5] optimizes the "suspend" phase which has the most visible effect (on
> > the systems in my office the speedup is in the 100 ms range which is around 20%
> > of the total device resume time).
> >
> > Patch [5/5] extend this to the "suspend late" and "suspend noirq" phases.
>
> I will try to have a closer look at patch 3->5 later in the week.

Thank you and thanks for all of the other reviews!
Saravana Kannan March 9, 2025, 10:37 p.m. UTC | #5
On Thu, Feb 27, 2025 at 8:23 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Thu, Feb 27, 2025 at 4:45 PM Saravana Kannan <saravanak@google.com> wrote:
> >
> > On Tue, Feb 25, 2025 at 8:46 AM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > >
> > > Hi Everyone,
> > >
> > > Initially, this was an attempt to address the problems described by
> > > Saravana related to spawning async work for any async device upfront
> > > in the resume path:
> > >
> > > https://lore.kernel.org/linux-pm/20241114220921.2529905-1-saravanak@google.com/
> > >
> > > but then I realized that it could be extended to the suspend path and
> > > used for speeding it up, which it really does.
> >
> > Btw, maybe I didn't  word it correctly, but my patch series was meant
> > to speed up the non-async case too.
>
> If "the non-async case" means the case with "async" suspend/resume
> disabled entirely, I don't think that the ordering in which devices
> are processed can be changed just because there are no known
> dependencies.
>
> > I was going to get around sending a v2 of my series, but was caught up
> > with some other work. But I'm okay if you want to finish up my effort
> > -- less work for me and I can focus on the other aspects of suspend :)
> >
> > Maybe add a Suggested-by: to the patches?
>
> Yeah, I can do that.
>
> > I definitely want to review the series, but very busy this week with
> > some other work. I'll get to this next week for sure.
>
> That should be fine.

Hi Rafael,

I looked at the full series and it has at least one bug and a few gaps
that I address in mine. And those are what make my patches have a
higher diff. Can we just continue with my series instead? That'll be
easier to explain and more forward for me than reviewing your patch
and making sure it covers everything I already tried to deal with.

You partially reviewed my patches, if you can give me more details on
my patch 1 and what else you want me to do for the rest of them, I'd
be happy to do that.

Thanks,
Saravana

>
> > > Overall, the idea is that instead of starting an async work item for every
> > > async device upfront, which is not very efficient because the majority of
> > > those devices will not be able to make progress due to dependencies anyway,
> > > the async handling is only started upfront for the devices that are likely
> > > to be able to make progress.  That is, devices without parents in the resume
> > > path and leaf devices (ie. devices without children or consumers) in the
> > > suspend path (the underlying observation here is that devices without parents
> > > are likely to have no suppliers too whereas devices without children that
> > > have consumers are not unheard of).  This allows to reduce the amount of
> > > processing that needs to be done to start with.
> > >
> > > Then, after processing every device ("async" or "sync"), "async" processing
> > > is started for some devices that have been "unblocked" by it, which are its
> > > children in the resume path or its parent and its suppliers in the suspend
> > > path.  This allows asynchronous handling to start as soon as it makes sense
> > > without delaying the "async" devices unnecessarily.
> > >
> > > Fortunately, the additional plumbing needed to implement this is not
> > > particularly complicated.
> > >
> > > The first two patches in the series are preparatory.
> > >
> > > Patch [3/5] deals with the resume path for all device resume phases.
> > >
> > > Patch [4/5] optimizes the "suspend" phase which has the most visible effect (on
> > > the systems in my office the speedup is in the 100 ms range which is around 20%
> > > of the total device resume time).
> > >
> > > Patch [5/5] extend this to the "suspend late" and "suspend noirq" phases.
> > >
> > > Thanks!
Rafael J. Wysocki March 10, 2025, 4:01 p.m. UTC | #6
On Sun, Mar 9, 2025 at 11:38 PM Saravana Kannan <saravanak@google.com> wrote:
>
> On Thu, Feb 27, 2025 at 8:23 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> >
> > On Thu, Feb 27, 2025 at 4:45 PM Saravana Kannan <saravanak@google.com> wrote:
> > >
> > > On Tue, Feb 25, 2025 at 8:46 AM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > > >
> > > > Hi Everyone,
> > > >
> > > > Initially, this was an attempt to address the problems described by
> > > > Saravana related to spawning async work for any async device upfront
> > > > in the resume path:
> > > >
> > > > https://lore.kernel.org/linux-pm/20241114220921.2529905-1-saravanak@google.com/
> > > >
> > > > but then I realized that it could be extended to the suspend path and
> > > > used for speeding it up, which it really does.
> > >
> > > Btw, maybe I didn't  word it correctly, but my patch series was meant
> > > to speed up the non-async case too.
> >
> > If "the non-async case" means the case with "async" suspend/resume
> > disabled entirely, I don't think that the ordering in which devices
> > are processed can be changed just because there are no known
> > dependencies.
> >
> > > I was going to get around sending a v2 of my series, but was caught up
> > > with some other work. But I'm okay if you want to finish up my effort
> > > -- less work for me and I can focus on the other aspects of suspend :)
> > >
> > > Maybe add a Suggested-by: to the patches?
> >
> > Yeah, I can do that.
> >
> > > I definitely want to review the series, but very busy this week with
> > > some other work. I'll get to this next week for sure.
> >
> > That should be fine.
>
> Hi Rafael,
>
> I looked at the full series and it has at least one bug and a few gaps
> that I address in mine.

What bug?

You need to tell me specifically because I'm not aware of any bugs in
this series and unless you tell me what it is and I agree that it is a
bug, I have no reason to believe that there are any.

As for the gaps, there are obvious differences between this patch
series and your work and it would be kind of nice to explain why they
matter in practice, in your view.

> And those are what make my patches have a
> higher diff. Can we just continue with my series instead?

Of course you are free to send a new version of it, but it is unlikely
to be a sufficient replacement for constructive feedback.
Saravana Kannan March 10, 2025, 8:31 p.m. UTC | #7
On Mon, Mar 10, 2025 at 9:01 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Sun, Mar 9, 2025 at 11:38 PM Saravana Kannan <saravanak@google.com> wrote:
> >
> > On Thu, Feb 27, 2025 at 8:23 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > >
> > > On Thu, Feb 27, 2025 at 4:45 PM Saravana Kannan <saravanak@google.com> wrote:
> > > >
> > > > On Tue, Feb 25, 2025 at 8:46 AM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > > > >
> > > > > Hi Everyone,
> > > > >
> > > > > Initially, this was an attempt to address the problems described by
> > > > > Saravana related to spawning async work for any async device upfront
> > > > > in the resume path:
> > > > >
> > > > > https://lore.kernel.org/linux-pm/20241114220921.2529905-1-saravanak@google.com/
> > > > >
> > > > > but then I realized that it could be extended to the suspend path and
> > > > > used for speeding it up, which it really does.
> > > >
> > > > Btw, maybe I didn't  word it correctly, but my patch series was meant
> > > > to speed up the non-async case too.
> > >
> > > If "the non-async case" means the case with "async" suspend/resume
> > > disabled entirely, I don't think that the ordering in which devices
> > > are processed can be changed just because there are no known
> > > dependencies.
> > >
> > > > I was going to get around sending a v2 of my series, but was caught up
> > > > with some other work. But I'm okay if you want to finish up my effort
> > > > -- less work for me and I can focus on the other aspects of suspend :)
> > > >
> > > > Maybe add a Suggested-by: to the patches?
> > >
> > > Yeah, I can do that.
> > >
> > > > I definitely want to review the series, but very busy this week with
> > > > some other work. I'll get to this next week for sure.
> > >
> > > That should be fine.
> >
> > Hi Rafael,
> >
> > I looked at the full series and it has at least one bug and a few gaps
> > that I address in mine.
>
> What bug?
>
> You need to tell me specifically because I'm not aware of any bugs in
> this series and unless you tell me what it is and I agree that it is a
> bug, I have no reason to believe that there are any.
>
> As for the gaps, there are obvious differences between this patch
> series and your work and it would be kind of nice to explain why they
> matter in practice, in your view.

Sure, I'll do this. But it just felt like an inefficient way to get to
close to where my series is. Instead of you just saying you don't like
about my series and giving me some feedback on how to fix it.

> > And those are what make my patches have a
> > higher diff. Can we just continue with my series instead?
>
> Of course you are free to send a new version of it, but it is unlikely
> to be a sufficient replacement for constructive feedback.

Ok, I'll point out the issues I see in this series and hopefully you
can point out the issues in my series and we can move forward with
mine if you agree with the additional issues my series is working
through.

Thanks,
Saravana
Rafael J. Wysocki March 10, 2025, 9:29 p.m. UTC | #8
On Mon, Mar 10, 2025 at 9:31 PM Saravana Kannan <saravanak@google.com> wrote:
>
> On Mon, Mar 10, 2025 at 9:01 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> >
> > On Sun, Mar 9, 2025 at 11:38 PM Saravana Kannan <saravanak@google.com> wrote:
> > >
> > > On Thu, Feb 27, 2025 at 8:23 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > > >
> > > > On Thu, Feb 27, 2025 at 4:45 PM Saravana Kannan <saravanak@google.com> wrote:
> > > > >
> > > > > On Tue, Feb 25, 2025 at 8:46 AM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > > > > >
> > > > > > Hi Everyone,
> > > > > >
> > > > > > Initially, this was an attempt to address the problems described by
> > > > > > Saravana related to spawning async work for any async device upfront
> > > > > > in the resume path:
> > > > > >
> > > > > > https://lore.kernel.org/linux-pm/20241114220921.2529905-1-saravanak@google.com/
> > > > > >
> > > > > > but then I realized that it could be extended to the suspend path and
> > > > > > used for speeding it up, which it really does.
> > > > >
> > > > > Btw, maybe I didn't  word it correctly, but my patch series was meant
> > > > > to speed up the non-async case too.
> > > >
> > > > If "the non-async case" means the case with "async" suspend/resume
> > > > disabled entirely, I don't think that the ordering in which devices
> > > > are processed can be changed just because there are no known
> > > > dependencies.
> > > >
> > > > > I was going to get around sending a v2 of my series, but was caught up
> > > > > with some other work. But I'm okay if you want to finish up my effort
> > > > > -- less work for me and I can focus on the other aspects of suspend :)
> > > > >
> > > > > Maybe add a Suggested-by: to the patches?
> > > >
> > > > Yeah, I can do that.
> > > >
> > > > > I definitely want to review the series, but very busy this week with
> > > > > some other work. I'll get to this next week for sure.
> > > >
> > > > That should be fine.
> > >
> > > Hi Rafael,
> > >
> > > I looked at the full series and it has at least one bug and a few gaps
> > > that I address in mine.
> >
> > What bug?
> >
> > You need to tell me specifically because I'm not aware of any bugs in
> > this series and unless you tell me what it is and I agree that it is a
> > bug, I have no reason to believe that there are any.
> >
> > As for the gaps, there are obvious differences between this patch
> > series and your work and it would be kind of nice to explain why they
> > matter in practice, in your view.
>
> Sure, I'll do this.

OK

> But it just felt like an inefficient way to get to close to where my series is.

I'm not sure where it is TBH.

> Instead of you just saying you don't like
> about my series and giving me some feedback on how to fix it.

You got feedback on it:

https://lore.kernel.org/linux-pm/CAJZ5v0grG7eSJ7_c73i9-bXaFhm5rfE2WmxtR6yLB-MGkd7sVg@mail.gmail.com/

And no response.

Also here:

https://lore.kernel.org/linux-pm/CAJZ5v0g9A1pZ5FjPAjdLY5ybNmefnBVVMJM7h3czW38p1fTfqQ@mail.gmail.com/

And there was a bunch of feedback from other people (and 0-day) on the
last patch.

> > > And those are what make my patches have a
> > > higher diff. Can we just continue with my series instead?
> >
> > Of course you are free to send a new version of it, but it is unlikely
> > to be a sufficient replacement for constructive feedback.
>
> Ok, I'll point out the issues I see in this series and hopefully you
> can point out the issues in my series and we can move forward with
> mine if you agree with the additional issues my series is working
> through.

Sure, but please address the feedback so far and send a new version.