From: dmitry.torokhov@gmail.com <dmitry.torokhov@gmail.com>
Sent: Monday, September 30, 2019 4:07 PM

On Mon, Sep 30, 2019 at 10:09:27PM +0000, Dexuan Cui wrote:

quoted

quoted

From: dmitry.torokhov@gmail.com <dmitry.torokhov@gmail.com>
Sent: Friday, September 27, 2019 5:32 PM

quoted

...
pm_wakeup_pending() is tested in a lot of places in the suspend
process and eventually an unintentional keystroke (or mouse movement,
when it comes to the Hyper-V mouse driver drivers/hid/hid-hyperv.c)
causes the whole hibernation process to be aborted. Usually this
behavior is not expected by the user, I think.

Why not? If a device is configured as wakeup source, then it activity
should wake up the system, unless you disable it.

Generally speaking, I agree, but compared to a physical machine, IMO
the scenario is a little different when it comes to a VM running on Hyper-V:
on the host there is a window that represents the VM, and the user can
unintentionally switch the keyboard input focus to the window (or move
the mouse/cursor over the window) and then the host automatically
sends some special keystrokes (and mouse events) , and this aborts the
hibernation process.

And, when it comes to the Hyper-V mouse device, IMO it's easy for the
user to unintentionally move the mouse after the "hibernation" button
is clicked. I suppose a physical machine would have the same issue, though.

If waking the machine up by mouse/keyboard activity is not desired in
Hyper-V environment, then simply disable them as wakeup sources.

Sorry for the late reply! I have been sidetracked by something else...

Several years ago, we marked Hyper-V mouse/keyboard devices as wake 
sources to fix such a bug: the VM can not be woken up after we run
"echo freeze > /sys/power/state". IMO we should keep the mouse/keyboard
as wakeup sources.
 

quoted

quoted

quoted

So, I use the notifier to set the flag variable and with it the driver can
know when it should not call pm_wakeup_hard_event().

No, please implement hibernation support properly, as notifier + flag is
a hack.

The keyboard/mouse driver can avoid the flag by disabling the
keyboard/mouse event handling, but the problem is that they don't know
when exactly they should disable the event handling. I think the PM
notifier is the only way to tell the drivers a hibernation process is ongoing.

Whatever initiates hibernation (in userspace) can adjust wakeup sources
as needed if you want them disabled completely.

Good to know this! I just found the userspace is able to disable the Hyper-V
mouse/keyboard as wakeup sources by something like:

echo disabled >  /sys/bus/vmbus/devices/XXX/power/wakeup
(XXX is the device GUID).
 

quoted

Do you think this idea (notifier + disabling event handling) is acceptable?

No, I believe this a hack, that is why I am pushing back on this.

Ok, I think we can get rid of the notifier completely, and tell the users to disable
the 2 wakeup sources, if they think the wakeup behavior is undesired.
 

quoted

If not, then I'll have to remove the notifier completely, and document this as
a known issue to the user: when a hibernation process is started, be careful
to not switch input focus and not touch the keyboard/mouse until the
hibernation process is finished. :-)

quoted

In this particular case you do not want to have your
hv_kbd_resume() to be called in place of pm_ops->thaw() as that is what
reenables the keyboard vmbus channel and causes the undesired wakeup
events.

This is only part of the issues. Another example: before the
pm_ops()->freeze()'s of all the devices are called, pm_wakeup_pending()
is already tested in a lot of places (e.g. in try_to_freeze_tasks ()) in the
suspend process, and can abort the whole suspend process upon the user's
unintentional input focus switch, keystroke and mouse movement.

How long is the prepare() phase on your systems? 

I have no specific data, but I know it's fast.

User may wiggle mouse at any time really, even before the notifier fires up.

This doesn't matter, because the counter "pm_abort_suspend" is cleared at
a later place. The code path is:

hibernate() ->
  __pm_notifier_call_chain(PM_HIBERNATION_PREPARE, -1, &nr_calls)
  freeze_processes() ->
    pm_wakeup_clear() -> 
      atomic_set(&pm_abort_suspend, 0);

This patch sets the flag in the PM_HIBERNATION_PREPARE notifier, so
there is no race.

Since I'm going to get rid of the notifier, we don't care at all about this now.
 

quoted

quoted

Your vmbus implementation should allow individual drivers to
control the set of PM operations that they wish to use, instead of
forcing everything through suspend/resume.

Dmitry

Since the devices are pure software-emulated devices, no PM operation was
supported in the past, and now suspend/resume are the only two PM

operations

quoted

we're going to support. If the idea (notifier + disabling event handling) is not
good enough, we'll have to document the issue to the user, as I described

above.

¯\_(ツ)_/¯ If you do not want to implement hibernation properly in vmbus
code that is totally up to you (have you read in pm.h how freeze() is
different from suspend()?).
Dmitry

I understand freeze() is different from suspend(). Here I treat suspend() as a
heavyweight freeze() for simplicity and IMHO the extra cost of time is
neglectable considering the long hibernation process, which can take 
5~10+ seconds.

Even if I implement all the pm ops, IMO the issue we're talking about
(i.e. the hibernation process can be aborted by user's keyboard/mouse
activities) still exists. Actually I think a physical Linux machine should have
the same issue.

In practice, IMO the issue is not a big concern, as the VM usually runs in
a remote data center, and the user has no access to the VM's 
keyboard/mouse. :-)

I hope I understood your comments. I'll post a v2 without the notifier. 
Please Ack the v2 if it looks good to you.

Thanks,
-- Dexuan