Re: [PATCH] freezer,sched: Rewrite core freezer logic

[Rafael J. Wysocki]

On Fri, Jun 11, 2021 at 10:47 AM Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:
>
>
> Rewrite the core freezer to behave better wrt thawing. By replacing
> PF_FROZEN with TASK_FROZEN, a special block state, it is ensured frozen
> tasks stay frozen until explicitly thawed and don't randomly wake up
> early, as is currently possible.
>
> As such, it does away with PF_FROZEN and PF_FREEZER_SKIP, freeing up
> two PF_flags (yay).
>
> The freezing was tested, and found good, using:
>
>   echo freezer > /sys/power/pm_test
>   echo mem > /sys/power/state
>
> Even while having a GDB session active.
>
> Another notable bit is in init/do_mounts_initrd.c; afaict that has been
> 'broken' for quite a while and is simply removed.
>
> Requested-by: Will Deacon <will@xxxxxxxxxx>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@xxxxxxxxxxxxx>

Overall, I like this and I've learned a couple of things from it.

Two comments below.

[cut]

> @@ -116,20 +174,8 @@ bool freeze_task(struct task_struct *p)
>  {
>         unsigned long flags;
>
> -       /*
> -        * This check can race with freezer_do_not_count, but worst case that
> -        * will result in an extra wakeup being sent to the task.  It does not
> -        * race with freezer_count(), the barriers in freezer_count() and
> -        * freezer_should_skip() ensure that either freezer_count() sees
> -        * freezing == true in try_to_freeze() and freezes, or
> -        * freezer_should_skip() sees !PF_FREEZE_SKIP and freezes the task
> -        * normally.
> -        */
> -       if (freezer_should_skip(p))
> -               return false;
> -
>         spin_lock_irqsave(&freezer_lock, flags);
> -       if (!freezing(p) || frozen(p)) {
> +       if (!freezing(p) || frozen(p) || __freeze_task(p)) {
>                 spin_unlock_irqrestore(&freezer_lock, flags);
>                 return false;
>         }
> @@ -137,7 +183,7 @@ bool freeze_task(struct task_struct *p)
>         if (!(p->flags & PF_KTHREAD))
>                 fake_signal_wake_up(p);
>         else
> -               wake_up_state(p, TASK_INTERRUPTIBLE);
> +               wake_up_state(p, TASK_INTERRUPTIBLE); // TASK_NORMAL ?!?

Yes, I think that using TASK_NORMAL here would make sense and I don't
see any drawbacks that may result from doing so.

>
>         spin_unlock_irqrestore(&freezer_lock, flags);
>         return true;
> @@ -148,8 +194,8 @@ void __thaw_task(struct task_struct *p)
>         unsigned long flags;
>
>         spin_lock_irqsave(&freezer_lock, flags);
> -       if (frozen(p))
> -               wake_up_process(p);
> +       WARN_ON_ONCE(freezing(p));
> +       wake_up_state(p, TASK_FROZEN);
>         spin_unlock_irqrestore(&freezer_lock, flags);
>  }
>
> --- a/kernel/futex.c
> +++ b/kernel/futex.c
> @@ -2582,7 +2582,7 @@ static void futex_wait_queue_me(struct f
>          * queue_me() calls spin_unlock() upon completion, both serializing
>          * access to the hash list and forcing another memory barrier.
>          */
> -       set_current_state(TASK_INTERRUPTIBLE);
> +       set_current_state(TASK_INTERRUPTIBLE|TASK_FREEZABLE);
>         queue_me(q, hb);
>
>         /* Arm the timer */
> @@ -2600,7 +2600,7 @@ static void futex_wait_queue_me(struct f
>                  * is no timeout, or if it has yet to expire.
>                  */
>                 if (!timeout || timeout->task)
> -                       freezable_schedule();
> +                       schedule();
>         }
>         __set_current_state(TASK_RUNNING);
>  }
> --- a/kernel/hung_task.c
> +++ b/kernel/hung_task.c
> @@ -92,8 +92,8 @@ static void check_hung_task(struct task_
>          * Ensure the task is not frozen.
>          * Also, skip vfork and any other user process that freezer should skip.
>          */
> -       if (unlikely(t->flags & (PF_FROZEN | PF_FREEZER_SKIP)))
> -           return;
> +       if (unlikely(t->state & (TASK_FREEZABLE | TASK_FROZEN)))
> +               return;
>
>         /*
>          * When a freshly created task is scheduled once, changes its state to
> --- a/kernel/power/main.c
> +++ b/kernel/power/main.c
> @@ -23,7 +23,8 @@
>
>  void lock_system_sleep(void)
>  {
> -       current->flags |= PF_FREEZER_SKIP;
> +       WARN_ON_ONCE(current->flags & PF_NOFREEZE);
> +       current->flags |= PF_NOFREEZE;

Because khreadd() sets PF_NOFREEZE for all kernel threads by default
and set_freezable() is called by a limited number of them, the
WARN_ON_ONCE() here is likely to trigger if any kernel thread that is
not freezable (which is the default) attempts to call this function.

This was the original reason why PF_FREEZER_SKIP was added as a
separate flag IIRC.

>         mutex_lock(&system_transition_mutex);
>  }
>  EXPORT_SYMBOL_GPL(lock_system_sleep);
> @@ -46,7 +47,7 @@ void unlock_system_sleep(void)
>          * Which means, if we use try_to_freeze() here, it would make them
>          * enter the refrigerator, thus causing hibernation to lockup.
>          */
> -       current->flags &= ~PF_FREEZER_SKIP;
> +       current->flags &= ~PF_NOFREEZE;
>         mutex_unlock(&system_transition_mutex);
>  }
>  EXPORT_SYMBOL_GPL(unlock_system_sleep);