Message-ID: <Yr11fp13yMRiEphS@zx2c4.com>
Date:   Thu, 30 Jun 2022 12:05:50 +0200
From:   "Jason A. Donenfeld" <Jason@...c4.com>
To:     Kalesh Singh <kaleshsingh@...gle.com>
Cc:     John Stultz <jstultz@...gle.com>, Christoph Hellwig <hch@....de>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Arve Hjønnevåg <arve@...roid.com>,
        Todd Kjos <tkjos@...roid.com>,
        Martijn Coenen <maco@...roid.com>,
        Joel Fernandes <joel@...lfernandes.org>,
        Christian Brauner <brauner@...nel.org>,
        Hridya Valsaraju <hridya@...gle.com>,
        Suren Baghdasaryan <surenb@...gle.com>,
        Theodore Ts'o <tytso@....edu>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        "Alex Xu (Hello71)" <alex_y_xu@...oo.ca>,
        Paolo Abeni <pabeni@...hat.com>, Rob Herring <robh@...nel.org>,
        "Paul E. McKenney" <paulmck@...nel.org>,
        Frederic Weisbecker <frederic@...nel.org>,
        Neeraj Upadhyay <quic_neeraju@...cinc.com>,
        Josh Triplett <josh@...htriplett.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
        Lai Jiangshan <jiangshanlai@...il.com>,
        Shuah Khan <shuah@...nel.org>,
        LKML <linux-kernel@...r.kernel.org>, wireguard@...ts.zx2c4.com,
        netdev@...r.kernel.org, rcu <rcu@...r.kernel.org>,
        "open list:KERNEL SELFTEST FRAMEWORK" 
        <linux-kselftest@...r.kernel.org>, sultan@...neltoast.com,
        android-kernel-team <android-kernel-team@...gle.com>,
        Saravana Kannan <saravanak@...gle.com>,
        "Rafael J. Wysocki" <rafael@...nel.org>
Subject: Re: [PATCH] remove CONFIG_ANDROID

Hi Kalesh,

On Wed, Jun 29, 2022 at 09:25:32PM -0700, Kalesh Singh wrote:
> On Wed, Jun 29, 2022 at 5:30 PM Jason A. Donenfeld <Jason@...c4.com> wrote:
> >
> > Hey again,
> >
> > On Thu, Jun 30, 2022 at 2:24 AM Jason A. Donenfeld <Jason@...c4.com> wrote:
> > > 1) Introduce a simple CONFIG_PM_CONTINUOUS_AUTOSLEEPING Kconfig thing
> > >    with lots of discouraging help text.
> > >
> > > 2) Go with the /sys/power tunable and bikeshed the naming of that a bit
> > >    to get it to something that reflects this better, and document it as
> > >    being undesirable except for Android phones.
> >
> > One other quick thought, which I had mentioned earlier to Kalesh:
> >
> > 3) Make the semantics a process holding open a file descriptor, rather
> >    than writing 0/1 into a file. It'd be called /sys/power/
> >    userspace_autosleep_ctrl, or something, and it'd enable this behavior
> >    while it's opened. And maybe down the line somebody will want to add
> >    ioctls to it for a different purpose. This way it's less of a tunable
> >    and more of an indication that there's a userspace app doing/controlling
> >    something.
> >
> > This idea (3) may be a lot of added complexity for basically nothing,
> > but it might fit the usage semantics concerns a bit better than (2). But
> > anyway, just an idea. Any one of those three are fine with me.
> 
> Two concerns John raised:
>   1) Adding new ABI we need to maintain
>   2) Having unclear config options
> 
> Another idea, I think, is to add the Kconfig option as
> CONFIG_SUSPEND_SKIP_RNG_RESEED? Similar to existing
> CONFIG_SUSPEND_SKIP_SYNC and I think it would address those concerns.

I mentioned in my reply to him that this doesn't really work for me:

| As a general rule, I don't expose knobs like that in wireguard /itself/,
| but wireguard has no problem with adapting to whatever machine properties
| it finds itself on. And besides, this *is* a very definite device
| property, something really particular and peculiar about the machine
| the kernel is running on. It's a concrete thing that the kernel should
| know about. So let's go with your "very clear description idea", above,
| instead.

IOW, we're not going to add a tunable on every possible place this is
used.

Anyway if you don't want a runtime switch, make a compiletime switch
called CONFIG_PM_CONTINUOUS_RAPID_AUTOSLEEPING or whatever, write some
very discouraging help text, and call it a day. And this way you don't
have to worry about ABI and we can change this later on and do the whole
thing as a no-big-deal change that somebody can tweak later without
issue.

The below diff is some boiler plate to help you get started with that
direction. Similar order of operations for this one:

1. You write a patch for Android's base config to enable this option and
   post it on Gerrit.

2. You take the diff below, clean it up or bikeshed the naming a bit or
   do whatever there, and submit it to the kernel, including as a `Link:
   ...` this thread and the Gerrit link.

3. When the patch lands, you submit the Gerrit CL.

4. When both have landed, Christoph moves forward with his
   CONFIG_ANDROID removal.

So really, just pick an option here -- the runtime switch or the
compiletime switch or the crazy fd thing I mentioned -- and run with it.

Jason

diff --git a/drivers/char/random.c b/drivers/char/random.c
index e3dd1dd3dd22..5332236cb1ad 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -756,7 +756,7 @@ static int random_pm_notification(struct notifier_block *nb, unsigned long actio

 	if (crng_ready() && (action == PM_RESTORE_PREPARE ||
 	    (action == PM_POST_SUSPEND &&
-	     !IS_ENABLED(CONFIG_PM_AUTOSLEEP) && !IS_ENABLED(CONFIG_ANDROID)))) {
+	     !IS_ENABLED(CONFIG_PM_AUTOSLEEP) && !IS_ENABLED(CONFIG_PM_RAPID_USERSPACE_AUTOSLEEP)))) {
 		crng_reseed();
 		pr_notice("crng reseeded on system resumption\n");
 	}
diff --git a/drivers/net/wireguard/device.c b/drivers/net/wireguard/device.c
index aa9a7a5970fd..b93171f2e6c9 100644
--- a/drivers/net/wireguard/device.c
+++ b/drivers/net/wireguard/device.c
@@ -69,7 +69,7 @@ static int wg_pm_notification(struct notifier_block *nb, unsigned long action, v
 	 * its normal operation rather than as a somewhat rare event, then we
 	 * don't actually want to clear keys.
 	 */
-	if (IS_ENABLED(CONFIG_PM_AUTOSLEEP) || IS_ENABLED(CONFIG_ANDROID))
+	if (IS_ENABLED(CONFIG_PM_AUTOSLEEP) || IS_ENABLED(CONFIG_PM_RAPID_USERSPACE_AUTOSLEEP))
 		return 0;

 	if (action != PM_HIBERNATION_PREPARE && action != PM_SUSPEND_PREPARE)
diff --git a/kernel/power/Kconfig b/kernel/power/Kconfig
index a12779650f15..bcbfbeb39d4f 100644
--- a/kernel/power/Kconfig
+++ b/kernel/power/Kconfig
@@ -150,6 +150,25 @@ config PM_WAKELOCKS
 	Allow user space to create, activate and deactivate wakeup source
 	objects with the help of a sysfs-based interface.

+config PM_RAPID_USERSPACE_AUTOSLEEP
+	bool "Tune for rapid and consistent userspace calls to sleep"
+	depends on PM_SLEEP
+	help
+	Change the behavior of various sleep-sensitive code to deal with
+	userspace autosuspend daemons that put the machine to sleep and wake it
+	up extremely often and for short periods of time.
+
+	This option mostly disables code paths that most users really should
+	keep enabled. In particular, only enable this if:
+
+	- It is very common to be asleep for only 2 seconds before being woken;	and
+	- It is very common to be awake for only 2 seconds before sleeping.
+
+	This likely only applies to Android devices, and not other machines.
+	Therefore, you should say N here, unless you're extremely certain that
+	this is what you want. The option otherwise has bad, undesirable
+	effects, and should not be enabled just for fun.
+
 config PM_WAKELOCKS_LIMIT
 	int "Maximum number of user space wakeup sources (0 = no limit)"
 	range 0 100000

Powered by blists - more mailing lists