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

[Pavel Machek <pavel@ucw.cz>]

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