Re: [PATCH] oom_reaper: switch to struct list_head for reap queue

[Michal Hocko]

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