linux-kernel - Re: [PATCH] oom_reaper: switch to struct list

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Mon, 20 Feb 2017 16:53:51 +0100
From:   Michal Hocko <mhocko@...nel.org>
To:     Aleksa Sarai <asarai@...e.de>
Cc:     Oleg Nesterov <oleg@...hat.com>,
        Johannes Weiner <hannes@...xchg.org>,
        Ingo Molnar <mingo@...hat.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        linux-kernel@...r.kernel.org, linux-mm@...ck.org, cyphar@...har.com
Subject: Re: [PATCH] oom_reaper: switch to struct list_head for reap queue

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