From mboxrd@z Thu Jan 1 00:00:00 1970 From: Roman Gushchin Subject: Re: [v10 4/6] mm, oom: introduce memory.oom_group Date: Thu, 5 Oct 2017 13:32:14 +0100 Message-ID: <20171005123214.GA15459@castle.dhcp.TheFacebook.com> References: <20171004154638.710-1-guro@fb.com> <20171004154638.710-5-guro@fb.com> <20171005120649.st2qt6brlf2xyncq@dhcp22.suse.cz> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Return-path: DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=fb.com; h=date : from : to : cc : subject : message-id : references : mime-version : content-type : in-reply-to; s=facebook; bh=22paTpx17Q7vLbu+HAr2eDg0ON+1RsAMZoPfJ8Arh1I=; b=UPvgfIP0gYQDeKjQxFMAjbYcahay1vPPdy7PApPwfST2/mVWCJDda6xZbtPDAnbvXqzY mmeUjyAWimzWDVPLqoL6pnGYY7XpHs6DUJE74ArzoE56gBNZVS6x7rGVUNB2o+hOa4FZ dHeH0uYyEYI/lKdcAdIdCMxykJ7ZSSOsKZ0= DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=fb.onmicrosoft.com; s=selector1-fb-com; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version; bh=22paTpx17Q7vLbu+HAr2eDg0ON+1RsAMZoPfJ8Arh1I=; b=gSQhRcqp0hKu+XfbW6/Cn5yth4cUXlDV+o/SRIVASh73836LX4iV0QCuh7MXhT14gJNXnPcj3Vo0bKI9gnBYQObla6BVOUdgZuTnZrkb39bULL8ZofxvyNeqdsmb12S47rQfy25GWJ0tFrKbzRKQXmIqFadV3AEgIA0KqwKRJig= Content-Disposition: inline In-Reply-To: <20171005120649.st2qt6brlf2xyncq@dhcp22.suse.cz> Sender: linux-doc-owner@vger.kernel.org List-ID: Content-Transfer-Encoding: 7bit To: Michal Hocko Cc: linux-mm@kvack.org, Vladimir Davydov , Johannes Weiner , Tetsuo Handa , David Rientjes , Andrew Morton , Tejun Heo , kernel-team@fb.com, cgroups@vger.kernel.org, linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org On Thu, Oct 05, 2017 at 02:06:49PM +0200, Michal Hocko wrote: > On Wed 04-10-17 16:46:36, Roman Gushchin wrote: > > The cgroup-aware OOM killer treats leaf memory cgroups as memory > > consumption entities and performs the victim selection by comparing > > them based on their memory footprint. Then it kills the biggest task > > inside the selected memory cgroup. > > > > But there are workloads, which are not tolerant to a such behavior. > > Killing a random task may leave the workload in a broken state. > > > > To solve this problem, memory.oom_group knob is introduced. > > It will define, whether a memory group should be treated as an > > indivisible memory consumer, compared by total memory consumption > > with other memory consumers (leaf memory cgroups and other memory > > cgroups with memory.oom_group set), and whether all belonging tasks > > should be killed if the cgroup is selected. > > > > If set on memcg A, it means that in case of system-wide OOM or > > memcg-wide OOM scoped to A or any ancestor cgroup, all tasks, > > belonging to the sub-tree of A will be killed. If OOM event is > > scoped to a descendant cgroup (A/B, for example), only tasks in > > that cgroup can be affected. OOM killer will never touch any tasks > > outside of the scope of the OOM event. > > > > Also, tasks with oom_score_adj set to -1000 will not be killed. > > I would extend the last sentence with an explanation. What about the > following: > " > Also, tasks with oom_score_adj set to -1000 will not be killed because > this has been a long established way to protect a particular process > from seeing an unexpected SIGKILL from the oom killer. Ignoring this > user defined configuration might lead to data corruptions or other > misbehavior. > " Added, thanks! > > few mostly nit picks below but this looks good other than that. Once the > fix mentioned in patch 3 is folded I will ack this. > > [...] > > > static void select_victim_memcg(struct mem_cgroup *root, struct oom_control *oc) > > { > > - struct mem_cgroup *iter; > > + struct mem_cgroup *iter, *group = NULL; > > + long group_score = 0; > > > > oc->chosen_memcg = NULL; > > oc->chosen_points = 0; > > > > /* > > + * If OOM is memcg-wide, and the memcg has the oom_group flag set, > > + * all tasks belonging to the memcg should be killed. > > + * So, we mark the memcg as a victim. > > + */ > > + if (oc->memcg && mem_cgroup_oom_group(oc->memcg)) { > > we have is_memcg_oom() helper which is esier to read and understand than > the explicit oc->memcg check It's defined in oom_kill.c and not exported, so I'm not sure. > > > + oc->chosen_memcg = oc->memcg; > > + css_get(&oc->chosen_memcg->css); > > + return; > > + } > > + > > + /* > > * The oom_score is calculated for leaf memory cgroups (including > > * the root memcg). > > + * Non-leaf oom_group cgroups accumulating score of descendant > > + * leaf memory cgroups. > > */ > > rcu_read_lock(); > > for_each_mem_cgroup_tree(iter, root) { > > long score; > > > > + /* > > + * We don't consider non-leaf non-oom_group memory cgroups > > + * as OOM victims. > > + */ > > + if (memcg_has_children(iter) && !mem_cgroup_oom_group(iter)) > > + continue; > > + > > + /* > > + * If group is not set or we've ran out of the group's sub-tree, > > + * we should set group and reset group_score. > > + */ > > + if (!group || group == root_mem_cgroup || > > + !mem_cgroup_is_descendant(iter, group)) { > > + group = iter; > > + group_score = 0; > > + } > > + > > hmm, I thought you would go with a recursive oom_evaluate_memcg > implementation that would result in a more readable code IMHO. It is > true that we would traverse oom_group more times. But I do not expect > we would have very deep memcg hierarchies in the majority of workloads > and even if we did then this is a cold path which should focus on > readability more than a performance. Also implementing > mem_cgroup_iter_skip_subtree shouldn't be all that hard if this ever > turns out a real problem. I've tried to go this way, but I didn't like the result. These both loops will share a lot of code (e.g. breaking on finding a previous victim, skipping non-leaf non-oom-group memcgs, etc), so the result is more messy. And actually it's strange to start a new loop to iterate exactly over the same sub-tree, which you want to skip in the first loop. > > Anyway this is nothing really fundamental so I will leave the decision > on you. > > > +static bool oom_kill_memcg_victim(struct oom_control *oc) > > +{ > > if (oc->chosen_memcg == NULL || oc->chosen_memcg == INFLIGHT_VICTIM) > > return oc->chosen_memcg; > > > > - /* Kill a task in the chosen memcg with the biggest memory footprint */ > > - oc->chosen_points = 0; > > - oc->chosen_task = NULL; > > - mem_cgroup_scan_tasks(oc->chosen_memcg, oom_evaluate_task, oc); > > - > > - if (oc->chosen_task == NULL || oc->chosen_task == INFLIGHT_VICTIM) > > - goto out; > > - > > - __oom_kill_process(oc->chosen_task); > > + /* > > + * If memory.oom_group is set, kill all tasks belonging to the sub-tree > > + * of the chosen memory cgroup, otherwise kill the task with the biggest > > + * memory footprint. > > + */ > > + if (mem_cgroup_oom_group(oc->chosen_memcg)) { > > + mem_cgroup_scan_tasks(oc->chosen_memcg, oom_kill_memcg_member, > > + NULL); > > + /* We have one or more terminating processes at this point. */ > > + oc->chosen_task = INFLIGHT_VICTIM; > > it took me a while to realize we need this because of return > !!oc->chosen_task in out_of_memory. Subtle... Also a reason to hate > oc->chosen_* thingy. As I've said in other reply, don't worry about this > I will probably turn my hate into a patch ;) > > > + } else { > > + oc->chosen_points = 0; > > + oc->chosen_task = NULL; > > + mem_cgroup_scan_tasks(oc->chosen_memcg, oom_evaluate_task, oc); > > + > > + if (oc->chosen_task == NULL || > > + oc->chosen_task == INFLIGHT_VICTIM) > > + goto out; > > How can this happen? There shouldn't be any INFLIGHT_VICTIM in our memcg > because we have checked for that already. I can see how we do not find > any task because those can terminate by the time we get here but no new > oom victim should appear we are under the oom_lock. You're probably right, but I would prefer to have this check in place, rather then get a panic on attempt to kill an INFLIGHT_VICTIM task one day. In general, I do not like this trick with using this special value to signal the existence of in-flight victims. It adds a lot of complexity, and non-obvious code. I assume, it's a good target for the following refactoring. Thanks! From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr0-f199.google.com (mail-wr0-f199.google.com [209.85.128.199]) by kanga.kvack.org (Postfix) with ESMTP id D59156B0033 for ; Thu, 5 Oct 2017 08:32:55 -0400 (EDT) Received: by mail-wr0-f199.google.com with SMTP id y44so3756893wry.3 for ; Thu, 05 Oct 2017 05:32:55 -0700 (PDT) Received: from mx0a-00082601.pphosted.com (mx0a-00082601.pphosted.com. [67.231.145.42]) by mx.google.com with ESMTPS id 60si1589205edb.321.2017.10.05.05.32.53 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 05 Oct 2017 05:32:54 -0700 (PDT) Date: Thu, 5 Oct 2017 13:32:14 +0100 From: Roman Gushchin Subject: Re: [v10 4/6] mm, oom: introduce memory.oom_group Message-ID: <20171005123214.GA15459@castle.dhcp.TheFacebook.com> References: <20171004154638.710-1-guro@fb.com> <20171004154638.710-5-guro@fb.com> <20171005120649.st2qt6brlf2xyncq@dhcp22.suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Disposition: inline In-Reply-To: <20171005120649.st2qt6brlf2xyncq@dhcp22.suse.cz> Sender: owner-linux-mm@kvack.org List-ID: To: Michal Hocko Cc: linux-mm@kvack.org, Vladimir Davydov , Johannes Weiner , Tetsuo Handa , David Rientjes , Andrew Morton , Tejun Heo , kernel-team@fb.com, cgroups@vger.kernel.org, linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org On Thu, Oct 05, 2017 at 02:06:49PM +0200, Michal Hocko wrote: > On Wed 04-10-17 16:46:36, Roman Gushchin wrote: > > The cgroup-aware OOM killer treats leaf memory cgroups as memory > > consumption entities and performs the victim selection by comparing > > them based on their memory footprint. Then it kills the biggest task > > inside the selected memory cgroup. > > > > But there are workloads, which are not tolerant to a such behavior. > > Killing a random task may leave the workload in a broken state. > > > > To solve this problem, memory.oom_group knob is introduced. > > It will define, whether a memory group should be treated as an > > indivisible memory consumer, compared by total memory consumption > > with other memory consumers (leaf memory cgroups and other memory > > cgroups with memory.oom_group set), and whether all belonging tasks > > should be killed if the cgroup is selected. > > > > If set on memcg A, it means that in case of system-wide OOM or > > memcg-wide OOM scoped to A or any ancestor cgroup, all tasks, > > belonging to the sub-tree of A will be killed. If OOM event is > > scoped to a descendant cgroup (A/B, for example), only tasks in > > that cgroup can be affected. OOM killer will never touch any tasks > > outside of the scope of the OOM event. > > > > Also, tasks with oom_score_adj set to -1000 will not be killed. > > I would extend the last sentence with an explanation. What about the > following: > " > Also, tasks with oom_score_adj set to -1000 will not be killed because > this has been a long established way to protect a particular process > from seeing an unexpected SIGKILL from the oom killer. Ignoring this > user defined configuration might lead to data corruptions or other > misbehavior. > " Added, thanks! > > few mostly nit picks below but this looks good other than that. Once the > fix mentioned in patch 3 is folded I will ack this. > > [...] > > > static void select_victim_memcg(struct mem_cgroup *root, struct oom_control *oc) > > { > > - struct mem_cgroup *iter; > > + struct mem_cgroup *iter, *group = NULL; > > + long group_score = 0; > > > > oc->chosen_memcg = NULL; > > oc->chosen_points = 0; > > > > /* > > + * If OOM is memcg-wide, and the memcg has the oom_group flag set, > > + * all tasks belonging to the memcg should be killed. > > + * So, we mark the memcg as a victim. > > + */ > > + if (oc->memcg && mem_cgroup_oom_group(oc->memcg)) { > > we have is_memcg_oom() helper which is esier to read and understand than > the explicit oc->memcg check It's defined in oom_kill.c and not exported, so I'm not sure. > > > + oc->chosen_memcg = oc->memcg; > > + css_get(&oc->chosen_memcg->css); > > + return; > > + } > > + > > + /* > > * The oom_score is calculated for leaf memory cgroups (including > > * the root memcg). > > + * Non-leaf oom_group cgroups accumulating score of descendant > > + * leaf memory cgroups. > > */ > > rcu_read_lock(); > > for_each_mem_cgroup_tree(iter, root) { > > long score; > > > > + /* > > + * We don't consider non-leaf non-oom_group memory cgroups > > + * as OOM victims. > > + */ > > + if (memcg_has_children(iter) && !mem_cgroup_oom_group(iter)) > > + continue; > > + > > + /* > > + * If group is not set or we've ran out of the group's sub-tree, > > + * we should set group and reset group_score. > > + */ > > + if (!group || group == root_mem_cgroup || > > + !mem_cgroup_is_descendant(iter, group)) { > > + group = iter; > > + group_score = 0; > > + } > > + > > hmm, I thought you would go with a recursive oom_evaluate_memcg > implementation that would result in a more readable code IMHO. It is > true that we would traverse oom_group more times. But I do not expect > we would have very deep memcg hierarchies in the majority of workloads > and even if we did then this is a cold path which should focus on > readability more than a performance. Also implementing > mem_cgroup_iter_skip_subtree shouldn't be all that hard if this ever > turns out a real problem. I've tried to go this way, but I didn't like the result. These both loops will share a lot of code (e.g. breaking on finding a previous victim, skipping non-leaf non-oom-group memcgs, etc), so the result is more messy. And actually it's strange to start a new loop to iterate exactly over the same sub-tree, which you want to skip in the first loop. > > Anyway this is nothing really fundamental so I will leave the decision > on you. > > > +static bool oom_kill_memcg_victim(struct oom_control *oc) > > +{ > > if (oc->chosen_memcg == NULL || oc->chosen_memcg == INFLIGHT_VICTIM) > > return oc->chosen_memcg; > > > > - /* Kill a task in the chosen memcg with the biggest memory footprint */ > > - oc->chosen_points = 0; > > - oc->chosen_task = NULL; > > - mem_cgroup_scan_tasks(oc->chosen_memcg, oom_evaluate_task, oc); > > - > > - if (oc->chosen_task == NULL || oc->chosen_task == INFLIGHT_VICTIM) > > - goto out; > > - > > - __oom_kill_process(oc->chosen_task); > > + /* > > + * If memory.oom_group is set, kill all tasks belonging to the sub-tree > > + * of the chosen memory cgroup, otherwise kill the task with the biggest > > + * memory footprint. > > + */ > > + if (mem_cgroup_oom_group(oc->chosen_memcg)) { > > + mem_cgroup_scan_tasks(oc->chosen_memcg, oom_kill_memcg_member, > > + NULL); > > + /* We have one or more terminating processes at this point. */ > > + oc->chosen_task = INFLIGHT_VICTIM; > > it took me a while to realize we need this because of return > !!oc->chosen_task in out_of_memory. Subtle... Also a reason to hate > oc->chosen_* thingy. As I've said in other reply, don't worry about this > I will probably turn my hate into a patch ;) > > > + } else { > > + oc->chosen_points = 0; > > + oc->chosen_task = NULL; > > + mem_cgroup_scan_tasks(oc->chosen_memcg, oom_evaluate_task, oc); > > + > > + if (oc->chosen_task == NULL || > > + oc->chosen_task == INFLIGHT_VICTIM) > > + goto out; > > How can this happen? There shouldn't be any INFLIGHT_VICTIM in our memcg > because we have checked for that already. I can see how we do not find > any task because those can terminate by the time we get here but no new > oom victim should appear we are under the oom_lock. You're probably right, but I would prefer to have this check in place, rather then get a panic on attempt to kill an INFLIGHT_VICTIM task one day. In general, I do not like this trick with using this special value to signal the existence of in-flight victims. It adds a lot of complexity, and non-obvious code. I assume, it's a good target for the following refactoring. Thanks! -- 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: email@kvack.org From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751381AbdJEMc5 (ORCPT ); Thu, 5 Oct 2017 08:32:57 -0400 Received: from mx0a-00082601.pphosted.com ([67.231.145.42]:49364 "EHLO mx0a-00082601.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751295AbdJEMcy (ORCPT ); Thu, 5 Oct 2017 08:32:54 -0400 Date: Thu, 5 Oct 2017 13:32:14 +0100 From: Roman Gushchin To: Michal Hocko CC: , Vladimir Davydov , Johannes Weiner , Tetsuo Handa , David Rientjes , Andrew Morton , Tejun Heo , , , , Subject: Re: [v10 4/6] mm, oom: introduce memory.oom_group Message-ID: <20171005123214.GA15459@castle.dhcp.TheFacebook.com> References: <20171004154638.710-1-guro@fb.com> <20171004154638.710-5-guro@fb.com> <20171005120649.st2qt6brlf2xyncq@dhcp22.suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Disposition: inline In-Reply-To: <20171005120649.st2qt6brlf2xyncq@dhcp22.suse.cz> User-Agent: Mutt/1.9.0 (2017-09-02) X-Originating-IP: [2620:10d:c092:200::1:a497] X-ClientProxiedBy: AM5PR06CA0032.eurprd06.prod.outlook.com (2603:10a6:206:2::45) To CO1PR15MB1077.namprd15.prod.outlook.com (2a01:111:e400:7b66::7) X-MS-PublicTrafficType: Email X-MS-Office365-Filtering-Correlation-Id: d94af318-cf95-4306-db12-08d50bed2736 X-Microsoft-Antispam: UriScan:;BCL:0;PCL:0;RULEID:(22001)(2017030254152)(2017052603199)(201703131423075)(201703031133081)(201702281549075);SRVR:CO1PR15MB1077; X-Microsoft-Exchange-Diagnostics: 1;CO1PR15MB1077;3:5leZKQlQgAtZ59o38NPGUG8+ywPlzLCG6qsEk5+Sfeb500gtTJK49q6SXFDI6gm3qOySH+7hD2jXCxCq4ZxQm4cHvaMa+5m8ePCWZM3oSmU4JiezHjZ3Ptoxu21v0wtAkvdclMv4zWntks/+zuwbvq11Qf1Ywqca6q6vY1n0tg2l4BLc2JYDWBagSDqOC5KqXSD3QeSZId0nB8rG6liU2He7QTTFpQFQBS0d/rOTneRactN2rOwMgaVJNVcFHrCh;25:nFfDlCI73/9YDnWcqr8OE/3gojkBNcKcfn4uuxEuI3xAzIiKMEPhuTe2wC+5IoSluFaP/2JEA2cEcltSnCb8YoE4wnZbmPXEMn0H3jWYMD87SQ2G1m2JYG4ox/YprJj2BYYpziq59C7q2UgjxP+yvwx1RMBXke53eihHAMD1hACJju3pn92qPZtsRTXnL9fX7P+sVrhP+fj6/4rktqXpo3B+P0+e2/wSexcIcAjEZAMWCU0GnfPZhHiU7uvesbF/kaU2FfUAXn3PxqGaRws4dLMd/vdGIPanHoPoBQJUWG48Q8Zx6SUIUpGwSJn4qD645nwV8ayILDq2azDjcaOyew==;31:a/RuiB0VUyf5DZX6msqTe/+Z6BHJI2/Oqtp+PZ563ADH5rpt/rqZ7hxYYBWCrU28WVSH4cynwaZGH98kEIJRYkMOmR+SciOtmJwu73zz1Yf4n9Vcfh3I9CUfy+uukVgN1meIxziAF1JqhOhZUM5+mzarSTIDI/eBnLvjSHShNlX2nSeIZMoOvdXPA3nwWUppiVaa4O3ufuqE405eyDqqxApamFg1qrUKCDFUQh/oHFo= X-MS-TrafficTypeDiagnostic: CO1PR15MB1077: X-Microsoft-Exchange-Diagnostics: 1;CO1PR15MB1077;20:O+LCYVCQnVCzFD5YnDCw6GhB3zPrfaBLQrSDj/aEZzy2MgePyWgUBoQzTXX6QbGHnLYmNY9Cl+rINCFZ9s9uesNToUZh6a8TAiYKhGno8Y5lOqZgANLbfn2sMVr+PwSx/w+be9FPRnRFLcYngutin7hWp82k8TqOua3w5JEMRRY2/1Ws/raN739K156+UQcJLHRzBM74rks8XeWNLpqLyXULmfFHY7z7TAY3comRq6UCHQTCMLCbAQqVsyGTG/nyxxW4iQEScQ8lT/NDztJSIpLv70DAMO9ajSvRRJB6j0de9gUio4falNlm/ImL9ES+RZbVpSC862OV6lCCelh64lhBJKYW4vnpSa7lJ84RtA1g0W8jHAftH3knH8nNfy8Y+bRuchnKOGbQItT5MxgwPqegrrRxMPSiaue7QcYq2f/raOTA/vDam4a0Om/ja8/9KVChw7NB0gESyQOIMNW0lO1RNpAN2TAmbkgd4hcNilmAyWrRI5hCRxfm9orTDfmT;4:nQ6sj7WipuxxQTJWPYp0mCVCpChIaeqfx0ISNH0QqiRmHf/FghzNidKyC90FpvUXCCuS9UtqJkfcVWBkL/Xh0OJq6q6yh2L+7tcyRfXFPoAu9CnwGyJo3XS46JnSoSzOzrEdpnGUdJzOggfP3iMyR0gLUr3FE63L8H6ZDwFPfdfjXJGig9l5Ao7Lq+8eVixXR3czlfs2WRb+s2m2mooKYVAZYwQsEWjQwO6Z8Abp5mTXOvjMt8ztduCScKpNXQw275atniO3UWLv7OHmlrTSndoMxDTgVO2tsQ9mSIn5Kwo= X-Exchange-Antispam-Report-Test: UriScan:(17755550239193); X-Microsoft-Antispam-PRVS: X-Exchange-Antispam-Report-CFA-Test: BCL:0;PCL:0;RULEID:(100000700101)(100105000095)(100000701101)(100105300095)(100000702101)(100105100095)(6040450)(2401047)(5005006)(8121501046)(3002001)(93006095)(93001095)(10201501046)(100000703101)(100105400095)(6041248)(20161123564025)(201703131423075)(201702281528075)(201703061421075)(201703061406153)(20161123560025)(20161123562025)(20161123555025)(20161123558100)(6072148)(201708071742011)(100000704101)(100105200095)(100000705101)(100105500095);SRVR:CO1PR15MB1077;BCL:0;PCL:0;RULEID:(100000800101)(100110000095)(100000801101)(100110300095)(100000802101)(100110100095)(100000803101)(100110400095)(100000804101)(100110200095)(100000805101)(100110500095);SRVR:CO1PR15MB1077; X-Forefront-PRVS: 04519BA941 X-Forefront-Antispam-Report: SFV:NSPM;SFS:(10019020)(6009001)(346002)(376002)(189002)(24454002)(377424004)(199003)(4326008)(1076002)(7736002)(5660300001)(7416002)(189998001)(86362001)(54356999)(101416001)(6246003)(9686003)(50986999)(6506006)(50466002)(53936002)(47776003)(39060400002)(478600001)(105586002)(55016002)(76176999)(33656002)(106356001)(6116002)(97736004)(305945005)(2906002)(8676002)(6666003)(81156014)(8936002)(2950100002)(25786009)(81166006)(54906003)(316002)(83506001)(58126008)(16586007)(68736007)(6916009)(229853002)(23726003)(18370500001)(42262002)(309714004);DIR:OUT;SFP:1102;SCL:1;SRVR:CO1PR15MB1077;H:castle.dhcp.TheFacebook.com;FPR:;SPF:None;PTR:InfoNoRecords;MX:1;A:1;LANG:en; X-Microsoft-Exchange-Diagnostics: =?us-ascii?Q?1;CO1PR15MB1077;23:nMObwKEtSJraDh4jOUaus2id1qqq/2DeHnxzZdSI5?= =?us-ascii?Q?O/xH+l6JLGf8Sysxi29ChvpP6divSQh8PaEh1NN2jgAR+fhUoU/KkDCTbSSK?= =?us-ascii?Q?KtgJcGl3kcZW67Xxaf1ItvHijzEMYrV3fRf7F7hfncT38JPIt/a0OaoBOKDn?= =?us-ascii?Q?z6l9dJsdO0IZhScTmUownyB5RIMgKGF+dyLHZXJUvFbF2OnWji+JDVxYVZPa?= =?us-ascii?Q?V+p+MCZnA/vB1ji21R9gwjxJXeSYakK2FsgslhnRkCL/x/jSlAOJHLhT4VHY?= =?us-ascii?Q?BJuSLWCXLInonk2uwroGTmDCJONSjVrN79jaehHNDY/3+euxBo+wa4hZdczs?= =?us-ascii?Q?oh8S3EavmEaO17/YpeKdWr5aoHW22rdH+9LY7iyzvsIzddjdeLQbK2lcKhvV?= =?us-ascii?Q?zU9lmlmbTMIPSUOZyeUjSlzuI4l75ZQ4xCEm9nAlaW3XssTuy4wqHAHllaHc?= =?us-ascii?Q?Q4MP1jTCiFKvd/8faJN6ErMXdaESVIQKRQgENWN+2e/nIH09xfaR5mXi6pkk?= =?us-ascii?Q?/fDpV/rBiBV52ppNSzRjNubFV85PblaAI/54X4t5KRwefuofGjSztKG6P3kB?= =?us-ascii?Q?VN30qTodLqMf6st1tDb4siS9A6wzyKhdFWlPd3oFWEuOTWSso43+QVWKmNWr?= =?us-ascii?Q?pNRFAc7SRg5f9Z1Ti5/hjcIetCVJMaGJdAMzf18A+JSLSRcFvsHMsCaTQH9p?= =?us-ascii?Q?bitNhFC4lZ3DDKT3CFWGuluVCTqdNNBlU3/1E2XPN/tyeKxe9eF49OB2xgXm?= =?us-ascii?Q?aWBxM/OXE/g2ftuVqGAsGxu6RqE+Dbluj6QzThD4YyLI++1Mn8gO92/c/Aci?= =?us-ascii?Q?fMZ8SLuRicRyBWOUHm5uNd1h09Y3dGnT/dpVVx9ZQgYfI19EZAPyeN+jh5m1?= =?us-ascii?Q?tNK45NT22D0j1VvobNJdBwHRXy1iT3CtU3Q3udp2itzUlI0hbnQKai3/KSuP?= =?us-ascii?Q?fuABas+o/4HH0qGHI0odiJuQnQKHWFpXKph0zUfXzB0XLBFpzdYm4VRPm9vq?= =?us-ascii?Q?5tzEnNZeuVPifWBdmJZgiK7+2bSoM3GDhX+8db/Z2dYkxuGpbyL2/N9AlEpU?= =?us-ascii?Q?71tkd4b8Mfa83eYcSOeyGbydwbKQGO5BZGx49ZdiwXOC1Mi6M0p9zey8VFDc?= =?us-ascii?Q?Zbi3vgndgRyTGSrPg36We39fP3mUsSA9SRSpgxYdppsOlOL/tYh0KMjiZj3x?= =?us-ascii?Q?SFbzsswatxF64zZ9a6FkH3VFfVGTQg6HL60xYS5YHJrdxP5EjYQIPuK8EQ/3?= =?us-ascii?Q?/NxEr/sfCkAhRoXKs4=3D?= X-Microsoft-Exchange-Diagnostics: 1;CO1PR15MB1077;6:J4ffb2twPOM00ImChH5BxpRNI8Mvnkqm+Pj36aMXEPWNhvqulCqhOr+xf9mgS8f+CVfPPIKvIxFWSrtw5hKsYa9dUsmWqQfKeNxHgaAdybVvo+PwGIHIhIGb9+z9vgHRlMhERa92VdBSkcCznBxNrWYvMtbn91fGrc/vuSxtfSqfBidQbpeTow9aylrlqANKrP6vqGWbhKdCbIncYY5mHIFy0qbu9MrNR+cPp6fU/QBr8xNMSUZgLx5mc61j7dfBjt4yRpq5rNib8jT00NbaZnLFs96/+HqIaF6VfRP/fmepxBIMmPQmq2i+17vTYydmucRtv2unhLxoCfJOt13/Ug==;5:Yp5lDQ4NxfK2q1SOBvV7osk76EFNWRMK/yL8hDp9adITJbcKDCZI+IxsyDYnyXR6zXnEJqamML52vOKNP3II5veVwdAvS9rZ8G9FL2Wm0j+DepE1IcyLJtTiL2c6grrcD4TkvcS2GwIQzpX8BXny/A==;24:+ffWPC1b6Rmw5phH6H/wi6YWnixd5rOCLMRf6eokq0toel8qY0XiXKDxOWD8CtNACoeLIdvNebad3XGdlwYmxQDBPTnOEr1hKmW8vaGD2xM=;7:Nz62DCddxVCqRW2t8yoTZqtWIhuirdmcCtgGPrTuHA7PqXhtGwhSB7vluZlJwVcdJzmGGg+LZc2YaSMYj9XKRqw4Y2ii+5pwyZVqADDzF8cprxjVcmGd06zmeKarRg1R31gJ3jpaNZ6DseYVQiaYd5lp0rcT/eoLy1OUToq3x5h1LIRWcfa21npq1e1HRVOeI0BgIJb/Wi/qSuTHdTTvAeVumMAFAaZePrtStshXzhY= SpamDiagnosticOutput: 1:99 SpamDiagnosticMetadata: NSPM X-Microsoft-Exchange-Diagnostics: 1;CO1PR15MB1077;20:IgmX9JCOb3zvFRaCFfED9upfMSMOhKtAp3gtjOzdCKwA7Toz1HtyB6yjFJHd/2LWmiW+zRJ0vUwgCWEhxtqXuQ0ibod5gueFH924tuN3LgLdD3jJ56GsfP36Wrg+BWPHyqDe42PDqIfWKhF9uoTTUPQya25FGo8trsvkmENoQzM= X-MS-Exchange-CrossTenant-OriginalArrivalTime: 05 Oct 2017 12:32:31.1634 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-CrossTenant-Id: 8ae927fe-1255-47a7-a2af-5f3a069daaa2 X-MS-Exchange-Transport-CrossTenantHeadersStamped: CO1PR15MB1077 X-OriginatorOrg: fb.com X-Proofpoint-Spam-Reason: safe X-FB-Internal: Safe X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-10-05_07:,, signatures=0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Oct 05, 2017 at 02:06:49PM +0200, Michal Hocko wrote: > On Wed 04-10-17 16:46:36, Roman Gushchin wrote: > > The cgroup-aware OOM killer treats leaf memory cgroups as memory > > consumption entities and performs the victim selection by comparing > > them based on their memory footprint. Then it kills the biggest task > > inside the selected memory cgroup. > > > > But there are workloads, which are not tolerant to a such behavior. > > Killing a random task may leave the workload in a broken state. > > > > To solve this problem, memory.oom_group knob is introduced. > > It will define, whether a memory group should be treated as an > > indivisible memory consumer, compared by total memory consumption > > with other memory consumers (leaf memory cgroups and other memory > > cgroups with memory.oom_group set), and whether all belonging tasks > > should be killed if the cgroup is selected. > > > > If set on memcg A, it means that in case of system-wide OOM or > > memcg-wide OOM scoped to A or any ancestor cgroup, all tasks, > > belonging to the sub-tree of A will be killed. If OOM event is > > scoped to a descendant cgroup (A/B, for example), only tasks in > > that cgroup can be affected. OOM killer will never touch any tasks > > outside of the scope of the OOM event. > > > > Also, tasks with oom_score_adj set to -1000 will not be killed. > > I would extend the last sentence with an explanation. What about the > following: > " > Also, tasks with oom_score_adj set to -1000 will not be killed because > this has been a long established way to protect a particular process > from seeing an unexpected SIGKILL from the oom killer. Ignoring this > user defined configuration might lead to data corruptions or other > misbehavior. > " Added, thanks! > > few mostly nit picks below but this looks good other than that. Once the > fix mentioned in patch 3 is folded I will ack this. > > [...] > > > static void select_victim_memcg(struct mem_cgroup *root, struct oom_control *oc) > > { > > - struct mem_cgroup *iter; > > + struct mem_cgroup *iter, *group = NULL; > > + long group_score = 0; > > > > oc->chosen_memcg = NULL; > > oc->chosen_points = 0; > > > > /* > > + * If OOM is memcg-wide, and the memcg has the oom_group flag set, > > + * all tasks belonging to the memcg should be killed. > > + * So, we mark the memcg as a victim. > > + */ > > + if (oc->memcg && mem_cgroup_oom_group(oc->memcg)) { > > we have is_memcg_oom() helper which is esier to read and understand than > the explicit oc->memcg check It's defined in oom_kill.c and not exported, so I'm not sure. > > > + oc->chosen_memcg = oc->memcg; > > + css_get(&oc->chosen_memcg->css); > > + return; > > + } > > + > > + /* > > * The oom_score is calculated for leaf memory cgroups (including > > * the root memcg). > > + * Non-leaf oom_group cgroups accumulating score of descendant > > + * leaf memory cgroups. > > */ > > rcu_read_lock(); > > for_each_mem_cgroup_tree(iter, root) { > > long score; > > > > + /* > > + * We don't consider non-leaf non-oom_group memory cgroups > > + * as OOM victims. > > + */ > > + if (memcg_has_children(iter) && !mem_cgroup_oom_group(iter)) > > + continue; > > + > > + /* > > + * If group is not set or we've ran out of the group's sub-tree, > > + * we should set group and reset group_score. > > + */ > > + if (!group || group == root_mem_cgroup || > > + !mem_cgroup_is_descendant(iter, group)) { > > + group = iter; > > + group_score = 0; > > + } > > + > > hmm, I thought you would go with a recursive oom_evaluate_memcg > implementation that would result in a more readable code IMHO. It is > true that we would traverse oom_group more times. But I do not expect > we would have very deep memcg hierarchies in the majority of workloads > and even if we did then this is a cold path which should focus on > readability more than a performance. Also implementing > mem_cgroup_iter_skip_subtree shouldn't be all that hard if this ever > turns out a real problem. I've tried to go this way, but I didn't like the result. These both loops will share a lot of code (e.g. breaking on finding a previous victim, skipping non-leaf non-oom-group memcgs, etc), so the result is more messy. And actually it's strange to start a new loop to iterate exactly over the same sub-tree, which you want to skip in the first loop. > > Anyway this is nothing really fundamental so I will leave the decision > on you. > > > +static bool oom_kill_memcg_victim(struct oom_control *oc) > > +{ > > if (oc->chosen_memcg == NULL || oc->chosen_memcg == INFLIGHT_VICTIM) > > return oc->chosen_memcg; > > > > - /* Kill a task in the chosen memcg with the biggest memory footprint */ > > - oc->chosen_points = 0; > > - oc->chosen_task = NULL; > > - mem_cgroup_scan_tasks(oc->chosen_memcg, oom_evaluate_task, oc); > > - > > - if (oc->chosen_task == NULL || oc->chosen_task == INFLIGHT_VICTIM) > > - goto out; > > - > > - __oom_kill_process(oc->chosen_task); > > + /* > > + * If memory.oom_group is set, kill all tasks belonging to the sub-tree > > + * of the chosen memory cgroup, otherwise kill the task with the biggest > > + * memory footprint. > > + */ > > + if (mem_cgroup_oom_group(oc->chosen_memcg)) { > > + mem_cgroup_scan_tasks(oc->chosen_memcg, oom_kill_memcg_member, > > + NULL); > > + /* We have one or more terminating processes at this point. */ > > + oc->chosen_task = INFLIGHT_VICTIM; > > it took me a while to realize we need this because of return > !!oc->chosen_task in out_of_memory. Subtle... Also a reason to hate > oc->chosen_* thingy. As I've said in other reply, don't worry about this > I will probably turn my hate into a patch ;) > > > + } else { > > + oc->chosen_points = 0; > > + oc->chosen_task = NULL; > > + mem_cgroup_scan_tasks(oc->chosen_memcg, oom_evaluate_task, oc); > > + > > + if (oc->chosen_task == NULL || > > + oc->chosen_task == INFLIGHT_VICTIM) > > + goto out; > > How can this happen? There shouldn't be any INFLIGHT_VICTIM in our memcg > because we have checked for that already. I can see how we do not find > any task because those can terminate by the time we get here but no new > oom victim should appear we are under the oom_lock. You're probably right, but I would prefer to have this check in place, rather then get a panic on attempt to kill an INFLIGHT_VICTIM task one day. In general, I do not like this trick with using this special value to signal the existence of in-flight victims. It adds a lot of complexity, and non-obvious code. I assume, it's a good target for the following refactoring. Thanks!