[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

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

[Len Brown]

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

Linux suspend-to-RAM was unreliable when first developed,
and so sys_sync() was invoked inside the kernel at the
start of every suspend flow.

Today, many devices are invoking suspend with
high reliability and high frequency, and they don't
want to be forced to pay for sync on every suspend.

So here we make it optional.
De-select CONFIG_PM_SLEEP_SYNC to delete the call entirely.
or keep CONFIG_PM_SLEEP and you can now clear the
sys/power/sleep_sync attribte to disable the sync
from user-space.

As we have had this call for a long time,
the default remains to keep the call, and to
invoke it on every suspend.

Signed-off-by: Len Brown <len.brown@intel.com>
---
 kernel/power/Kconfig   |  8 ++++++++
 kernel/power/main.c    | 33 +++++++++++++++++++++++++++++++++
 kernel/power/power.h   |  1 +
 kernel/power/suspend.c | 10 +++++++---
 4 files changed, 49 insertions(+), 3 deletions(-)

diff --git a/kernel/power/Kconfig b/kernel/power/Kconfig
index 2fac9cc..d2927ce 100644
--- a/kernel/power/Kconfig
+++ b/kernel/power/Kconfig
@@ -102,6 +102,14 @@ config PM_SLEEP_SMP
 	depends on PM_SLEEP
 	select HOTPLUG_CPU
 
+config PM_SLEEP_SYNC
+	bool "Suspend to RAM starts with in-kernel call to sync()"
+	default y
+	depends on PM_SLEEP
+	---help---
+	Build a call to sync() into the Linux kernel suspend to RAM flow.
+	The sync can still be skipped by clearing /sys/power/sleep_sync.
+
 config PM_AUTOSLEEP
 	bool "Opportunistic sleep"
 	depends on PM_SLEEP
diff --git a/kernel/power/main.c b/kernel/power/main.c
index 1d1bf63..1102871 100644
--- a/kernel/power/main.c
+++ b/kernel/power/main.c
@@ -71,6 +71,36 @@ static ssize_t pm_async_store(struct kobject *kobj, struct kobj_attribute *attr,
 
 power_attr(pm_async);
 
+#ifdef CONFIG_PM_SLEEP_SYNC
+
+/* If set, sync file systems at start of suspend flow */
+int pm_sleep_sync_enabled = 1;
+
+static ssize_t pm_sleep_sync_show(struct kobject *kobj, struct kobj_attribute *attr,
+			     char *buf)
+{
+	return sprintf(buf, "%d\n", pm_sleep_sync_enabled);
+}
+
+static ssize_t pm_sleep_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_sleep_sync_enabled = val;
+	return n;
+}
+
+power_attr(pm_sleep_sync);
+
+#endif /* CONFIG_PM_SLEEP_SYNC */
+
 #ifdef CONFIG_PM_DEBUG
 int pm_test_level = TEST_NONE;
 
@@ -592,6 +622,9 @@ static struct attribute * g[] = {
 #ifdef CONFIG_PM_SLEEP
 	&pm_async_attr.attr,
 	&wakeup_count_attr.attr,
+#ifdef CONFIG_PM_SLEEP_SYNC
+	&pm_sleep_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 7d4b7ff..dca60a7 100644
--- a/kernel/power/power.h
+++ b/kernel/power/power.h
@@ -200,6 +200,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_sleep_sync_enabled;
 #endif
 
 #ifdef CONFIG_HIGHMEM
diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
index 62ee437..2fb2f51 100644
--- a/kernel/power/suspend.c
+++ b/kernel/power/suspend.c
@@ -333,9 +333,13 @@ static int enter_state(suspend_state_t state)
 	if (state == PM_SUSPEND_FREEZE)
 		freeze_begin();
 
-	printk(KERN_INFO "PM: Syncing filesystems ... ");
-	sys_sync();
-	printk("done.\n");
+#ifdef CONFIG_PM_SLEEP_SYNC
+	if (pm_sleep_sync_enabled) {
+		printk(KERN_INFO "PM: Syncing filesystems ... ");
+		sys_sync();
+		printk("done.\n");
+	}
+#endif /* CONFIG_PM_SLEEP_SYNC */
 
 	pr_debug("PM: Preparing system for %s sleep\n", pm_states[state]);
 	error = suspend_prepare(state);
-- 
1.8.3.2

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

[Zhang Rui]

On Thu, 2014-01-23 at 02:11 -0500, Len Brown wrote:
> From: Len Brown <len.brown@intel.com>
> 
> Linux suspend-to-RAM was unreliable when first developed,
> and so sys_sync() was invoked inside the kernel at the
> start of every suspend flow.
> 
> Today, many devices are invoking suspend with
> high reliability and high frequency, and they don't
> want to be forced to pay for sync on every suspend.
> 
> So here we make it optional.
> De-select CONFIG_PM_SLEEP_SYNC to delete the call entirely.
> or keep CONFIG_PM_SLEEP and you can now clear the
> sys/power/sleep_sync attribte to disable the sync
> from user-space.
> 
> As we have had this call for a long time,
> the default remains to keep the call, and to
> invoke it on every suspend.
> 
> Signed-off-by: Len Brown <len.brown@intel.com>
> ---
>  kernel/power/Kconfig   |  8 ++++++++
>  kernel/power/main.c    | 33 +++++++++++++++++++++++++++++++++
>  kernel/power/power.h   |  1 +
>  kernel/power/suspend.c | 10 +++++++---
>  4 files changed, 49 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/power/Kconfig b/kernel/power/Kconfig
> index 2fac9cc..d2927ce 100644
> --- a/kernel/power/Kconfig
> +++ b/kernel/power/Kconfig
> @@ -102,6 +102,14 @@ config PM_SLEEP_SMP
>  	depends on PM_SLEEP
>  	select HOTPLUG_CPU
>  
> +config PM_SLEEP_SYNC
> +	bool "Suspend to RAM starts with in-kernel call to sync()"
> +	default y
> +	depends on PM_SLEEP

this is actually a suspend specific feature, and it should depends on
SUSPEND instead?

thanks,
rui
> +	---help---
> +	Build a call to sync() into the Linux kernel suspend to RAM flow.
> +	The sync can still be skipped by clearing /sys/power/sleep_sync.
> +
>  config PM_AUTOSLEEP
>  	bool "Opportunistic sleep"
>  	depends on PM_SLEEP
> diff --git a/kernel/power/main.c b/kernel/power/main.c
> index 1d1bf63..1102871 100644
> --- a/kernel/power/main.c
> +++ b/kernel/power/main.c
> @@ -71,6 +71,36 @@ static ssize_t pm_async_store(struct kobject *kobj, struct kobj_attribute *attr,
>  
>  power_attr(pm_async);
>  
> +#ifdef CONFIG_PM_SLEEP_SYNC
> +
> +/* If set, sync file systems at start of suspend flow */
> +int pm_sleep_sync_enabled = 1;
> +
> +static ssize_t pm_sleep_sync_show(struct kobject *kobj, struct kobj_attribute *attr,
> +			     char *buf)
> +{
> +	return sprintf(buf, "%d\n", pm_sleep_sync_enabled);
> +}
> +
> +static ssize_t pm_sleep_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_sleep_sync_enabled = val;
> +	return n;
> +}
> +
> +power_attr(pm_sleep_sync);
> +
> +#endif /* CONFIG_PM_SLEEP_SYNC */
> +
>  #ifdef CONFIG_PM_DEBUG
>  int pm_test_level = TEST_NONE;
>  
> @@ -592,6 +622,9 @@ static struct attribute * g[] = {
>  #ifdef CONFIG_PM_SLEEP
>  	&pm_async_attr.attr,
>  	&wakeup_count_attr.attr,
> +#ifdef CONFIG_PM_SLEEP_SYNC
> +	&pm_sleep_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 7d4b7ff..dca60a7 100644
> --- a/kernel/power/power.h
> +++ b/kernel/power/power.h
> @@ -200,6 +200,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_sleep_sync_enabled;
>  #endif
>  
>  #ifdef CONFIG_HIGHMEM
> diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
> index 62ee437..2fb2f51 100644
> --- a/kernel/power/suspend.c
> +++ b/kernel/power/suspend.c
> @@ -333,9 +333,13 @@ static int enter_state(suspend_state_t state)
>  	if (state == PM_SUSPEND_FREEZE)
>  		freeze_begin();
>  
> -	printk(KERN_INFO "PM: Syncing filesystems ... ");
> -	sys_sync();
> -	printk("done.\n");
> +#ifdef CONFIG_PM_SLEEP_SYNC
> +	if (pm_sleep_sync_enabled) {
> +		printk(KERN_INFO "PM: Syncing filesystems ... ");
> +		sys_sync();
> +		printk("done.\n");
> +	}
> +#endif /* CONFIG_PM_SLEEP_SYNC */
>  
>  	pr_debug("PM: Preparing system for %s sleep\n", pm_states[state]);
>  	error = suspend_prepare(state);

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

[Brown, Len]

> > +	depends on PM_SLEEP
>
> this is actually a suspend specific feature, and it should depends on
> SUSPEND instead?

yup, will update.

thanks,
-Len

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

[Srivatsa S. Bhat]

On 01/23/2014 12:41 PM, Len Brown wrote:
> From: Len Brown <len.brown@intel.com>
> 
> Linux suspend-to-RAM was unreliable when first developed,
> and so sys_sync() was invoked inside the kernel at the
> start of every suspend flow.
> 
> Today, many devices are invoking suspend with
> high reliability and high frequency, and they don't
> want to be forced to pay for sync on every suspend.
> 
> So here we make it optional.
> De-select CONFIG_PM_SLEEP_SYNC to delete the call entirely.
> or keep CONFIG_PM_SLEEP and you can now clear the
> sys/power/sleep_sync attribte to disable the sync
> from user-space.
> 
> As we have had this call for a long time,
> the default remains to keep the call, and to
> invoke it on every suspend.
> 
> Signed-off-by: Len Brown <len.brown@intel.com>
> ---
>  kernel/power/Kconfig   |  8 ++++++++
>  kernel/power/main.c    | 33 +++++++++++++++++++++++++++++++++
>  kernel/power/power.h   |  1 +
>  kernel/power/suspend.c | 10 +++++++---
>  4 files changed, 49 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/power/Kconfig b/kernel/power/Kconfig
> index 2fac9cc..d2927ce 100644
> --- a/kernel/power/Kconfig
> +++ b/kernel/power/Kconfig
> @@ -102,6 +102,14 @@ config PM_SLEEP_SMP
>  	depends on PM_SLEEP
>  	select HOTPLUG_CPU
> 
> +config PM_SLEEP_SYNC

How about naming it as PM_SLEEP_FS_SYNC (and similarly in the sysfs files
and variable names as well). Just to avoid confusion with "synchronous/async".

Regards,
Srivatsa S. Bhat

> +	bool "Suspend to RAM starts with in-kernel call to sync()"
> +	default y
> +	depends on PM_SLEEP
> +	---help---
> +	Build a call to sync() into the Linux kernel suspend to RAM flow.
> +	The sync can still be skipped by clearing /sys/power/sleep_sync.
> +
>  config PM_AUTOSLEEP
>  	bool "Opportunistic sleep"
>  	depends on PM_SLEEP
> diff --git a/kernel/power/main.c b/kernel/power/main.c
> index 1d1bf63..1102871 100644
> --- a/kernel/power/main.c
> +++ b/kernel/power/main.c
> @@ -71,6 +71,36 @@ static ssize_t pm_async_store(struct kobject *kobj, struct kobj_attribute *attr,
> 
>  power_attr(pm_async);
> 
> +#ifdef CONFIG_PM_SLEEP_SYNC
> +
> +/* If set, sync file systems at start of suspend flow */
> +int pm_sleep_sync_enabled = 1;
> +
> +static ssize_t pm_sleep_sync_show(struct kobject *kobj, struct kobj_attribute *attr,
> +			     char *buf)
> +{
> +	return sprintf(buf, "%d\n", pm_sleep_sync_enabled);
> +}
> +
> +static ssize_t pm_sleep_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_sleep_sync_enabled = val;
> +	return n;
> +}
> +
> +power_attr(pm_sleep_sync);
> +
> +#endif /* CONFIG_PM_SLEEP_SYNC */
> +
>  #ifdef CONFIG_PM_DEBUG
>  int pm_test_level = TEST_NONE;
> 
> @@ -592,6 +622,9 @@ static struct attribute * g[] = {
>  #ifdef CONFIG_PM_SLEEP
>  	&pm_async_attr.attr,
>  	&wakeup_count_attr.attr,
> +#ifdef CONFIG_PM_SLEEP_SYNC
> +	&pm_sleep_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 7d4b7ff..dca60a7 100644
> --- a/kernel/power/power.h
> +++ b/kernel/power/power.h
> @@ -200,6 +200,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_sleep_sync_enabled;
>  #endif
> 
>  #ifdef CONFIG_HIGHMEM
> diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
> index 62ee437..2fb2f51 100644
> --- a/kernel/power/suspend.c
> +++ b/kernel/power/suspend.c
> @@ -333,9 +333,13 @@ static int enter_state(suspend_state_t state)
>  	if (state == PM_SUSPEND_FREEZE)
>  		freeze_begin();
> 
> -	printk(KERN_INFO "PM: Syncing filesystems ... ");
> -	sys_sync();
> -	printk("done.\n");
> +#ifdef CONFIG_PM_SLEEP_SYNC
> +	if (pm_sleep_sync_enabled) {
> +		printk(KERN_INFO "PM: Syncing filesystems ... ");
> +		sys_sync();
> +		printk("done.\n");
> +	}
> +#endif /* CONFIG_PM_SLEEP_SYNC */
> 
>  	pr_debug("PM: Preparing system for %s sleep\n", pm_states[state]);
>  	error = suspend_prepare(state);
> 

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

[Brown, Len]

> How about naming it as PM_SLEEP_FS_SYNC (and similarly in the sysfs
> files
> and variable names as well). Just to avoid confusion with
> "synchronous/async".


good point -- thanks!

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

[Pavel Machek]

Hi!

> From: Len Brown <len.brown@intel.com>
> 
> Linux suspend-to-RAM was unreliable when first developed,
> and so sys_sync() was invoked inside the kernel at the
> start of every suspend flow.
> 
> Today, many devices are invoking suspend with
> high reliability and high frequency, and they don't
> want to be forced to pay for sync on every suspend.
> 
> So here we make it optional.
> De-select CONFIG_PM_SLEEP_SYNC to delete the call entirely.
> or keep CONFIG_PM_SLEEP and you can now clear the
> sys/power/sleep_sync attribte to disable the sync
> from user-space.

Dunno. Config option plus sysfs attribute is overdoing it a bit.
Maybe we could do

echo memfast > state

to enter suspend without sync? Patch should be simpler that way,
and we won't have to keep the state...

						Pavel

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

[Len Brown]

On Sun, Jan 26, 2014 at 4:08 PM, Pavel Machek <pavel@ucw.cz> wrote:

> Dunno. Config option plus sysfs attribute is overdoing it a bit.

Agreed.
Have discussed w/ Rafael, and current plan is to simply delete.
Updated patch on the way...

thanks,
Len Brown, Intel Open Source Technology Center