Re: [PATCH v2 1/2] alarmtimer: Create alarmtimer sysfs to make duration of kernel suspend check configurable

[John Stultz]

On Thu, Feb 8, 2024 at 11:56 AM Pranav Prasad <pranavpp@xxxxxxxxxx> 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@xxxxxxxxxx>

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[] = "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 = 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