mm, oom: Tolerate processes sharing mm with different view of oom_score_adj.
diff mbox series

Message ID 1547636121-9229-1-git-send-email-penguin-kernel@I-love.SAKURA.ne.jp
State New
Headers show
Series
  • mm, oom: Tolerate processes sharing mm with different view of oom_score_adj.
Related show

Commit Message

Tetsuo Handa Jan. 16, 2019, 10:55 a.m. UTC
This patch reverts both commit 44a70adec910d692 ("mm, oom_adj: make sure
processes sharing mm have same view of oom_score_adj") and commit
97fd49c2355ffded ("mm, oom: kill all tasks sharing the mm") in order to
close a race and reduce the latency at __set_oom_adj(), and reduces the
warning at __oom_kill_process() in order to minimize the latency.

Commit 36324a990cf578b5 ("oom: clear TIF_MEMDIE after oom_reaper managed
to unmap the address space") introduced the worst case mentioned in
44a70adec910d692. But since the OOM killer skips mm with MMF_OOM_SKIP set,
only administrators can trigger the worst case.

Since 44a70adec910d692 did not take latency into account, we can hold RCU
for minutes and trigger RCU stall warnings by calling printk() on many
thousands of thread groups. Even without calling printk(), the latency is
mentioned by Yong-Taek Lee [1]. And I noticed that 44a70adec910d692 is
racy, and trying to fix the race will require a global lock which is too
costly for rare events.

If the worst case in 44a70adec910d692 happens, it is an administrator's
request. Therefore, tolerate the worst case and speed up __set_oom_adj().

[1] https://lkml.kernel.org/r/20181008011931epcms1p82dd01b7e5c067ea99946418bc97de46a@epcms1p8

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Reported-by: Yong-Taek Lee <ytk.lee@samsung.com>
---
 fs/proc/base.c     | 46 ----------------------------------------------
 include/linux/mm.h |  2 --
 mm/oom_kill.c      | 10 ++++++----
 3 files changed, 6 insertions(+), 52 deletions(-)

Comments

Michal Hocko Jan. 16, 2019, 11:09 a.m. UTC | #1
On Wed 16-01-19 19:55:21, Tetsuo Handa wrote:
> This patch reverts both commit 44a70adec910d692 ("mm, oom_adj: make sure
> processes sharing mm have same view of oom_score_adj") and commit
> 97fd49c2355ffded ("mm, oom: kill all tasks sharing the mm") in order to
> close a race and reduce the latency at __set_oom_adj(), and reduces the
> warning at __oom_kill_process() in order to minimize the latency.
> 
> Commit 36324a990cf578b5 ("oom: clear TIF_MEMDIE after oom_reaper managed
> to unmap the address space") introduced the worst case mentioned in
> 44a70adec910d692. But since the OOM killer skips mm with MMF_OOM_SKIP set,
> only administrators can trigger the worst case.
> 
> Since 44a70adec910d692 did not take latency into account, we can hold RCU
> for minutes and trigger RCU stall warnings by calling printk() on many
> thousands of thread groups. Even without calling printk(), the latency is
> mentioned by Yong-Taek Lee [1]. And I noticed that 44a70adec910d692 is
> racy, and trying to fix the race will require a global lock which is too
> costly for rare events.
> 
> If the worst case in 44a70adec910d692 happens, it is an administrator's
> request. Therefore, tolerate the worst case and speed up __set_oom_adj().

I really do not think we care about latency. I consider the overal API
sanity much more important. Besides that the original report you are
referring to was never exaplained/shown to represent real world usecase.
oom_score_adj is not really a an interface to be tweaked in hot paths.

I can be convinced otherwise but that really requires some _real_
usecase with an explanation why there is no other way. Until then

Nacked-by: Michal Hocko <mhocko@suse.com>

> 
> [1] https://lkml.kernel.org/r/20181008011931epcms1p82dd01b7e5c067ea99946418bc97de46a@epcms1p8
> 
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Reported-by: Yong-Taek Lee <ytk.lee@samsung.com>
> ---
>  fs/proc/base.c     | 46 ----------------------------------------------
>  include/linux/mm.h |  2 --
>  mm/oom_kill.c      | 10 ++++++----
>  3 files changed, 6 insertions(+), 52 deletions(-)
> 
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index 633a634..41ece8f 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -1020,7 +1020,6 @@ static ssize_t oom_adj_read(struct file *file, char __user *buf, size_t count,
>  static int __set_oom_adj(struct file *file, int oom_adj, bool legacy)
>  {
>  	static DEFINE_MUTEX(oom_adj_mutex);
> -	struct mm_struct *mm = NULL;
>  	struct task_struct *task;
>  	int err = 0;
>  
> @@ -1050,55 +1049,10 @@ static int __set_oom_adj(struct file *file, int oom_adj, bool legacy)
>  		}
>  	}
>  
> -	/*
> -	 * Make sure we will check other processes sharing the mm if this is
> -	 * not vfrok which wants its own oom_score_adj.
> -	 * pin the mm so it doesn't go away and get reused after task_unlock
> -	 */
> -	if (!task->vfork_done) {
> -		struct task_struct *p = find_lock_task_mm(task);
> -
> -		if (p) {
> -			if (atomic_read(&p->mm->mm_users) > 1) {
> -				mm = p->mm;
> -				mmgrab(mm);
> -			}
> -			task_unlock(p);
> -		}
> -	}
> -
>  	task->signal->oom_score_adj = oom_adj;
>  	if (!legacy && has_capability_noaudit(current, CAP_SYS_RESOURCE))
>  		task->signal->oom_score_adj_min = (short)oom_adj;
>  	trace_oom_score_adj_update(task);
> -
> -	if (mm) {
> -		struct task_struct *p;
> -
> -		rcu_read_lock();
> -		for_each_process(p) {
> -			if (same_thread_group(task, p))
> -				continue;
> -
> -			/* do not touch kernel threads or the global init */
> -			if (p->flags & PF_KTHREAD || is_global_init(p))
> -				continue;
> -
> -			task_lock(p);
> -			if (!p->vfork_done && process_shares_mm(p, mm)) {
> -				pr_info("updating oom_score_adj for %d (%s) from %d to %d because it shares mm with %d (%s). Report if this is unexpected.\n",
> -						task_pid_nr(p), p->comm,
> -						p->signal->oom_score_adj, oom_adj,
> -						task_pid_nr(task), task->comm);
> -				p->signal->oom_score_adj = oom_adj;
> -				if (!legacy && has_capability_noaudit(current, CAP_SYS_RESOURCE))
> -					p->signal->oom_score_adj_min = (short)oom_adj;
> -			}
> -			task_unlock(p);
> -		}
> -		rcu_read_unlock();
> -		mmdrop(mm);
> -	}
>  err_unlock:
>  	mutex_unlock(&oom_adj_mutex);
>  	put_task_struct(task);
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 80bb640..28879c1 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -2690,8 +2690,6 @@ static inline int in_gate_area(struct mm_struct *mm, unsigned long addr)
>  }
>  #endif	/* __HAVE_ARCH_GATE_AREA */
>  
> -extern bool process_shares_mm(struct task_struct *p, struct mm_struct *mm);
> -
>  #ifdef CONFIG_SYSCTL
>  extern int sysctl_drop_caches;
>  int drop_caches_sysctl_handler(struct ctl_table *, int,
> diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> index f0e8cd9..c7005b1 100644
> --- a/mm/oom_kill.c
> +++ b/mm/oom_kill.c
> @@ -478,7 +478,7 @@ static void dump_header(struct oom_control *oc, struct task_struct *p)
>   * task's threads: if one of those is using this mm then this task was also
>   * using it.
>   */
> -bool process_shares_mm(struct task_struct *p, struct mm_struct *mm)
> +static bool process_shares_mm(struct task_struct *p, struct mm_struct *mm)
>  {
>  	struct task_struct *t;
>  
> @@ -896,12 +896,14 @@ static void __oom_kill_process(struct task_struct *victim)
>  			continue;
>  		if (same_thread_group(p, victim))
>  			continue;
> -		if (is_global_init(p)) {
> +		if (is_global_init(p) ||
> +		    p->signal->oom_score_adj == OOM_SCORE_ADJ_MIN) {
>  			can_oom_reap = false;
> -			set_bit(MMF_OOM_SKIP, &mm->flags);
> -			pr_info("oom killer %d (%s) has mm pinned by %d (%s)\n",
> +			if (!test_bit(MMF_OOM_SKIP, &mm->flags))
> +				pr_info("oom killer %d (%s) has mm pinned by %d (%s)\n",
>  					task_pid_nr(victim), victim->comm,
>  					task_pid_nr(p), p->comm);
> +			set_bit(MMF_OOM_SKIP, &mm->flags);
>  			continue;
>  		}
>  		/*
> -- 
> 1.8.3.1
Tetsuo Handa Jan. 16, 2019, 11:30 a.m. UTC | #2
On 2019/01/16 20:09, Michal Hocko wrote:
> On Wed 16-01-19 19:55:21, Tetsuo Handa wrote:
>> This patch reverts both commit 44a70adec910d692 ("mm, oom_adj: make sure
>> processes sharing mm have same view of oom_score_adj") and commit
>> 97fd49c2355ffded ("mm, oom: kill all tasks sharing the mm") in order to
>> close a race and reduce the latency at __set_oom_adj(), and reduces the
>> warning at __oom_kill_process() in order to minimize the latency.
>>
>> Commit 36324a990cf578b5 ("oom: clear TIF_MEMDIE after oom_reaper managed
>> to unmap the address space") introduced the worst case mentioned in
>> 44a70adec910d692. But since the OOM killer skips mm with MMF_OOM_SKIP set,
>> only administrators can trigger the worst case.
>>
>> Since 44a70adec910d692 did not take latency into account, we can hold RCU
>> for minutes and trigger RCU stall warnings by calling printk() on many
>> thousands of thread groups. Even without calling printk(), the latency is
>> mentioned by Yong-Taek Lee [1]. And I noticed that 44a70adec910d692 is
>> racy, and trying to fix the race will require a global lock which is too
>> costly for rare events.
>>
>> If the worst case in 44a70adec910d692 happens, it is an administrator's
>> request. Therefore, tolerate the worst case and speed up __set_oom_adj().
> 
> I really do not think we care about latency. I consider the overal API
> sanity much more important. Besides that the original report you are
> referring to was never exaplained/shown to represent real world usecase.
> oom_score_adj is not really a an interface to be tweaked in hot paths.

I do care about the latency. Holding RCU for more than 2 minutes is insane.

----------
#define _GNU_SOURCE
#include <stdio.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <unistd.h>
#include <sched.h>
#include <sys/mman.h>
#include <signal.h>

#define STACKSIZE 8192
static int child(void *unused)
{
        pause();
        return 0;
}
int main(int argc, char *argv[])
{
        int fd = open("/proc/self/oom_score_adj", O_WRONLY);
        int i;
        char *stack = mmap(NULL, STACKSIZE, PROT_WRITE | PROT_READ, MAP_ANONYMOUS | MAP_PRIVATE, EOF, 0);
        for (i = 0; i < 8192 * 4; i++)
                if (clone(child, stack + STACKSIZE, CLONE_VM, NULL) == -1)
                        break;
        write(fd, "0\n", 2);
        kill(0, SIGSEGV);
        return 0;
}
----------

> 
> I can be convinced otherwise but that really requires some _real_
> usecase with an explanation why there is no other way. Until then
> 
> Nacked-by: Michal Hocko <mhocko@suse.com>
Michal Hocko Jan. 16, 2019, 12:19 p.m. UTC | #3
On Wed 16-01-19 20:30:25, Tetsuo Handa wrote:
> On 2019/01/16 20:09, Michal Hocko wrote:
> > On Wed 16-01-19 19:55:21, Tetsuo Handa wrote:
> >> This patch reverts both commit 44a70adec910d692 ("mm, oom_adj: make sure
> >> processes sharing mm have same view of oom_score_adj") and commit
> >> 97fd49c2355ffded ("mm, oom: kill all tasks sharing the mm") in order to
> >> close a race and reduce the latency at __set_oom_adj(), and reduces the
> >> warning at __oom_kill_process() in order to minimize the latency.
> >>
> >> Commit 36324a990cf578b5 ("oom: clear TIF_MEMDIE after oom_reaper managed
> >> to unmap the address space") introduced the worst case mentioned in
> >> 44a70adec910d692. But since the OOM killer skips mm with MMF_OOM_SKIP set,
> >> only administrators can trigger the worst case.
> >>
> >> Since 44a70adec910d692 did not take latency into account, we can hold RCU
> >> for minutes and trigger RCU stall warnings by calling printk() on many
> >> thousands of thread groups. Even without calling printk(), the latency is
> >> mentioned by Yong-Taek Lee [1]. And I noticed that 44a70adec910d692 is
> >> racy, and trying to fix the race will require a global lock which is too
> >> costly for rare events.
> >>
> >> If the worst case in 44a70adec910d692 happens, it is an administrator's
> >> request. Therefore, tolerate the worst case and speed up __set_oom_adj().
> > 
> > I really do not think we care about latency. I consider the overal API
> > sanity much more important. Besides that the original report you are
> > referring to was never exaplained/shown to represent real world usecase.
> > oom_score_adj is not really a an interface to be tweaked in hot paths.
> 
> I do care about the latency. Holding RCU for more than 2 minutes is insane.

Creating 8k threads could be considered insane as well. But more
seriously. I absolutely do not insist on holding a single RCU section
for the whole operation. But that doesn't really mean that we want to
revert these changes. for_each_process is by far not only called from
this path.
Tetsuo Handa Jan. 16, 2019, 1:32 p.m. UTC | #4
On 2019/01/16 21:19, Michal Hocko wrote:
> On Wed 16-01-19 20:30:25, Tetsuo Handa wrote:
>> On 2019/01/16 20:09, Michal Hocko wrote:
>>> On Wed 16-01-19 19:55:21, Tetsuo Handa wrote:
>>>> This patch reverts both commit 44a70adec910d692 ("mm, oom_adj: make sure
>>>> processes sharing mm have same view of oom_score_adj") and commit
>>>> 97fd49c2355ffded ("mm, oom: kill all tasks sharing the mm") in order to
>>>> close a race and reduce the latency at __set_oom_adj(), and reduces the
>>>> warning at __oom_kill_process() in order to minimize the latency.
>>>>
>>>> Commit 36324a990cf578b5 ("oom: clear TIF_MEMDIE after oom_reaper managed
>>>> to unmap the address space") introduced the worst case mentioned in
>>>> 44a70adec910d692. But since the OOM killer skips mm with MMF_OOM_SKIP set,
>>>> only administrators can trigger the worst case.
>>>>
>>>> Since 44a70adec910d692 did not take latency into account, we can hold RCU
>>>> for minutes and trigger RCU stall warnings by calling printk() on many
>>>> thousands of thread groups. Even without calling printk(), the latency is
>>>> mentioned by Yong-Taek Lee [1]. And I noticed that 44a70adec910d692 is
>>>> racy, and trying to fix the race will require a global lock which is too
>>>> costly for rare events.
>>>>
>>>> If the worst case in 44a70adec910d692 happens, it is an administrator's
>>>> request. Therefore, tolerate the worst case and speed up __set_oom_adj().
>>>
>>> I really do not think we care about latency. I consider the overal API
>>> sanity much more important. Besides that the original report you are
>>> referring to was never exaplained/shown to represent real world usecase.
>>> oom_score_adj is not really a an interface to be tweaked in hot paths.
>>
>> I do care about the latency. Holding RCU for more than 2 minutes is insane.
> 
> Creating 8k threads could be considered insane as well. But more
> seriously. I absolutely do not insist on holding a single RCU section
> for the whole operation. But that doesn't really mean that we want to
> revert these changes. for_each_process is by far not only called from
> this path.

Unlike check_hung_uninterruptible_tasks() where failing to resume after
breaking RCU section is tolerable, failing to resume after breaking RCU
section for __set_oom_adj() is not tolerable; it leaves the possibility
of different oom_score_adj. Unless it is inevitable (e.g. SysRq-t), I think
that calling printk() on each thread from RCU section is a poor choice.

What if thousands of threads concurrently called __set_oom_adj() when
each __set_oom_adj() call involves printk() on thousands of threads
which can take more than 2 minutes? How long will it take to complete?
Michal Hocko Jan. 16, 2019, 1:41 p.m. UTC | #5
On Wed 16-01-19 22:32:50, Tetsuo Handa wrote:
> On 2019/01/16 21:19, Michal Hocko wrote:
> > On Wed 16-01-19 20:30:25, Tetsuo Handa wrote:
> >> On 2019/01/16 20:09, Michal Hocko wrote:
> >>> On Wed 16-01-19 19:55:21, Tetsuo Handa wrote:
> >>>> This patch reverts both commit 44a70adec910d692 ("mm, oom_adj: make sure
> >>>> processes sharing mm have same view of oom_score_adj") and commit
> >>>> 97fd49c2355ffded ("mm, oom: kill all tasks sharing the mm") in order to
> >>>> close a race and reduce the latency at __set_oom_adj(), and reduces the
> >>>> warning at __oom_kill_process() in order to minimize the latency.
> >>>>
> >>>> Commit 36324a990cf578b5 ("oom: clear TIF_MEMDIE after oom_reaper managed
> >>>> to unmap the address space") introduced the worst case mentioned in
> >>>> 44a70adec910d692. But since the OOM killer skips mm with MMF_OOM_SKIP set,
> >>>> only administrators can trigger the worst case.
> >>>>
> >>>> Since 44a70adec910d692 did not take latency into account, we can hold RCU
> >>>> for minutes and trigger RCU stall warnings by calling printk() on many
> >>>> thousands of thread groups. Even without calling printk(), the latency is
> >>>> mentioned by Yong-Taek Lee [1]. And I noticed that 44a70adec910d692 is
> >>>> racy, and trying to fix the race will require a global lock which is too
> >>>> costly for rare events.
> >>>>
> >>>> If the worst case in 44a70adec910d692 happens, it is an administrator's
> >>>> request. Therefore, tolerate the worst case and speed up __set_oom_adj().
> >>>
> >>> I really do not think we care about latency. I consider the overal API
> >>> sanity much more important. Besides that the original report you are
> >>> referring to was never exaplained/shown to represent real world usecase.
> >>> oom_score_adj is not really a an interface to be tweaked in hot paths.
> >>
> >> I do care about the latency. Holding RCU for more than 2 minutes is insane.
> > 
> > Creating 8k threads could be considered insane as well. But more
> > seriously. I absolutely do not insist on holding a single RCU section
> > for the whole operation. But that doesn't really mean that we want to
> > revert these changes. for_each_process is by far not only called from
> > this path.
> 
> Unlike check_hung_uninterruptible_tasks() where failing to resume after
> breaking RCU section is tolerable, failing to resume after breaking RCU
> section for __set_oom_adj() is not tolerable; it leaves the possibility
> of different oom_score_adj.

Then make sure that no threads are really missed. Really I fail to see
what you are actually arguing about. for_each_process is expensive. No
question about that. If you can replace it for this specific and odd
usecase then go ahead. But there is absolutely zero reason to have a
broken oom_score_adj semantic just because somebody might have thousands
of threads and want to update the score faster.

> Unless it is inevitable (e.g. SysRq-t), I think
> that calling printk() on each thread from RCU section is a poor choice.
> 
> What if thousands of threads concurrently called __set_oom_adj() when
> each __set_oom_adj() call involves printk() on thousands of threads
> which can take more than 2 minutes? How long will it take to complete?

I really do not mind removing printk if that is what really bothers
users. The primary purpose of this printk was to catch users who
wouldn't expect this change. There were exactly zero.
Tetsuo Handa Jan. 17, 2019, 10:40 a.m. UTC | #6
On 2019/01/16 22:41, Michal Hocko wrote:
>>>> I do care about the latency. Holding RCU for more than 2 minutes is insane.
>>>
>>> Creating 8k threads could be considered insane as well. But more
>>> seriously. I absolutely do not insist on holding a single RCU section
>>> for the whole operation. But that doesn't really mean that we want to
>>> revert these changes. for_each_process is by far not only called from
>>> this path.
>>
>> Unlike check_hung_uninterruptible_tasks() where failing to resume after
>> breaking RCU section is tolerable, failing to resume after breaking RCU
>> section for __set_oom_adj() is not tolerable; it leaves the possibility
>> of different oom_score_adj.
> 
> Then make sure that no threads are really missed. Really I fail to see
> what you are actually arguing about.

Impossible unless we hold the global rw_semaphore for read during
copy_process()/do_exit() while hold the global rw_semaphore for write
during __set_oom_adj(). We won't accept such giant lock in order to close
the __set_oom_adj() race.

>                                      for_each_process is expensive. No
> question about that.

I'm saying that printk() is far more expensive. Current __set_oom_adj() code
allows wasting CPU by printing pointless message

  [ 1270.265958][ T8549] updating oom_score_adj for 30876 (a.out) from 0 to 0 because it shares mm with 8549 (a.out). Report if this is unexpected.
  [ 1270.265959][ T8549] updating oom_score_adj for 30877 (a.out) from 0 to 0 because it shares mm with 8549 (a.out). Report if this is unexpected.
  [ 1270.265961][ T8549] updating oom_score_adj for 30878 (a.out) from 0 to 0 because it shares mm with 8549 (a.out). Report if this is unexpected.
  [ 1270.265964][ T8549] updating oom_score_adj for 30879 (a.out) from 0 to 0 because it shares mm with 8549 (a.out). Report if this is unexpected.
  [ 1270.389516][ T8549] updating oom_score_adj for 30880 (a.out) from 0 to 0 because it shares mm with 8549 (a.out). Report if this is unexpected.
  [ 1270.395223][ T8549] updating oom_score_adj for 30881 (a.out) from 0 to 0 because it shares mm with 8549 (a.out). Report if this is unexpected.
  [ 1270.400871][ T8549] updating oom_score_adj for 30882 (a.out) from 0 to 0 because it shares mm with 8549 (a.out). Report if this is unexpected.
  [ 1270.406757][ T8549] updating oom_score_adj for 30883 (a.out) from 0 to 0 because it shares mm with 8549 (a.out). Report if this is unexpected.
  [ 1270.412509][ T8549] updating oom_score_adj for 30884 (a.out) from 0 to 0 because it shares mm with 8549 (a.out). Report if this is unexpected.

for _longer than one month_ ('2 minutes for one __set_oom_adj() call' x '32000
thread groups concurrently do "echo 0 > /proc/self/oom_score_adj"' = 44 days
to complete). This is nothing but a DoS attack vector.

>                      If you can replace it for this specific and odd
> usecase then go ahead. But there is absolutely zero reason to have a
> broken oom_score_adj semantic just because somebody might have thousands
> of threads and want to update the score faster.
> 
>> Unless it is inevitable (e.g. SysRq-t), I think
>> that calling printk() on each thread from RCU section is a poor choice.
>>
>> What if thousands of threads concurrently called __set_oom_adj() when
>> each __set_oom_adj() call involves printk() on thousands of threads
>> which can take more than 2 minutes? How long will it take to complete?
> 
> I really do not mind removing printk if that is what really bothers
> users. The primary purpose of this printk was to catch users who
> wouldn't expect this change. There were exactly zero.
> 

This printk() is pointless. There is no need to flood like above. Once is enough.
What is bad, it says "Report if this is unexpected." rather than "Report if you saw
this message.". If the user thinks that 'Oh, what a nifty caretaker. I need to do
"echo 0 > /proc/self/oom_score_adj" for only once.', that user won't report it.

And I estimate that we will need to wait for several more years to make sure that
all users upgrade their kernels to Linux 4.8+ which has __set_oom_adj() code. So far
"exactly zero" does not mean "changing oom_score_adj semantics is allowable". (But
so far "exactly zero" might suggest that there is absolutely no "CLONE_VM without
CLONE_SIGNAHD" user at all and thus preserving __oom_score_adj() code makes no sense.)



Given that said, I think that querying "CLONE_VM without CLONE_SIGNAHD" users at
copy_process() for the reason of that combination can improve the code.

--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1732,6 +1732,15 @@ static __latent_entropy struct task_struct *copy_process(
 	}
 
 	/*
+	 * Shared VM without signal handlers leads to complicated OOM-killer
+	 * handling. Let's ask such users why they want such combination.
+	 */
+	if ((clone_flags & CLONE_VM) && !(clone_flags & CLONE_SIGHAND))
+		pr_warn_once("***** %s(%d) is trying to create a thread sharing memory without signal handlers. Please be sure to report to linux-mm@kvack.org the reason why you want to use this combination. Otherwise, this combination will be forbidden in future kernels in order to simplify OOM-killer handling. *****\n",
+			     current->comm, task_pid_nr(current));
+
+
+	/*
 	 * Force any signals received before this point to be delivered
 	 * before the fork happens.  Collect up signals sent to multiple
 	 * processes that happen during the fork and delay them so that

If we waited enough period and there is no user, we can forbid that combination
and eliminate OOM handling code for "CLONE_VM without CLONE_SIGNAHD" which is
forcing current __set_oom_adj() code.
Michal Hocko Jan. 17, 2019, 3:51 p.m. UTC | #7
[I am mostly offline for the rest of the week]

On Wed 16-01-19 14:41:31, Michal Hocko wrote:
> On Wed 16-01-19 22:32:50, Tetsuo Handa wrote:
> > On 2019/01/16 21:19, Michal Hocko wrote:
> > > On Wed 16-01-19 20:30:25, Tetsuo Handa wrote:
> > >> On 2019/01/16 20:09, Michal Hocko wrote:
> > >>> On Wed 16-01-19 19:55:21, Tetsuo Handa wrote:
> > >>>> This patch reverts both commit 44a70adec910d692 ("mm, oom_adj: make sure
> > >>>> processes sharing mm have same view of oom_score_adj") and commit
> > >>>> 97fd49c2355ffded ("mm, oom: kill all tasks sharing the mm") in order to
> > >>>> close a race and reduce the latency at __set_oom_adj(), and reduces the
> > >>>> warning at __oom_kill_process() in order to minimize the latency.
> > >>>>
> > >>>> Commit 36324a990cf578b5 ("oom: clear TIF_MEMDIE after oom_reaper managed
> > >>>> to unmap the address space") introduced the worst case mentioned in
> > >>>> 44a70adec910d692. But since the OOM killer skips mm with MMF_OOM_SKIP set,
> > >>>> only administrators can trigger the worst case.
> > >>>>
> > >>>> Since 44a70adec910d692 did not take latency into account, we can hold RCU
> > >>>> for minutes and trigger RCU stall warnings by calling printk() on many
> > >>>> thousands of thread groups. Even without calling printk(), the latency is
> > >>>> mentioned by Yong-Taek Lee [1]. And I noticed that 44a70adec910d692 is
> > >>>> racy, and trying to fix the race will require a global lock which is too
> > >>>> costly for rare events.
> > >>>>
> > >>>> If the worst case in 44a70adec910d692 happens, it is an administrator's
> > >>>> request. Therefore, tolerate the worst case and speed up __set_oom_adj().
> > >>>
> > >>> I really do not think we care about latency. I consider the overal API
> > >>> sanity much more important. Besides that the original report you are
> > >>> referring to was never exaplained/shown to represent real world usecase.
> > >>> oom_score_adj is not really a an interface to be tweaked in hot paths.
> > >>
> > >> I do care about the latency. Holding RCU for more than 2 minutes is insane.
> > > 
> > > Creating 8k threads could be considered insane as well. But more
> > > seriously. I absolutely do not insist on holding a single RCU section
> > > for the whole operation. But that doesn't really mean that we want to
> > > revert these changes. for_each_process is by far not only called from
> > > this path.
> > 
> > Unlike check_hung_uninterruptible_tasks() where failing to resume after
> > breaking RCU section is tolerable, failing to resume after breaking RCU
> > section for __set_oom_adj() is not tolerable; it leaves the possibility
> > of different oom_score_adj.
> 
> Then make sure that no threads are really missed. Really I fail to see
> what you are actually arguing about. for_each_process is expensive. No
> question about that. If you can replace it for this specific and odd
> usecase then go ahead. But there is absolutely zero reason to have a
> broken oom_score_adj semantic just because somebody might have thousands
> of threads and want to update the score faster.

Btw. the current implementation annoyance is caused by the fact
that the oom_score_adj is per signal_struct rather than mm_struct. The
reason is that we really need:
	if (!vfork()) {
		set_oom_score_adj()
		exec()
	}
to work properly. One way around that is to special case oom_score_adj
for tasks in vfork and store their shadow value into the task_struct.
The shadow value would get transfered over to the mm struct once a new
one is allocated. So something very coarsly like

short tsk_get_oom_score_adj(struct task_struct *tsk)
{
	if (tsk->oom_score_adj != OOM_SCORE_ADJ_INVALID)
		return tsk->oom_score_adj;

	return tsk->signal->oom_score_adj;
}

use this helper instead of direct oom_score_adj usage. Then we need to
special case the setting in __set_oom_adj and dup_mm to copy the value
over instead of copy_signal.

I think this is doable.

Patch
diff mbox series

diff --git a/fs/proc/base.c b/fs/proc/base.c
index 633a634..41ece8f 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -1020,7 +1020,6 @@  static ssize_t oom_adj_read(struct file *file, char __user *buf, size_t count,
 static int __set_oom_adj(struct file *file, int oom_adj, bool legacy)
 {
 	static DEFINE_MUTEX(oom_adj_mutex);
-	struct mm_struct *mm = NULL;
 	struct task_struct *task;
 	int err = 0;
 
@@ -1050,55 +1049,10 @@  static int __set_oom_adj(struct file *file, int oom_adj, bool legacy)
 		}
 	}
 
-	/*
-	 * Make sure we will check other processes sharing the mm if this is
-	 * not vfrok which wants its own oom_score_adj.
-	 * pin the mm so it doesn't go away and get reused after task_unlock
-	 */
-	if (!task->vfork_done) {
-		struct task_struct *p = find_lock_task_mm(task);
-
-		if (p) {
-			if (atomic_read(&p->mm->mm_users) > 1) {
-				mm = p->mm;
-				mmgrab(mm);
-			}
-			task_unlock(p);
-		}
-	}
-
 	task->signal->oom_score_adj = oom_adj;
 	if (!legacy && has_capability_noaudit(current, CAP_SYS_RESOURCE))
 		task->signal->oom_score_adj_min = (short)oom_adj;
 	trace_oom_score_adj_update(task);
-
-	if (mm) {
-		struct task_struct *p;
-
-		rcu_read_lock();
-		for_each_process(p) {
-			if (same_thread_group(task, p))
-				continue;
-
-			/* do not touch kernel threads or the global init */
-			if (p->flags & PF_KTHREAD || is_global_init(p))
-				continue;
-
-			task_lock(p);
-			if (!p->vfork_done && process_shares_mm(p, mm)) {
-				pr_info("updating oom_score_adj for %d (%s) from %d to %d because it shares mm with %d (%s). Report if this is unexpected.\n",
-						task_pid_nr(p), p->comm,
-						p->signal->oom_score_adj, oom_adj,
-						task_pid_nr(task), task->comm);
-				p->signal->oom_score_adj = oom_adj;
-				if (!legacy && has_capability_noaudit(current, CAP_SYS_RESOURCE))
-					p->signal->oom_score_adj_min = (short)oom_adj;
-			}
-			task_unlock(p);
-		}
-		rcu_read_unlock();
-		mmdrop(mm);
-	}
 err_unlock:
 	mutex_unlock(&oom_adj_mutex);
 	put_task_struct(task);
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 80bb640..28879c1 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -2690,8 +2690,6 @@  static inline int in_gate_area(struct mm_struct *mm, unsigned long addr)
 }
 #endif	/* __HAVE_ARCH_GATE_AREA */
 
-extern bool process_shares_mm(struct task_struct *p, struct mm_struct *mm);
-
 #ifdef CONFIG_SYSCTL
 extern int sysctl_drop_caches;
 int drop_caches_sysctl_handler(struct ctl_table *, int,
diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index f0e8cd9..c7005b1 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -478,7 +478,7 @@  static void dump_header(struct oom_control *oc, struct task_struct *p)
  * task's threads: if one of those is using this mm then this task was also
  * using it.
  */
-bool process_shares_mm(struct task_struct *p, struct mm_struct *mm)
+static bool process_shares_mm(struct task_struct *p, struct mm_struct *mm)
 {
 	struct task_struct *t;
 
@@ -896,12 +896,14 @@  static void __oom_kill_process(struct task_struct *victim)
 			continue;
 		if (same_thread_group(p, victim))
 			continue;
-		if (is_global_init(p)) {
+		if (is_global_init(p) ||
+		    p->signal->oom_score_adj == OOM_SCORE_ADJ_MIN) {
 			can_oom_reap = false;
-			set_bit(MMF_OOM_SKIP, &mm->flags);
-			pr_info("oom killer %d (%s) has mm pinned by %d (%s)\n",
+			if (!test_bit(MMF_OOM_SKIP, &mm->flags))
+				pr_info("oom killer %d (%s) has mm pinned by %d (%s)\n",
 					task_pid_nr(victim), victim->comm,
 					task_pid_nr(p), p->comm);
+			set_bit(MMF_OOM_SKIP, &mm->flags);
 			continue;
 		}
 		/*