PM: suspend: select SUSPEND_SKIP_SYNC too if PM_USERSPACE_AUTOSLEEP is selected

Commit 2fd77fff4b44 ("PM / suspend: make sync() on suspend-to-RAM build-time
optional") added an option to skip sync() on suspend entry to avoid heavy
overhead on platforms with frequent suspends.

Years later, commit 261e224d6a5c ("pm/sleep: Add PM_USERSPACE_AUTOSLEEP
Kconfig") added a dedicated config for indicating that the kernel is subject to
frequent suspends.

While SUSPEND_SKIP_SYNC is also available as a knob that the userspace can
configure, it makes sense to enable this by default if PM_USERSPACE_AUTOSLEEP
is selected already.

Signed-off-by: Juhyung Park <qkrwngud825@gmail.com>
---
 kernel/power/Kconfig | 1 +
 1 file changed, 1 insertion(+)

Hi Rafael,

On Wed, Aug 24, 2022 at 10:11 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Wed, Aug 24, 2022 at 6:41 AM Juhyung Park <qkrwngud825@gmail.com> wrote:
> >
> > Commit 2fd77fff4b44 ("PM / suspend: make sync() on suspend-to-RAM build-time
> > optional") added an option to skip sync() on suspend entry to avoid heavy
> > overhead on platforms with frequent suspends.
> >
> > Years later, commit 261e224d6a5c ("pm/sleep: Add PM_USERSPACE_AUTOSLEEP
> > Kconfig") added a dedicated config for indicating that the kernel is subject to
> > frequent suspends.
> >
> > While SUSPEND_SKIP_SYNC is also available as a knob that the userspace can
> > configure, it makes sense to enable this by default if PM_USERSPACE_AUTOSLEEP
> > is selected already.
> >
> > Signed-off-by: Juhyung Park <qkrwngud825@gmail.com>
> > ---
> >  kernel/power/Kconfig | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/kernel/power/Kconfig b/kernel/power/Kconfig
> > index 60a1d3051cc7..5725df6c573b 100644
> > --- a/kernel/power/Kconfig
> > +++ b/kernel/power/Kconfig
> > @@ -23,6 +23,7 @@ config SUSPEND_SKIP_SYNC
> >         bool "Skip kernel's sys_sync() on suspend to RAM/standby"
> >         depends on SUSPEND
> >         depends on EXPERT
> > +       default PM_USERSPACE_AUTOSLEEP
>
> Why is this better than selecting SUSPEND_SKIP_SYNC from PM_USERSPACE_AUTOSLEEP?

That won't allow developers to opt-out from SUSPEND_SKIP_SYNC when
they still want PM_USERSPACE_AUTOSLEEP. (Can't think of a valid reason
for this though, as PM_USERSPACE_AUTOSLEEP is only used by Android and
probably Chromium, afaik.)

I don't think SUSPEND_SKIP_SYNC is critical enough to enforce when
PM_USERSPACE_AUTOSLEEP is enabled, but I don't have a strong opinion
on this either.
(We could do `imply SUSPEND_SKIP_SYNC` from PM_USERSPACE_AUTOSLEEP,
but that doesn't look good semantically imho.)

If you want, I can send a v2 with 'PM_USERSPACE_AUTOSLEEP select
SUSPEND_SKIP_SYNC'.

Thanks.

>
> >         help
> >           Skip the kernel sys_sync() before freezing user processes.
> >           Some systems prefer not to pay this cost on every invocation
> > --

On Thu, Aug 25, 2022 at 12:01 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Wed, Aug 24, 2022 at 3:36 PM Juhyung Park <qkrwngud825@gmail.com> wrote:
> >
> > Hi Rafael,
> >
> > On Wed, Aug 24, 2022 at 10:11 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > >
> > > On Wed, Aug 24, 2022 at 6:41 AM Juhyung Park <qkrwngud825@gmail.com> wrote:
> > > >
> > > > Commit 2fd77fff4b44 ("PM / suspend: make sync() on suspend-to-RAM build-time
> > > > optional") added an option to skip sync() on suspend entry to avoid heavy
> > > > overhead on platforms with frequent suspends.
> > > >
> > > > Years later, commit 261e224d6a5c ("pm/sleep: Add PM_USERSPACE_AUTOSLEEP
> > > > Kconfig") added a dedicated config for indicating that the kernel is subject to
> > > > frequent suspends.
> > > >
> > > > While SUSPEND_SKIP_SYNC is also available as a knob that the userspace can
> > > > configure, it makes sense to enable this by default if PM_USERSPACE_AUTOSLEEP
> > > > is selected already.
> > > >
> > > > Signed-off-by: Juhyung Park <qkrwngud825@gmail.com>
> > > > ---
> > > >  kernel/power/Kconfig | 1 +
> > > >  1 file changed, 1 insertion(+)
> > > >
> > > > diff --git a/kernel/power/Kconfig b/kernel/power/Kconfig
> > > > index 60a1d3051cc7..5725df6c573b 100644
> > > > --- a/kernel/power/Kconfig
> > > > +++ b/kernel/power/Kconfig
> > > > @@ -23,6 +23,7 @@ config SUSPEND_SKIP_SYNC
> > > >         bool "Skip kernel's sys_sync() on suspend to RAM/standby"
> > > >         depends on SUSPEND
> > > >         depends on EXPERT
> > > > +       default PM_USERSPACE_AUTOSLEEP
> > >
> > > Why is this better than selecting SUSPEND_SKIP_SYNC from PM_USERSPACE_AUTOSLEEP?
> >
> > That won't allow developers to opt-out from SUSPEND_SKIP_SYNC when
> > they still want PM_USERSPACE_AUTOSLEEP.
>
> I see.
>
> It is not particularly clear, so at least please mention it in the changelog.

Will do.

>
>
> > (Can't think of a valid reason
> > for this though, as PM_USERSPACE_AUTOSLEEP is only used by Android and
> > probably Chromium, afaik.)
> >
> > I don't think SUSPEND_SKIP_SYNC is critical enough to enforce when
> > PM_USERSPACE_AUTOSLEEP is enabled, but I don't have a strong opinion
> > on this either.
> > (We could do `imply SUSPEND_SKIP_SYNC` from PM_USERSPACE_AUTOSLEEP,
> > but that doesn't look good semantically imho.)
> >
> > If you want, I can send a v2 with 'PM_USERSPACE_AUTOSLEEP select
> > SUSPEND_SKIP_SYNC'.
>
> Personally, I would use "select" and I would amend the
> PM_USERSPACE_AUTOSLEEP help text to say that it will disable sync on
> suspend by default explicitly.
>
> IMV otherwise it is more confusing than it needs to be.

Agreed.

>
> I'm also wondering about a particular use case addressed by this
> change.  Is there any?

I've personally manually enabled SUSPEND_SKIP_SYNC for all my Android
kernels for the past 7-8 years (even before SUSPEND_SKIP_SYNC was
implemented upstream) as it was quite apparent that the sync() path
during suspend was quite expensive, more so when the phone was under a
"suspend storm" which tries to enter suspend dozens of times per
second but was aborted/awakened due to devices with spurious
interrupts or suspend code path failures.

Do note that I do not represent any vendor at this moment.

Also, I think I'll have to mention that Google's Android kernel
(GKI/ACK) currently does not enable SUSPEND_SKIP_SYNC by default while
some OEMs do (can't think of which ones to be specific). So this
changes the default Android behavior, which is why I cc'ed Kalesh and
f2fs folks. I do not know if SUSPEND_SKIP_SYNC on Android was simply
overlooked or was consciously disabled. Android userspace also doesn't
change this via sysfs knob.

I still think this patch makes enough sense, but I'd love to hear
others' thoughts.

Thanks.