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 D71D0C433FE for ; Wed, 23 Nov 2022 09:26:00 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 742C46B0071; Wed, 23 Nov 2022 04:26:00 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 6F2F06B0073; Wed, 23 Nov 2022 04:26:00 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 5BB536B0074; Wed, 23 Nov 2022 04:26:00 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0011.hostedemail.com [216.40.44.11]) by kanga.kvack.org (Postfix) with ESMTP id 4B3026B0071 for ; Wed, 23 Nov 2022 04:26:00 -0500 (EST) Received: from smtpin13.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay02.hostedemail.com (Postfix) with ESMTP id 0316C12113C for ; Wed, 23 Nov 2022 09:25:59 +0000 (UTC) X-FDA: 80164175280.13.52FB483 Received: from mail-io1-f48.google.com (mail-io1-f48.google.com [209.85.166.48]) by imf10.hostedemail.com (Postfix) with ESMTP id ACE63C000E for ; Wed, 23 Nov 2022 09:25:59 +0000 (UTC) Received: by mail-io1-f48.google.com with SMTP id h206so12739348iof.10 for ; Wed, 23 Nov 2022 01:25:59 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=zfp4k1sKmziFXjGhxPUQOEODXUQiY7lJXnmdU3Z7u40=; b=r6us3HO2cpO+/juFHf/7AFGmFJRyTVVSJ90hkFyxxYWsiDt/6aUGQhLIWhkk0PwSTj xVub2pqsj9xv0xQbhGdd9XKOlLMAAZlVoSm5kCRHGdmvPa3T/XlH9qRc/p/nw6HalDg6 sSmBDCEunURsqvvLl9+l4/YRatTsy/gVecvcU9gWueM5gFwMHmogfOjx1H3y1Xz+tLeo VsEp1RYA/wP9FRgTjaAdYyH9DemF4OpLSr+uCR5LGHxQed+PAVa0yjXLpYPMVUxb1Smj SOgTkKzTKeLqPhUFS75z6QpZBZqSQqMuo7wvd/Rzeq4eHDQdlN7QupWoYj057Y8Lr+4r /dYg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=zfp4k1sKmziFXjGhxPUQOEODXUQiY7lJXnmdU3Z7u40=; b=mcBfjaOZyJribSiQ5rROwDnTzhnxeJC1A9TOD/8hmQ5tpP/gTZK2Tc/uCEiEmpiPmQ HNyEpAr9s7OqEE5PM4ISixizw9y/0Q5/bG1I3n1UDbBqdoeQUW4bWiZwSVGtaKdJlIhi adJLXosSXbg0+M2ll5RzlkYMiCxooLWq2WmgHCcUf1IkjkD173jA2DRffwBLsdAgfIV0 VCSgJWKs9UtYnDJAYO7XvkoKuCRQfcoxZ1QVv7s4hqjnnb1tVyp8qj6ApP1dk0SoeT9y /ScFgSyE+DIUoU77u3S625KXpxRRCI9tvuBquKIvoIWPFfSRbS4gT1AwwcZo2OvfFGjt Wbew== X-Gm-Message-State: ANoB5pmwsYvvSAQsT9A8tlqfVVFesDQAB41s3QfjRGvTy+9Yg3q3ryb5 nFZNfg4JTEs7dtDniG4vFh3D2w/GybU2twYtn3d7aQ== X-Google-Smtp-Source: AA0mqf77pd4P8P/sGrZTQOi1dKZNs4aduy+YQPImiNt4QrXMlDSAmAs0JLMcZKDUKdjhOCIzRqOIS8G6A9+w53Dqp2U= X-Received: by 2002:a02:2422:0:b0:375:1ad6:e860 with SMTP id f34-20020a022422000000b003751ad6e860mr13348375jaa.191.1669195558797; Wed, 23 Nov 2022 01:25:58 -0800 (PST) MIME-Version: 1.0 References: <20221122232721.2306102-1-yosryahmed@google.com> In-Reply-To: From: Yosry Ahmed Date: Wed, 23 Nov 2022 01:25:22 -0800 Message-ID: Subject: Re: [PATCH] mm: memcg: fix stale protection of reclaim target memcg To: Roman Gushchin Cc: Shakeel Butt , Johannes Weiner , Michal Hocko , Yu Zhao , Muchun Song , "Matthew Wilcox (Oracle)" , Vasily Averin , Vlastimil Babka , Chris Down , linux-kernel@vger.kernel.org, linux-mm@kvack.org Content-Type: text/plain; charset="UTF-8" ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1669195559; a=rsa-sha256; cv=none; b=RXUHoqzsKGAg0plF5XhIJkq1lR1aRD6uCwBFoGIRxlG/x4FAkbeVXytOPyqWb5WPnqve7a Sw8sLby/8LQrPay/N9HanzbotxzpZjzeSYAm3fYxKli+n3XMVxjI0StgfiopXwKLxIjsjf rlnoBps+4h8DjB2N3iyEyhYDF5luTC0= ARC-Authentication-Results: i=1; imf10.hostedemail.com; dkim=pass header.d=google.com header.s=20210112 header.b=r6us3HO2; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf10.hostedemail.com: domain of yosryahmed@google.com designates 209.85.166.48 as permitted sender) smtp.mailfrom=yosryahmed@google.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1669195559; 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: in-reply-to:in-reply-to:references:references:dkim-signature; bh=zfp4k1sKmziFXjGhxPUQOEODXUQiY7lJXnmdU3Z7u40=; b=L3BtJsAFZSuU9pQ8UauUHSgdCDiz8VgIimKD9aCH3ep3Q0JL2lpmki2/V2xMcYBer0nLK6 ibQjPsriszv6D+u5xAp+mgpOP2XtdrR75sOqc85m/EUWFTxIMoDs9odDysYjgcYJBCnZ4f JPv3toGgUlKdRDixozH7/cdUJrh6z1Q= X-Stat-Signature: rqesdwoxbi39im4k5ojy7k9rgpr95g6o X-Rspamd-Server: rspam05 X-Rspamd-Queue-Id: ACE63C000E Authentication-Results: imf10.hostedemail.com; dkim=pass header.d=google.com header.s=20210112 header.b=r6us3HO2; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf10.hostedemail.com: domain of yosryahmed@google.com designates 209.85.166.48 as permitted sender) smtp.mailfrom=yosryahmed@google.com X-Rspam-User: X-HE-Tag: 1669195559-570346 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000024, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Tue, Nov 22, 2022 at 4:37 PM Roman Gushchin wrote: > > On Tue, Nov 22, 2022 at 11:27:21PM +0000, Yosry Ahmed wrote: > > During reclaim, mem_cgroup_calculate_protection() is used to determine > > the effective protection (emin and elow) values of a memcg. The > > protection of the reclaim target is ignored, but we cannot set their > > effective protection to 0 due to a limitation of the current > > implementation (see comment in mem_cgroup_protection()). Instead, > > we leave their effective protection values unchaged, and later ignore it > > in mem_cgroup_protection(). > > > > However, mem_cgroup_protection() is called later in > > shrink_lruvec()->get_scan_count(), which is after the > > mem_cgroup_below_{min/low}() checks in shrink_node_memcgs(). As a > > result, the stale effective protection values of the target memcg may > > lead us to skip reclaiming from the target memcg entirely, before > > calling shrink_lruvec(). This can be even worse with recursive > > protection, where the stale target memcg protection can be higher than > > its standalone protection. > > > > An example where this can happen is as follows. Consider the following > > hierarchy with memory_recursiveprot: > > ROOT > > | > > A (memory.min = 50M) > > | > > B (memory.min = 10M, memory.high = 40M) > > > > Consider the following scenarion: > > - B has memory.current = 35M. > > - The system undergoes global reclaim (target memcg is NULL). > > - B will have an effective min of 50M (all of A's unclaimed protection). > > - B will not be reclaimed from. > > - Now allocate 10M more memory in B, pushing it above it's high limit. > > - The system undergoes memcg reclaim from B (target memcg is B) > > - In shrink_node_memcgs(), we call mem_cgroup_calculate_protection(), > > which immediately returns for B without doing anything, as B is the > > target memcg, relying on mem_cgroup_protection() to ignore B's stale > > effective min (still 50M). > > - Directly after mem_cgroup_calculate_protection(), we will call > > mem_cgroup_below_min(), which will read the stale effective min for B > > and skip it (instead of ignoring its protection as intended). In this > > case, it's really bad because we are not just considering B's > > standalone protection (10M), but we are reading a much higher stale > > protection (50M) which will cause us to not reclaim from B at all. > > > > This is an artifact of commit 45c7f7e1ef17 ("mm, memcg: decouple > > e{low,min} state mutations from protection checks") which made > > mem_cgroup_calculate_protection() only change the state without > > returning any value. Before that commit, we used to return > > MEMCG_PROT_NONE for the target memcg, which would cause us to skip the > > mem_cgroup_below_{min/low}() checks. After that commit we do not return > > anything and we end up checking the min & low effective protections for > > the target memcg, which are stale. > > > > Add mem_cgroup_ignore_protection() that checks if we are reclaiming from > > the target memcg, and call it in mem_cgroup_below_{min/low}() to ignore > > the stale protection of the target memcg. > > > > Fixes: 45c7f7e1ef17 ("mm, memcg: decouple e{low,min} state mutations from protection checks") > > Signed-off-by: Yosry Ahmed > > Great catch! > The fix looks good to me, only a couple of cosmetic suggestions. > > > --- > > include/linux/memcontrol.h | 33 +++++++++++++++++++++++++++------ > > mm/vmscan.c | 11 ++++++----- > > 2 files changed, 33 insertions(+), 11 deletions(-) > > > > diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h > > index e1644a24009c..22c9c9f9c6b1 100644 > > --- a/include/linux/memcontrol.h > > +++ b/include/linux/memcontrol.h > > @@ -625,18 +625,32 @@ static inline bool mem_cgroup_supports_protection(struct mem_cgroup *memcg) > > > > } > > > > -static inline bool mem_cgroup_below_low(struct mem_cgroup *memcg) > > +static inline bool mem_cgroup_ignore_protection(struct mem_cgroup *target, > > + struct mem_cgroup *memcg) > > { > > - if (!mem_cgroup_supports_protection(memcg)) > > How about to merge mem_cgroup_supports_protection() and your new helper into > something like mem_cgroup_possibly_protected()? It seems like they never used > separately and unlikely ever will be used. > Also, I'd swap target and memcg arguments. > > Thank you! > > > PS If it's not too hard, please, consider adding a new kselftest to cover this case. > Thank you! Sent v2 with mem_cgroup_supports_protection() and mem_cgroup_ignore_protection() merged into mem_cgroup_unprotected(). Also added a test case to test_memcontrol.c:test_memcg_protection. Since the scenario in the bash test and the v1 commit log was too complicated, I extended the existing test with a simpler scenario based on proactive reclaim, and reused some functionality from test_memcg_reclaim(). I also included explaining that simple proactive reclaim scenario in the commit log of the fix. Writing a test for the more complex scenario with recursive protection would be more involved, so I think this should be enough for now :)