Re: [patch -mm] mm, oom: move oom notifiers to page allocator

[Michal Hocko <mhocko@kernel.org>]

On Wed 15-07-15 15:44:36, David Rientjes wrote:
> OOM notifiers exist to give one last chance at reclaiming memory before
> the oom killer does its work.
> 
> Thus, they don't actually belong in the oom killer proper, but rather in
> the page allocator where reclaim is invoked.

Why this is not needed in the page fault oom path anymore?

> Move the oom notifiers to their proper place: before out_of_memory(),
> which now deals solely with providing access to memory reserves and
> ensuring a process is exiting to free its memory.
> 
> This also fixes an issue that invoked the oom notifiers and aborted oom
> kill when triggered manually with sysrq+f.  Sysrq+f now properly triggers
> an oom kill in all instances.
> 
> Such callbacks should use register_shrinker() so they are a part of
> reclaim, and there should be no need for oom notifiers at all.  Thus,
> add a new comment directed to reclaim rather than continuing to use this
> interface.

I do not see anybody from the subsystems using oom notifiers CCed here
for their opinion.

I guess Reported-by could have been preserved

> Signed-off-by: David Rientjes <rientjes@google.com>
> ---
>  mm/oom_kill.c   | 20 --------------------
>  mm/page_alloc.c | 22 ++++++++++++++++++++++
>  2 files changed, 22 insertions(+), 20 deletions(-)
> 
> diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> --- a/mm/oom_kill.c
> +++ b/mm/oom_kill.c
> @@ -615,20 +615,6 @@ void check_panic_on_oom(struct oom_control *oc, enum oom_constraint constraint,
>  		sysctl_panic_on_oom == 2 ? "compulsory" : "system-wide");
>  }
>  
> -static BLOCKING_NOTIFIER_HEAD(oom_notify_list);
> -
> -int register_oom_notifier(struct notifier_block *nb)
> -{
> -	return blocking_notifier_chain_register(&oom_notify_list, nb);
> -}
> -EXPORT_SYMBOL_GPL(register_oom_notifier);
> -
> -int unregister_oom_notifier(struct notifier_block *nb)
> -{
> -	return blocking_notifier_chain_unregister(&oom_notify_list, nb);
> -}
> -EXPORT_SYMBOL_GPL(unregister_oom_notifier);
> -
>  /**
>   * out_of_memory - kill the "best" process when we run out of memory
>   * @oc: pointer to struct oom_control
> @@ -642,18 +628,12 @@ bool out_of_memory(struct oom_control *oc)
>  {
>  	struct task_struct *p;
>  	unsigned long totalpages;
> -	unsigned long freed = 0;
>  	unsigned int uninitialized_var(points);
>  	enum oom_constraint constraint = CONSTRAINT_NONE;
>  
>  	if (oom_killer_disabled)
>  		return false;
>  
> -	blocking_notifier_call_chain(&oom_notify_list, 0, &freed);
> -	if (freed > 0)
> -		/* Got some memory back in the last second. */
> -		return true;
> -
>  	/*
>  	 * If current has a pending SIGKILL or is exiting, then automatically
>  	 * select it.  The goal is to allow it to allocate so that it may
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -2676,6 +2676,23 @@ void warn_alloc_failed(gfp_t gfp_mask, int order, const char *fmt, ...)
>  		show_mem(filter);
>  }
>  
> +static BLOCKING_NOTIFIER_HEAD(oom_notify_list);
> +/*
> + * Deprecated -- no new callers of this interface should be added.  Instead,
> + * use reclaim shrinkers: see register_shrinker().
> + */
> +int register_oom_notifier(struct notifier_block *nb)
> +{
> +	return blocking_notifier_chain_register(&oom_notify_list, nb);
> +}
> +EXPORT_SYMBOL_GPL(register_oom_notifier);
> +
> +int unregister_oom_notifier(struct notifier_block *nb)
> +{
> +	return blocking_notifier_chain_unregister(&oom_notify_list, nb);
> +}
> +EXPORT_SYMBOL_GPL(unregister_oom_notifier);
> +
>  static inline struct page *
>  __alloc_pages_may_oom(gfp_t gfp_mask, unsigned int order,
>  	const struct alloc_context *ac, unsigned long *did_some_progress)
> @@ -2736,6 +2753,11 @@ __alloc_pages_may_oom(gfp_t gfp_mask, unsigned int order,
>  		if (gfp_mask & __GFP_THISNODE)
>  			goto out;
>  	}
> +
> +	blocking_notifier_call_chain(&oom_notify_list, 0, did_some_progress);
> +	if (*did_some_progress > 0)
> +		goto out;
> +
>  	/* Exhausted what can be done so it's blamo time */
>  	if (out_of_memory(&oc) || WARN_ON_ONCE(gfp_mask & __GFP_NOFAIL))
>  		*did_some_progress = 1;

-- 
Michal Hocko
SUSE Labs