From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752169AbeA2WiO (ORCPT ); Mon, 29 Jan 2018 17:38:14 -0500 Received: from mail-io0-f179.google.com ([209.85.223.179]:32964 "EHLO mail-io0-f179.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752118AbeA2WiF (ORCPT ); Mon, 29 Jan 2018 17:38:05 -0500 X-Google-Smtp-Source: AH8x224w8WKuIXOllWw1qNlk1hneHHP+LJUgBp8UZ6kweZ9dUoIPkete8BZSphYXVfptmaub6CDrgg== Date: Mon, 29 Jan 2018 14:38:02 -0800 (PST) From: David Rientjes X-X-Sender: rientjes@chino.kir.corp.google.com To: Michal Hocko cc: Andrew Morton , Roman Gushchin , Vladimir Davydov , Johannes Weiner , Tetsuo Handa , Tejun Heo , kernel-team@fb.com, cgroups@vger.kernel.org, linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org, linux-mm@kvack.org Subject: Re: [patch -mm v2 1/3] mm, memcg: introduce per-memcg oom policy tunable In-Reply-To: <20180126171548.GB16763@dhcp22.suse.cz> Message-ID: References: <20180126171548.GB16763@dhcp22.suse.cz> User-Agent: Alpine 2.10 (DEB 1266 2009-07-14) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, 26 Jan 2018, Michal Hocko wrote: > > The cgroup aware oom killer is needlessly declared for the entire system > > by a mount option. It's unnecessary to force the system into a single > > oom policy: either cgroup aware, or the traditional process aware. > > > > This patch introduces a memory.oom_policy tunable for all mem cgroups. > > It is currently a no-op: it can only be set to "none", which is its > > default policy. It will be expanded in the next patch to define cgroup > > aware oom killer behavior. > > > > This is an extensible interface that can be used to define cgroup aware > > assessment of mem cgroup subtrees or the traditional process aware > > assessment. > > > > So what is the actual semantic and scope of this policy. Does it apply > only down the hierarchy. Also how do you compare cgroups with different > policies? Let's say you have > root > / | \ > A B C > / \ / \ > D E F G > > Assume A: cgroup, B: oom_group=1, C: tree, G: oom_group=1 > At each level of the hierarchy, memory.oom_policy compares immediate children, it's the only way that an admin can lock in a specific oom policy like "tree" and then delegate the subtree to the user. If you've configured it as above, comparing A and C should be the same based on the cumulative usage of their child mem cgroups. The policy for B hasn't been specified, but since it does not have any children "cgroup" and "tree" should be the same. > Now we have the global OOM killer to choose a victim. From a quick > glance over those patches, it seems that we will be comparing only > tasks because root->oom_policy != MEMCG_OOM_POLICY_CGROUP. A, B and C > policies are ignored. Right, a policy of "none" reverts its subtree back to per-process comparison if you are either not using the cgroup aware oom killer or your subtree is not using the cgroup aware oom killer. > Moreover If I select any of B's tasks then I will > happily kill it breaking the expectation that the whole memcg will go > away. Weird, don't you think? Or did I misunderstand? > It's just as weird as the behavior of memory.oom_group today without using the mount option :) In that case, mem_cgroup_select_oom_victim() always returns false and the value of memory.oom_group is ignored. I agree that it's weird in -mm and there's nothing preventing us from separating memory.oom_group from the cgroup aware oom killer and allowing it to be set regardless of a selection change. If memory.oom_group is set, and the kill originates from that mem cgroup, kill all processes attached to it and its subtree. This is a criticism of the current implementation in -mm, however, my extension only respects its weirdness. > So let's assume that root: cgroup. Then we are finally comparing > cgroups. D, E, B, C. Of those D, E and F do not have any > policy. Do they inherit their policy from the parent? If they don't then > we should be comparing their tasks separately, no? The code disagrees > because once we are in the cgroup mode, we do not care about separate > tasks. > No, perhaps I wasn't clear in the documentation: the policy at each level of the hierarchy is specified by memory.oom_policy and compares its immediate children with that policy. So the per-cgroup usage of A, B, and C and compared regardless of A, B, and C's own oom policies. > Let's say we choose C because it has the largest cumulative consumption. > It is not oom_group so it will select a task from F, G. Again you are > breaking oom_group policy of G if you kill a single task. So you would > have to be recursive here. That sounds fixable though. Just be > recursive. > I fully agree, but that's (another) implementation detail of what is in -mm that isn't specified. I think where you're going is the complete separation of mem cgroup selection from memory.oom_group. I agree, and we can fix that. memory.oom_group also shouldn't depend on any mount option, it can be set or unset depending on the properties of the workload. > Then you say > > > Another benefit of such an approach is that an admin can lock in a > > certain policy for the system or for a mem cgroup subtree and can > > delegate the policy decision to the user to determine if the kill should > > originate from a subcontainer, as indivisible memory consumers > > themselves, or selection should be done per process. > > And the code indeed doesn't check oom_policy on each level of the > hierarchy, unless I am missing something. So the subgroup is simply > locked in to the oom_policy parent has chosen. That is not the case for > the tree policy. > > So look how we are comparing cumulative groups without policy with > groups with policy with subtrees. Either I have grossly misunderstood > something or this is massively inconsistent and it doesn't make much > sense to me. Root memcg without cgroup policy will simply turn off the > whole thing for the global OOM case. So you really need to enable it > there but then it is not really clear how to configure lower levels. > > From the above it seems that you are more interested in memcg OOMs and > want to give different hierarchies different policies but you quickly > hit the similar inconsistencies there as well. > > I am not sure how extensible this is actually. How do we place > priorities on top? > If you're referring to strict priorities where one cgroup can be preferred or biased against regardless of usage, that would be an extension with yet another tunable. Userspace influence over the selection is not addressed by this patchset, nor is the unfair comparison of the root mem cgroup with leaf mem cgroups. My suggestion previously was a memory.oom_value tunable, which is configured depending on its parent's memory.oom_policy. If "cgroup" or "tree" it behaves as oom_score_adj-type behavior, i.e. it's an adjustment on the usage. This way, a subtree's usage can have a certain amount of memory discounted, for example, because it is supposed to use more than 50% of memory. If a new "priority" memory.oom_policy were implemented, which would be trivial, comparison between child cgroups would be as simple as comparing memory.oom_value integers.