On Thu 08-12-16 20:00:39, Tetsuo Handa wrote:
> Cc'ing people involved in commit dc56401fc9f25e8f ("mm: oom_kill: simplify
> OOM killer locking") and Sergey as printk() expert. Topic started from
> http://lkml.kernel.org/r/1481020439-5867-1-git-send-email-penguin-kernel@xxxxxxxxxxxxxxxxxxx .
>
> Vlastimil Babka wrote:
> > > May I? Something like below? With patch below, the OOM killer can send
> > > SIGKILL smoothly and printk() can report smoothly (the frequency of
> > > "** XXX printk messages dropped **" messages is significantly reduced).
> > >
> > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > > index 2c6d5f6..ee0105b 100644
> > > --- a/mm/page_alloc.c
> > > +++ b/mm/page_alloc.c
> > > @@ -3075,7 +3075,7 @@ void warn_alloc(gfp_t gfp_mask, const char *fmt, ...)
> > > * Acquire the oom lock. If that fails, somebody else is
> > > * making progress for us.
> > > */
> >
> > The comment above could use some updating then. Although maybe "somebody
> > killed us" is also technically "making progress for us" :)
>
> I think we can update the comment. But since __GFP_KILLABLE does not exist,
> SIGKILL is pending does not imply that current thread will make progress by
> leaving the retry loop immediately. Therefore,
Although this is true I do not think that cluttering the code with this
case is anyhow useful. In the vast majority of cases SIGKILL pending
will be a result of the oom killer.
[...]
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 6de9440..6c43d8e 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -3037,12 +3037,16 @@ void warn_alloc(gfp_t gfp_mask, const char *fmt, ...)
> *did_some_progress = 0;
>
> /*
> - * Acquire the oom lock. If that fails, somebody else is
> - * making progress for us.
> + * Give the OOM killer enough CPU time for sending SIGKILL.
> + * Do not return without a short sleep unless TIF_MEMDIE is set, for
> + * currently tsk_is_oom_victim(current) == true does not make
> + * gfp_pfmemalloc_allowed() == true via TIF_MEMDIE until
> + * mark_oom_victim(current) is called.
> */
> - if (!mutex_trylock(&oom_lock)) {
> + if (mutex_lock_killable(&oom_lock)) {
> *did_some_progress = 1;
> - schedule_timeout_uninterruptible(1);
> + if (!test_thread_flag(TIF_MEMDIE))
> + schedule_timeout_uninterruptible(1);
I am not really sure this is necessary. Just return outside and for
those unlikely cases where the current task was killed before entering
the page allocator simply do not matter imho. I would rather go with
simplicity here.
> return NULL;
> }
>
> --
> 1.8.3.1
--
Michal Hocko
SUSE Labs
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>