Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1236631 > unrolled thread

[PATCH -mm v2 1/3] mm/oom_kill: remove the wrong fatal_signal_pending() check in oom_kill_process()

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH -mm v2 1/3] mm/oom_kill: remove the wrong  fatal_signal_pending() check in oom_kill_process() Oleg Nesterov <oleg@redhat.com> - 2015-09-30 20:30 +0200
    Re: [PATCH -mm v2 1/3] mm/oom_kill: remove the wrong fatal_signal_pending()check in oom_kill_process() Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> - 2015-10-01 13:00 +0200
    Re: [PATCH -mm v2 1/3] mm/oom_kill: remove the wrong  fatal_signal_pending() check in oom_kill_process() Michal Hocko <mhocko@kernel.org> - 2015-10-01 14:50 +0200
      Re: [PATCH -mm v2 1/3] mm/oom_kill: remove the wrong  fatal_signal_pending() check in oom_kill_process() Oleg Nesterov <oleg@redhat.com> - 2015-10-01 17:10 +0200
        Re: [PATCH -mm v2 1/3] mm/oom_kill: remove the wrong  fatal_signal_pending() check in oom_kill_process() Michal Hocko <mhocko@kernel.org> - 2015-10-01 17:30 +0200
          Re: [PATCH -mm v2 1/3] mm/oom_kill: remove the wrong  fatal_signal_pending() check in oom_kill_process() Oleg Nesterov <oleg@redhat.com> - 2015-10-01 17:50 +0200
            Re: [PATCH -mm v2 1/3] mm/oom_kill: remove the wrong  fatal_signal_pending() check in oom_kill_process() Michal Hocko <mhocko@kernel.org> - 2015-10-01 18:20 +0200
              Re: [PATCH -mm v2 1/3] mm/oom_kill: remove the wrong  fatal_signal_pending() check in oom_kill_process() Oleg Nesterov <oleg@redhat.com> - 2015-10-01 20:00 +0200
                Re: [PATCH -mm v2 1/3] mm/oom_kill: remove the wrong fatal_signal_pending() check in oom_kill_process() Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> - 2015-10-02 13:40 +0200
                  Re: [PATCH -mm v2 1/3] mm/oom_kill: remove the wrong  fatal_signal_pending() check in oom_kill_process() Michal Hocko <mhocko@kernel.org> - 2015-10-02 14:20 +0200
                    Re: [PATCH -mm v2 1/3] mm/oom_kill: remove the wrong fatal_signal_pending() check in oom_kill_process() Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> - 2015-10-02 14:40 +0200
                      Re: [PATCH -mm v2 1/3] mm/oom_kill: remove the wrong  fatal_signal_pending() check in oom_kill_process() Michal Hocko <mhocko@kernel.org> - 2015-10-02 15:40 +0200
                        Re: [PATCH -mm v2 1/3] mm/oom_kill: remove the wrong fatal_signal_pending() check in oom_kill_process() Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> - 2015-10-02 16:10 +0200
                          Re: [PATCH -mm v2 1/3] mm/oom_kill: remove the wrong  fatal_signal_pending() check in oom_kill_process() Oleg Nesterov <oleg@redhat.com> - 2015-10-02 16:20 +0200
                        Re: [PATCH -mm v2 1/3] mm/oom_kill: remove the wrong  fatal_signal_pending() check in oom_kill_process() Oleg Nesterov <oleg@redhat.com> - 2015-10-02 16:10 +0200
                          Re: [PATCH -mm v2 1/3] mm/oom_kill: remove the wrong  fatal_signal_pending() check in oom_kill_process() Michal Hocko <mhocko@kernel.org> - 2015-10-02 16:30 +0200
                  Re: [PATCH -mm v2 1/3] mm/oom_kill: remove the wrong  fatal_signal_pending() check in oom_kill_process() Oleg Nesterov <oleg@redhat.com> - 2015-10-02 16:00 +0200
                    Re: [PATCH -mm v2 1/3] mm/oom_kill: remove the wrong fatal_signal_pending() check in oom_kill_process() Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> - 2015-10-02 16:40 +0200

#1236631 — [PATCH -mm v2 1/3] mm/oom_kill: remove the wrong fatal_signal_pending() check in oom_kill_process()

The fatal_signal_pending() was added to suppress unnecessary "sharing
same memory" message, but it can't 100% help anyway because it can be
false-negative; SIGKILL can be already dequeued.

And worse, it can be false-positive due to exec or coredump. exec is
mostly fine, but coredump is not. It is possible that the group leader
has the pending SIGKILL because its sub-thread originated the coredump,
in this case we must not skip this process.

We could probably add the additional ->group_exit_task check but this
pach just removes the wrong check along with pr_info().

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 mm/oom_kill.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index 4766e25..b6b8c78 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -588,11 +588,7 @@ void oom_kill_process(struct oom_control *oc, struct task_struct *p,
 		    !(p->flags & PF_KTHREAD)) {
 			if (p->signal->oom_score_adj == OOM_SCORE_ADJ_MIN)
 				continue;
-			if (fatal_signal_pending(p))
-				continue;
 
-			pr_info("Kill process %d (%s) sharing same memory\n",
-				task_pid_nr(p), p->comm);
 			do_send_sig_info(SIGKILL, SEND_SIG_FORCED, p, true);
 		}
 	rcu_read_unlock();
-- 
2.4.3

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

[toc] | [next] | [standalone]

#1237264 — Re: [PATCH -mm v2 1/3] mm/oom_kill: remove the wrong fatal_signal_pending()check in oom_kill_process()

David Rientjes wrote:
> On Wed, 30 Sep 2015, Oleg Nesterov wrote:
> 
> > The fatal_signal_pending() was added to suppress unnecessary "sharing
> > same memory" message, but it can't 100% help anyway because it can be
> > false-negative; SIGKILL can be already dequeued.
> > 
> > And worse, it can be false-positive due to exec or coredump. exec is
> > mostly fine, but coredump is not. It is possible that the group leader
> > has the pending SIGKILL because its sub-thread originated the coredump,
> > in this case we must not skip this process.
> > 
> > We could probably add the additional ->group_exit_task check but this
> > pach just removes the wrong check along with pr_info().
> > 
> > Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> 
> Acked-by: David Rientjes <rientjes@google.com>
> 

Please s/pach/patch/ when applying.

Acked-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1237387

On Wed 30-09-15 20:24:05, Oleg Nesterov wrote:
[...]
> It is possible that the group leader
> has the pending SIGKILL because its sub-thread originated the coredump,
> in this case we must not skip this process.

I do not understand this. If the group leader has SIGKILL pending it
will die anyway regardless whether we send another sigkill or not, no?

Or is the issue that another SIGKILL will wake it up?
-- 
Michal Hocko
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1237488

On 10/01, Michal Hocko wrote:
>
> On Wed 30-09-15 20:24:05, Oleg Nesterov wrote:
> [...]
> > It is possible that the group leader
> > has the pending SIGKILL because its sub-thread originated the coredump,
> > in this case we must not skip this process.
>
> I do not understand this. If the group leader has SIGKILL pending it
> will die anyway regardless whether we send another sigkill or not, no?

Yes it will die, but only after the coredump is finished.

Suppose we have a thread group with the group leader P and another
thread T. If T starts the coredump, it sends SIGKILL to P and waits
until it parks in exit_mm(). Then T actually dumps the core which may
need more memory, a lot of time, etc.

We need to kill this process. Yes, P is already killed and it sleeps
in TASK_UNINTERRUPTIBLE so this thread does not need SIGKILL. But
do_send_sig_info(P) will also find T and kill it too to make
dump_interrupted() == T.

Oleg.

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

[toc] | [prev] | [next] | [standalone]

#1237507

On Thu 01-10-15 17:00:10, Oleg Nesterov wrote:
> On 10/01, Michal Hocko wrote:
> >
> > On Wed 30-09-15 20:24:05, Oleg Nesterov wrote:
> > [...]
> > > It is possible that the group leader
> > > has the pending SIGKILL because its sub-thread originated the coredump,
> > > in this case we must not skip this process.
> >
> > I do not understand this. If the group leader has SIGKILL pending it
> > will die anyway regardless whether we send another sigkill or not, no?
> 
> Yes it will die, but only after the coredump is finished.
> 
> Suppose we have a thread group with the group leader P and another
> thread T. If T starts the coredump, it sends SIGKILL to P and waits
> until it parks in exit_mm(). Then T actually dumps the core which may
> need more memory, a lot of time, etc.
> 
> We need to kill this process. Yes, P is already killed and it sleeps
> in TASK_UNINTERRUPTIBLE so this thread does not need SIGKILL. But
> do_send_sig_info(P) will also find T and kill it too to make
> dump_interrupted() == T.

I am still utterly confused :( Where do we kill T if it is not in the
same thread group with P?
-- 
Michal Hocko
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1237527

On 10/01, Michal Hocko wrote:
>
> On Thu 01-10-15 17:00:10, Oleg Nesterov wrote:
> > On 10/01, Michal Hocko wrote:
> > >
> > > On Wed 30-09-15 20:24:05, Oleg Nesterov wrote:
> > > [...]
> > > > It is possible that the group leader
> > > > has the pending SIGKILL because its sub-thread originated the coredump,
> > > > in this case we must not skip this process.
> > >
> > > I do not understand this. If the group leader has SIGKILL pending it
> > > will die anyway regardless whether we send another sigkill or not, no?
> >
> > Yes it will die, but only after the coredump is finished.
> >
> > Suppose we have a thread group with the group leader P and another
> > thread T. If T starts the coredump, it sends SIGKILL to P and waits
> > until it parks in exit_mm(). Then T actually dumps the core which may
> > need more memory, a lot of time, etc.
> >
> > We need to kill this process. Yes, P is already killed and it sleeps
> > in TASK_UNINTERRUPTIBLE so this thread does not need SIGKILL. But
> > do_send_sig_info(P) will also find T and kill it too to make
> > dump_interrupted() == T.
>
> I am still utterly confused :( Where do we kill T if it is not in the
> same thread group with P?

But it is in the same thread group?

Oleg.

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

[toc] | [prev] | [next] | [standalone]

#1237550

On Thu 01-10-15 17:41:15, Oleg Nesterov wrote:
> On 10/01, Michal Hocko wrote:
> >
> > On Thu 01-10-15 17:00:10, Oleg Nesterov wrote:
> > > On 10/01, Michal Hocko wrote:
> > > >
> > > > On Wed 30-09-15 20:24:05, Oleg Nesterov wrote:
> > > > [...]
> > > > > It is possible that the group leader
> > > > > has the pending SIGKILL because its sub-thread originated the coredump,
> > > > > in this case we must not skip this process.
> > > >
> > > > I do not understand this. If the group leader has SIGKILL pending it
> > > > will die anyway regardless whether we send another sigkill or not, no?
> > >
> > > Yes it will die, but only after the coredump is finished.
> > >
> > > Suppose we have a thread group with the group leader P and another
> > > thread T. If T starts the coredump, it sends SIGKILL to P and waits
> > > until it parks in exit_mm(). Then T actually dumps the core which may
> > > need more memory, a lot of time, etc.
> > >
> > > We need to kill this process. Yes, P is already killed and it sleeps
> > > in TASK_UNINTERRUPTIBLE so this thread does not need SIGKILL. But
> > > do_send_sig_info(P) will also find T and kill it too to make
> > > dump_interrupted() == T.
> >
> > I am still utterly confused :( Where do we kill T if it is not in the
> > same thread group with P?
> 
> But it is in the same thread group?

The whole loop is about sending sigkill to a process from a different
thread group though. And this is what confused me completely. But I got
the point finally. zap_process will add SIGKILL to all threads but the
current which will go on without being killed and if this is not a
thread group leader then we would miss it.

Thanks for the clarification and feel free to add
Acked-by: Michal Hocko <mhocko@suse.com>

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

[toc] | [prev] | [next] | [standalone]

#1237606

On 10/01, Michal Hocko wrote:
>
> zap_process will add SIGKILL to all threads but the
> current which will go on without being killed and if this is not a
> thread group leader then we would miss it.

Yes. And note that de_thread() does the same. Speaking of oom-killer
this is mostly fine, the execing thread is going to release its old
->mm and it has already passed the copy_strings() stage which can use
a lot more memory.

But in theory (in practice currently this seems impossible without
SIGKILL) exec can fail before exec_mmap(), so if we want to zap its
->mm we need to ensure it can't return to user space.

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

Thanks!

Oleg.

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

[toc] | [prev] | [next] | [standalone]

#1238134 — Re: [PATCH -mm v2 1/3] mm/oom_kill: remove the wrong fatal_signal_pending() check in oom_kill_process()

Oleg Nesterov wrote:
> On 10/01, Michal Hocko wrote:
> >
> > zap_process will add SIGKILL to all threads but the
> > current which will go on without being killed and if this is not a
> > thread group leader then we would miss it.
> 
> Yes. And note that de_thread() does the same. Speaking of oom-killer
> this is mostly fine, the execing thread is going to release its old
> ->mm and it has already passed the copy_strings() stage which can use
> a lot more memory.

So, we have the same wrong fatal_signal_pending() check in out_of_memory()

        /*
         * 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
         * quickly exit and free its memory.
         *
         * But don't select if current has already released its mm and cleared
         * TIF_MEMDIE flag at exit_mm(), otherwise an OOM livelock may occur.
         */
        if (current->mm &&
            (fatal_signal_pending(current) || task_will_free_mem(current))) {
                mark_oom_victim(current);
                return true;
        }

because it is possible that T starts the coredump, T sends SIGKILL to P,
P calls out_of_memory() on GFP_FS allocation, P misses to set SIGKILL on T?

Since T sends SIGKILL to all clone(CLONE_VM) tasks upon coredump, P needs
to do

        rcu_read_lock();
        for_each_process(p) {
                if (!process_shares_mm(p, current->mm))
                        continue;
                if (unlikely(p->flags & PF_KTHREAD))
                        continue;
                if (p->signal->oom_score_adj == OOM_SCORE_ADJ_MIN)
                        continue;

                do_send_sig_info(SIGKILL, SEND_SIG_FORCED, p, true);
        }
        rcu_read_unlock();

after mark_oom_victim(current) in case T is not in the same thread group?

If yes, what happens if some task failed to receive SIGKILL due to
p->signal->oom_score_adj == OOM_SCORE_ADJ_MIN condition?
Will we hit mm->mmap_sem livelock?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1238153

On Fri 02-10-15 20:32:41, Tetsuo Handa wrote:
> Oleg Nesterov wrote:
> > On 10/01, Michal Hocko wrote:
> > >
> > > zap_process will add SIGKILL to all threads but the
> > > current which will go on without being killed and if this is not a
> > > thread group leader then we would miss it.
> > 
> > Yes. And note that de_thread() does the same. Speaking of oom-killer
> > this is mostly fine, the execing thread is going to release its old
> > ->mm and it has already passed the copy_strings() stage which can use
> > a lot more memory.
> 
> So, we have the same wrong fatal_signal_pending() check in out_of_memory()
> 
>         /*
>          * 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
>          * quickly exit and free its memory.
>          *
>          * But don't select if current has already released its mm and cleared
>          * TIF_MEMDIE flag at exit_mm(), otherwise an OOM livelock may occur.
>          */
>         if (current->mm &&
>             (fatal_signal_pending(current) || task_will_free_mem(current))) {
>                 mark_oom_victim(current);
>                 return true;
>         }
> 
> because it is possible that T starts the coredump, T sends SIGKILL to P,
> P calls out_of_memory() on GFP_FS allocation, P misses to set SIGKILL on T?

So what? P will get an access to memory reserves to move on with the
allocation. This has nothing to do with other thread. If the current
thread (P) doesn't release any memory we would get to regular oom killer
path and eventually send the signal.

This is a simple heuristic to prevent from unnecessary killing. If it
doesn't help we should still be able to kill something. If you really
want to prevent from an unlikely case where the current is the only task
in the OOM path and TIF_MEMDIE didn't help then disable the heuristic if
the current already has TIF_MEMDIE set.

> Since T sends SIGKILL to all clone(CLONE_VM) tasks upon coredump, P needs
> to do

It does that only to all threads in the _same_ thread group AFAIU.

> 
>         rcu_read_lock();
>         for_each_process(p) {
>                 if (!process_shares_mm(p, current->mm))
>                         continue;
>                 if (unlikely(p->flags & PF_KTHREAD))
>                         continue;
>                 if (p->signal->oom_score_adj == OOM_SCORE_ADJ_MIN)
>                         continue;
> 
>                 do_send_sig_info(SIGKILL, SEND_SIG_FORCED, p, true);
>         }
>         rcu_read_unlock();
> 
> after mark_oom_victim(current) in case T is not in the same thread group?

What does this have to do with coredumping? Have you fallen into the
same confusion trap I did yesterday?

> If yes, what happens if some task failed to receive SIGKILL due to
> p->signal->oom_score_adj == OOM_SCORE_ADJ_MIN condition?
> Will we hit mm->mmap_sem livelock?

I am not sure which livelock you mean here (exit_mm not being able to
proceed because some of the task is holding mmap_sem for write and
looping in the allocator)?

Anyway, having tasks sharing mm but having incompatible OOM_SCORE_ADJ_MIN
is basically a misconfiguration IMHO. I wouldn't lose sleep over it to
be honest. And yes if one process is pinning the address space then our
chances.
-- 
Michal Hocko
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1238163 — Re: [PATCH -mm v2 1/3] mm/oom_kill: remove the wrong fatal_signal_pending() check in oom_kill_process()

Michal Hocko wrote:
> > Since T sends SIGKILL to all clone(CLONE_VM) tasks upon coredump, P needs
> > to do
> 
> It does that only to all threads in the _same_ thread group AFAIU.

I'm confused. What the _same_ thread group?

I can observe that SIGKILL is sent to all

  clone(CLONE_THREAD | CLONE_SIGHAND | CLONE_VM)
  clone(CLONE_SIGHAND | CLONE_VM)
  clone(CLONE_VM)

threads upon coredump.

---------- testing program start ----------
#define _GNU_SOURCE
#include <stdlib.h>
#include <sys/types.h>
#include <unistd.h>
#include <fcntl.h>
#include <sched.h>
#include <sys/mman.h>

static int file_mapper(void *unused)
{
        const int fd = open("/proc/self/exe", O_RDONLY);
        void *ptr[10000]; /* Will cause SIGSEGV due to stack overflow */
        int i;
        sleep(2);
        while (1) {
                for (i = 0; i < 10000; i++)
                        ptr[i] = mmap(NULL, 4096, PROT_READ, MAP_PRIVATE, fd,
                                      0);
                for (i = 0; i < 10000; i++)
                        munmap(ptr[i], 4096);
        }
        return 0;
}

int main(int argc, char *argv[])
{
        int i;
        for (i = 0; i < 5; i++) {
                char *cp = malloc(4 * 1024);
                if (!cp || clone(file_mapper, cp + 4 * 1024,
                                 CLONE_THREAD | CLONE_SIGHAND | CLONE_VM, NULL) == -1)
                        break;
        }
        for (i = 0; i < 5; i++) {
                char *cp = malloc(4 * 1024);
                if (!cp || clone(file_mapper, cp + 4 * 1024,
                                 CLONE_SIGHAND | CLONE_VM, NULL) == -1)
                        break;
        }
        for (i = 0; i < 5; i++) {
                char *cp = malloc(4 * 1024);
                if (!cp || clone(file_mapper, cp + 4 * 1024,
                                 CLONE_VM, NULL) == -1)
                        break;
        }
        while (1)
                pause();
        return 0;
}
---------- testing program end ----------

---------- debug printk() patch start ----------
--- a/fs/coredump.c
+++ b/fs/coredump.c
@@ -295,6 +295,8 @@ static int zap_process(struct task_struct *start, int exit_code, int flags)
        for_each_thread(start, t) {
                task_clear_jobctl_pending(t, JOBCTL_PENDING_MASK);
                if (t != current && t->mm) {
+                       printk(KERN_INFO "Setting SIGKILL to %s(%u)\n",
+                              t->comm, t->pid);
                        sigaddset(&t->pending.signal, SIGKILL);
                        signal_wake_up(t, 1);
                        nr++;
---------- debug printk() patch end ----------

---------- kernel log start ----------
[ 4829.770899] a.out[11614]: segfault at 1e1f768 ip 00000000004007be sp 0000000001e1f770 error 6 in a.out[400000+1000]
[ 4829.774190] Setting SIGKILL to a.out(11613)
[ 4829.775954] Setting SIGKILL to a.out(11615)
[ 4829.777191] Setting SIGKILL to a.out(11616)
[ 4829.778381] Setting SIGKILL to a.out(11617)
[ 4829.779537] Setting SIGKILL to a.out(11618)
[ 4829.781057] Setting SIGKILL to a.out(11619)
[ 4829.782236] Setting SIGKILL to a.out(11620)
[ 4829.783401] Setting SIGKILL to a.out(11621)
[ 4829.784569] Setting SIGKILL to a.out(11622)
[ 4829.785700] Setting SIGKILL to a.out(11623)
[ 4829.786848] Setting SIGKILL to a.out(11624)
[ 4829.788001] Setting SIGKILL to a.out(11625)
[ 4829.789132] Setting SIGKILL to a.out(11626)
[ 4829.790332] Setting SIGKILL to a.out(11627)
[ 4829.791593] Setting SIGKILL to a.out(11628)
[ 4829.792941] a.out[11624]: segfault at 1e29808 ip 00000000004007be sp 0000000001e29810 error 6 in a.out[400000+1000]
[ 4829.795493] a.out[11622]: segfault at 1e277e8 ip 00000000004007be sp 0000000001e277f0 error 6
[ 4829.797171] a.out[11623]: segfault at 1e287f8 ip 00000000004007be sp 0000000001e28800 error 6
[ 4829.797545] a.out[11621]: segfault at 1e267d8 ip 00000000004007be sp 0000000001e267e0 error 6
[ 4829.797547] a.out[11618]: segfault at 1e237a8 ip 00000000004007be sp 0000000001e237b0 error 6
[ 4829.797548] a.out[11617]: segfault at 1e22798 ip 00000000004007be sp 0000000001e227a0 error 6
[ 4829.797550] a.out[11619]: segfault at 1e247b8 ip 00000000004007be sp 0000000001e247c0 error 6
[ 4829.797552] a.out[11620]: segfault at 1e257c8 ip 00000000004007be sp 0000000001e257d0 error 6
[ 4829.802631] a.out[11615]: segfault at 1e20778 ip 00000000004007be sp 0000000001e20780 error 6
[ 4829.802633]  in a.out[400000+1000]
[ 4829.802639]  in a.out[400000+1000]
[ 4829.802642]  in a.out[400000+1000]
[ 4829.802655]  in a.out[400000+1000]
[ 4829.802659]  in a.out[400000+1000]
[ 4829.802662]  in a.out[400000+1000]
[ 4829.814605]  in a.out[400000+1000]
[ 4829.819500]  in a.out[400000+1000]
---------- kernel log end ----------
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1238243

On Fri 02-10-15 21:33:08, Tetsuo Handa wrote:
> Michal Hocko wrote:
> > > Since T sends SIGKILL to all clone(CLONE_VM) tasks upon coredump, P needs
> > > to do
> > 
> > It does that only to all threads in the _same_ thread group AFAIU.
> 
> I'm confused. What the _same_ thread group?
> 
> I can observe that SIGKILL is sent to all
> 
>   clone(CLONE_THREAD | CLONE_SIGHAND | CLONE_VM)
>   clone(CLONE_SIGHAND | CLONE_VM)
>   clone(CLONE_VM)

I might be missing something crucial here but
copy_process has the following:
        if (clone_flags & CLONE_THREAD) {
                p->exit_signal = -1;
                p->group_leader = current->group_leader;
                p->tgid = current->tgid;
        } else {
                if (clone_flags & CLONE_PARENT)
                        p->exit_signal = current->group_leader->exit_signal;
                else
                        p->exit_signal = (clone_flags & CSIGNAL);
                p->group_leader = p;
                p->tgid = p->pid;
        }

So clone without CLONE_THREAD should create a new thread group leader
and so create a new thread group. Unless there is some other trickery
which I do not see right now for_each_thread from the parent task
shouldn't see those which are cloned without CLONE_THREAD.

[...]
> --- a/fs/coredump.c
> +++ b/fs/coredump.c
> @@ -295,6 +295,8 @@ static int zap_process(struct task_struct *start, int exit_code, int flags)
>         for_each_thread(start, t) {
>                 task_clear_jobctl_pending(t, JOBCTL_PENDING_MASK);
>                 if (t != current && t->mm) {
> +                       printk(KERN_INFO "Setting SIGKILL to %s(%u)\n",
> +                              t->comm, t->pid);
>                         sigaddset(&t->pending.signal, SIGKILL);
>                         signal_wake_up(t, 1);
>                         nr++;
> ---------- debug printk() patch end ----------

OK, but all your tasks should trigger SEGV. You cannot find out whether
all of them happened from the same zap_process, can you.
-- 
Michal Hocko
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1238305 — Re: [PATCH -mm v2 1/3] mm/oom_kill: remove the wrong fatal_signal_pending() check in oom_kill_process()

Michal Hocko wrote:
> > --- a/fs/coredump.c
> > +++ b/fs/coredump.c
> > @@ -295,6 +295,8 @@ static int zap_process(struct task_struct *start, int exit_code, int flags)
> >         for_each_thread(start, t) {
> >                 task_clear_jobctl_pending(t, JOBCTL_PENDING_MASK);
> >                 if (t != current && t->mm) {
> > +                       printk(KERN_INFO "Setting SIGKILL to %s(%u)\n",
> > +                              t->comm, t->pid);
> >                         sigaddset(&t->pending.signal, SIGKILL);
> >                         signal_wake_up(t, 1);
> >                         nr++;
> > ---------- debug printk() patch end ----------
> 
> OK, but all your tasks should trigger SEGV. You cannot find out whether
> all of them happened from the same zap_process, can you.

Indeed. I retested with updated patch. Not all of them are killed from
the same zap_process, but all of them are killed by the same SEGV event.
I think that coredump stops all threads sharing the same memory.

---------- debug printk() patch start ----------
--- a/fs/coredump.c
+++ b/fs/coredump.c
@@ -292,14 +292,18 @@ static int zap_process(struct task_struct *start, int exit_code, int flags)
        start->signal->group_exit_code = exit_code;
        start->signal->group_stop_count = 0;

+       printk(KERN_INFO "%s(%d): Started zap_process()\n", current->comm, current->pid);
        for_each_thread(start, t) {
                task_clear_jobctl_pending(t, JOBCTL_PENDING_MASK);
                if (t != current && t->mm) {
+                       printk(KERN_INFO "%s(%d): Setting SIGKILL to %s(%u)\n",
+                              current->comm, current->pid, t->comm, t->pid);
                        sigaddset(&t->pending.signal, SIGKILL);
                        signal_wake_up(t, 1);
                        nr++;
                }
        }
+       printk(KERN_INFO "%s(%d): zap_process() returned %d\n", current->comm, current->pid, nr);

        return nr;
 }
---------- debug printk() patch end ----------

---------- kernel log start ----------
[   71.808316] a.out[11057]: segfault at 7ac768 ip 00000000004007be sp 00000000007ac770 error 6 in a.out[400000+1000]
[   71.811003] a.out[11058]: segfault at 7ad778 ip 00000000004007be sp 00000000007ad780 error 6
[   71.811005]  in a.out[400000+1000]
[   71.813817] a.out(11058): Started zap_process()
[   71.813818] a.out(11058): Setting SIGKILL to a.out(11056)
[   71.813841] a.out(11058): Setting SIGKILL to a.out(11057)
[   71.813854] a.out(11058): Setting SIGKILL to a.out(11059)
[   71.813855] a.out(11058): Setting SIGKILL to a.out(11060)
[   71.813855] a.out(11058): Setting SIGKILL to a.out(11061)
[   71.813857] a.out(11058): zap_process() returned 5
[   71.813880] a.out(11058): Started zap_process()
[   71.813880] a.out(11058): Setting SIGKILL to a.out(11062)
[   71.813881] a.out(11058): zap_process() returned 1
[   71.813881] a.out(11058): Started zap_process()
[   71.813882] a.out(11058): Setting SIGKILL to a.out(11063)
[   71.813882] a.out(11058): zap_process() returned 1
[   71.813882] a.out(11058): Started zap_process()
[   71.813883] a.out(11058): Setting SIGKILL to a.out(11064)
[   71.813883] a.out(11058): zap_process() returned 1
[   71.813884] a.out(11058): Started zap_process()
[   71.813884] a.out(11058): Setting SIGKILL to a.out(11065)
[   71.813899] a.out(11058): zap_process() returned 1
[   71.813900] a.out(11058): Started zap_process()
[   71.813900] a.out(11058): Setting SIGKILL to a.out(11066)
[   71.813901] a.out(11058): zap_process() returned 1
[   71.813901] a.out(11058): Started zap_process()
[   71.813902] a.out(11058): Setting SIGKILL to a.out(11067)
[   71.813902] a.out(11058): zap_process() returned 1
[   71.813903] a.out(11058): Started zap_process()
[   71.813904] a.out(11058): Setting SIGKILL to a.out(11068)
[   71.813904] a.out(11058): zap_process() returned 1
[   71.813905] a.out(11058): Started zap_process()
[   71.813905] a.out(11058): Setting SIGKILL to a.out(11069)
[   71.813906] a.out(11058): zap_process() returned 1
[   71.813906] a.out(11058): Started zap_process()
[   71.813907] a.out(11058): Setting SIGKILL to a.out(11070)
[   71.813907] a.out(11058): zap_process() returned 1
[   71.813908] a.out(11058): Started zap_process()
[   71.813908] a.out(11058): Setting SIGKILL to a.out(11071)
[   71.813908] a.out(11058): zap_process() returned 1
[   71.813925] a.out[11068]: segfault at 7b7818 ip 00000000004007be sp 00000000007b7820 error 6 in a.out[400000+1000]
[   71.813938] a.out[11063]: segfault at 7b27c8 ip 00000000004007be sp 00000000007b27d0 error 6
[   71.813940] a.out[11064]: segfault at 7b37d8 ip 00000000004007be sp 00000000007b37e0 error 6
[   71.813941] a.out[11066]: segfault at 7b57f8 ip 00000000004007be sp 00000000007b5800 error 6
[   71.813943] a.out[11060]: segfault at 7af798 ip 00000000004007be sp 00000000007af7a0 error 6
[   71.813945] a.out[11062]: segfault at 7b17b8 ip 00000000004007be sp 00000000007b17c0 error 6
[   71.813946] a.out[11067]: segfault at 7b6808 ip 00000000004007be sp 00000000007b6810 error 6
[   71.813994] a.out[11070]: segfault at 7b9838 ip 00000000004007be sp 00000000007b9840 error 6
[   71.813995]  in a.out[400000+1000]
[   71.813998]  in a.out[400000+1000]
[   71.814002]  in a.out[400000+1000]
[   71.814004]  in a.out[400000+1000]
[   71.814008]  in a.out[400000+1000]
[   71.814011]  in a.out[400000+1000]
[   71.814015]  in a.out[400000+1000]
---------- kernel log end ----------
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1238311

On 10/02, Tetsuo Handa wrote:
>
> Indeed. I retested with updated patch. Not all of them are killed from
> the same zap_process, but all of them are killed by the same SEGV event.
> I think that coredump stops all threads sharing the same memory.

Yes sure. Please see other emails.

Oleg.

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

[toc] | [prev] | [next] | [standalone]

#1238309

On 10/02, Michal Hocko wrote:
>
> So clone without CLONE_THREAD should create a new thread group leader
> and so create a new thread group.

Yes.

> Unless there is some other trickery
> which I do not see right now for_each_thread from the parent task
> shouldn't see those which are cloned without CLONE_THREAD.

Yes.

But I still do not understand what are you talking about, sorry ;)

So let me say just in case that coredump (namely zap_threads()) will
also kill other thread groups with the same ->mm.

Oleg.

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

[toc] | [prev] | [next] | [standalone]

#1238319

On Fri 02-10-15 15:57:56, Oleg Nesterov wrote:
> On 10/02, Michal Hocko wrote:
> >
> > So clone without CLONE_THREAD should create a new thread group leader
> > and so create a new thread group.
> 
> Yes.
> 
> > Unless there is some other trickery
> > which I do not see right now for_each_thread from the parent task
> > shouldn't see those which are cloned without CLONE_THREAD.
> 
> Yes.
> 
> But I still do not understand what are you talking about, sorry ;)

yes this whole thing was off-topic and I am sorry to add more confusion
to it.

> So let me say just in case that coredump (namely zap_threads()) will
> also kill other thread groups with the same ->mm.

Yes, I wanted to make sure to clarify what the thread_group is supposed
to mean here but I only made it more confusing now that I am reading the
whole thing again. Sorry about that.

Calling processes which only share the mm as threads is confusing and
only obscures the discussion.

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

[toc] | [prev] | [next] | [standalone]

#1238300

Tetsuo, sorry, I don't understand your question...

On 10/02, Tetsuo Handa wrote:
>
> Oleg Nesterov wrote:
> > On 10/01, Michal Hocko wrote:
> > >
> > > zap_process will add SIGKILL to all threads but the
> > > current which will go on without being killed and if this is not a
> > > thread group leader then we would miss it.
> >
> > Yes. And note that de_thread() does the same. Speaking of oom-killer
> > this is mostly fine, the execing thread is going to release its old
> > ->mm and it has already passed the copy_strings() stage which can use
> > a lot more memory.
>
> So, we have the same wrong fatal_signal_pending() check in out_of_memory()

Yes, sure, it is not right too. Again, this is even documented in
d003f371b27016354c:

    fatal_signal_pending() can be true because of SIGNAL_GROUP_COREDUMP so
    out_of_memory() and mem_cgroup_out_of_memory() shouldn't blindly trust it.

This is off-topic in a sense that this series only tries to ensure that
if we are going to kill a memory hog we can't miss a process which shares
the same mm (ignoring the OOM_SCORE_ADJ_MIN condition below).

>         /*
>          * 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
>          * quickly exit and free its memory.
>          *
>          * But don't select if current has already released its mm and cleared
>          * TIF_MEMDIE flag at exit_mm(), otherwise an OOM livelock may occur.
>          */
>         if (current->mm &&
>             (fatal_signal_pending(current) || task_will_free_mem(current))) {
>                 mark_oom_victim(current);
>                 return true;
>         }
>
> because it is possible that T starts the coredump, T sends SIGKILL to P,
> P calls out_of_memory() on GFP_FS allocation,

yes, and since fatal_signal_pending() == T we do not even check
task_will_free_mem().

> P misses to set SIGKILL on T?
>
> Since T sends SIGKILL to all clone(CLONE_VM) tasks upon coredump, P needs
> to do
>
> [...snip...]

> after mark_oom_victim(current) in case T is not in the same thread group?

I do not see how this depends on "not in the same thread group". This
fatal_signal_pending() doesn't look right in any case.


> If yes, what happens if some task failed to receive SIGKILL due to
> p->signal->oom_score_adj == OOM_SCORE_ADJ_MIN condition?

Oh. This is another issue. I already tried to suggest to remove this
check. But this needs more discussion, hopefully we can do this later.

Oleg.

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

[toc] | [prev] | [next] | [standalone]

#1238325 — Re: [PATCH -mm v2 1/3] mm/oom_kill: remove the wrong fatal_signal_pending() check in oom_kill_process()

Oleg Nesterov wrote:
> Tetsuo, sorry, I don't understand your question...
> 
> > because it is possible that T starts the coredump, T sends SIGKILL to P,
> > P calls out_of_memory() on GFP_FS allocation,
> 
> yes, and since fatal_signal_pending() == T we do not even check
> task_will_free_mem().
> 
> > P misses to set SIGKILL on T?
> >
> > Since T sends SIGKILL to all clone(CLONE_VM) tasks upon coredump, P needs
> > to do
> >
> > [...snip...]
> 
> > after mark_oom_victim(current) in case T is not in the same thread group?
> 
> I do not see how this depends on "not in the same thread group". This
> fatal_signal_pending() doesn't look right in any case.

You already answered my question. ;-)
You confirmed this is a possible silent hang up path (I mean, hang up
without OOM killer messages).

> > If yes, what happens if some task failed to receive SIGKILL due to
> > p->signal->oom_score_adj == OOM_SCORE_ADJ_MIN condition?
> 
> Oh. This is another issue. I already tried to suggest to remove this
> check. But this needs more discussion, hopefully we can do this later.

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web