Re: [Patch v4 1/2] freezer: check OOM kill while being frozen

[Michal Hocko]

On Wed 10-09-14 15:24:17, Michal Hocko wrote:
> On Tue 09-09-14 22:53:32, Rafael J. Wysocki wrote:
> > On Wednesday, September 10, 2014 01:46:58 AM Tejun Heo wrote:
> > > Hello,
> > > 
> > > On Tue, Sep 09, 2014 at 06:06:25PM +0200, Michal Hocko wrote:
> > > > > Even for userland tasks, try_to_freeze() can currently be anywhere in
> > > > > the kernel.  The frequently used ones are few but there are some odd
> > > > 
> > > > I always thought that user space tasks can be in the fridge only on the
> > > > way out from the kernel (get_signal_to_deliver). I have quickly greped
> > > 
> > > It *can* be anywhere.  We used to have some deep in nfs.  They got
> > > removed later due to deadlocks but in theory they still can be
> > > anywhere.
> > 
> > Well, it would be good to determine the difference between theory and practice
> > in this particular respect, because if in practice it can't be anywhere,
> > we can just set an "every new instance of try_to_freeze() has to be documented"
> > rule (which may not be a bad idea anyway) and disallow people to break things.
> 
> What do you think about this way to help distinguish kernel threads and
> user processes and keep the future maintenance of freezer saner?

This is not enough. I have completely missed freezer_count... Will have
to think about this. There is quite large base of callers of this... It
won't be trivial to check whether all of them are safe. Especially after
467de1fc67d1b which made the functionality usable for !user tasks as
well. Btw. is this even desirable? The follow up patch (33e638b9070b)
have (ab)used freezer_{do_not_}count just to be reverted later by
72081624d5ad3. I haven't checked other kernel thread users which might be
added later. Don't rather put try_to_freeze_user_task into freezer_count
to reflect the original intention (assuming that a separate API for user
and kthread is desirable of course).

> ---
> From 0a28200a4439283753c10acabd1bcd7cc5989aae Mon Sep 17 00:00:00 2001
> From: Michal Hocko <mhocko@xxxxxxx>
> Date: Wed, 10 Sep 2014 11:04:40 +0200
> Subject: [PATCH] freezer: make freezing of user tasks and kernel threads clear
> 
> There are slightly different requirements for kernel threads and user
> tasks freezing. User tasks might get killed while they are in the fridge
> (e.g. by OOM killer) and we have to make sure that such a task will die
> as soon as possible. This means that try_to_freeze cannot be called from
> an arbitrary code path.
> 
> The obviously safe place is on the way out from the kernel. We have two
> such places currently, get_signal_to_deliver which is the main point
> for user tasks freezing called on the way to userspace and lguest which
> expedites return to the userspace as well when signals are pending.
> 
> Any new caller should be added with extreme caution and proper
> justification. This is quite hard right now because both kernel threads
> and user tasks context are calling the same function. This patch
> keeps the old try_to_freeze only for kernel threads and adds a new
> try_to_freeze_user_task which is aimed at user tasks. Both check process
> flag and warn about improper usage.
> 
> Signed-off-by: Michal Hocko <mhocko@xxxxxxx>
> ---
>  drivers/lguest/core.c   |  2 +-
>  include/linux/freezer.h | 28 +++++++++++++++++++++++++++-
>  kernel/signal.c         |  2 +-
>  3 files changed, 29 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/lguest/core.c b/drivers/lguest/core.c
> index 6590558d1d31..d8eb5ec8bcc7 100644
> --- a/drivers/lguest/core.c
> +++ b/drivers/lguest/core.c
> @@ -239,7 +239,7 @@ int run_guest(struct lg_cpu *cpu, unsigned long __user *user)
>  		 * thing called the freezer.  If the Host is trying to suspend,
>  		 * it stops us.
>  		 */
> -		try_to_freeze();
> +		try_to_freeze_user_task();
>  
>  		/* Check for signals */
>  		if (signal_pending(current))
> diff --git a/include/linux/freezer.h b/include/linux/freezer.h
> index 7fd81b8c4897..2683ad5fdfe8 100644
> --- a/include/linux/freezer.h
> +++ b/include/linux/freezer.h
> @@ -59,13 +59,39 @@ static inline bool try_to_freeze_unsafe(void)
>  	return __refrigerator(false);
>  }
>  
> -static inline bool try_to_freeze(void)
> +/* A helper function. Do not use directly. */
> +static inline bool __try_to_freeze(void)
>  {
>  	if (!(current->flags & PF_NOFREEZE))
>  		debug_check_no_locks_held();
>  	return try_to_freeze_unsafe();
>  }
>  
> +/*
> + * Only kernel threads are allowed to call try_to_freeze.
> + */
> +static inline bool try_to_freeze(void)
> +{
> +	WARN_ON(!(current->flags & PF_KTHREAD));
> +
> +	return __try_to_freeze();
> +}
> +
> +/*
> + * User tasks might get frozen only on specific places
> + * where we know they will get to user space or die on
> + * signal very quickly.
> + *
> + * Every new user of this function has to be consulted with
> + * PM/freezer maintainers.
> + */
> +static inline bool try_to_freeze_user_task(void)
> +{
> +	WARN_ON(current->flags & PF_KTHREAD);
> +
> +	return __try_to_freeze();
> +}
> +
>  extern bool freeze_task(struct task_struct *p);
>  extern bool set_freezable(void);
>  
> diff --git a/kernel/signal.c b/kernel/signal.c
> index a4077e90f19f..c1860564a74d 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -2184,7 +2184,7 @@ int get_signal_to_deliver(siginfo_t *info, struct k_sigaction *return_ka,
>  	 * do_signal_stop() and ptrace_stop() do freezable_schedule() and
>  	 * thus do not need another check after return.
>  	 */
> -	try_to_freeze();
> +	try_to_freeze_user_task();
>  
>  relock:
>  	spin_lock_irq(&sighand->siglock);
> -- 
> 2.1.0
> 
> -- 
> Michal Hocko
> SUSE Labs
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

-- 
Michal Hocko
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/