Re: [PATCH] mm/page_alloc: Wait for oom_lock before retrying.
From: Michal Hocko <mhocko@suse.com>
Date: 2016-12-08 13:32:33
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@I-love.SAKURA.ne.jp . Vlastimil Babka wrote:quoted
quoted
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. [...]
quoted hunk ↗ jump to hunk
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@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>