Re: [PATCH] mm: Mark the OOM reaper thread as freezable

[Michal Hocko]

On Sat 18-09-21 16:39:20, Sultan Alsawaf wrote:
> From: Sultan Alsawaf <sultan@xxxxxxxxxxxxxxx>
> 
> The OOM reaper thread uses wait_event_freezable() without actually being
> marked as freezable. Fix it by adding a set_freezable() call.

After the follow up discussion it is clear what the patch does and why
it is needed. The changelog really begs for some clarification. I would
propose something like

"
The OOM reaper kthread uses wait_event_freezable while it is waiting for
any work. It is safe to freeze it while waiting. We, however, need to
prevent any activity after global freezer quiescent state because the
oom reaping is altering address space and this might alter the snapshot
theoretically (please note that this is mostly a theoretical concern not
being observed in practice so far) so the freezer has to wait for an
explicit freezing.

The current implementation doesn't work that way though because all
kernel threads are created with PF_NOFREEZE flag so they are
automatically excluded from freezing operation. This means that
oom_reaper can race with the system snapshoting if it was processing
while the system is being frozen. Fix that by set_freezable which will
make the oom_reaper visible to the freezer.
"

> 
> Fixes: aac453635549 ("mm, oom: introduce oom reaper")
> Signed-off-by: Sultan Alsawaf <sultan@xxxxxxxxxxxxxxx>

With the above or otherwise improved changelog feel free to add
Acked-by: Michal Hocko <mhocko@xxxxxxxx>

Thanks!

> ---
>  mm/oom_kill.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> index 831340e7ad8b..46a742b57735 100644
> --- a/mm/oom_kill.c
> +++ b/mm/oom_kill.c
> @@ -641,6 +641,8 @@ static void oom_reap_task(struct task_struct *tsk)
>  
>  static int oom_reaper(void *unused)
>  {
> +	set_freezable();
> +
>  	while (true) {
>  		struct task_struct *tsk = NULL;
>  
> -- 
> 2.33.0

-- 
Michal Hocko
SUSE Labs