Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1184210 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH v4 0/1] suspend: make sync() on suspend-to-RAM optional Len Brown <lenb@kernel.org> - 2015-07-15 04:30 +0200
    [PATCH 1/1] suspend: make sync() on suspend-to-RAM optional Len Brown <lenb@kernel.org> - 2015-07-15 04:30 +0200
      Re: [PATCH 1/1] suspend: make sync() on suspend-to-RAM optional Pavel Machek <pavel@ucw.cz> - 2015-07-15 08:50 +0200
        Re: [PATCH 1/1] suspend: make sync() on suspend-to-RAM optional Austin S Hemmelgarn <ahferroin7@gmail.com> - 2015-07-15 16:10 +0200
          RE: [PATCH 1/1] suspend: make sync() on suspend-to-RAM optional "Brown, Len" <len.brown@intel.com> - 2015-07-15 17:00 +0200
    Re: [PATCH 1/1] suspend: make sync() on suspend-to-RAM optional Pavel Machek <pavel@ucw.cz> - 2015-07-22 09:30 +0200
    Re: [PATCH 1/1] suspend: make sync() on suspend-to-RAM optional Oliver Neukum <oneukum@suse.com> - 2015-07-22 11:00 +0200

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

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.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

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

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

[Multipart message — attachments visible in raw view] — view raw

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.

[toc] | [prev] | [next] | [standalone]

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

> -----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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web