| Message ID | 20220824044013.29354-1-qkrwngud825@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | PM: suspend: select SUSPEND_SKIP_SYNC too if PM_USERSPACE_AUTOSLEEP is selected | expand |
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? > help > Skip the kernel sys_sync() before freezing user processes. > Some systems prefer not to pay this cost on every invocation > --
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 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. > (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. I'm also wondering about a particular use case addressed by this change. Is there any?
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.
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 help Skip the kernel sys_sync() before freezing user processes. Some systems prefer not to pay this cost on every invocation
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(+)