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 Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 0A18DC6FD1D for ; Tue, 21 Mar 2023 02:41:54 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 272896B0075; Mon, 20 Mar 2023 22:41:54 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 221E36B0078; Mon, 20 Mar 2023 22:41:54 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 0EA126B007B; Mon, 20 Mar 2023 22:41:54 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0014.hostedemail.com [216.40.44.14]) by kanga.kvack.org (Postfix) with ESMTP id 001066B0075 for ; Mon, 20 Mar 2023 22:41:53 -0400 (EDT) Received: from smtpin23.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay08.hostedemail.com (Postfix) with ESMTP id C65F514052B for ; Tue, 21 Mar 2023 02:41:53 +0000 (UTC) X-FDA: 80591355306.23.753923B Received: from mail-pl1-f171.google.com (mail-pl1-f171.google.com [209.85.214.171]) by imf17.hostedemail.com (Postfix) with ESMTP id E847540009 for ; Tue, 21 Mar 2023 02:41:51 +0000 (UTC) Authentication-Results: imf17.hostedemail.com; dkim=pass header.d=shopee.com header.s=shopee.com header.b=EPNaKpwD; spf=pass (imf17.hostedemail.com: domain of haifeng.xu@shopee.com designates 209.85.214.171 as permitted sender) smtp.mailfrom=haifeng.xu@shopee.com; dmarc=pass (policy=reject) header.from=shopee.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1679366512; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=HEkJyYt73vejLSpG2+x8Ouk/b+GBlTD7f0hr6+7FBys=; b=M45SKfqJAJ6krJ6ZBfBp8dzJJbIlJ7ci5SC8traGgCBqSU3JRAeUOrveBYoODXlMyWg6OU nAjcAj/n4Hzdz5SnhhVSOJ78wvysI14ZyLTM/A3FBKaH2l1sAoU90F0lvll/u+gaq9sGnl iDMm9FNxgc1jwxaeGS9PKjo13X5OPNw= ARC-Authentication-Results: i=1; imf17.hostedemail.com; dkim=pass header.d=shopee.com header.s=shopee.com header.b=EPNaKpwD; spf=pass (imf17.hostedemail.com: domain of haifeng.xu@shopee.com designates 209.85.214.171 as permitted sender) smtp.mailfrom=haifeng.xu@shopee.com; dmarc=pass (policy=reject) header.from=shopee.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1679366512; a=rsa-sha256; cv=none; b=nFBasmyYBQtcTRS574sUt9Do8Qe/LlDqxX9CWx9b7BK4nUy2RUVl+ZffKEziLJ4fwcKqxU FCQn1pCQHHAjN4Szz65ZPpOAakjogIobD1iBe9ixGM587BlGb7XuvIulQcutnvIXmDN8Vd +j42WmHIK8818NuLRRnctHENNbOoQcI= Received: by mail-pl1-f171.google.com with SMTP id u5so14598465plq.7 for ; Mon, 20 Mar 2023 19:41:51 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=shopee.com; s=shopee.com; t=1679366511; h=content-transfer-encoding:in-reply-to:from:references:cc:to:subject :user-agent:mime-version:date:message-id:from:to:cc:subject:date :message-id:reply-to; bh=HEkJyYt73vejLSpG2+x8Ouk/b+GBlTD7f0hr6+7FBys=; b=EPNaKpwDedmJIHjvq//QFdMZJ1bwSI6kGwROkrd4BE2qCSHOgENUIKyNMNO3GMZm90 peuHU4GCfLn4X/rjSkIWVJcazbUFJevTL66PwPCgHGZsU1zigewsuD5oruZvte+U141p Hugu8vxs1/RvD3ssusgS67w5W8cUzUmTh+FZXgAzZdJGFNrqSefGc/yaMmtv0+o9Tw7S TVms7IxFiSARjYd2eUlbSYruuKA+xreCtibVaikE+pn2HyW4nMQ16WTQGKZgcZiXdqem Bddx7U3M0hPB1Xc4DJbO3WRbsa2uWdNsqbmMnkDKyOtNsMlMfhdZ+I/mhcU1ayZvD2Ye AEqA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1679366511; h=content-transfer-encoding:in-reply-to:from:references:cc:to:subject :user-agent:mime-version:date:message-id:x-gm-message-state:from:to :cc:subject:date:message-id:reply-to; bh=HEkJyYt73vejLSpG2+x8Ouk/b+GBlTD7f0hr6+7FBys=; b=ELAnURPZOLBNrkPO16bMe9frakhCu/InJ2uud0+azdqO0jh6O06fut1QD/3NFU6igr cuTUCibwcbEHz3XESMmGWPQCKdBL4YsjZ2wPAVVAOUEM8PTwO6PooPYjMBtqOjU03LNt SyS+hIh4K1Gn2NheMwdn//Ses0yY1e3NcIL32RyYpqAV7I8SFL7acy5NyZKXseOFNMWi TtNnCquv/Xt6zsmKW7x+0qx1BsS5ffBwAu10hTnH1E2Xzt9coF9QncY5f8T4KM4xhRRJ bJAri7fNQjcwRQBW2Z6fubsGk8H9o7z3V55XwEVG/t0jzI1kFsygsrP4lcfbEXhUMKbQ ZsBA== X-Gm-Message-State: AO0yUKUC3CGOyMfSMDLHhY9lvDJnTD+OVAZd66BI31GzQSb2n+5/rBiT Ucj64cpd6ujznbn+YLDDH5ztFQ== X-Google-Smtp-Source: AK7set+7Allahs6jz++T+GpHLmunKF6U2IvwRRyY0XJ3UBLtK8KojpT1JqDPIX/pfX8LcN/dzZJ0Rg== X-Received: by 2002:a17:902:e195:b0:1a0:485c:a6c with SMTP id y21-20020a170902e19500b001a0485c0a6cmr602540pla.8.1679366510603; Mon, 20 Mar 2023 19:41:50 -0700 (PDT) Received: from [10.54.24.141] ([143.92.118.3]) by smtp.gmail.com with ESMTPSA id u8-20020a170902bf4800b0018b025d9a40sm7341062pls.256.2023.03.20.19.41.47 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 20 Mar 2023 19:41:49 -0700 (PDT) Message-ID: Date: Tue, 21 Mar 2023 10:41:44 +0800 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0) Gecko/20100101 Thunderbird/102.9.0 Subject: Re: [RFC] memcg, oom: clean up mem_cgroup_oom_synchronize To: Michal Hocko Cc: hannes@cmpxchg.org, shakeelb@google.com, akpm@linux-foundation.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org References: <20230315070302.268316-1-haifeng.xu@shopee.com> From: Haifeng Xu In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Rspam-User: X-Rspamd-Server: rspam04 X-Rspamd-Queue-Id: E847540009 X-Stat-Signature: hu1sdhq3hoijsfchii44q6pjo8k4m9i1 X-HE-Tag: 1679366511-539653 X-HE-Meta: U2FsdGVkX1+T1qo4d2ale+DHd4pB1Ju2zVJfKEIQEDP9e0TrqnMSEIkyNa71fQkrpu6k4+Nr/Sre7eQghCWrQLCxYPbiJqacA8G3+yD3FwT0NGSUoLfhsD6tqNVEHNKI/UnFDcIELuWMr8cfC4aPK2ezWuqzHTOwyMBsYfE2XtdVV4CsGpp0EdDX5McT8rN0DqgRT5P2Z2+VRfKguWeOhNJjM1jZOTaHA3W6TZ/xfjg550N8xi8QQ1ZYPrHfD7oJfM4gPisHAMQyrooJVB+IJvK3FK2Y0ObOgpc1Md1DRPdiE+KmwTgjzSCWHXxAET0VMzJQ3OWPZkgxebRsBoyDy+jUwcKz8umSg3wuPgLRe5/yEbt7tFjKo48YfE7CtvAN/pYPl1SqCPcwOTL4lgQ3p0hF8Q874NBzROPFs+07CWVsYZsC84xU9i+8fY1bLwlhylN5bS3qshTkODqBLMrMd+DMBNIMumi7m/V4NUFDhx3o5J+2692oS3Z7mQcsDntcvn6UVCuL9EpTe063s+G1RCYOjC1Z5yPEUHD13fkmShJp3EzcVQJeE7gBGqb9mL10CrAYV2Ddxzcx8hQKVxz8caKYnuz6QW1Nhloob2AAmLnIFL79h/GcVDAYGjESV9XTFnKz8Y7Bv3kyoQrUFHELPyMbTVdBgG8dpwZl/+WoueJXUDR9blsQljXfybu+RDoKQisgZxzM2gaVCd8ttnPHaE+2EsDI06xchXwyjKUmBcUqnwyL+3TjlukOQecW/raludI1Hc2SnTOkcKVYDulJXJwScOJL7TFprNb7QLQlZzb8sNzk4gCYWOLAWRncRb/GTwKOqPBVgXyCGSojRWgdgx1tewIf5dhi+PtDF9+F2NCr4ItT2DPz/4xbDO6HBTRvJynio1vNy1hxLmE41swqOtugN1OhxV9C16CduketscusaG6ghIuAfewO8WrxeMUroYmoD2m5VyiJv2vCLke NZkH69Cl LGJHNW7p7iRGZCkNAiDw1xAdEEfrX7319Mch88cOzPvwvuYPjdKPrSkk94/ZAtCEKcOtbnCjxK8r/4lGt7DYYYYz8ab31G1p8+ub93zBsEGEgH339kjI3TGQLu7XIV6FqlqVBjzoInXus/BBbes2Dm7h38xeFYrJiW0kmiUFWq6o6XLOYD/NhzDu90IEOZ2ohRh7iUBVilZAWzATXSFxvBmAF+MtpJoFyzE6rnSgAXKNrOHdTn1VJ1Khd7Ve2uXIMdYANOzLBc8449JF9qDFMIkxHKNwTw2TriJIkEaVuPiNeX7sCYsFA0IbCEs3M8JCzGwVN6b2tGNaz/ejCyiXZhuNQy1IsHa2evegFnUy1mN//gT6ZuKdDSvLUUpRY8X1Wx4RbJ1t3b44Z5EKPn2nvozU2WRgerDuG4+2rLrhKIEji2vCNaC0C8vJXdYirHeaVUgo0+Hdqtdlm+IQPF9x4eJ1bCEEZruiTdogY 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 2023/3/17 19:47, Michal Hocko wrote: > On Wed 15-03-23 07:03:02, Haifeng Xu wrote: >> Since commit 29ef680ae7c2 ("memcg, oom: move out_of_memory back to >> the charge path"), only oom_kill_disable is set, oom killer will >> be delayed to page fault path. In the charge patch, even if the >> oom_lock in memcg can't be acquired, the oom handing can also be >> invoked. In order to keep the behavior consistent with it, remove >> the lock check, just leave oom_kill_disable check behind in the >> page fault path. > > I do not understand the actual problem you are trying to deal with here. > >> Furthermore, the lock contender won't be scheduled out, this doesn't >> fit the sixth description in commit fb2a6fc56be66 ("mm: memcg: >> rework and document OOM waiting and wakeup"). So remove the explicit >> wakeup for the lock holder. >> >> Fixes: fb2a6fc56be6 ("mm: memcg: rework and document OOM waiting and wakeup") > > The subject mentions a clean up but the fixes tag would indicate an > acutal fix. > >> Signed-off-by: Haifeng Xu >> --- >> mm/memcontrol.c | 11 ++--------- >> 1 file changed, 2 insertions(+), 9 deletions(-) >> >> diff --git a/mm/memcontrol.c b/mm/memcontrol.c >> index 5abffe6f8389..360fa7cf7879 100644 >> --- a/mm/memcontrol.c >> +++ b/mm/memcontrol.c >> @@ -1999,7 +1999,7 @@ bool mem_cgroup_oom_synchronize(bool handle) >> if (locked) >> mem_cgroup_oom_notify(memcg); >> >> - if (locked && !memcg->oom_kill_disable) { >> + if (!memcg->oom_kill_disable) { >> mem_cgroup_unmark_under_oom(memcg); >> finish_wait(&memcg_oom_waitq, &owait.wait); >> mem_cgroup_out_of_memory(memcg, current->memcg_oom_gfp_mask, > > Now looking at the actual code I suspect you in fact want to simplify > the logic here as mem_cgroup_oom_synchronize is only ever triggered whe > oom_kill_disable == true because current->memcg_in_oom is never non NULL > otherwise. So the check is indeed unnecessary. Your patch, however > doesn't really simplify the code much. > > Did you want this instead? > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > index 12559c08d976..a77dc88cfa12 100644 > --- a/mm/memcontrol.c > +++ b/mm/memcontrol.c > @@ -1999,16 +1999,9 @@ bool mem_cgroup_oom_synchronize(bool handle) > if (locked) > mem_cgroup_oom_notify(memcg); > > - if (locked && !READ_ONCE(memcg->oom_kill_disable)) { > - mem_cgroup_unmark_under_oom(memcg); > - finish_wait(&memcg_oom_waitq, &owait.wait); > - mem_cgroup_out_of_memory(memcg, current->memcg_oom_gfp_mask, > - current->memcg_oom_order); > - } else { > - schedule(); > - mem_cgroup_unmark_under_oom(memcg); > - finish_wait(&memcg_oom_waitq, &owait.wait); > - } > + schedule(); > + mem_cgroup_unmark_under_oom(memcg); > + finish_wait(&memcg_oom_waitq, &owait.wait); > > if (locked) { > mem_cgroup_oom_unlock(memcg); > Yes, the chance that someone else disable the oom_kill_disable again in the page fault path is quite low. >> @@ -2010,15 +2010,8 @@ bool mem_cgroup_oom_synchronize(bool handle) >> finish_wait(&memcg_oom_waitq, &owait.wait); >> } >> >> - if (locked) { >> + if (locked) >> mem_cgroup_oom_unlock(memcg); >> - /* >> - * There is no guarantee that an OOM-lock contender >> - * sees the wakeups triggered by the OOM kill >> - * uncharges. Wake any sleepers explicitly. >> - */ >> - memcg_oom_recover(memcg); >> - } > > Hmm, so this seems unneded as well for the oom_kill_disable case as > well. Rather than referring to fb2a6fc56be66 it would be better to > why the explicit recovery is not really needed anymore. > >> cleanup: >> current->memcg_in_oom = NULL; >> css_put(&memcg->css); > Thank you for your suggestion. I'll post an official patch later.