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=-5.7 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED 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 032E5C433E0 for ; Fri, 5 Feb 2021 12:20:28 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 6EA6A64F3F for ; Fri, 5 Feb 2021 12:20:27 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 6EA6A64F3F Authentication-Results: mail.kernel.org; dmarc=fail (p=quarantine dis=none) header.from=suse.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id B8FFC6B0073; Fri, 5 Feb 2021 07:20:26 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id B42456B0072; Fri, 5 Feb 2021 07:20:26 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id A56646B0073; Fri, 5 Feb 2021 07:20:26 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0240.hostedemail.com [216.40.44.240]) by kanga.kvack.org (Postfix) with ESMTP id 8F85F6B0071 for ; Fri, 5 Feb 2021 07:20:26 -0500 (EST) Received: from smtpin29.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay03.hostedemail.com (Postfix) with ESMTP id 5811B824999B for ; Fri, 5 Feb 2021 12:20:26 +0000 (UTC) X-FDA: 77784122052.29.river85_290a1bb275e4 Received: from filter.hostedemail.com (10.5.16.251.rfc1918.com [10.5.16.251]) by smtpin29.hostedemail.com (Postfix) with ESMTP id DE3D218086CCA for ; Fri, 5 Feb 2021 12:20:25 +0000 (UTC) X-HE-Tag: river85_290a1bb275e4 X-Filterd-Recvd-Size: 5086 Received: from mx2.suse.de (mx2.suse.de [195.135.220.15]) by imf10.hostedemail.com (Postfix) with ESMTP for ; Fri, 5 Feb 2021 12:20:25 +0000 (UTC) X-Virus-Scanned: by amavisd-new at test-mx.suse.de DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.com; s=susede1; t=1612527624; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=e794d5QOMRuyLO+4aj8w2Dw3VWViAx9HXq/tTwzEeDI=; b=LT0qKMyi+Q90u4ZHAYw81y7/nq1FWF4UHsTm4PWXl1MiOoJ5lNbVslxKOlX6bLgR9+8Par 84u6AJoCgzPlVAkvE5ixhJJXNPUHKJ/UwSaWt98jHdR4Akq81DP6ESAmn1Bu0ZST6Mmn6l IfUuRC0OowjDTTeDkt0IhIOPGfdJWQg= Received: from relay2.suse.de (unknown [195.135.221.27]) by mx2.suse.de (Postfix) with ESMTP id 39C21ACBA; Fri, 5 Feb 2021 12:20:24 +0000 (UTC) Date: Fri, 5 Feb 2021 13:20:23 +0100 From: Michal Hocko To: Muchun Song Cc: Johannes Weiner , Vladimir Davydov , Andrew Morton , Cgroups , Linux Memory Management List , LKML Subject: Re: [External] Re: [PATCH] mm: memcontrol: fix missing wakeup oom task Message-ID: References: <20210205062310.74268-1-songmuchun@bytedance.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: 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 Fri 05-02-21 19:04:19, Muchun Song wrote: > On Fri, Feb 5, 2021 at 6:21 PM Michal Hocko wrote: > > > > On Fri 05-02-21 17:55:10, Muchun Song wrote: > > > On Fri, Feb 5, 2021 at 4:24 PM Michal Hocko wrote: > > > > > > > > On Fri 05-02-21 14:23:10, Muchun Song wrote: > > > > > We call memcg_oom_recover() in the uncharge_batch() to wakeup OOM task > > > > > when page uncharged, but for the slab pages, we do not do this when page > > > > > uncharged. > > > > > > > > How does the patch deal with this? > > > > > > When we uncharge a slab page via __memcg_kmem_uncharge, > > > actually, this path forgets to do this for us compared to > > > uncharge_batch(). Right? > > > > Yes this was more more or less clear (still would have been nicer to be > > explicit). But you still haven't replied to my question I believe. I > > assume you rely on refill_stock doing draining but how does this address > > the problem? Is it sufficient to do wakeups in the batched way? > > Sorry, the subject title may not be suitable. IIUC, memcg_oom_recover > aims to wake up the OOM task when we uncharge the page. Yes, your understanding is correct. This is a way to pro-actively wake up oom victims when the memcg oom handling is outsourced to the userspace. Please note that I haven't objected to the problem statement. I was questioning the fix for the problem. > I see uncharge_batch always do this. I am confused why > __memcg_kmem_uncharge does not. Very likely an omission. I haven't checked closely but I suspect this has been introduced by the recent kmem accounting changes. Why didn't you simply do the same thing and call memcg_oom_recover unconditionally and instead depend on the draining? I suspect this was because you wanted to recover also when draining which is not necessary as pointed out in other email. [...] > > > > Does this lead to any code generation improvements? I would expect > > > > compiler to be clever enough to inline static functions if that pays > > > > off. If yes make this a patch on its own. > > > > > > I have disassembled the code, I see memcg_oom_recover is not > > > inline. Maybe because memcg_oom_recover has a lot of callers. > > > Just guess. > > > > > > (gdb) disassemble uncharge_batch > > > [...] > > > 0xffffffff81341c73 <+227>: callq 0xffffffff8133c420 > > > 0xffffffff81341c78 <+232>: jmpq 0xffffffff81341bc0 > > > 0xffffffff81341c7d <+237>: callq 0xffffffff8133e2c0 > > > > So does it really help to do the inlining? > > I just think memcg_oom_recover is very small, inline maybe > a good choice. Maybe I am wrong. In general I am not overly keen on changes without a proper justification. In this particular case I would understand that a function call that will almost never do anything but the test (because oom_disabled is a rarely used) is just waste of cycles in some hot paths (e.g. kmem uncharge). Maybe this even has some visible performance benefit. If this is really the case then would it make sense to guard this test by the existing cgroup_subsys_on_dfl(memory_cgrp_subsys)? -- Michal Hocko SUSE Labs