Re: [RFC][PATCH 2/2] time: alarmtimer: Use TASK_FREEZABLE to cleanup freezer handling

From: Michael Nazzareno Trimarchi
Date: Wed Feb 15 2023 - 03:23:14 EST


Hi John

On Sat, Feb 11, 2023 at 7:45 AM John Stultz <jstultz@xxxxxxxxxx> wrote:
>
> Instead of trying to handle the freezer waking up tasks from
> schedule() in nanosleep on alarmtimers explicitly, use
> TASK_FREEZABLE which marks the task freezable when it goes
> to schedule, which prevents the signal wakeup.
>
> This allows for the freezer handling to be removed, simplifying
> the code.
>
> Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> Cc: Stephen Boyd <sboyd@xxxxxxxxxx>
> Cc: Arnd Bergmann <arnd@xxxxxxxx>
> Cc: Michael <michael@xxxxxxxxx>
> Cc: Michael Trimarchi <michael@xxxxxxxxxxxxxxxxxxxx>
> Cc: kernel-team@xxxxxxxxxxx
> Originally-by: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> Link: https://lore.kernel.org/lkml/alpine.DEB.2.21.1909021247250.3955@xxxxxxxxxxxxxxxxxxxxxxx/
> [jstultz: Forward ported to 6.2-rc and split out from a separate
> fix.]
> Signed-off-by: John Stultz <jstultz@xxxxxxxxxx>
> ---
> kernel/time/alarmtimer.c | 53 ++--------------------------------------
> 1 file changed, 2 insertions(+), 51 deletions(-)
>
> diff --git a/kernel/time/alarmtimer.c b/kernel/time/alarmtimer.c
> index f7b2128f64e2..15ecde8fcc1b 100644
> --- a/kernel/time/alarmtimer.c
> +++ b/kernel/time/alarmtimer.c
> @@ -49,14 +49,6 @@ static struct alarm_base {
> clockid_t base_clockid;
> } alarm_bases[ALARM_NUMTYPE];
>
> -#if defined(CONFIG_POSIX_TIMERS) || defined(CONFIG_RTC_CLASS)
> -/* freezer information to handle clock_nanosleep triggered wakeups */
> -static enum alarmtimer_type freezer_alarmtype;
> -static ktime_t freezer_expires;
> -static ktime_t freezer_delta;
> -static DEFINE_SPINLOCK(freezer_delta_lock);
> -#endif
> -
> #ifdef CONFIG_RTC_CLASS
> /* rtc timer and device for setting alarm wakeups at suspend */
> static struct rtc_timer rtctimer;
> @@ -241,19 +233,12 @@ EXPORT_SYMBOL_GPL(alarm_expires_remaining);
> */
> static int alarmtimer_suspend(struct device *dev)
> {
> - ktime_t min, now, expires;
> + ktime_t now, expires, min = KTIME_MAX;
> int i, ret, type;
> struct rtc_device *rtc;
> unsigned long flags;
> struct rtc_time tm;
>
> - spin_lock_irqsave(&freezer_delta_lock, flags);
> - min = freezer_delta;
> - expires = freezer_expires;
> - type = freezer_alarmtype;
> - freezer_delta = KTIME_MAX;
> - spin_unlock_irqrestore(&freezer_delta_lock, flags);
> -
> rtc = alarmtimer_get_rtcdev();
> /* If we have no rtcdev, just return */
> if (!rtc)
> @@ -480,38 +465,6 @@ u64 alarm_forward_now(struct alarm *alarm, ktime_t interval)
> EXPORT_SYMBOL_GPL(alarm_forward_now);
>
> #ifdef CONFIG_POSIX_TIMERS
> -
> -static void alarmtimer_freezerset(ktime_t absexp, enum alarmtimer_type type)
> -{
> - struct alarm_base *base;
> - unsigned long flags;
> - ktime_t delta;
> -
> - switch(type) {
> - case ALARM_REALTIME:
> - base = &alarm_bases[ALARM_REALTIME];
> - type = ALARM_REALTIME_FREEZER;
> - break;
> - case ALARM_BOOTTIME:
> - base = &alarm_bases[ALARM_BOOTTIME];
> - type = ALARM_BOOTTIME_FREEZER;
> - break;
> - default:
> - WARN_ONCE(1, "Invalid alarm type: %d\n", type);
> - return;
> - }
> -
> - delta = ktime_sub(absexp, base->get_ktime());
> -
> - spin_lock_irqsave(&freezer_delta_lock, flags);
> - if (delta < freezer_delta) {
> - freezer_delta = delta;
> - freezer_expires = absexp;
> - freezer_alarmtype = type;
> - }
> - spin_unlock_irqrestore(&freezer_delta_lock, flags);
> -}
> -
> /**
> * clock2alarm - helper that converts from clockid to alarmtypes
> * @clockid: clockid.
> @@ -750,7 +703,7 @@ static int alarmtimer_do_nsleep(struct alarm *alarm, ktime_t absexp,
> struct restart_block *restart;
> alarm->data = (void *)current;
> do {
> - set_current_state(TASK_INTERRUPTIBLE);
> + set_current_state(TASK_INTERRUPTIBLE | TASK_FREEZABLE);

For kernel 5.10.x and lts is possible to use freezable_schedule and
let this set_current_state as was before.
I have seen patch that introduce the new state but I suppose that in
order to be compatible to stable this should be the
change. Am I right?

Michael

> alarm_start(alarm, absexp);
> if (likely(alarm->data))
> schedule();
> @@ -765,8 +718,6 @@ static int alarmtimer_do_nsleep(struct alarm *alarm, ktime_t absexp,
> if (!alarm->data)
> return 0;
>
> - if (freezing(current))
> - alarmtimer_freezerset(absexp, type);
> restart = &current->restart_block;
> if (restart->nanosleep.type != TT_NONE) {
> struct timespec64 rmt;
> --
> 2.39.1.581.gbfd45094c4-goog
>


--
Michael Nazzareno Trimarchi
Co-Founder & Chief Executive Officer
M. +39 347 913 2170
michael@xxxxxxxxxxxxxxxxxxxx
__________________________________

Amarula Solutions BV
Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
T. +31 (0)85 111 9172
info@xxxxxxxxxxxxxxxxxxxx
www.amarulasolutions.com