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

Groups > linux.kernel > #1176287 > unrolled thread

Re: [PATCH 1/1] suspend: delete sys_sync()

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 1/1] suspend: delete sys_sync() Dave Chinner <david@fromorbit.com> - 2015-07-03 03:50 +0200
    Re: [PATCH 1/1] suspend: delete sys_sync() "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2015-07-04 02:40 +0200
      Re: [PATCH 1/1] suspend: delete sys_sync() Alan Stern <stern@rowland.harvard.edu> - 2015-07-05 11:10 +0200
        Re: [PATCH 1/1] suspend: delete sys_sync() Pavel Machek <pavel@ucw.cz> - 2015-07-07 12:30 +0200

#1176287 — Re: [PATCH 1/1] suspend: delete sys_sync()

On Wed, Jul 01, 2015 at 11:07:29PM -0400, Len Brown wrote:
> >> The _vast_ majority of systems using Linux suspend today are under
> >> an Android user-space.  Android has no assumption that that suspend to
> >> mem will necessarily stay suspended for a long time.
> >
> > Indeed, however your change was not android-specific, and it is not
> > "comfortable" on x86-style hardware and usage patterns.
> 
> "comfortable on x86-style and usage patterns"?
> If you mean "traditional" instead of "comfortable",
> where "tradition" is based on 10-year old systems, then sure.

Even if this were true(*) we don't break things that currently work
just because something different is "just around the corner". e.g.
if you shut the lid on your laptop and it suspends to RAM, you can
pull the USB drive out that you just copied stuff to and plug it
into another machine and find all the data you copied there is
present.

Remove the sync() from the freeze code, and this isn't guaranteed to
work anymore. It is now dependent on userspace implementations for
this to work, and we know what userspace developers will choose in
this situation. i.e. fast and "works for me", not "safe for
everyone".

(*) Which it clearly isn't true because, as this example shows, my
shiny new laptop still has exactly the same data integrity
requirements as the laptop I was using 10 years ago.

Just because there are lots of Android or Chrome out there it
doesn't mean we can just ignore the requirements of everything
else...

> > That said, as long as x86 will still try to safeguard my data during mem
> > sleep/resume as it does today, I have no strong feelings about
> > light/heavy-weight "mem" sleep being strictly a compile-time selectable
> > thing, or a more flexible runtime-selectable behavior.
> 
> The observation here is that the kernel should not force every system
> to sys_sync() on every suspend.  The only question is how to best
> implement that.

No, your observation was that "sync is slow". Your *solution* is "we
need to remove sync".

However, your arguement so far has these problems:

	- repeated sync from outside the suspend context is does not
	  demonstrate the problem you are seeing during suspend, and
	  you have not yet identified why this is the case.
	- it has been demonstrated that inode cache size plays a
	  significant role in sync latency, but you haven't provided
	  any information to tell us what the cache sizes were when
	  you see large latencies.
	- it has been demonstrated that there are patches pending
	  that improve clean filesystem sync speed, but you have not
	  produced numbers to demonstrate that those patches do not
	  meet your requirements.
	- In several tests your "sync latency" monitoring was
	  dirtying the filesystem and hence *causing* the repeated
	  syncs to be slow.
	- you have not told us whether your suspend monitoring was
	  the cause of the suspend sync latency or not.
	- you have been testing on hardware with questionable power
	  management behaviour.

IOWs, you have not yet identified the root cause of the slow sync
behaviour on suspend, you have not determined if pending work fixes
the latency problems, and you have not reproduced your results after
fixing  the flaws in your testing methodology.

> The obvious solution was to delete this forced policy
> from the kernel, and let user-space handle it.
> Rafael has not agreed to push that obvious, though less-than-gentle
> solution upstream, and so I'll re-send the historic patch
> that allows distros to still sync like it is 1999, if they want to:-)

Please stop shouting about "obvious" solutions until you've actually
proved there is a problem and that problems you find aren't already
fixed by the pending sync changes....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com
--
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]

#1176824

On Friday, July 03, 2015 11:42:50 AM Dave Chinner wrote:
> On Wed, Jul 01, 2015 at 11:07:29PM -0400, Len Brown wrote:
> > >> The _vast_ majority of systems using Linux suspend today are under
> > >> an Android user-space.  Android has no assumption that that suspend to
> > >> mem will necessarily stay suspended for a long time.
> > >
> > > Indeed, however your change was not android-specific, and it is not
> > > "comfortable" on x86-style hardware and usage patterns.
> > 
> > "comfortable on x86-style and usage patterns"?
> > If you mean "traditional" instead of "comfortable",
> > where "tradition" is based on 10-year old systems, then sure.
> 
> Even if this were true(*) we don't break things that currently work
> just because something different is "just around the corner". e.g.
> if you shut the lid on your laptop and it suspends to RAM, you can
> pull the USB drive out that you just copied stuff to and plug it
> into another machine and find all the data you copied there is
> present.
> 
> Remove the sync() from the freeze code, and this isn't guaranteed to
> work anymore. It is now dependent on userspace implementations for
> this to work, and we know what userspace developers will choose in
> this situation. i.e. fast and "works for me", not "safe for
> everyone".
> 
> (*) Which it clearly isn't true because, as this example shows, my
> shiny new laptop still has exactly the same data integrity
> requirements as the laptop I was using 10 years ago.
> 
> Just because there are lots of Android or Chrome out there it
> doesn't mean we can just ignore the requirements of everything
> else...
> 
> > > That said, as long as x86 will still try to safeguard my data during mem
> > > sleep/resume as it does today, I have no strong feelings about
> > > light/heavy-weight "mem" sleep being strictly a compile-time selectable
> > > thing, or a more flexible runtime-selectable behavior.
> > 
> > The observation here is that the kernel should not force every system
> > to sys_sync() on every suspend.  The only question is how to best
> > implement that.
> 
> No, your observation was that "sync is slow". Your *solution* is "we
> need to remove sync".

Not only slow, but pointless too.  The argument goes: "It is slow and
pointless and so it may be dropped."

Now, I can agree that it wasn't clearly demonstrated that the unconditional
sys_sync() in the suspend code path was pointless, but it also has never
been clearly shown why it is not pointless on systems that suspend and resume
reliably.

[The argument that the user can pull removable storage devices out of the
system while suspended doesn't hold any water to me, because the user can
pull them out of the system when not suspended just as well and cause the
same kind of damage to happen.]

When we were adding it, the thinking was along the lines of "Well, suspend
isn't too reliable, so let's put sys_sync() in there to possibly reduce the
damage from suspend/resume crashes and suspend is slow anyway, so the possible
effect on performance from that shouldn't be noticeable".  Clearly, the world
has changed since then and suspend is far more reliable than it used to be in
general and it is not that slow too at least on some systems (especially the
suspend-to-idle flavor).

The only argument against dropping sys_sync() from the suspend code path
I've seen in this thread that I entirely agree with is that it may lead to
regressions, because we've done it practically forever and it may hide latent
bugs somewhere in block drivers etc.  Dropping it, though, is the only way
to see those bugs, if any, and if we want to ever fix them, we need to see
them.  That's why I think that it may be a good idea to allow people to
drop it if they are willing to accept some extra risk (via the kernel
command line, for example).

Moreover, question is if we really need to carry out the sync on *every*
suspend even if it is not pointless overall.  That shouldn't really be
necessary if we suspend and resume often enough or if we resume only for
a while and then suspend again.  Maybe it should be rate limited somehow
at least?

Thanks,
Rafael

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

#1176916

On Sat, 4 Jul 2015, Rafael J. Wysocki wrote:

> The only argument against dropping sys_sync() from the suspend code path
> I've seen in this thread that I entirely agree with is that it may lead to
> regressions, because we've done it practically forever and it may hide latent
> bugs somewhere in block drivers etc.  Dropping it, though, is the only way
> to see those bugs, if any, and if we want to ever fix them, we need to see
> them.  That's why I think that it may be a good idea to allow people to
> drop it if they are willing to accept some extra risk (via the kernel
> command line, for example).

I'd be perfectly happy to have the sync selectable at runtime, one way 
or another.  The three most reasonable options seem to be:

	kernel command line

	sysfs file

	sysctl setting

The command line is less flexible (it can't be changed after booting).  
Either of the other two would be fine with me.

> Moreover, question is if we really need to carry out the sync on *every*
> suspend even if it is not pointless overall.  That shouldn't really be
> necessary if we suspend and resume often enough or if we resume only for
> a while and then suspend again.  Maybe it should be rate limited somehow
> at least?

For example, skip the sync if the system has been awake for < 100 ms?  
The cutoff time could also be controlled by the sysfs file: -1 =>
never sync, 0 => always sync, > 0 => sync if the system has been awake 
longer than the value.

Alan Stern

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

#1178315

On Mon 2015-07-06 15:59:15, Rafael J. Wysocki wrote:
> On Monday, July 06, 2015 01:06:45 PM Pavel Machek wrote:
> > On Mon 2015-07-06 01:28:20, Rafael J. Wysocki wrote:
> > > On Saturday, July 04, 2015 10:19:55 AM Alan Stern wrote:
> > > > On Sat, 4 Jul 2015, Rafael J. Wysocki wrote:
> > > > 
> > > > > The only argument against dropping sys_sync() from the suspend code path
> > > > > I've seen in this thread that I entirely agree with is that it may lead to
> > > > > regressions, because we've done it practically forever and it may hide latent
> > > > > bugs somewhere in block drivers etc.  Dropping it, though, is the only way
> > > > > to see those bugs, if any, and if we want to ever fix them, we need to see
> > > > > them.  That's why I think that it may be a good idea to allow people to
> > > > > drop it if they are willing to accept some extra risk (via the kernel
> > > > > command line, for example).
> > > > 
> > > > I'd be perfectly happy to have the sync selectable at runtime, one way 
> > > > or another.  The three most reasonable options seem to be:
> > > > 
> > > > 	kernel command line
> > > > 
> > > > 	sysfs file
> > > > 
> > > > 	sysctl setting
> > > > 
> > > > The command line is less flexible (it can't be changed after booting).  
> > > > Either of the other two would be fine with me.
> > > 
> > > We'll probably use a sysfs file (possibly plus a Kconfig option to set the
> > > boot time default).
> > 
> > Android people can already do sync-less s2ram using existing
> > interface. IMO they should just do it.
> > 
> > In any case, sysfs file + Kconfig is an overkill. We already have too
> > many Kconfig options.
> 
> I don't think we can reach a general agreement on what's the *right* approach
> with respect to the sys_sync() in the suspend code path, so the only way out
> of this situation I can see is to make it configurable.

So first: not having general agreement does not mean we should
introduce Kconfig + sysfs file. Second: your proposal of "lets sync if
runtime was shorter than xxx" is over complex, but at least should not
need Kconfig support... Third: we have ioctl() based interface, and I
guess android should use that one; it already has "s2ram without sync"
method.

> > There's not a single Android phone supported by mainline
> > kernel. I'm sure they have bigger problems than Android setting
> > default sysfs values...
> 
> But perhaps we'd like to change that?

We'd like to, but lets start with the real hard stuff (merging support
for Qualcomm chipsets) that is 1000000 LoC+, not with trivial tweaks
that would be 1-line change, but we pollute code with Kconfig+sysfs
making it 100..

									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] | [standalone]

Back to top | Article view | linux.kernel

csiph-web