All of lore.kernel.org
 help / color / mirror / Atom feed
From: Linus Torvalds <torvalds@linux-foundation.org>
To: Michal Hocko <mhocko@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	Mel Gorman <mgorman@suse.de>,
	Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>,
	David Rientjes <rientjes@google.com>,
	Oleg Nesterov <oleg@redhat.com>, Hugh Dickins <hughd@google.com>,
	Andrea Argangeli <andrea@kernel.org>,
	Rik van Riel <riel@redhat.com>, linux-mm <linux-mm@kvack.org>,
	LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 1/2] mm, oom: introduce oom reaper
Date: Thu, 17 Dec 2015 11:55:11 -0800	[thread overview]
Message-ID: <CA+55aFxkzeqtxDY8KyR_FA+WKNkQXEHVA_zO8XhW6rqRr778Zw@mail.gmail.com> (raw)
In-Reply-To: <20151217130223.GE18625@dhcp22.suse.cz>

On Thu, Dec 17, 2015 at 5:02 AM, Michal Hocko <mhocko@kernel.org> wrote:
> Ups. You are right. I will go with msleep_interruptible(100).

I don't think that's right.

If a signal happens, that loop is now (again) just busy-looping. That
doesn't sound right, although with the maximum limit of 10 attempts,
maybe it's fine - the thing is technically "busylooping", but it will
definitely not busy-loop for very long.

So maybe that code is fine, but I think the signal case might at least
merit a comment?

Also, if you actually do want UNINTERRUPTIBLE (no reaction to signals
at all), but don't want to be seen as being "load" on the system, you
can use TASK_IDLE, which is a combination of TASK_UNINTERRUPTIBLE |
TASK_NOLOAD.

Because if you sleep interruptibly, you do generally need to handle
signals (although that limit count may make it ok in this case).

There's basically three levels:

 - TASK_UNINTERRUPTIBLE: no signal handling at all

 - TASK_KILLABLE: no normal signal handling, but ok to be killed
(needs to check fatal_signal_pending() and exit)

 - TASK_INTERRUPTIBLE: will react to signals

(and then that TASK_IDLE thing that is semantically the same as
uninterruptible, but doesn't count against the load average).

The main use for TASK_KILLABLE is in places where expected semantics
do not allow a EINTR return, but we know that because the process is
about to be killed, we can ignore that, for the simple reason that
nobody will ever *see* the EINTR.

Btw, I think you might want to re-run your test-case after this
change, since the whole "busy loop vs actually sleeping" might just
have changed the result..

              Linus

WARNING: multiple messages have this Message-ID (diff)
From: Linus Torvalds <torvalds@linux-foundation.org>
To: Michal Hocko <mhocko@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	Mel Gorman <mgorman@suse.de>,
	Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>,
	David Rientjes <rientjes@google.com>,
	Oleg Nesterov <oleg@redhat.com>, Hugh Dickins <hughd@google.com>,
	Andrea Argangeli <andrea@kernel.org>,
	Rik van Riel <riel@redhat.com>, linux-mm <linux-mm@kvack.org>,
	LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 1/2] mm, oom: introduce oom reaper
Date: Thu, 17 Dec 2015 11:55:11 -0800	[thread overview]
Message-ID: <CA+55aFxkzeqtxDY8KyR_FA+WKNkQXEHVA_zO8XhW6rqRr778Zw@mail.gmail.com> (raw)
In-Reply-To: <20151217130223.GE18625@dhcp22.suse.cz>

On Thu, Dec 17, 2015 at 5:02 AM, Michal Hocko <mhocko@kernel.org> wrote:
> Ups. You are right. I will go with msleep_interruptible(100).

I don't think that's right.

If a signal happens, that loop is now (again) just busy-looping. That
doesn't sound right, although with the maximum limit of 10 attempts,
maybe it's fine - the thing is technically "busylooping", but it will
definitely not busy-loop for very long.

So maybe that code is fine, but I think the signal case might at least
merit a comment?

Also, if you actually do want UNINTERRUPTIBLE (no reaction to signals
at all), but don't want to be seen as being "load" on the system, you
can use TASK_IDLE, which is a combination of TASK_UNINTERRUPTIBLE |
TASK_NOLOAD.

Because if you sleep interruptibly, you do generally need to handle
signals (although that limit count may make it ok in this case).

There's basically three levels:

 - TASK_UNINTERRUPTIBLE: no signal handling at all

 - TASK_KILLABLE: no normal signal handling, but ok to be killed
(needs to check fatal_signal_pending() and exit)

 - TASK_INTERRUPTIBLE: will react to signals

(and then that TASK_IDLE thing that is semantically the same as
uninterruptible, but doesn't count against the load average).

The main use for TASK_KILLABLE is in places where expected semantics
do not allow a EINTR return, but we know that because the process is
about to be killed, we can ignore that, for the simple reason that
nobody will ever *see* the EINTR.

Btw, I think you might want to re-run your test-case after this
change, since the whole "busy loop vs actually sleeping" might just
have changed the result..

              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>

  reply	other threads:[~2015-12-17 19:55 UTC|newest]

Thread overview: 78+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-12-15 18:36 [PATCH 1/2] mm, oom: introduce oom reaper Michal Hocko
2015-12-15 18:36 ` Michal Hocko
2015-12-17  0:50 ` Andrew Morton
2015-12-17  0:50   ` Andrew Morton
2015-12-17 13:02   ` Michal Hocko
2015-12-17 13:02     ` Michal Hocko
2015-12-17 19:55     ` Linus Torvalds [this message]
2015-12-17 19:55       ` Linus Torvalds
2015-12-17 20:00       ` Andrew Morton
2015-12-17 20:00         ` Andrew Morton
2015-12-18 11:54         ` Michal Hocko
2015-12-18 11:54           ` Michal Hocko
2015-12-18 21:14           ` Andrew Morton
2015-12-18 21:14             ` Andrew Morton
2015-12-21  8:38             ` Michal Hocko
2015-12-21  8:38               ` Michal Hocko
2015-12-17 21:13     ` Andrew Morton
2015-12-17 21:13       ` Andrew Morton
2015-12-18 12:11       ` Michal Hocko
2015-12-18 12:11         ` Michal Hocko
2015-12-18 12:10     ` Tetsuo Handa
2015-12-18 12:10       ` Tetsuo Handa
2015-12-20  7:14       ` Tetsuo Handa
2015-12-20  7:14         ` Tetsuo Handa
2015-12-18  0:15 ` Andrew Morton
2015-12-18  0:15   ` Andrew Morton
2015-12-18 11:48   ` Michal Hocko
2015-12-18 11:48     ` Michal Hocko
2015-12-21 20:38 ` Paul Gortmaker
2015-12-21 20:38   ` Paul Gortmaker
2016-01-06  9:10   ` Michal Hocko
2016-01-06  9:10     ` Michal Hocko
2016-01-06 14:26     ` Paul Gortmaker
2016-01-06 14:26       ` Paul Gortmaker
2016-01-06 15:00       ` Michal Hocko
2016-01-06 15:00         ` Michal Hocko
2015-12-23 23:00 ` Ross Zwisler
2015-12-23 23:00   ` Ross Zwisler
2015-12-24  9:47   ` Michal Hocko
2015-12-24  9:47     ` Michal Hocko
2015-12-24 11:06     ` Tetsuo Handa
2015-12-24 11:06       ` Tetsuo Handa
2015-12-24 20:39       ` Ross Zwisler
2015-12-24 20:39         ` Ross Zwisler
2015-12-25 11:41       ` Michal Hocko
2015-12-25 11:41         ` Michal Hocko
2015-12-24 20:44     ` Ross Zwisler
2015-12-24 20:44       ` Ross Zwisler
2015-12-25 11:35       ` Michal Hocko
2015-12-25 11:35         ` Michal Hocko
2015-12-25 11:44         ` Michal Hocko
2015-12-25 11:44           ` Michal Hocko
2016-01-06 15:42 [PATCH 0/2 -mm] oom reaper v4 Michal Hocko
2016-01-06 15:42 ` [PATCH 1/2] mm, oom: introduce oom reaper Michal Hocko
2016-01-06 15:42   ` Michal Hocko
2016-01-07 11:23   ` Tetsuo Handa
2016-01-07 11:23     ` Tetsuo Handa
2016-01-07 12:30     ` Michal Hocko
2016-01-07 12:30       ` Michal Hocko
2016-01-11 22:54   ` Andrew Morton
2016-01-11 22:54     ` Andrew Morton
2016-01-12  8:16     ` Michal Hocko
2016-01-12  8:16       ` Michal Hocko
2016-01-28  1:28   ` David Rientjes
2016-01-28  1:28     ` David Rientjes
2016-01-28 21:42     ` Michal Hocko
2016-01-28 21:42       ` Michal Hocko
2016-02-02  3:02       ` David Rientjes
2016-02-02  3:02         ` David Rientjes
2016-02-02  8:57         ` Michal Hocko
2016-02-02  8:57           ` Michal Hocko
2016-02-02 11:48           ` Tetsuo Handa
2016-02-02 11:48             ` Tetsuo Handa
2016-02-02 22:55             ` David Rientjes
2016-02-02 22:55               ` David Rientjes
2016-02-02 22:51           ` David Rientjes
2016-02-02 22:51             ` David Rientjes
2016-02-03 10:31             ` Tetsuo Handa
2016-02-03 10:31               ` Tetsuo Handa

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CA+55aFxkzeqtxDY8KyR_FA+WKNkQXEHVA_zO8XhW6rqRr778Zw@mail.gmail.com \
    --to=torvalds@linux-foundation.org \
    --cc=akpm@linux-foundation.org \
    --cc=andrea@kernel.org \
    --cc=hughd@google.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=mgorman@suse.de \
    --cc=mhocko@kernel.org \
    --cc=oleg@redhat.com \
    --cc=penguin-kernel@i-love.sakura.ne.jp \
    --cc=riel@redhat.com \
    --cc=rientjes@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.