Received-SPF: pass (google.com: domain of linux-kernel+bounces-59913-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Precedence: bulk
MIME-Version: 1.0
References: <20240208195622.758765-1-pranavpp@google.com> <20240208195622.758765-2-pranavpp@google.com>
In-Reply-To: <20240208195622.758765-2-pranavpp@google.com>
From: John Stultz <jstultz@google.com>
Date: Fri, 9 Feb 2024 12:01:10 -0800
Message-ID: <CANDhNCovFCekU3wKYNMZWNgcr1=yyDnKNoehXe-x3Ep6-c58eg@mail.gmail.com>
Subject: Re: [PATCH v2 1/2] alarmtimer: Create alarmtimer sysfs to make
 duration of kernel suspend check configurable
To: Pranav Prasad <pranavpp@google.com>
Cc: tglx@linutronix.de, sboyd@kernel.org, linux-kernel@vger.kernel.org, 
	krossmo@google.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Thu, Feb 8, 2024 at 11:56=E2=80=AFAM Pranav Prasad <pranavpp@google.com>=
 wrote:
>
> Currently, the alarmtimer_suspend does not allow the kernel
> to suspend if the next alarm is within 2 seconds.
> Create alarmtimer sysfs to make the value of 2 seconds configurable.
> This allows flexibility to provide a different value based on the
> type of device running the Linux kernel. As a data point, about 40% of
> kernel suspend failures in a subset of Android devices were due to
> this check. A differently configured value can avoid these suspend
> failures which performs a lot of additional work affecting the
> power consumption of these Android devices.
>
> Signed-off-by: Pranav Prasad <pranavpp@google.com>

I might suggest flipping the order of these two patches, as I'm more
wary of UABI changes, so I don't want to hold up the second patch on
interface bike shedding.

> ---
>  kernel/time/alarmtimer.c | 61 ++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 58 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/time/alarmtimer.c b/kernel/time/alarmtimer.c
> index 4657cb8e8b1f..e4b88c8dc0e1 100644
> --- a/kernel/time/alarmtimer.c
> +++ b/kernel/time/alarmtimer.c
> @@ -33,6 +33,8 @@
>  #define CREATE_TRACE_POINTS
>  #include <trace/events/alarmtimer.h>
>
> +static const char alarmtimer_group_name[] =3D "alarmtimer";
> +
>  /**
>   * struct alarm_base - Alarm timer bases
>   * @lock:              Lock for syncrhonized access to the base
> @@ -63,6 +65,56 @@ static struct rtc_timer              rtctimer;
>  static struct rtc_device       *rtcdev;
>  static DEFINE_SPINLOCK(rtcdev_lock);
>
> +/* Duration to check for soonest alarm during kernel suspend */
> +static unsigned long suspend_check_duration_ms =3D 2 * MSEC_PER_SEC;

Naming is hard, but "suspend_check_duration" feels particularly opaque
for a tunable knob.
I can't say I've got a better suggestion off the top of my head, but
this might be something worth thinking a bit more on.

"imminent_alarm_window" maybe? Though that's not obvious it is
connected to suspend, and maybe sounds more urgent than it should.
"suspend_alarm_pending_window"?

It might also be nice to provide some more details in the commit
message about why this should be configurable, and how a user of the
interface might choose a proper value to use (including the downsides
of going too far in either direction?).

thanks
-john