On Tue, Oct 02, 2018 at 07:05:45PM +0200, Benjamin Berg wrote:
> Hi
>
> On Tue, 2018-10-02 at 14:34 +0200, Hans de Goede wrote:
> > On 02-10-18 14:26, Justin Forbes wrote:
> > > On Tue, Oct 2, 2018 at 5:53 AM, Hans de Goede <hdegoede(a)redhat.com> wrote:
> > > > Hi Justin,
> > > >
> > > > On 01-10-18 16:14, Justin Forbes wrote:
> > > > > On Sun, Sep 30, 2018 at 1:52 PM, Hans de Goede <hdegoede(a)redhat.com>
> > > > > wrote:
> > > > > > Hello Fedora kernel team,
> > > > > >
> > > > > > On the Fedora desktop list there has been a discussion about
> > > > > > systemd now offering a new suspend-then-hibernate option and
> > > > > > gnome-settings-daemon's media-keys plugin using this when
> > > > > > the power-button gets pressed and systemd saying this is
> > > > > > available on the system.
> > > > > >
> > > > > > What this does is suspend the system normally and set
> > > > > > a RTC wakeup 3 hours in the future, then when the RTC wake
> > > > > > happens it hibernates the system.
> > > > > >
> > > > > > As discussed on the desktop list this is not really desirable
> > > > > > as default behavior for F29 (and later) since the hibernate
> > > > > > code is not really something which gets used enough to be
> > > > > > well tested and is really not something which we can support.
> > > > > >
> > > > > > So after that the discussion has gone in the direction of
> > > > > > how to disable the new suspend-then-hibernate behavior.
> > > > > >
> > > > > > Lennart made a really interesting observation here, systemd
> > > > > > is just proxying if "cat /sys/power/disk" indicates that
> > > > > > hibernate is supported.
> > > > > >
> > > > >
> > > > > No, that is not what systemd is doing. The kernel provides a
> > > > > mechanism, it does absolutely nothing with that mechanism unless told
> > > > > to do so. What systemd is actually doing is creating a policy around
> > > > > that mechanism.
> > > >
> > > > systemd is really *not* creating policy here, it has a DBUS API
> > > > call called CanHibernate which really just proxies what the kernel
> > > > advertises.
> > > >
> > > > What is new is that GNOME (g-s-d) now uses it be default through
> > > > by choosing suspend-then-hibernate as suspend action when
> > > > hibernation is available.
> > > >
> > >
> > > Right, so systemd really is just proxying, and Gnome is now creating a
> > > policy. It does not change the underlying issue, we shouldn't cripple
> > > a mechanism because someone wants to introduce a new undesirable
> > > policy on top of it. Especially when the thing introducing that
> > > policy is not the only user of the mechanism.
> > >
> > > > > > So if we really don't want to support hibernation as a normal
> > > > > > option, while still allowing adventurous user to use it, what
> > > > > > really should happen is for the kernel to stop advertising
> > > > > > hibernate support. Thinking about this I agree, if we say
> > > > > > that we cannot support it, the kernel really should not be
> > > > > > advertising support for it by default.
> > > > > >
> > > > >
> > > > > "We have decided that the policy created is not desirable, so we want
> > > > > to disable the mechanism"
> > > >
> > > > Default to off is a different thing then disabling this.
> > > >
> > >
> > > There is a difference between a "policy default to off", and "turning
> > > off the mechanism". I would expect a new policy defaulting to off
> > > would actually default to whatever is currently there. When a user
> > > upgrades from F28 to F30, it seems wrong that their power
> > > configuration would change in a way that is unexpected, and frankly
> > > more difficult to manage. A regular user can run gnome-tweaks and
> > > decide on lid behavior. A regular user cannot edit the kernel command
> > > line once a system is booted. Now, it requires root. And we are
> > > making it harder for people to edit it as a system boots with other
> > > planned changes.
> > >
> > > > TBH I'm a bit surprised about your objections against this:
> > > >
> > > > 1) We all seem to agree that this is something which may or
> > > > may not work, but is not something which we want to advertise
> > > > as a "supported" Fedora feature
> > > >
> > > > 2) Given 1) we also all agree that we should not use it
> > > > by default
> > > >
> > > > 3) If we should not use it by default then shouldn't the
> > > > feature default to off ? That is all what is being suggested,
> > > > basically the equivalent of adding "nohibernate" to the
> > > > kernel commandline by default.
> > > >
> > >
> > > And this is the problem, there is no reason anyone should have to edit
> > > the kernel command line for choosing a power policy setting. Gnome is
> > > certainly not the only desktop in use, practically all of the
> > > documentation out there explains how to enable or disable hibernate
> > > with common tools. We are invalidating all of that documentation just
> > > so that gnome can implement a bad default policy. Agreeing that we
> > > should not use it by default doesn't mean we should make it any harder
> > > to use than it already is. We certainly have no motivation to
> > > discourage people for giving it a try, they just have to know that
> > > they are hibernating, and have chosen to do so. I would be just fine
> > > with it being easier to access in gnome than installing gnome-tweaks,
> > > hibernate, if supported by the system, could be easily selected under
> > > the power menu in settings.
> > >
> > > > I expect the kernel changes for this to be about 3 lines of
> > > > actual code (+a 15 lines or so Kconfig addition) and I expect
> > > > this to go upstream without much issues as this seems like
> > > > an entirely reasonable thing to do.
> > > >
> > > > Reading further along the discussion you say that if this
> > > > were a new feature you would likely agree to defaulting it
> > > > to off. But since this has been there for years we should
> > > > not change it ? That seems like a weak argument to me, we
> > > > have always been doing this in a sub-optimal way so lets
> > > > continue doing this in a sub-optimal way ?
> > > >
> > > I don't agree that the way we have been doing this is sub-optimal at
> > > all. In fact, I think this proposed patch is the sub-optimal way. My
> > > point is, new features, particularly with possible undesirable
> > > results, are often defaulted to off under a "tech preview" model. The
> > > mechanism in the kernel is not new. it may not work for everyone, but
> > > it has been working fairly well for those who it does work for. Why
> > > would we change them when a piece of userspace (that some of them
> > > might never even use) wants to create a poor default policy?
> > >
> > > > I agree that we should not change it in the middle of a
> > > > Fedora cycle. Hence I wrote:
> > > >
> > > > > > Against:
> > > > > >
> > > > > > Currently we do have some users using hibernation without
> > > > > > adding any options to the kernel commandline. These users
> > > > > > will have to now add "hibernate=yes" to their kernel commandline.
> > > > > >
> > > > > > I'm thinking that yes we want this, but maybe this needs to
> > > > > > go through the change process for proper communication, so for
> > > > > > F29 we need another fix, and we can do this for F30?
> > > >
> > > > I believe that if we put this through the change process,
> > > > we can make sure that we properly communicate the need to add
> > > > "hinernate=yes" to the kernel commandline for people who use
> > > > it and want to keep using it.
> > > >
> > > > I also expect this to, if anything, lower the load for the Fedora
> > > > kernel team, since it avoids users enabling hibernation without
> > > > really knowing what they are doing and then filing bugs as a result
> > > > of this. E.g.:
> > > > * ATM in F28 hibernation is a simple click in gnome-tweaks away.
> > > > * Even if we revert the GNOME change which triggered this discussion
> > > > many other DEs will still advertise hibernate support in some way.
> > > >
> > > > Can you please elaborate a bit on your objections against this?
> > > >
> > >
> > > It really is simple. You don't cripple a mechanism so that you can
> > > install a bad default policy. The kernel is providing a plain and
> > > neutral mechanism here. If people agree that the default policy is
> > > wrong, it is the policy that should change. While I strongly feel
> > > that the default power behavior should not be hibernate, I also fee
> > > pretty strongly that it should be very easy to people to change.
> >
> > Ok fair enough. Keeping it easy for users to try out hibernate is
> > a valid argument.
> >
> > So that puts the ball back into the court of the GNOME devs, adding
> > Benjamin (g-s-d co-maintainer) to the Cc and I'll also ping him
> > on IRC about this.
>
> So, it still looks like we are at a bit of a dead end with this
> discussion. We know we need to disable the use of hibernation by
> default but where depends on how you look at the issue (is it a policy
> issue or are features falsely advertised).
>
> From a GNOME Settings Daemon perspective I am tempted to simply remove
> the feature again. There are two main reasons for considering this:
>
> 1. It would solve the pressing issue for F29.
> 2. The current implementation is incomplete and causes
> inconsistencies for users.
>
> I only realised the issue with 2. when looking into it more because of
> this discussion. We currently have at least 6 reasons to suspend with
> different components creating the policy:
>
> * lid action:
> - systemd-logind
> - gsd-power inhibits in some cases
>
> * power button:
> - gsd-media-keys: power-button-action
> gsd-power settings keys
>
> * soft button:
> - gnome-shell: hardcoded to suspend (logind call)
>
> * user is idle
> - gsd-power: sleep-inactive-ac-type, sleep-inactive-battery-type
> gsd-power settings keys
>
> * battery critical:
> - upower: /etc/UPower/UPower.conf (defaults to hybrid suspend)
>
> * screen blanking:
> - gsd-power: hardcoded to suspend on tablets
>
> The patches that introduced the policy only made the change for the
> "power button" and "user is idle" cases (and "screen blanking"). In
> particular the "soft button" and "lid" cases are not covered, meaning
> that in many cases users will not actually suspend-then-hibernate[1].
Yeah, that seems quite inconsistent. I'd say "power button" > "lid" > "user is idle",
in the sense that events on the right should use lighter suspend modes.
> So, my proposal from the g-s-d side is:
> 1. For the *stable* release cycle, hide the new "feature" behind a
> compile time switch, disable it by default and announce properly
> in the release notes.
+1, even though a hidden setting key would be even nicer
> 2. For the unstable release cycle plan on finding a more consistent
> solution to control the policy.
> Whether this is user configurable or not could still be something
> to discuss.
That'd be great too.
> I would be happy if the underlying CanSuspendThenHibernate and
> CanSuspend calls would not be advertising features that are unusable.
Working on it ;)
> My personal view is that it would be good if these calls were only
> advertising the feature if it actually works, but I also see how this
> can be hard or impossible to detect.
Yes, this is impossible in the general case.
> If systemd (or the kernel) is adjusted, then g-s-d may not need a
> change. But even then it may still make sense to allow disabling the
> new behaviour just to avoid the policy inconsistencies in the short
> term.
Yes, I think g-s-d should change too, even if systemd improves, since
it's g-s-d and/or user that sets the policy.
Zbyszek
