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

Groups > linux.kernel > #1227268 > unrolled thread

[PATCH] mm/oom_kill.c: don't kill TASK_UNINTERRUPTIBLE tasks

Back to article view | Back to linux.kernel

Contents

  [PATCH] mm/oom_kill.c: don't kill TASK_UNINTERRUPTIBLE tasks Kyle Walker <kwalker@redhat.com> - 2015-09-17 20:10 +0200
    Re: [PATCH] mm/oom_kill.c: don't kill TASK_UNINTERRUPTIBLE tasks Oleg Nesterov <oleg@redhat.com> - 2015-09-17 21:30 +0200
      Re: [PATCH] mm/oom_kill.c: don't kill TASK_UNINTERRUPTIBLE tasks Christoph Lameter <cl@linux.com> - 2015-09-18 17:50 +0200
        Re: [PATCH] mm/oom_kill.c: don't kill TASK_UNINTERRUPTIBLE tasks Oleg Nesterov <oleg@redhat.com> - 2015-09-18 18:30 +0200
          Re: [PATCH] mm/oom_kill.c: don't kill TASK_UNINTERRUPTIBLE tasks Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> - 2015-09-18 18:50 +0200
            Re: [PATCH] mm/oom_kill.c: don't kill TASK_UNINTERRUPTIBLE tasks Oleg Nesterov <oleg@redhat.com> - 2015-09-18 19:00 +0200
          Re: [PATCH] mm/oom_kill.c: don't kill TASK_UNINTERRUPTIBLE tasks Christoph Lameter <cl@linux.com> - 2015-09-18 19:10 +0200
            Re: [PATCH] mm/oom_kill.c: don't kill TASK_UNINTERRUPTIBLE tasks Oleg Nesterov <oleg@redhat.com> - 2015-09-18 21:20 +0200
              Re: [PATCH] mm/oom_kill.c: don't kill TASK_UNINTERRUPTIBLE tasks Christoph Lameter <cl@linux.com> - 2015-09-18 21:20 +0200
                Re: [PATCH] mm/oom_kill.c: don't kill TASK_UNINTERRUPTIBLE tasks Christoph Lameter <cl@linux.com> - 2015-09-19 00:10 +0200
            Re: [PATCH] mm/oom_kill.c: don't kill TASK_UNINTERRUPTIBLE tasks Michal Hocko <mhocko@kernel.org> - 2015-09-19 10:40 +0200
              Re: [PATCH] mm/oom_kill.c: don't kill TASK_UNINTERRUPTIBLE tasks Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> - 2015-09-19 16:40 +0200
                Re: [PATCH] mm/oom_kill.c: don't kill TASK_UNINTERRUPTIBLE tasks Michal Hocko <mhocko@kernel.org> - 2015-09-19 18:00 +0200
                Re: [PATCH] mm/oom_kill.c: don't kill TASK_UNINTERRUPTIBLE tasks David Rientjes <rientjes@google.com> - 2015-09-22 01:40 +0200
                  Re: [PATCH] mm/oom_kill.c: don't kill TASK_UNINTERRUPTIBLE tasks Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> - 2015-09-22 07:40 +0200
              Re: [PATCH] mm/oom_kill.c: don't kill TASK_UNINTERRUPTIBLE tasks Oleg Nesterov <oleg@redhat.com> - 2015-09-19 16:50 +0200
            Re: [PATCH] mm/oom_kill.c: don't kill TASK_UNINTERRUPTIBLE tasks David Rientjes <rientjes@google.com> - 2015-09-22 01:30 +0200
        Re: [PATCH] mm/oom_kill.c: don't kill TASK_UNINTERRUPTIBLE tasks Michal Hocko <mhocko@kernel.org> - 2015-09-19 10:30 +0200
    Re: [PATCH] mm/oom_kill.c: don't kill TASK_UNINTERRUPTIBLE tasks Michal Hocko <mhocko@kernel.org> - 2015-09-19 10:30 +0200
      Re: [PATCH] mm/oom_kill.c: don't kill TASK_UNINTERRUPTIBLE tasks David Rientjes <rientjes@google.com> - 2015-09-22 01:10 +0200
    can't oom-kill zap the victim's memory? Oleg Nesterov <oleg@redhat.com> - 2015-09-19 17:10 +0200
      Re: can't oom-kill zap the victim's memory? Oleg Nesterov <oleg@redhat.com> - 2015-09-19 17:20 +0200
      Re: can't oom-kill zap the victim's memory? Michal Hocko <mhocko@kernel.org> - 2015-09-19 18:00 +0200
        Re: can't oom-kill zap the victim's memory? Oleg Nesterov <oleg@redhat.com> - 2015-09-20 15:20 +0200
      Re: can't oom-kill zap the victim's memory? Linus Torvalds <torvalds@linux-foundation.org> - 2015-09-20 00:30 +0200
        Re: can't oom-kill zap the victim's memory? Raymond Jennings <shentino@gmail.com> - 2015-09-20 01:10 +0200
          Re: can't oom-kill zap the victim's memory? Linus Torvalds <torvalds@linux-foundation.org> - 2015-09-20 01:20 +0200
        Re: can't oom-kill zap the victim's memory? Michal Hocko <mhocko@kernel.org> - 2015-09-20 11:40 +0200
          Re: can't oom-kill zap the victim's memory? Oleg Nesterov <oleg@redhat.com> - 2015-09-20 15:10 +0200
        Re: can't oom-kill zap the victim's memory? Oleg Nesterov <oleg@redhat.com> - 2015-09-20 15:10 +0200
          Re: can't oom-kill zap the victim's memory? Linus Torvalds <torvalds@linux-foundation.org> - 2015-09-20 20:10 +0200
            Re: can't oom-kill zap the victim's memory? Raymond Jennings <shentino@gmail.com> - 2015-09-20 21:10 +0200
              Re: can't oom-kill zap the victim's memory? Oleg Nesterov <oleg@redhat.com> - 2015-09-21 16:10 +0200
            Re: can't oom-kill zap the victim's memory? Oleg Nesterov <oleg@redhat.com> - 2015-09-21 15:50 +0200
              Re: can't oom-kill zap the victim's memory? Michal Hocko <mhocko@kernel.org> - 2015-09-21 16:30 +0200
                Re: can't oom-kill zap the victim's memory? Oleg Nesterov <oleg@redhat.com> - 2015-09-21 17:40 +0200
                  Re: can't oom-kill zap the victim's memory? Michal Hocko <mhocko@kernel.org> - 2015-09-21 18:20 +0200
                  Re: can't oom-kill zap the victim's memory? Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> - 2015-09-21 19:00 +0200
                  Re: can't oom-kill zap the victim's memory? David Rientjes <rientjes@google.com> - 2015-09-22 01:50 +0200
              Re: can't oom-kill zap the victim's memory? Linus Torvalds <torvalds@linux-foundation.org> - 2015-09-21 19:00 +0200
      Re: can't oom-kill zap the victim's memory? Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> - 2015-09-20 17:00 +0200
        Re: can't oom-kill zap the victim's memory? Oleg Nesterov <oleg@redhat.com> - 2015-09-20 17:00 +0200

Page 2 of 3 — ← Prev page 1 [2] 3  Next page →

#1228457 — can't oom-kill zap the victim's memory?

On 09/17, Kyle Walker wrote:
>
> Currently, the oom killer will attempt to kill a process that is in
> TASK_UNINTERRUPTIBLE state. For tasks in this state for an exceptional
> period of time, such as processes writing to a frozen filesystem during
> a lengthy backup operation, this can result in a deadlock condition as
> related processes memory access will stall within the page fault
> handler.

And there are other potential reasons for deadlock.

Stupid idea. Can't we help the memory hog to free its memory? This is
orthogonal to other improvements we can do.

Please don't tell me the patch below is ugly, incomplete and suboptimal
in many ways, I know ;) I am not sure it is even correct. Just to explain
what I mean.

Perhaps oom_unmap_func() should only zap the anonymous vmas... and there
are a lot of other details which should be discussed if this can make any
sense.

Oleg.
---

--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -493,6 +493,26 @@ void oom_killer_enable(void)
 	up_write(&oom_sem);
 }
 
+static struct mm_struct *oom_unmap_mm;
+
+static void oom_unmap_func(struct work_struct *work)
+{
+	struct mm_struct *mm = xchg(&oom_unmap_mm, NULL);
+
+	if (!atomic_inc_not_zero(&mm->mm_users))
+		return;
+
+	// If this is not safe we can do use_mm() + unuse_mm()
+	down_read(&mm->mmap_sem);
+	if (mm->mmap)
+		zap_page_range(mm->mmap, 0, TASK_SIZE, NULL);
+	up_read(&mm->mmap_sem);
+
+	mmput(mm);
+	mmdrop(mm);
+}
+static DECLARE_WORK(oom_unmap_work, oom_unmap_func);
+
 #define K(x) ((x) << (PAGE_SHIFT-10))
 /*
  * Must be called while holding a reference to p, which will be released upon
@@ -570,8 +590,8 @@ void oom_kill_process(struct task_struct *p, gfp_t gfp_mask, int order,
 		victim = p;
 	}
 
-	/* mm cannot safely be dereferenced after task_unlock(victim) */
 	mm = victim->mm;
+	atomic_inc(&mm->mm_count);
 	mark_tsk_oom_victim(victim);
 	pr_err("Killed process %d (%s) total-vm:%lukB, anon-rss:%lukB, file-rss:%lukB\n",
 		task_pid_nr(victim), victim->comm, K(victim->mm->total_vm),
@@ -604,6 +624,10 @@ void oom_kill_process(struct task_struct *p, gfp_t gfp_mask, int order,
 	rcu_read_unlock();
 
 	do_send_sig_info(SIGKILL, SEND_SIG_FORCED, victim, true);
+	if (cmpxchg(&oom_unmap_mm, NULL, mm))
+		mmdrop(mm);
+	else
+		queue_work(system_unbound_wq, &oom_unmap_work);
 	put_task_struct(victim);
 }
 #undef K

--
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]

#1228459 — Re: can't oom-kill zap the victim's memory?

(off-topic)

On 09/19, Oleg Nesterov wrote:
>
> @@ -570,8 +590,8 @@ void oom_kill_process(struct task_struct *p, gfp_t gfp_mask, int order,
>  		victim = p;
>  	}
>
> -	/* mm cannot safely be dereferenced after task_unlock(victim) */
>  	mm = victim->mm;
> +	atomic_inc(&mm->mm_count);

Btw, I think we need this change anyway. This is pure theoretical, but
otherwise this task can exit and free its mm_struct right after task_unlock(),
then this mm_struct can be reallocated and used by another task, so we
can't trust the "p->mm == mm" check below.

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]

#1228467 — Re: can't oom-kill zap the victim's memory?

On Sat 19-09-15 17:03:16, Oleg Nesterov wrote:
> On 09/17, Kyle Walker wrote:
> >
> > Currently, the oom killer will attempt to kill a process that is in
> > TASK_UNINTERRUPTIBLE state. For tasks in this state for an exceptional
> > period of time, such as processes writing to a frozen filesystem during
> > a lengthy backup operation, this can result in a deadlock condition as
> > related processes memory access will stall within the page fault
> > handler.
> 
> And there are other potential reasons for deadlock.
> 
> Stupid idea. Can't we help the memory hog to free its memory? This is
> orthogonal to other improvements we can do.
> 
> Please don't tell me the patch below is ugly, incomplete and suboptimal
> in many ways, I know ;) I am not sure it is even correct. Just to explain
> what I mean.

Unmapping the memory for the oom victim has been already mentioned as a
way to improve the OOM killer behavior. Nobody has implemented that yet
though unfortunately. I have that on my TODO list since we have
discussed it with Mel at LSF.

> Perhaps oom_unmap_func() should only zap the anonymous vmas... and there
> are a lot of other details which should be discussed if this can make any
> sense.

I have just returned from an internal conference so my head is
completely cabbaged. I will have a look on Monday. From a quick look
the idea is feasible. You cannot rely on the worker context because
workqueues might be completely stuck with at this stage. You also cannot
do take mmap_sem directly because that might be held already so you need
a try_lock instead. Focusing on anonymous vmas first sounds like a good
idea to me because that would be simpler I guess.

> 
> Oleg.
> ---
> 
> --- a/mm/oom_kill.c
> +++ b/mm/oom_kill.c
> @@ -493,6 +493,26 @@ void oom_killer_enable(void)
>  	up_write(&oom_sem);
>  }
>  
> +static struct mm_struct *oom_unmap_mm;
> +
> +static void oom_unmap_func(struct work_struct *work)
> +{
> +	struct mm_struct *mm = xchg(&oom_unmap_mm, NULL);
> +
> +	if (!atomic_inc_not_zero(&mm->mm_users))
> +		return;
> +
> +	// If this is not safe we can do use_mm() + unuse_mm()
> +	down_read(&mm->mmap_sem);
> +	if (mm->mmap)
> +		zap_page_range(mm->mmap, 0, TASK_SIZE, NULL);
> +	up_read(&mm->mmap_sem);
> +
> +	mmput(mm);
> +	mmdrop(mm);
> +}
> +static DECLARE_WORK(oom_unmap_work, oom_unmap_func);
> +
>  #define K(x) ((x) << (PAGE_SHIFT-10))
>  /*
>   * Must be called while holding a reference to p, which will be released upon
> @@ -570,8 +590,8 @@ void oom_kill_process(struct task_struct *p, gfp_t gfp_mask, int order,
>  		victim = p;
>  	}
>  
> -	/* mm cannot safely be dereferenced after task_unlock(victim) */
>  	mm = victim->mm;
> +	atomic_inc(&mm->mm_count);
>  	mark_tsk_oom_victim(victim);
>  	pr_err("Killed process %d (%s) total-vm:%lukB, anon-rss:%lukB, file-rss:%lukB\n",
>  		task_pid_nr(victim), victim->comm, K(victim->mm->total_vm),
> @@ -604,6 +624,10 @@ void oom_kill_process(struct task_struct *p, gfp_t gfp_mask, int order,
>  	rcu_read_unlock();
>  
>  	do_send_sig_info(SIGKILL, SEND_SIG_FORCED, victim, true);
> +	if (cmpxchg(&oom_unmap_mm, NULL, mm))
> +		mmdrop(mm);
> +	else
> +		queue_work(system_unbound_wq, &oom_unmap_work);
>  	put_task_struct(victim);
>  }
>  #undef K

-- 
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]

#1228883 — Re: can't oom-kill zap the victim's memory?

On 09/19, Michal Hocko wrote:
>
> On Sat 19-09-15 17:03:16, Oleg Nesterov wrote:
> >
> > Stupid idea. Can't we help the memory hog to free its memory? This is
> > orthogonal to other improvements we can do.
> >
> > Please don't tell me the patch below is ugly, incomplete and suboptimal
> > in many ways, I know ;) I am not sure it is even correct. Just to explain
> > what I mean.
>
> Unmapping the memory for the oom victim has been already mentioned as a
> way to improve the OOM killer behavior. Nobody has implemented that yet
> though unfortunately. I have that on my TODO list since we have
> discussed it with Mel at LSF.

OK, good. So perhaps we should try to do this.

>
> > Perhaps oom_unmap_func() should only zap the anonymous vmas... and there
> > are a lot of other details which should be discussed if this can make any
> > sense.
>
> I have just returned from an internal conference so my head is
> completely cabbaged. I will have a look on Monday. From a quick look
> the idea is feasible. You cannot rely on the worker context because
> workqueues might be completely stuck with at this stage.

Yes this is true. See another email, probably oom-kill.c needs its own
kthread.

And again, we should actually try to avoid queue_work or queue_kthread_work
in any case. But not in the initial implementation. And initial implementation
could use workqueues, I think. I the likely case system_unbound_wq pool
should have an idle thread.

> You also cannot
> do take mmap_sem directly because that might be held already so you need
> a try_lock instead.

Still can't understand this part. See other emails, perhaps I missed
something.

> Focusing on anonymous vmas first sounds like a good
> idea to me because that would be simpler I guess.

And safer.

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]

#1228756 — Re: can't oom-kill zap the victim's memory?

On Sat, Sep 19, 2015 at 8:03 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> +
> +static void oom_unmap_func(struct work_struct *work)
> +{
> +       struct mm_struct *mm = xchg(&oom_unmap_mm, NULL);
> +
> +       if (!atomic_inc_not_zero(&mm->mm_users))
> +               return;
> +
> +       // If this is not safe we can do use_mm() + unuse_mm()
> +       down_read(&mm->mmap_sem);

I don't think this is safe.

What makes you sure that we might not deadlock on the mmap_sem here?
For all we know, the process that is going out of memory is in the
middle of a mmap(), and already holds the mmap_sem for writing. No?

So at the very least that needs to be a trylock, I think. And I'm not
sure zap_page_range() is ok with the mmap_sem only held for reading.
Normally our rule is that you can *populate* the page tables
concurrently, but you can't tear the down.

                Linus
--
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]

#1228759 — Re: can't oom-kill zap the victim's memory?

On 09/19/15 15:24, Linus Torvalds wrote:
> On Sat, Sep 19, 2015 at 8:03 AM, Oleg Nesterov <oleg@redhat.com> wrote:
>> +
>> +static void oom_unmap_func(struct work_struct *work)
>> +{
>> +       struct mm_struct *mm = xchg(&oom_unmap_mm, NULL);
>> +
>> +       if (!atomic_inc_not_zero(&mm->mm_users))
>> +               return;
>> +
>> +       // If this is not safe we can do use_mm() + unuse_mm()
>> +       down_read(&mm->mmap_sem);
> I don't think this is safe.
>
> What makes you sure that we might not deadlock on the mmap_sem here?
> For all we know, the process that is going out of memory is in the
> middle of a mmap(), and already holds the mmap_sem for writing. No?

Potentially stupid question that others may be asking: Is it legal to 
return EINTR from mmap() to let a SIGKILL from the OOM handler punch the 
task out of the kernel and back to userspace?

(sorry for the dupe btw, new email client snuck in html and I got bounced)

> So at the very least that needs to be a trylock, I think. And I'm not
> sure zap_page_range() is ok with the mmap_sem only held for reading.
> Normally our rule is that you can *populate* the page tables
> concurrently, but you can't tear the down.
>
>                  Linus
>
> --
> 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>

--
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]

#1228774 — Re: can't oom-kill zap the victim's memory?

On Sat, Sep 19, 2015 at 4:00 PM, Raymond Jennings <shentino@gmail.com> wrote:
>
> Potentially stupid question that others may be asking: Is it legal to return
> EINTR from mmap() to let a SIGKILL from the OOM handler punch the task out
> of the kernel and back to userspace?

Yes. Note that mmap() itself seldom sleeps or allocates much memory
(yeah, there's the vma itself and soem minimal stuff), so it's mainly
an issue for things like MAP_POPULATE etc.

The more common situation is things like uninterruptible reads when a
device (or network) is not responding, and we have special support for
"killable" waits that act like normal uninterruptible waits but can be
interrupted by deadly signals, exactly because for those cases we
don't need to worry about things like POSIX return value guarantees
("all or nothing" for file reads) etc.

So you do generally have to write extra code for the "killable sleep".
But it's a good thing to do, if you notice that certain cases aren't
responding well to oom killing because they keep on waiting.

                Linus
--
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]

#1228854 — Re: can't oom-kill zap the victim's memory?

On Sat 19-09-15 15:24:02, Linus Torvalds wrote:
> On Sat, Sep 19, 2015 at 8:03 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> > +
> > +static void oom_unmap_func(struct work_struct *work)
> > +{
> > +       struct mm_struct *mm = xchg(&oom_unmap_mm, NULL);
> > +
> > +       if (!atomic_inc_not_zero(&mm->mm_users))
> > +               return;
> > +
> > +       // If this is not safe we can do use_mm() + unuse_mm()
> > +       down_read(&mm->mmap_sem);
> 
> I don't think this is safe.
> 
> What makes you sure that we might not deadlock on the mmap_sem here?
> For all we know, the process that is going out of memory is in the
> middle of a mmap(), and already holds the mmap_sem for writing. No?
> 
> So at the very least that needs to be a trylock, I think.

Agreed.

> And I'm not
> sure zap_page_range() is ok with the mmap_sem only held for reading.
> Normally our rule is that you can *populate* the page tables
> concurrently, but you can't tear the down

Actually mmap_sem for reading should be sufficient because we do not
alter the layout. Both MADV_DONTNEED and MADV_FREE require read mmap_sem
for example.

-- 
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]

#1228882 — Re: can't oom-kill zap the victim's memory?

On 09/20, Michal Hocko wrote:
>
> On Sat 19-09-15 15:24:02, Linus Torvalds wrote:
> > On Sat, Sep 19, 2015 at 8:03 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> > > +
> > > +static void oom_unmap_func(struct work_struct *work)
> > > +{
> > > +       struct mm_struct *mm = xchg(&oom_unmap_mm, NULL);
> > > +
> > > +       if (!atomic_inc_not_zero(&mm->mm_users))
> > > +               return;
> > > +
> > > +       // If this is not safe we can do use_mm() + unuse_mm()
> > > +       down_read(&mm->mmap_sem);
> >
> > I don't think this is safe.
> >
> > What makes you sure that we might not deadlock on the mmap_sem here?
> > For all we know, the process that is going out of memory is in the
> > middle of a mmap(), and already holds the mmap_sem for writing. No?
> >
> > So at the very least that needs to be a trylock, I think.
>
> Agreed.

Why? See my reply to Linus's email.

Just in case, yes sure the unconditonal down_read() is suboptimal, but
this is minor compared to other problems we need to solve.

> > And I'm not
> > sure zap_page_range() is ok with the mmap_sem only held for reading.
> > Normally our rule is that you can *populate* the page tables
> > concurrently, but you can't tear the down
>
> Actually mmap_sem for reading should be sufficient because we do not
> alter the layout. Both MADV_DONTNEED and MADV_FREE require read mmap_sem
> for example.

Yes, but see the ->vm_flags check in madvise_dontneed().

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]

#1228881 — Re: can't oom-kill zap the victim's memory?

On 09/19, Linus Torvalds wrote:
>
> On Sat, Sep 19, 2015 at 8:03 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> > +
> > +static void oom_unmap_func(struct work_struct *work)
> > +{
> > +       struct mm_struct *mm = xchg(&oom_unmap_mm, NULL);
> > +
> > +       if (!atomic_inc_not_zero(&mm->mm_users))
> > +               return;
> > +
> > +       // If this is not safe we can do use_mm() + unuse_mm()
> > +       down_read(&mm->mmap_sem);
>
> I don't think this is safe.
>
> What makes you sure that we might not deadlock on the mmap_sem here?
> For all we know, the process that is going out of memory is in the
> middle of a mmap(), and already holds the mmap_sem for writing. No?

In this case the workqueue thread will block. But it can not block
forever. I mean if it can then the killed process will never exit
(exit_mm does down_read) and release its memory, so we lose anyway.

But let me repeat this patch is obviously not complete/etc,

> So at the very least that needs to be a trylock, I think.

And we want to avoid using workqueues when the caller can do this
directly. And in this case we certainly need trylock. But this needs
some refactoring: we do not want to do this under oom_lock, otoh it
makes sense to do this from mark_oom_victim() if current && killed,
and a lot more details.

The workqueue thread has other reasons for trylock, but probably not
in the initial version of this patch. And perhaps we should use a
dedicated kthread and do not use workqueues at all. And yes, a single
"mm_struct *oom_unmap_mm" is ugly, it should be the list of mm's to
unmap, but then at least we need MMF_MEMDIE.

> And I'm not
> sure zap_page_range() is ok with the mmap_sem only held for reading.
> Normally our rule is that you can *populate* the page tables
> concurrently, but you can't tear the down.

Well, according to madvise_need_mmap_write() MADV_DONTNEED does this
under down_read().

But yes, yes, this is probably not right anyway. Say, VM_LOCKED...
That is why I mentioned that perhaps this should only unmap the
anonymous pages. We can probably add zap_details->for_oom hint.



Another question if it is safe to abuse the foreign mm this way.
Well, zap_page_range_single() does this, so this is probably safe.
But we can do use_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]

#1228930 — Re: can't oom-kill zap the victim's memory?

On Sun, Sep 20, 2015 at 5:56 AM, Oleg Nesterov <oleg@redhat.com> wrote:
>
> In this case the workqueue thread will block.

What workqueue thread?

   pagefault_out_of_memory ->
      out_of_memory ->
         oom_kill_process

as far as I can tell, this can be called by any task. Now, that
pagefault case should only happen when the page fault comes from user
space, but we also have

   __alloc_pages_slowpath ->
      __alloc_pages_may_oom ->
         out_of_memory ->
            oom_kill_process

which can be called from just about any context (but atomic
allocations will never get here, so it can schedule etc).

So what's your point? Explain again just how do you guarantee that you
can take the mmap_sem.

                       Linus
--
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]

#1228952 — Re: can't oom-kill zap the victim's memory?

On 09/20/15 11:05, Linus Torvalds wrote:
> On Sun, Sep 20, 2015 at 5:56 AM, Oleg Nesterov <oleg@redhat.com> wrote:
>> In this case the workqueue thread will block.
> What workqueue thread?
>
>     pagefault_out_of_memory ->
>        out_of_memory ->
>           oom_kill_process
>
> as far as I can tell, this can be called by any task. Now, that
> pagefault case should only happen when the page fault comes from user
> space, but we also have
>
>     __alloc_pages_slowpath ->
>        __alloc_pages_may_oom ->
>           out_of_memory ->
>              oom_kill_process
>
> which can be called from just about any context (but atomic
> allocations will never get here, so it can schedule etc).

I think in this case the oom killer should just slap a SIGKILL on the 
task and then back out, and whatever needed the memory should just wait 
patiently for the sacrificial lamb to commit seppuku.

Which, btw, we should IMO encourage ASAP in the context of the lamb by 
having anything potentially locky or semaphory pay attention to if the 
task in question has a fatal signal pending, and if so, drop everything 
and run like hell so that the task can cough up any locks or semaphores.
> So what's your point? Explain again just how do you guarantee that you
> can take the mmap_sem.
>
>                         Linus
>
> --
> 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>

Also, I observed that a task in the middle of dumping core doesn't 
respond to signals while it's dumping, and I would guess that might be 
the case even if the task receives a SIGKILL from the OOM handler.  Just 
a potential observation.

--
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]

#1229407 — Re: can't oom-kill zap the victim's memory?

On 09/20, Raymond Jennings wrote:
>
> On 09/20/15 11:05, Linus Torvalds wrote:
>>
>> which can be called from just about any context (but atomic
>> allocations will never get here, so it can schedule etc).
>
> I think in this case the oom killer should just slap a SIGKILL on the
> task and then back out, and whatever needed the memory should just wait
> patiently for the sacrificial lamb to commit seppuku.

Not sure I understand you correctly, but this is what we currently do.
The only problem is that this doesn't work sometimes.

> Also, I observed that a task in the middle of dumping core doesn't
> respond to signals while it's dumping,

How did you observe this? The coredumping is killable.

Although yes, we have problems here in oom condition. In particular
with CLONE_VM tasks.

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]

#1229349 — Re: can't oom-kill zap the victim's memory?

On 09/20, Linus Torvalds wrote:
>
> On Sun, Sep 20, 2015 at 5:56 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> >
> > In this case the workqueue thread will block.
>
> What workqueue thread?

I must have missed something. I can't understand your and Michal's
concerns.

>    pagefault_out_of_memory ->
>       out_of_memory ->
>          oom_kill_process
>
> as far as I can tell, this can be called by any task. Now, that
> pagefault case should only happen when the page fault comes from user
> space, but we also have
>
>    __alloc_pages_slowpath ->
>       __alloc_pages_may_oom ->
>          out_of_memory ->
>             oom_kill_process
>
> which can be called from just about any context (but atomic
> allocations will never get here, so it can schedule etc).

So yes, in general oom_kill_process() can't call oom_unmap_func() directly.
That is why the patch uses queue_work(oom_unmap_func). The workqueue thread
takes mmap_sem and frees the memory allocated by user space.

If this can lead to deadlock somehow, then we can hit the same deadlock
when an oom-killed thread calls exit_mm().

> So what's your point?

This can help if the killed process refuse to die and (of course) it
doesn't hold the mmap_sem for writing. Say, it waits for some mutex
held by the task which tries to alloc the memory and triggers oom.

> Explain again just how do you guarantee that you
> can take the mmap_sem.

This is not guaranteed, down_read(mmap_sem) can block forever. But this
means that the (killed) victim never drops mmap_sem / never exits, so
we lose anyway. We have no memory, oom-killer is blocked, etc.

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]

#1229428 — Re: can't oom-kill zap the victim's memory?

On Mon 21-09-15 15:44:14, Oleg Nesterov wrote:
[...]
> So yes, in general oom_kill_process() can't call oom_unmap_func() directly.
> That is why the patch uses queue_work(oom_unmap_func). The workqueue thread
> takes mmap_sem and frees the memory allocated by user space.

OK, this might have been a bit confusing. I didn't mean you cannot use
mmap_sem directly from the workqueue context. You _can_ AFAICS. But I've
mentioned that you _shouldn't_ use workqueue context in the first place
because all the workers might be blocked on locks and new workers cannot
be created due to memory pressure. This has been demostrated already
where sysrq+f couldn't trigger OOM killer because the work item to do so
was waiting for a worker which never came...

So I think we probably need to do this in the OOM killer context (with
try_lock) or hand over to a special kernel thread. I am not sure a
special kernel thread is really worth that but maybe it will turn out to
be a better choice.
-- 
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]

#1229481 — Re: can't oom-kill zap the victim's memory?

On 09/21, Michal Hocko wrote:
>
> On Mon 21-09-15 15:44:14, Oleg Nesterov wrote:
> [...]
> > So yes, in general oom_kill_process() can't call oom_unmap_func() directly.
> > That is why the patch uses queue_work(oom_unmap_func). The workqueue thread
> > takes mmap_sem and frees the memory allocated by user space.
>
> OK, this might have been a bit confusing. I didn't mean you cannot use
> mmap_sem directly from the workqueue context. You _can_ AFAICS. But I've
> mentioned that you _shouldn't_ use workqueue context in the first place
> because all the workers might be blocked on locks and new workers cannot
> be created due to memory pressure.

Yes, yes, and I already tried to comment this part. We probably need a
dedicated kernel thread, but I still think (although I am not sure) that
initial change can use workueue. In the likely case system_unbound_wq pool
should have an idle thread, if not - OK, this change won't help in this
case. This is minor.

> So I think we probably need to do this in the OOM killer context (with
> try_lock)

Yes we should try to do this in the OOM killer context, and in this case
(of course) we need trylock. Let me quote my previous email:

	And we want to avoid using workqueues when the caller can do this
	directly. And in this case we certainly need trylock. But this needs
	some refactoring: we do not want to do this under oom_lock, otoh it
	makes sense to do this from mark_oom_victim() if current && killed,
	and a lot more details.

and probably this is another reason why do we need MMF_MEMDIE. But again,
I think the initial change should be simple.

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]

#1229524 — Re: can't oom-kill zap the victim's memory?

On Mon 21-09-15 17:32:52, Oleg Nesterov wrote:
> On 09/21, Michal Hocko wrote:
> >
> > On Mon 21-09-15 15:44:14, Oleg Nesterov wrote:
> > [...]
> > > So yes, in general oom_kill_process() can't call oom_unmap_func() directly.
> > > That is why the patch uses queue_work(oom_unmap_func). The workqueue thread
> > > takes mmap_sem and frees the memory allocated by user space.
> >
> > OK, this might have been a bit confusing. I didn't mean you cannot use
> > mmap_sem directly from the workqueue context. You _can_ AFAICS. But I've
> > mentioned that you _shouldn't_ use workqueue context in the first place
> > because all the workers might be blocked on locks and new workers cannot
> > be created due to memory pressure.
> 
> Yes, yes, and I already tried to comment this part.

OK then we are on the same page, good.

> We probably need a
> dedicated kernel thread, but I still think (although I am not sure) that
> initial change can use workueue. In the likely case system_unbound_wq pool
> should have an idle thread, if not - OK, this change won't help in this
> case. This is minor.

The point is that the implementation should be robust from the very
beginning. I am not sure what you mean by the idle thread here but the
rescuer can get stuck the very same way other workers. So I think that
we cannot rely on WQ for a real solution here.

> > So I think we probably need to do this in the OOM killer context (with
> > try_lock)
> 
> Yes we should try to do this in the OOM killer context, and in this case
> (of course) we need trylock. Let me quote my previous email:
> 
> 	And we want to avoid using workqueues when the caller can do this
> 	directly. And in this case we certainly need trylock. But this needs
> 	some refactoring: we do not want to do this under oom_lock,

Why do you think oom_lock would be a big deal? Address space of the
victim might be really large but we can back off after a batch of
unmapped pages.

>       otoh it
> 	makes sense to do this from mark_oom_victim() if current && killed,
> 	and a lot more details.
> 
> and probably this is another reason why do we need MMF_MEMDIE. But again,
> I think the initial change should be simple.

I definitely agree with the simplicity for the first iteration. That
means only unmap private exclusive pages and release at most few megs of
them. I am still not sure about some details, e.g. futex sitting in such
a memory. Wouldn't threads blow up when they see an unmapped futex page,
try to page it in and it would be in an uninitialized state? Maybe this
is safe because they will die anyway but I am not familiar with that
code.
-- 
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]

#1229565 — Re: can't oom-kill zap the victim's memory?

Oleg Nesterov wrote:
> Yes, yes, and I already tried to comment this part. We probably need a
> dedicated kernel thread, but I still think (although I am not sure) that
> initial change can use workueue. In the likely case system_unbound_wq pool
> should have an idle thread, if not - OK, this change won't help in this
> case. This is minor.
> 
I imagined a dedicated kernel thread doing something like shown below.
(I don't know about mm->mmap management.)
mm->mmap_zapped corresponds to MMF_MEMDIE.
I think this kernel thread can be used for normal kill(pid, SIGKILL) cases.

----------
bool has_sigkill_task;
wait_queue_head_t kick_mm_zapper;

static void mm_zapper(void *unused)
{
	struct task_struct *g, *p;
	struct mm_struct *mm;

sleep:
	wait_event(kick_remover, has_sigkill_task);
	has_sigkill_task = false;
restart:
	rcu_read_lock();
	for_each_process_thread(g, p) {
		if (likely(!fatal_signal_pending(p)))
			continue;
		task_lock(p);
		mm = p->mm;
		if (mm && mm->mmap && !mm->mmap_zapped && down_read_trylock(&mm->mmap_sem)) {
			atomic_inc(&mm->mm_users);
			task_unlock(p);
			rcu_read_unlock();
			if (mm->mmap && !mm->mmap_zapped)
				zap_page_range(mm->mmap, 0, TASK_SIZE, NULL);
			mm->mmap_zapped = 1;
			up_read(&mm->mmap_sem);
			mmput(mm);
			cond_resched();
			goto restart;
		}
		task_unlock(p);
	}
	rcu_read_unlock();
	goto sleep;
}

kthread_run(mm_zapper, NULL, "mm_zapper");
----------
--
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]

#1229833 — Re: can't oom-kill zap the victim's memory?

On Mon, 21 Sep 2015, Oleg Nesterov wrote:

> Yes we should try to do this in the OOM killer context, and in this case
> (of course) we need trylock. Let me quote my previous email:
> 
> 	And we want to avoid using workqueues when the caller can do this
> 	directly. And in this case we certainly need trylock. But this needs
> 	some refactoring: we do not want to do this under oom_lock, otoh it
> 	makes sense to do this from mark_oom_victim() if current && killed,
> 	and a lot more details.
> 
> and probably this is another reason why do we need MMF_MEMDIE. But again,
> I think the initial change should be simple.
> 

I agree with the direction and I don't think it would be too complex to 
have a dedicated kthread that is kicked when we queue an mm to do 
MADV_DONTNEED behavior, and have that happen only if a trylock in 
oom_kill_process() fails to do it itself for anonymous mappings.  We may 
have different opinions of simplicity.
--
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]

#1229567 — Re: can't oom-kill zap the victim's memory?

On Mon, Sep 21, 2015 at 6:44 AM, Oleg Nesterov <oleg@redhat.com> wrote:
>
> I must have missed something. I can't understand your and Michal's
> concerns.

Heh.  I looked at that patch, and apparently entirely missed the
queue_work() part of the whole patch, thinking it was a direct call.

So never mind.

                Linus
--
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]

Page 2 of 3 — ← Prev page 1 [2] 3  Next page →

Back to top | Article view | linux.kernel

csiph-web