linux-kernel - Re: [v11 4/6] mm, oom: introduce memory.oom

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20171006121739.2rsr6aiqeodcczoe@dhcp22.suse.cz>
Date:   Fri, 6 Oct 2017 14:17:39 +0200
From:   Michal Hocko <mhocko@...nel.org>
To:     Roman Gushchin <guro@...com>
Cc:     linux-mm@...ck.org, Vladimir Davydov <vdavydov.dev@...il.com>,
        Johannes Weiner <hannes@...xchg.org>,
        Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>,
        David Rientjes <rientjes@...gle.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Tejun Heo <tj@...nel.org>, kernel-team@...com,
        cgroups@...r.kernel.org, linux-doc@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [v11 4/6] mm, oom: introduce memory.oom_group

On Fri 06-10-17 13:04:35, Roman Gushchin wrote:
> On Thu, Oct 05, 2017 at 04:31:04PM +0200, Michal Hocko wrote:
> > Btw. here is how I would do the recursive oom badness. The diff is not
> > the nicest one because there is some code moving but the resulting code
> > is smaller and imho easier to grasp. Only compile tested though
> 
> Thanks!
> 
> I'm not against this approach, and maybe it can lead to a better code,
> but the version you sent is just not there yet.
> 
> There are some problems with it:
> 
> 1) If there are nested cgroups with oom_group set, you will calculate
> a badness multiple times, and rely on the fact, that top memcg will
> become the largest score. It can be optimized, of course, but it's
> additional code.

right. As I've said we can introduce iterator helper to skip the subtree
but I suspect it will not make much of a difference.

> 
> 2) cgroup_has_tasks() probably requires additional locking.
> Maybe it's ok to read nr_populated_csets without explicit locking,
> but it's not obvious for me.

I do not see why. Tasks are free to come and go and you only know at the
time you are killing.

> 3) Returning -1 from memcg_oom_badness() if eligible is equal to 0
> is suspicious.

I didn't spend too much time on it. I merely wanted to point out my
thinking more specifically than the pseudo code posted earlier. But this
should be ok, because that would mean that either all tasks are
OOM_SCORE_ADJ_MIN (eligible = 0) or there is a inflight victim (eligible
= -1). Anyway the initialization should go inside the tree walk

> Right now your version has exactly the same amount of code
> (skipping comments). I assume, this approach just requires some additional
> thinking/rework.

Well, this is not about the amount of code but more about the clear
logic implemented at the correct level. It is simply much easier when
you evaluate the killable entity at one place rather open code it.

But as I've said nothing I would want to enforce.
-- 
Michal Hocko
SUSE Labs