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=-3.6 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,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 0E0CCC54FCB for ; Fri, 24 Apr 2020 16:09:20 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id BAF1A2071E for ; Fri, 24 Apr 2020 16:09:19 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="hVx5ME2c" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org BAF1A2071E Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=gmail.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 599928E0005; Fri, 24 Apr 2020 12:09:19 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 5231D8E0003; Fri, 24 Apr 2020 12:09:19 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 411DF8E0005; Fri, 24 Apr 2020 12:09:19 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0193.hostedemail.com [216.40.44.193]) by kanga.kvack.org (Postfix) with ESMTP id 26ECE8E0003 for ; Fri, 24 Apr 2020 12:09:19 -0400 (EDT) Received: from smtpin13.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay03.hostedemail.com (Postfix) with ESMTP id DC7D68248068 for ; Fri, 24 Apr 2020 16:09:18 +0000 (UTC) X-FDA: 76743233196.13.slope80_51491c857b750 X-HE-Tag: slope80_51491c857b750 X-Filterd-Recvd-Size: 6557 Received: from mail-il1-f195.google.com (mail-il1-f195.google.com [209.85.166.195]) by imf42.hostedemail.com (Postfix) with ESMTP for ; Fri, 24 Apr 2020 16:09:18 +0000 (UTC) Received: by mail-il1-f195.google.com with SMTP id e8so9749253ilm.7 for ; Fri, 24 Apr 2020 09:09:18 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=UsWMYKkrDkOQbtsHGozhmdLBVJ2HR7o3td5AsSj8Ooo=; b=hVx5ME2cl3wqVtYwDU7hc9fb8HnT8Ghl1gSJgIavqHYnGV4dJT88CW3TOv9rANUe82 9/nZ3fVzXM8fZMBa735q+1TJfoQSSmBg+ozuN8B03UUHShVK/Q/VgJNG1vN+0MgPR76a h1fSAFukJXC0QgNe1iTWqO+WXc4Rmx/ichTjHUJqJFX0vvrn17hu7KQgwgdkSuBlCki7 tAPfHxzZDx5ILTEfpwXFNXMhOb8bUC3Hm40vTvWOIip/8h9oeKHjQOwvt6Gs9O4no+yg xq/MGoKIQZmkW/VkU1n3RAM/kkE6H+SHE9lnwXTjw7JCd5uBh+s5wKuHCupUF2xUQYLF rhtA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=UsWMYKkrDkOQbtsHGozhmdLBVJ2HR7o3td5AsSj8Ooo=; b=c6PDgElMnp+pjzFLlmcpdlJ+O6+H8Je7Nuc6oBnTwsJhA0itkS0zaRGEdJC9XNws3j dDdtqmMNr0peCoa/apu/E8OkOAC1obZo8EGNojgcgxt6Bp1u/JN3QGcIuF8iRCprY/5g htyQgDRS+gU3bN2TECapgfa2Pbv2k1iMI+x1UMzFhCYdJsk0gOQaCXsW3A9xRVvinLNo Hj7m6g00E6Vkt8NymcGmQIHy+Qbi2erksvkBqh0Ej9omLsOgtMEG4iViyLjp2g2XaC1O 9zeEuW2DaJ+6nbCDCjHPYZDyl7iJWm9vqKsm49a7ibbMB0YIHin0/IelJK1rO851aPG7 29kA== X-Gm-Message-State: AGi0PuafAGm86NZrGSztCv+V6eMA1Zgoox8azhPCh0nvllKtz8uuJAhq FqZMkJ60w+qldm19ISqCwrKdFM6CkosDH4ABll4= X-Google-Smtp-Source: APiQypIeTxVmg6JO2iVMzmoVMcLH7YmiNH/ZlHttKBpbcrvkFqm1c7j+GsdDnw3m+h7ip3zo8h3tc/71AB81cjK0yDw= X-Received: by 2002:a92:5c57:: with SMTP id q84mr9568753ilb.203.1587744557899; Fri, 24 Apr 2020 09:09:17 -0700 (PDT) MIME-Version: 1.0 References: <20200423061629.24185-1-laoar.shao@gmail.com> <20200424131450.GA495720@cmpxchg.org> <20200424134438.GA496852@cmpxchg.org> In-Reply-To: <20200424134438.GA496852@cmpxchg.org> From: Yafang Shao Date: Sat, 25 Apr 2020 00:08:41 +0800 Message-ID: Subject: Re: [PATCH] mm, memcg: fix wrong mem cgroup protection To: Johannes Weiner Cc: Andrew Morton , Michal Hocko , Vladimir Davydov , Linux MM , Chris Down , Roman Gushchin , stable@vger.kernel.org Content-Type: text/plain; charset="UTF-8" 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, Apr 24, 2020 at 9:44 PM Johannes Weiner wrote: > > On Fri, Apr 24, 2020 at 09:14:52AM -0400, Johannes Weiner wrote: > > However, mem_cgroup_protected() never expected anybody to look at the > > effective protection values when it indicated that the cgroup is above > > its protection. As a result, a query during limit reclaim may return > > stale protection values that were calculated by a previous reclaim > > cycle in which the cgroup did have siblings. > > Btw, I think there is opportunity to make this a bit less error prone. > > We have a mem_cgroup_protected() that returns yes or no, essentially, > but protection isn't a binary state anymore. > > It's also been a bit iffy that it looks like a simple predicate > function, but it indeed needs to run procedurally for each cgroup in > order for the calculations throughout the tree to be correct. > > It might be better to have a > > mem_cgroup_calculate_protection() > > that runs for every cgroup we visit and sets up the internal state; > then have more self-explanatory query functions on top of that: > > mem_cgroup_below_min() > mem_cgroup_below_low() > mem_cgroup_protection() > > What do you guys think? > > diff --git a/mm/vmscan.c b/mm/vmscan.c > index e0f502b5fca6..dbd3f75d39b9 100644 > --- a/mm/vmscan.c > +++ b/mm/vmscan.c > @@ -2615,14 +2615,15 @@ static void shrink_node_memcgs(pg_data_t *pgdat, struct scan_control *sc) > unsigned long reclaimed; > unsigned long scanned; > > - switch (mem_cgroup_protected(target_memcg, memcg)) { > - case MEMCG_PROT_MIN: > + mem_cgroup_calculate_protection(target_memcg, memcg); > + > + if (mem_cgroup_below_min(memcg)) { > /* > * Hard protection. > * If there is no reclaimable memory, OOM. > */ > continue; > - case MEMCG_PROT_LOW: > + } else if (mem_cgroup_below_low(memcg)) { > /* > * Soft protection. > * Respect the protection only as long as > @@ -2634,16 +2635,6 @@ static void shrink_node_memcgs(pg_data_t *pgdat, struct scan_control *sc) > continue; > } > memcg_memory_event(memcg, MEMCG_LOW); > - break; > - case MEMCG_PROT_NONE: > - /* > - * All protection thresholds breached. We may > - * still choose to vary the scan pressure > - * applied based on by how much the cgroup in > - * question has exceeded its protection > - * thresholds (see get_scan_count). > - */ > - break; > } > > reclaimed = sc->nr_reclaimed; After my revist of the memcg protection. I have another idea. The emin and elow is not decided by the memcg(struct mem_cgroup), but they are really decided by the reclaim context(struct srhink_control). So they should not be bound into struct mem_cgroup, while they are really should be bound into struct srhink_control. IOW, we should move emin and elow from struct mem_cgroup into struct srhink_control. And they two members in shrink_control will be updated when a new memcg is to be shrinked. I haven't thought it deeply, but I think this should be the right thing to do. Thanks Yafang -- Thanks Yafang