All of lore.kernel.org
 help / color / mirror / Atom feed
From: Michal Hocko <mhocko@kernel.org>
To: Aleksa Sarai <asarai@suse.de>
Cc: Oleg Nesterov <oleg@redhat.com>,
	Johannes Weiner <hannes@cmpxchg.org>,
	Ingo Molnar <mingo@redhat.com>,
	Peter Zijlstra <peterz@infradead.org>,
	Andrew Morton <akpm@linux-foundation.org>,
	linux-kernel@vger.kernel.org, linux-mm@kvack.org,
	cyphar@cyphar.com
Subject: Re: [PATCH] oom_reaper: switch to struct list_head for reap queue
Date: Mon, 20 Feb 2017 16:53:51 +0100	[thread overview]
Message-ID: <20170220155350.GL2431@dhcp22.suse.cz> (raw)
In-Reply-To: <a35d6271-f9b3-834c-79da-30d522ec4813@suse.de>

On Wed 15-02-17 20:01:33, Aleksa Sarai wrote:
> > > > This is an extra pointer to task_struct and more lines of code to
> > > > accomplish the same thing. Why would we want to do that?
> > > 
> > > I don't think it's more "actual" lines of code (I think the wrapping is
> > > inflating the line number count),
> > 
> > I too think it doesn't make sense to blow task_struct and the generated code.
> > And to me this patch doesn't make the source code more clean.
> > 
> > > but switching it means that it's more in
> > > line with other queues in the kernel (it took me a bit to figure out what
> > > was going on with oom_reaper_list beforehand).
> > 
> > perhaps you can turn oom_reaper_list into llist_head then. This will also
> > allow to remove oom_reaper_lock. Not sure this makes sense too.
> 
> Actually, I just noticed that the original implementation is a stack not a
> queue. So the reaper will always reap the *most recent* task to get OOMed as
> opposed to the least recent. Since select_bad_process() will always pick
> worse processes first, this means that the reaper will reap "less bad"
> processes (lower oom score) before it reaps worse ones (higher oom score).
> 
> While it's not a /huge/ deal (N is going to be small in most OOM cases), is
> this something that we should consider?

Not really. Because the oom killer will back off if there is an oom
victim in the same oom domain currently selected (see
oom_evaluate_task). So more oom tasks queued for the oom reaper will
usually happen when we have parallel OOM in different oom domains
(cpusets/node_masks, memcgs) and then it really doesn't matter which one
we choose first.

-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

WARNING: multiple messages have this Message-ID (diff)
From: Michal Hocko <mhocko@kernel.org>
To: Aleksa Sarai <asarai@suse.de>
Cc: Oleg Nesterov <oleg@redhat.com>,
	Johannes Weiner <hannes@cmpxchg.org>,
	Ingo Molnar <mingo@redhat.com>,
	Peter Zijlstra <peterz@infradead.org>,
	Andrew Morton <akpm@linux-foundation.org>,
	linux-kernel@vger.kernel.org, linux-mm@kvack.org,
	cyphar@cyphar.com
Subject: Re: [PATCH] oom_reaper: switch to struct list_head for reap queue
Date: Mon, 20 Feb 2017 16:53:51 +0100	[thread overview]
Message-ID: <20170220155350.GL2431@dhcp22.suse.cz> (raw)
In-Reply-To: <a35d6271-f9b3-834c-79da-30d522ec4813@suse.de>

On Wed 15-02-17 20:01:33, Aleksa Sarai wrote:
> > > > This is an extra pointer to task_struct and more lines of code to
> > > > accomplish the same thing. Why would we want to do that?
> > > 
> > > I don't think it's more "actual" lines of code (I think the wrapping is
> > > inflating the line number count),
> > 
> > I too think it doesn't make sense to blow task_struct and the generated code.
> > And to me this patch doesn't make the source code more clean.
> > 
> > > but switching it means that it's more in
> > > line with other queues in the kernel (it took me a bit to figure out what
> > > was going on with oom_reaper_list beforehand).
> > 
> > perhaps you can turn oom_reaper_list into llist_head then. This will also
> > allow to remove oom_reaper_lock. Not sure this makes sense too.
> 
> Actually, I just noticed that the original implementation is a stack not a
> queue. So the reaper will always reap the *most recent* task to get OOMed as
> opposed to the least recent. Since select_bad_process() will always pick
> worse processes first, this means that the reaper will reap "less bad"
> processes (lower oom score) before it reaps worse ones (higher oom score).
> 
> While it's not a /huge/ deal (N is going to be small in most OOM cases), is
> this something that we should consider?

Not really. Because the oom killer will back off if there is an oom
victim in the same oom domain currently selected (see
oom_evaluate_task). So more oom tasks queued for the oom reaper will
usually happen when we have parallel OOM in different oom domains
(cpusets/node_masks, memcgs) and then it really doesn't matter which one
we choose first.

-- 
Michal Hocko
SUSE Labs

  reply	other threads:[~2017-02-20 15:53 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-02-14 15:07 [PATCH] oom_reaper: switch to struct list_head for reap queue Aleksa Sarai
2017-02-14 15:07 ` Aleksa Sarai
2017-02-14 16:30 ` Johannes Weiner
2017-02-14 16:30   ` Johannes Weiner
2017-02-14 16:52   ` Aleksa Sarai
2017-02-14 16:52     ` Aleksa Sarai
2017-02-14 17:37     ` Oleg Nesterov
2017-02-14 17:37       ` Oleg Nesterov
2017-02-15  9:01       ` Aleksa Sarai
2017-02-15  9:01         ` Aleksa Sarai
2017-02-20 15:53         ` Michal Hocko [this message]
2017-02-20 15:53           ` Michal Hocko
2017-02-15  8:08     ` Ingo Molnar
2017-02-15  8:08       ` Ingo Molnar

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=20170220155350.GL2431@dhcp22.suse.cz \
    --to=mhocko@kernel.org \
    --cc=akpm@linux-foundation.org \
    --cc=asarai@suse.de \
    --cc=cyphar@cyphar.com \
    --cc=hannes@cmpxchg.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=mingo@redhat.com \
    --cc=oleg@redhat.com \
    --cc=peterz@infradead.org \
    /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.