From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-1.0 required=3.0 tests=MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 91D75C1975A for ; Tue, 17 Mar 2020 18:55:34 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 4793F20724 for ; Tue, 17 Mar 2020 18:55:34 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 4793F20724 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id E85A46B0005; Tue, 17 Mar 2020 14:55:33 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id E364B6B0006; Tue, 17 Mar 2020 14:55:33 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id D4BF76B0007; Tue, 17 Mar 2020 14:55:33 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0008.hostedemail.com [216.40.44.8]) by kanga.kvack.org (Postfix) with ESMTP id BAD996B0005 for ; Tue, 17 Mar 2020 14:55:33 -0400 (EDT) Received: from smtpin13.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay04.hostedemail.com (Postfix) with ESMTP id 77CE65851 for ; Tue, 17 Mar 2020 18:55:33 +0000 (UTC) X-FDA: 76605757746.13.tax15_307401c6ec625 X-HE-Tag: tax15_307401c6ec625 X-Filterd-Recvd-Size: 6913 Received: from mail-wr1-f66.google.com (mail-wr1-f66.google.com [209.85.221.66]) by imf39.hostedemail.com (Postfix) with ESMTP for ; Tue, 17 Mar 2020 18:55:32 +0000 (UTC) Received: by mail-wr1-f66.google.com with SMTP id f3so20155721wrw.7 for ; Tue, 17 Mar 2020 11:55:32 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=kA35++hY6q6St63mCVJsFqLRQRhSP7DteenkrHTj+MA=; b=MXiujMHe+j+EeF7a2SMDISE9oAO2cXne/b7jrbW5bO2nE4jjZW3Eg3+wNxyZNkceZV qgmEXmxWAUuBNU2dUxx9blr+L6GotoIaQ9tTliGILFkB+9kzurSBaoTgXOSltkxVWp+E 3FDIH5FUuxQBUGoffF3rMx2QZpLzRq9BBiNDTZq3Notc91+kPjIeoYGp/2HGsBnqXQea glvQ5qLC9ppby12un2Ju58wDVbqG2ftXxIvirRfe/rwse+xRuW5KNK7a5pw16aObgPpY IoNdzF/B8uvfWFnUDXM8Bk6LKt+UGYunWvysQJnV46d4l4lbSLiEnfvDuwrwKyyb+G/R mxdQ== X-Gm-Message-State: ANhLgQ3eNXewSPLtlrQHegDDBhZoMJN9WJogynbUFVIPZ3mC8x0Fd2kg q/fDFB6NyRMs0k//YUszUZ0= X-Google-Smtp-Source: ADFU+vvHda6xA5nFihIzhCAXUiwMj6Pin5FtSn4qO67ev+GZKBdb4CtSolVRWVWnQ6uGx/8j2xdNdA== X-Received: by 2002:adf:97d0:: with SMTP id t16mr412590wrb.343.1584471331555; Tue, 17 Mar 2020 11:55:31 -0700 (PDT) Received: from localhost (ip-37-188-255-121.eurotel.cz. [37.188.255.121]) by smtp.gmail.com with ESMTPSA id d15sm5702400wrp.37.2020.03.17.11.55.29 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 17 Mar 2020 11:55:30 -0700 (PDT) Date: Tue, 17 Mar 2020 19:55:29 +0100 From: Michal Hocko To: Roman Gushchin Cc: Andrew Morton , linux-mm@kvack.org, kernel-team@fb.com, linux-kernel@vger.kernel.org Subject: Re: [PATCH] mm: memcg: make memory.oom.group tolerable to task migration Message-ID: <20200317185529.GV26018@dhcp22.suse.cz> References: <20200316223510.3176148-1-guro@fb.com> <20200317075212.GC26018@dhcp22.suse.cz> <20200317183836.GA276471@carbon.DHCP.thefacebook.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20200317183836.GA276471@carbon.DHCP.thefacebook.com> X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Tue 17-03-20 11:38:36, Roman Gushchin wrote: > On Tue, Mar 17, 2020 at 08:52:12AM +0100, Michal Hocko wrote: > > On Mon 16-03-20 15:35:10, Roman Gushchin wrote: > > > If a task is getting moved out of the OOMing cgroup, it might > > > result in unexpected OOM killings if memory.oom.group is used > > > anywhere in the cgroup tree. > > > > > > Imagine the following example: > > > > > > A (oom.group = 1) > > > / \ > > > (OOM) B C > > > > > > Let's say B's memory.max is exceeded and it's OOMing. The OOM killer > > > selects a task in B as a victim, but someone asynchronously moves > > > the task into C. > > > > I can see Reported-by here, does that mean that the race really happened > > in real workloads? If yes, I would be really curious. Mostly because > > moving tasks outside of the oom domain is quite questionable without > > charge migration. > > Yes, I've got a number of OOM messages where oom_cgroup != task_cgroup. > The only reasonable explanation is that the task has been moved out after > being selected as a victim. In my case it resulted in killing all tasks > in A, and it what hurt the workload. Is this an expected behavior of the workload or potentially a bug. Because really, migrating outside of the oom domain is problematic already. Essentially you are going to kill a wrong task if the largest memory consumer is migrating before the oom killer manages to find the task. > > > mem_cgroup_get_oom_group() will iterate over all > > > ancestors of C up to the root cgroup. In theory it had to stop > > > at the oom_domain level - the memory cgroup which is OOMing. > > > But because B is not an ancestor of C, it's not happening. > > > Instead it chooses A (because it's oom.group is set), and kills > > > all tasks in A. This behavior is wrong because the OOM happened in B, > > > so there is no reason to kill anything outside. > > > > > > Fix this by checking it the memory cgroup to which the task belongs > > > is a descendant of the oom_domain. If not, memory.oom.group should > > > be ignored, and the OOM killer should kill only the victim task. > > > > I was about to suggest storing the memcg in oom_evaluate_task but then I > > have realized that this would be both more complex and I am not yet > > sure it would be better so much better after all. > > > > The thing is that killing the selected task makes a lot of sense > > because it was the largest consumer. No matter it has run away. On the > > other hand if your B was oom.group = 1 then one could expect that any > > OOM killer event in that group will result in the whole group tear > > down. This is however a gray zone because we do emit MEMCG_OOM event but > > MEMCG_OOM_KILL event will go to the victim's at-the-time memcg. So the > > observer B could think that the oom was resolved without killing while > > observer C would see a kill event without oom. > > I agree. Killing the task outside of the OOMing cgroup is already strange. Strange? Maybe but if you think about it, not that much in fact because you are still killing a task that was in the memcg at the time of the evaluation. Sure that largest task might not be the biggest contributor to the charged memory - as mentioned above - but well, this is what you ask for when migrating over oom domains. > Should we somehow lock the OOMing cgroup? So that tasks can not escape > and enter it until the finish of the OOM killing? I do not think this is going to help all that much. Sure we can note the memcg at the oom_evaluate_task and use it later for the group oom handling. But races will always be there. Having oom path to depend on locks used elsewhere is a can of worms. It would add a very hard to evaluate dependences. > It seems to be a better idea, because it will also make the oom.group > killing less racy: currently a forking app can potentially escape from it. > > And the we can put something like > if (WARN_ON_ONCE(!mem_cgroup_is_descendant(memcg, oom_domain))) > goto out; > to mem_cgroup_get_oom_group? This would be a user triggerable warning and that sounds like a bad idea to me. We should just live with races. The only question I still do not have a proper answer for is how much we do care. If we do not care all that much about the original memcg then go with your patch. But if we want to be slightly more careful then we should note the memcg in oom_evaluate_task and use it when killing. -- Michal Hocko SUSE Labs