[PATCH v4 0/1] suspend: make sync() on suspend-to-RAM optional

[PATCH v4 0/1] suspend: make sync() on suspend-to-RAM optional

[Len Brown]

Based on discussion resulting from...
https://lkml.org/lkml/2015/5/8/82 [PATCH 1/1] suspend: delete sys_sync(),
this patch makes sys_sync() optional, rather than deleting it entirely.

This is an update to the original patch for this issue from Jan, 2014:
patch https://lkml.org/lkml/2014/1/23/73
[PATCH v3] suspend: make sync() on suspend-to-RAM optional

Aside from applying to Linux 4.1, rather than 3.13...
the change is that suspend always checks the flag from the
sysfs attribute before invoking sys_sync(); and the config option
just sets the default value.  Before the config option deleted all code.
Also, the config dependency is corrected, and some varialbes re-named.

[PATCH 1/1] suspend: make sync() on suspend-to-RAM optional

[Len Brown]

From: Len Brown <len.brown@intel.com>

The Linux kernel suspend path has traditionally invoked sys_sync().

But sys_sync() can be expensive, and some systems do not want
to pay the cost of sys_sync() on every suspend.

So make sys_sync on suspend optional.

Create sysfs attribute /sys/power/pm_suspend_do_sync.
When set to 1, the kernel will sys_sync() on suspend,
When set to 0, it will not.

This attribute can be changed by root at run-time.
Kernel build parameter CONFIG_PM_SUSPEND_DO_SYNC_DEFAULT.
As this is 1, by default, this patch does not change
default behavior.

Signed-off-by: Len Brown <len.brown@intel.com>
---
 Documentation/ABI/testing/sysfs-power | 10 ++++++++++
 kernel/power/Kconfig                  | 10 ++++++++++
 kernel/power/main.c                   | 31 +++++++++++++++++++++++++++++++
 kernel/power/power.h                  |  1 +
 kernel/power/suspend.c                | 12 +++++++-----
 5 files changed, 59 insertions(+), 5 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-power b/Documentation/ABI/testing/sysfs-power
index f455181..37df98d 100644
--- a/Documentation/ABI/testing/sysfs-power
+++ b/Documentation/ABI/testing/sysfs-power
@@ -256,3 +256,13 @@ Description:
 		Writing a "1" enables this printing while writing a "0"
 		disables it.  The default value is "0".  Reading from this file
 		will display the current value.
+
+What:		/sys/power/pm_suspend_do_sync
+Date:		July, 2015
+Contact:	Len Brown <len.brown@intel.com>
+Description:
+		The /sys/power/pm_suspend_do_sync file controls whether the kernel
+		will invoke sys_sync() on entry to the suspend to RAM path.
+		Yes if 1, no if 0.  The default is set by build option
+		CONFIG_PM_SUSPEND_DO_SYNC_DEFAULT.
+
diff --git a/kernel/power/Kconfig b/kernel/power/Kconfig
index 7e01f78..28976ed 100644
--- a/kernel/power/Kconfig
+++ b/kernel/power/Kconfig
@@ -103,6 +103,16 @@ config PM_SLEEP_SMP
 	depends on PM_SLEEP
 	select HOTPLUG_CPU
 
+config PM_SUSPEND_DO_SYNC_DEFAULT
+	int "Default value for /sys/power/suspend_do_sync"
+	range 0 1
+	default "1"
+	depends on SUSPEND
+	---help---
+	Set default value for /sys/power/suspend_sync,
+	which controls whether kernel invokes sys_sync() on suspend to RAM.
+	Value 1 will do the sys_sync(), 0 will not.
+
 config PM_AUTOSLEEP
 	bool "Opportunistic sleep"
 	depends on PM_SLEEP
diff --git a/kernel/power/main.c b/kernel/power/main.c
index 86e8157..a16d369 100644
--- a/kernel/power/main.c
+++ b/kernel/power/main.c
@@ -71,6 +71,34 @@ static ssize_t pm_async_store(struct kobject *kobj, struct kobj_attribute *attr,
 
 power_attr(pm_async);
 
+#ifdef CONFIG_SUSPEND
+/* Execute sys_sync() in suspend to RAM path */
+int pm_suspend_do_sync  = CONFIG_PM_SUSPEND_DO_SYNC_DEFAULT;
+
+static ssize_t pm_suspend_do_sync_show(struct kobject *kobj,
+			struct kobj_attribute *attr, char *buf)
+{
+	return sprintf(buf, "%d\n", pm_suspend_do_sync);
+}
+
+static ssize_t pm_suspend_do_sync_store(struct kobject *kobj,
+			struct kobj_attribute *attr, const char *buf, size_t n)
+{
+	unsigned long val;
+
+	if (kstrtoul(buf, 10, &val))
+		return -EINVAL;
+
+	if (val > 1)
+		return -EINVAL;
+
+	pm_suspend_do_sync  = val;
+	return n;
+}
+
+power_attr(pm_suspend_do_sync);
+#endif
+
 #ifdef CONFIG_PM_DEBUG
 int pm_test_level = TEST_NONE;
 
@@ -592,6 +620,9 @@ static struct attribute * g[] = {
 #ifdef CONFIG_PM_SLEEP
 	&pm_async_attr.attr,
 	&wakeup_count_attr.attr,
+#ifdef CONFIG_SUSPEND
+	&pm_suspend_do_sync_attr.attr,
+#endif
 #ifdef CONFIG_PM_AUTOSLEEP
 	&autosleep_attr.attr,
 #endif
diff --git a/kernel/power/power.h b/kernel/power/power.h
index ce9b832..a6120b9 100644
--- a/kernel/power/power.h
+++ b/kernel/power/power.h
@@ -201,6 +201,7 @@ static inline void suspend_test_finish(const char *label) {}
 #ifdef CONFIG_PM_SLEEP
 /* kernel/power/main.c */
 extern int pm_notifier_call_chain(unsigned long val);
+extern int pm_suspend_do_sync;
 #endif
 
 #ifdef CONFIG_HIGHMEM
diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
index 8d7a1ef..aab5dca 100644
--- a/kernel/power/suspend.c
+++ b/kernel/power/suspend.c
@@ -482,11 +482,13 @@ static int enter_state(suspend_state_t state)
 	if (state == PM_SUSPEND_FREEZE)
 		freeze_begin();
 
-	trace_suspend_resume(TPS("sync_filesystems"), 0, true);
-	printk(KERN_INFO "PM: Syncing filesystems ... ");
-	sys_sync();
-	printk("done.\n");
-	trace_suspend_resume(TPS("sync_filesystems"), 0, false);
+	if (pm_suspend_do_sync) {
+		trace_suspend_resume(TPS("sync_filesystems"), 0, true);
+		printk(KERN_INFO "PM: Syncing filesystems ... ");
+		sys_sync();
+		printk("done.\n");
+		trace_suspend_resume(TPS("sync_filesystems"), 0, false);
+	}
 
 	pr_debug("PM: Preparing system for %s sleep\n", pm_states[state]);
 	error = suspend_prepare(state);
-- 
2.4.1.314.g9532ead

Re: [PATCH 1/1] suspend: make sync() on suspend-to-RAM optional

[Pavel Machek]

On Tue 2015-07-14 22:24:51, Len Brown wrote:
> From: Len Brown <len.brown@intel.com>
> 
> The Linux kernel suspend path has traditionally invoked sys_sync().
> 
> But sys_sync() can be expensive, and some systems do not want
> to pay the cost of sys_sync() on every suspend.

Have you measured how expesive it can be, and why it is expensive?

> So make sys_sync on suspend optional.
> 
> Create sysfs attribute /sys/power/pm_suspend_do_sync.
> When set to 1, the kernel will sys_sync() on suspend,
> When set to 0, it will not.
> 
> This attribute can be changed by root at run-time.
> Kernel build parameter CONFIG_PM_SUSPEND_DO_SYNC_DEFAULT.
> As this is 1, by default, this patch does not change
> default behavior.

Why do you need CONFIG parameter?


									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 1/1] suspend: make sync() on suspend-to-RAM optional

[Austin S Hemmelgarn]

[-- Attachment #1: Type: text/plain, Size: 735 bytes --]

On 2015-07-15 02:43, Pavel Machek wrote:
> On Tue 2015-07-14 22:24:51, Len Brown wrote:
>> From: Len Brown <len.brown@intel.com>
>>
>> The Linux kernel suspend path has traditionally invoked sys_sync().
>>
>> But sys_sync() can be expensive, and some systems do not want
>> to pay the cost of sys_sync() on every suspend.
>
> Have you measured how expesive it can be, and why it is expensive?
How expensive it is can vary widely, but it pretty much boils down to 
how much dirty data still needs written out, and how slow the storage it 
needs written to is.  There's not really much that can be done in the 
kernel to change this, and most userspace suspend systems call sync 
themselves during the suspend sequence.


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 2967 bytes --]

RE: [PATCH 1/1] suspend: make sync() on suspend-to-RAM optional

[Brown, Len]

> -----Original Message-----
> From: Austin S Hemmelgarn [mailto:ahferroin7@gmail.com]
> Sent: Wednesday, July 15, 2015 10:07 AM
> To: Pavel Machek; Len Brown
> Cc: rjw@rjwysocki.net; linux-pm@vger.kernel.org; linux-
> kernel@vger.kernel.org; Brown, Len
> Subject: Re: [PATCH 1/1] suspend: make sync() on suspend-to-RAM optional
> 
> On 2015-07-15 02:43, Pavel Machek wrote:
> > On Tue 2015-07-14 22:24:51, Len Brown wrote:
> >> From: Len Brown <len.brown@intel.com>
> >>
> >> The Linux kernel suspend path has traditionally invoked sys_sync().
> >>
> >> But sys_sync() can be expensive, and some systems do not want
> >> to pay the cost of sys_sync() on every suspend.
> >
> > Have you measured how expensive it can be, and why it is expensive?

> How expensive it is can vary widely, but it pretty much boils down to
> how much dirty data still needs written out, and how slow the storage it
> needs written to is.  There's not really much that can be done in the
> kernel to change this, and most userspace suspend systems call sync
> themselves during the suspend sequence.

Right.
And now, user-space gets is no longer forced to incur that
delay on every suspend if they do not want it.

Yes, have measured this under many conditions.
The bottom line is that sys_sync() is rarely as fast as 1ms,
and is sometimes as slow as hundreds of ms.

>> Why do you need CONFIG parameter?

So that an OS that doesn't want to change their user-space,
can build a kernel that does what they want by default.

Originally I had the config parameter remove this code entirely,
which would achieve the same goal.
But Rafael prefers the sysfs attribute always exist
and the config simply set the default.

thanks,
-Len

Re: [PATCH 1/1] suspend: make sync() on suspend-to-RAM optional

[Rafael J. Wysocki]

On Wednesday, July 15, 2015 02:58:22 PM Brown, Len wrote:
> 
> > -----Original Message-----
> > From: Austin S Hemmelgarn [mailto:ahferroin7@gmail.com]
> > Sent: Wednesday, July 15, 2015 10:07 AM
> > To: Pavel Machek; Len Brown
> > Cc: rjw@rjwysocki.net; linux-pm@vger.kernel.org; linux-
> > kernel@vger.kernel.org; Brown, Len
> > Subject: Re: [PATCH 1/1] suspend: make sync() on suspend-to-RAM optional
> > 
> > On 2015-07-15 02:43, Pavel Machek wrote:
> > > On Tue 2015-07-14 22:24:51, Len Brown wrote:
> > >> From: Len Brown <len.brown@intel.com>
> > >>
> > >> The Linux kernel suspend path has traditionally invoked sys_sync().
> > >>
> > >> But sys_sync() can be expensive, and some systems do not want
> > >> to pay the cost of sys_sync() on every suspend.
> > >
> > > Have you measured how expensive it can be, and why it is expensive?
> 
> > How expensive it is can vary widely, but it pretty much boils down to
> > how much dirty data still needs written out, and how slow the storage it
> > needs written to is.  There's not really much that can be done in the
> > kernel to change this, and most userspace suspend systems call sync
> > themselves during the suspend sequence.
> 
> Right.
> And now, user-space gets is no longer forced to incur that
> delay on every suspend if they do not want it.
> 
> Yes, have measured this under many conditions.
> The bottom line is that sys_sync() is rarely as fast as 1ms,
> and is sometimes as slow as hundreds of ms.
> 
> >> Why do you need CONFIG parameter?
> 
> So that an OS that doesn't want to change their user-space,
> can build a kernel that does what they want by default.
> 
> Originally I had the config parameter remove this code entirely,
> which would achieve the same goal.
> But Rafael prefers the sysfs attribute always exist
> and the config simply set the default.

Indeed.

And so I'm queuing this patch up for 4.3 (with a couple of minor fixups).

Thanks,
Rafael

Re: [PATCH 1/1] suspend: make sync() on suspend-to-RAM optional

[Pavel Machek]

On Sat 2015-07-18 01:54:09, Rafael J. Wysocki wrote:
> On Wednesday, July 15, 2015 02:58:22 PM Brown, Len wrote:
> > 
> > > -----Original Message-----
> > > From: Austin S Hemmelgarn [mailto:ahferroin7@gmail.com]
> > > Sent: Wednesday, July 15, 2015 10:07 AM
> > > To: Pavel Machek; Len Brown
> > > Cc: rjw@rjwysocki.net; linux-pm@vger.kernel.org; linux-
> > > kernel@vger.kernel.org; Brown, Len
> > > Subject: Re: [PATCH 1/1] suspend: make sync() on suspend-to-RAM optional
> > > 
> > > On 2015-07-15 02:43, Pavel Machek wrote:
> > > > On Tue 2015-07-14 22:24:51, Len Brown wrote:
> > > >> From: Len Brown <len.brown@intel.com>
> > > >>
> > > >> The Linux kernel suspend path has traditionally invoked sys_sync().
> > > >>
> > > >> But sys_sync() can be expensive, and some systems do not want
> > > >> to pay the cost of sys_sync() on every suspend.
> > > >
> > > > Have you measured how expensive it can be, and why it is expensive?
> > 
> > > How expensive it is can vary widely, but it pretty much boils down to
> > > how much dirty data still needs written out, and how slow the storage it
> > > needs written to is.  There's not really much that can be done in the
> > > kernel to change this, and most userspace suspend systems call sync
> > > themselves during the suspend sequence.
> > 
> > Right.
> > And now, user-space gets is no longer forced to incur that
> > delay on every suspend if they do not want it.
> > 
> > Yes, have measured this under many conditions.
> > The bottom line is that sys_sync() is rarely as fast as 1ms,
> > and is sometimes as slow as hundreds of ms.

..and failed to figure out what is going on there. Yes, sync needs to
be slow if there's a lot of data to write. But if you suspend
frequently, there should not be a lot of data to write. (And you
should not be suspending with ton of dirty data in the first place).

> > >> Why do you need CONFIG parameter?
> > 
> > So that an OS that doesn't want to change their user-space,
> > can build a kernel that does what they want by default.
> > 
> > Originally I had the config parameter remove this code entirely,
> > which would achieve the same goal.
> > But Rafael prefers the sysfs attribute always exist
> > and the config simply set the default.
> 
> Indeed.
> 
> And so I'm queuing this patch up for 4.3 (with a couple of minor fixups).

Please don't.

"OS that doesn't want to change the user-space to speed up suspend by
few milliseconds" is not a valid reason for asking about million users
one more config question. Affected users can't run mainline kernel
anyway, and will have to change their userland in non-trivial ways to
get there.

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 1/1] suspend: make sync() on suspend-to-RAM optional

[Rafael J. Wysocki]

Hi Pavel,

On Tue, Jul 21, 2015 at 11:38 AM, Pavel Machek <pavel@ucw.cz> wrote:
> On Sat 2015-07-18 01:54:09, Rafael J. Wysocki wrote:
>> On Wednesday, July 15, 2015 02:58:22 PM Brown, Len wrote:

[cut]

>> > >> Why do you need CONFIG parameter?
>> >
>> > So that an OS that doesn't want to change their user-space,
>> > can build a kernel that does what they want by default.
>> >
>> > Originally I had the config parameter remove this code entirely,
>> > which would achieve the same goal.
>> > But Rafael prefers the sysfs attribute always exist
>> > and the config simply set the default.
>>
>> Indeed.
>>
>> And so I'm queuing this patch up for 4.3 (with a couple of minor fixups).
>
> Please don't.
>
> "OS that doesn't want to change the user-space to speed up suspend by
> few milliseconds" is not a valid reason for asking about million users
> one more config question.

That's your opinion and I beg to differ.

> Affected users can't run mainline kernel
> anyway, and will have to change their userland in non-trivial ways to
> get there.

And I'm not sure what you're talking about here.  Who are the
"affected users" in particular?

Rafael

Re: [PATCH 1/1] suspend: make sync() on suspend-to-RAM optional

[Pavel Machek]

On Tue 2015-07-21 16:41:12, Rafael J. Wysocki wrote:
> Hi Pavel,
> 
> On Tue, Jul 21, 2015 at 11:38 AM, Pavel Machek <pavel@ucw.cz> wrote:
> > On Sat 2015-07-18 01:54:09, Rafael J. Wysocki wrote:
> >> On Wednesday, July 15, 2015 02:58:22 PM Brown, Len wrote:
> 
> [cut]
> 
> >> > >> Why do you need CONFIG parameter?
> >> >
> >> > So that an OS that doesn't want to change their user-space,
> >> > can build a kernel that does what they want by default.
> >> >
> >> > Originally I had the config parameter remove this code entirely,
> >> > which would achieve the same goal.
> >> > But Rafael prefers the sysfs attribute always exist
> >> > and the config simply set the default.
> >>
> >> Indeed.
> >>
> >> And so I'm queuing this patch up for 4.3 (with a couple of minor fixups).
> >
> > Please don't.
> >
> > "OS that doesn't want to change the user-space to speed up suspend by
> > few milliseconds" is not a valid reason for asking about million users
> > one more config question.
> 
> That's your opinion and I beg to differ.

Perhaps explaining your opinion would help here? Having to echo value
to file to improve performance over reliability does not look too
burdensome on the users.

> > Affected users can't run mainline kernel
> > anyway, and will have to change their userland in non-trivial ways to
> > get there.
> 
> And I'm not sure what you're talking about here.  Who are the
> "affected users" in particular?

Who does enter suspend to ram multiple times a second? Only android,
AFAICT. Can you run android on mainline kernel? No. Can you run
android on kernel with less that 100k lines of patches? No.

So who benefits from the new config option? No one.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 1/1] suspend: make sync() on suspend-to-RAM optional

[Austin S Hemmelgarn]

[-- Attachment #1: Type: text/plain, Size: 2737 bytes --]

On 2015-07-21 11:19, Pavel Machek wrote:
> On Tue 2015-07-21 16:41:12, Rafael J. Wysocki wrote:
>> Hi Pavel,
>>
>> On Tue, Jul 21, 2015 at 11:38 AM, Pavel Machek <pavel@ucw.cz> wrote:
>>> On Sat 2015-07-18 01:54:09, Rafael J. Wysocki wrote:
>>>> On Wednesday, July 15, 2015 02:58:22 PM Brown, Len wrote:
>>
>> [cut]
>>
>>>>>>> Why do you need CONFIG parameter?
>>>>>
>>>>> So that an OS that doesn't want to change their user-space,
>>>>> can build a kernel that does what they want by default.
>>>>>
>>>>> Originally I had the config parameter remove this code entirely,
>>>>> which would achieve the same goal.
>>>>> But Rafael prefers the sysfs attribute always exist
>>>>> and the config simply set the default.
>>>>
>>>> Indeed.
>>>>
>>>> And so I'm queuing this patch up for 4.3 (with a couple of minor fixups).
>>>
>>> Please don't.
>>>
>>> "OS that doesn't want to change the user-space to speed up suspend by
>>> few milliseconds" is not a valid reason for asking about million users
>>> one more config question.
>>
>> That's your opinion and I beg to differ.
>
> Perhaps explaining your opinion would help here? Having to echo value
> to file to improve performance over reliability does not look too
> burdensome on the users.
>
>>> Affected users can't run mainline kernel
>>> anyway, and will have to change their userland in non-trivial ways to
>>> get there.
>>
>> And I'm not sure what you're talking about here.  Who are the
>> "affected users" in particular?
>
> Who does enter suspend to ram multiple times a second? Only android,
> AFAICT. Can you run android on mainline kernel? No. Can you run
> android on kernel with less that 100k lines of patches? No.
>
> So who benefits from the new config option? No one.
You mean aside from the fact that calling sync multiple times during the 
suspend path is wasteful and (potentially) puts unnecessary stress on 
the hardware.  All userspace that I know of calls sync() at least once 
(and sometimes twice) before telling the kernel to enter suspend.  I 
will accede that for some people, having sys_sync() get called by the 
kernel during suspend is useful (for example, I don't use any special 
userspace software for STR on my laptop, because it works fine without 
it, I just 'echo mem > /sys/power/state').

Also, I'd be willing to bet that FirefoxOS, and whatever Amazon is using 
on their tablets and phones also suspend multiple times a second, and 
some ChromeOS systems might in the near future as well (and ChromeOS is 
pretty darn close to being able to run on mainline).

I would personally say this should be default y and presenting it as a 
choice should require CONFIG_EXPERT to be y.



[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 3019 bytes --]

Re: [PATCH 1/1] suspend: make sync() on suspend-to-RAM optional

[Pavel Machek]

Hi!

> >>And I'm not sure what you're talking about here.  Who are the
> >>"affected users" in particular?
> >
> >Who does enter suspend to ram multiple times a second? Only android,
> >AFAICT. Can you run android on mainline kernel? No. Can you run
> >android on kernel with less that 100k lines of patches? No.
> >
> >So who benefits from the new config option? No one.
> You mean aside from the fact that calling sync multiple times during the
> suspend path is wasteful and (potentially) puts unnecessary stress on the
> hardware.  All userspace that I know of calls sync() at least once (and
> sometimes twice) before telling the kernel to enter suspend.  I will
> >>accede

Fix the userland, if that problem bothers you, then.

sync() in userland is racy, sync() after freezer is not supposed to
be.

If you suspend multiple times a second, surely removing extra sync()
calls from your userland is not that much to ask? And actually... do
you know someone who suspends that often an has extra sync? I bet not.

> Also, I'd be willing to bet that FirefoxOS, and whatever Amazon is using on
> their tablets and phones also suspend multiple times a second, and some
> ChromeOS systems might in the near future as well (and ChromeOS is pretty
> darn close to being able to run on mainline).

So can someone from FirefoxOS, Amazon, or ChromeOS present their
requirements?

Because Maemo is in the same league, but got their interfaces right,
and uses low-power CPU modes and runtime power saving to get very nice
results.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

RE: [PATCH 1/1] suspend: make sync() on suspend-to-RAM optional

[Brown, Len]

> Who does enter suspend to ram multiple times a second? Only android

One significant customer for fast suspend/resume
is sufficient to justify Linux supporting fast suspend/resume.

> Can you run android on mainline kernel? No

This is something we need to work together to help fix.

thanks,
-Len

Re: [PATCH 1/1] suspend: make sync() on suspend-to-RAM optional

[Pavel Machek]

On Tue 2015-07-21 20:01:15, Brown, Len wrote:
> > Who does enter suspend to ram multiple times a second? Only android
> 
> One significant customer for fast suspend/resume
> is sufficient to justify Linux supporting fast suspend/resume.

So which customer is that? And does that customer really need config
option, as opposed to writing to sysfs file?

> > Can you run android on mainline kernel? No
> 
> This is something we need to work together to help fix.

Yes, that would be nice, but I'm pretty sure it will require changes
in android userland, as they did not get all the interfaces right.

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 1/1] suspend: make sync() on suspend-to-RAM optional

[Rafael J. Wysocki]

On Tuesday, July 21, 2015 05:19:41 PM Pavel Machek wrote:
> On Tue 2015-07-21 16:41:12, Rafael J. Wysocki wrote:
> > Hi Pavel,
> > 
> > On Tue, Jul 21, 2015 at 11:38 AM, Pavel Machek <pavel@ucw.cz> wrote:
> > > On Sat 2015-07-18 01:54:09, Rafael J. Wysocki wrote:
> > >> On Wednesday, July 15, 2015 02:58:22 PM Brown, Len wrote:
> > 
> > [cut]
> > 
> > >> > >> Why do you need CONFIG parameter?
> > >> >
> > >> > So that an OS that doesn't want to change their user-space,
> > >> > can build a kernel that does what they want by default.
> > >> >
> > >> > Originally I had the config parameter remove this code entirely,
> > >> > which would achieve the same goal.
> > >> > But Rafael prefers the sysfs attribute always exist
> > >> > and the config simply set the default.
> > >>
> > >> Indeed.
> > >>
> > >> And so I'm queuing this patch up for 4.3 (with a couple of minor fixups).
> > >
> > > Please don't.
> > >
> > > "OS that doesn't want to change the user-space to speed up suspend by
> > > few milliseconds" is not a valid reason for asking about million users
> > > one more config question.
> > 
> > That's your opinion and I beg to differ.
> 
> Perhaps explaining your opinion would help here? Having to echo value
> to file to improve performance over reliability does not look too
> burdensome on the users.

That's for the people for whom changing the kernel is easier than messing
up with user space.

Also if your user space does the sync before suspending, it's better to
make "no kernel sync" the default, because that saves you some overhead
and energy too (either on the extra redundant sync on every suspend or
on the write to the sysfs attribute on every boot).

> 
> > > Affected users can't run mainline kernel
> > > anyway, and will have to change their userland in non-trivial ways to
> > > get there.
> > 
> > And I'm not sure what you're talking about here.  Who are the
> > "affected users" in particular?
> 
> Who does enter suspend to ram multiple times a second? Only android,
> AFAICT. Can you run android on mainline kernel? No. Can you run
> android on kernel with less that 100k lines of patches? No.
> 
> So who benefits from the new config option? No one.

I am, for one.  None of the systems I use actually needs the sync in the
kernel.  I bet there are more people like me, because I have a stock
distro installed on my systems.

And it is more pain for me to change the user space on each of them to
write to the new sysfs file on every boot than to set a kernel Kconfig
option once.

Thanks,
Rafael

Re: [PATCH 1/1] suspend: make sync() on suspend-to-RAM optional

[Pavel Machek]

On Wed 2015-07-22 03:25:41, Rafael J. Wysocki wrote:
> On Tuesday, July 21, 2015 05:19:41 PM Pavel Machek wrote:
> > On Tue 2015-07-21 16:41:12, Rafael J. Wysocki wrote:
> > > Hi Pavel,
> > > 
> > > On Tue, Jul 21, 2015 at 11:38 AM, Pavel Machek <pavel@ucw.cz> wrote:
> > > > On Sat 2015-07-18 01:54:09, Rafael J. Wysocki wrote:
> > > >> On Wednesday, July 15, 2015 02:58:22 PM Brown, Len wrote:
> > > 
> > > [cut]
> > > 
> > > >> > >> Why do you need CONFIG parameter?
> > > >> >
> > > >> > So that an OS that doesn't want to change their user-space,
> > > >> > can build a kernel that does what they want by default.
> > > >> >
> > > >> > Originally I had the config parameter remove this code entirely,
> > > >> > which would achieve the same goal.
> > > >> > But Rafael prefers the sysfs attribute always exist
> > > >> > and the config simply set the default.
> > > >>
> > > >> Indeed.
> > > >>
> > > >> And so I'm queuing this patch up for 4.3 (with a couple of minor fixups).
> > > >
> > > > Please don't.
> > > >
> > > > "OS that doesn't want to change the user-space to speed up suspend by
> > > > few milliseconds" is not a valid reason for asking about million users
> > > > one more config question.
> > > 
> > > That's your opinion and I beg to differ.
> > 
> > Perhaps explaining your opinion would help here? Having to echo value
> > to file to improve performance over reliability does not look too
> > burdensome on the users.
> 
> That's for the people for whom changing the kernel is easier than messing
> up with user space.

You are maintainer of s2ram package, IIRC. If you don't like extra
syncs, you should remove them from the user part (where they are racy,
unlike in kernel).

Unfortunately, it will be harder to do that if CONFIG_ option is
introduced, because now s2ram will need extra work to determine if
kernel will sync or not.

> Also if your user space does the sync before suspending, it's better to
> make "no kernel sync" the default, because that saves you some overhead
> and energy too (either on the extra redundant sync on every suspend or
> on the write to the sysfs attribute on every boot).

So you want to "save the overhead of writing to sysfs file on boot" (30usec?)
but are happy to "add overhead of extra config question" (5 seconds
_user_ has to decide what is going on)...? What about overhead of
reading longer config file on each kernel compile?

> > > > Affected users can't run mainline kernel
> > > > anyway, and will have to change their userland in non-trivial ways to
> > > > get there.
> > > 
> > > And I'm not sure what you're talking about here.  Who are the
> > > "affected users" in particular?
> > 
> > Who does enter suspend to ram multiple times a second? Only android,
> > AFAICT. Can you run android on mainline kernel? No. Can you run
> > android on kernel with less that 100k lines of patches? No.
> > 
> > So who benefits from the new config option? No one.
> 
> I am, for one.  None of the systems I use actually needs the sync in the
> kernel.  I bet there are more people like me, because I have a stock
> distro installed on my systems.

If the bug is in distro (extra syncs) it should be solved in distro
(remove the extra syncs, keep the in-kernel one that works). CONFIG_
option actually makes that harder.
									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 1/1] suspend: make sync() on suspend-to-RAM optional

[Oliver Neukum]

On Wed, 2015-07-22 at 03:25 +0200, Rafael J. Wysocki wrote:
> And it is more pain for me to change the user space on each of them to
> write to the new sysfs file on every boot than to set a kernel Kconfig
> option once.

So why at all? If you really need this in sysfs, why not write
something like "memfast" into /sys/power/state ?

	Regards
		Oliver

Re: [PATCH 1/1] suspend: make sync() on suspend-to-RAM optional

[Len Brown]

On Wed, Jul 22, 2015 at 4:55 AM, Oliver Neukum <oneukum@suse.com> wrote:
> On Wed, 2015-07-22 at 03:25 +0200, Rafael J. Wysocki wrote:
>> And it is more pain for me to change the user space on each of them to
>> write to the new sysfs file on every boot than to set a kernel Kconfig
>> option once.
>
> So why at all? If you really need this in sysfs, why not write
> something like "memfast" into /sys/power/state ?

We fought this battle, and lost.

When we came out with "freeze", which is faster than "mem",
no user-space changed to take advantage of it.
"mem" is what they use on all platforms, and they simply want it to be fast.

I don't like the run-time sysfs attribute in this patch.
There are only 4 use-cases, and we can handle the 3 that matter
without a sysfs attribute:

1. OS wants sync, run-time never changes mind
    compile kernel with sync
    this is default, and what everybody is accustomed to.

2. OS does not want sync, run-time never changes mind
    compile kernel without sync
    This gives OS' that care about suspend/resume latency what they want.
    I'm fine with Austin's suggestion "depends on EXPERT"

3. OS  wants sync, run-time wants to opt-OUT
    Sorry, we'll not support his case.
    If your run a distro kernel that builds in the sync, you are stuck with it.
    If you care, then build a kernel from scratch, or run a different distro.

4. OS does not want sync, run-time sometimes wants to opt-IN
    As it turns out, Linux kernel has always invoked sync before freezing
    user-threads.  Sorry, invoking sync in kernel suspend path does not close
    a race that isn't also currently present by invoking sync from user-space.

   (yes, as discussed, the proper long term fix involves notifying file systems)

    As this user will not have a sysfs attribute to tweak to tell the
kernel to sync,
    they can simply invoke sync from user-space.

thanks,
Len Brown, Intel Open Source Technology Center

Re: [PATCH 1/1] suspend: make sync() on suspend-to-RAM optional

[Rafael J. Wysocki]

On Friday, July 31, 2015 12:02:36 PM Len Brown wrote:
> On Wed, Jul 22, 2015 at 4:55 AM, Oliver Neukum <oneukum@suse.com> wrote:
> > On Wed, 2015-07-22 at 03:25 +0200, Rafael J. Wysocki wrote:
> >> And it is more pain for me to change the user space on each of them to
> >> write to the new sysfs file on every boot than to set a kernel Kconfig
> >> option once.
> >
> > So why at all? If you really need this in sysfs, why not write
> > something like "memfast" into /sys/power/state ?
> 
> We fought this battle, and lost.
> 
> When we came out with "freeze", which is faster than "mem",
> no user-space changed to take advantage of it.

I do think that Chrome is going to use "freeze", so maybe it's not a lost
battle after all?

The problem with "memfast" and similar things is we'd also need "freezefast"
and "standbyfast" then, for consistency if nothing else, which makes a little
sense to me.

BTW, it should be noted that the whole "sync in the kernel is better, because
it doesn't race with user space writing to disks" argument was completely
bogus and useless, because in fact the sync in the kernel is done before
freezing user space and which means that it is susceptible to the very same
race condition as the sync from user space.

So if your user space does the sync before suspending, the next one in the
kernel is completely useless.

Thanks,
Rafael

Re: [PATCH 1/1] suspend: make sync() on suspend-to-RAM optional

[Pavel Machek]

On Sat 2015-08-01 01:56:19, Rafael J. Wysocki wrote:
> On Friday, July 31, 2015 12:02:36 PM Len Brown wrote:
> > On Wed, Jul 22, 2015 at 4:55 AM, Oliver Neukum <oneukum@suse.com> wrote:
> > > On Wed, 2015-07-22 at 03:25 +0200, Rafael J. Wysocki wrote:
> > >> And it is more pain for me to change the user space on each of them to
> > >> write to the new sysfs file on every boot than to set a kernel Kconfig
> > >> option once.
> > >
> > > So why at all? If you really need this in sysfs, why not write
> > > something like "memfast" into /sys/power/state ?
> > 
> > We fought this battle, and lost.
> > 
> > When we came out with "freeze", which is faster than "mem",
> > no user-space changed to take advantage of it.
> 
> I do think that Chrome is going to use "freeze", so maybe it's not a lost
> battle after all?
> 
> The problem with "memfast" and similar things is we'd also need "freezefast"
> and "standbyfast" then, for consistency if nothing else, which makes a little
> sense to me.
> 
> BTW, it should be noted that the whole "sync in the kernel is better, because
> it doesn't race with user space writing to disks" argument was completely
> bogus and useless, because in fact the sync in the kernel is done before
> freezing user space and which means that it is susceptible to the very same
> race condition as the sync from user space.

That seems like a bug to me... when did that start happening? I'm pretty sure
it was originally done after freeze...

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html