From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-qk1-f176.google.com (mail-qk1-f176.google.com [209.85.222.176]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id D26C02EB10 for ; Sun, 29 Mar 2026 06:48:02 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.222.176 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1774766884; cv=none; b=uutrjZD99fCDCl2LVifW7r9+35sdKQLtnjIsTcqcZ0X4trjjO2uxdhMV0jDHxcCSWDkSZCV8RraxSwG7sWcMm/9xREkRsdob/aOC1TJubz55z7uRXNnCwgGQ4PBSSMlTxtd5pdkb10LkCb9793LEgJ5hAJOOYXIiQuyf/KL4V2M= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1774766884; c=relaxed/simple; bh=gIxg0o9eXYO9CYyMV92e8a6+LW/1f+sTSreX+v8d8kA=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=Zx0axZ+YzqDDWL7AeHKFR4qq/WNmgYekL/1B2IY9vF2wJtlzY/0qlGiYiG9JzMJ0TbED9bi+swzMqUe5FDZy2d3hX2a7kxo9Dt7qzgA+w/Yl6SxipIATyh4nVWt12qn05x60qBMkWBprDbrx51NoUMsjZ+xaFezxtGKWq1PddYM= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com; spf=pass smtp.mailfrom=gmail.com; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b=SlZthQWV; arc=none smtp.client-ip=209.85.222.176 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=gmail.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="SlZthQWV" Received: by mail-qk1-f176.google.com with SMTP id af79cd13be357-8cfc497a604so484077585a.3 for ; Sat, 28 Mar 2026 23:48:02 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20251104; t=1774766882; x=1775371682; darn=vger.kernel.org; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=gQqiwkS82VX9Ubj17wq6gcqrqBCzX78+3UD7c/Wp8mc=; b=SlZthQWVfVQ/rOPRgBsvAdq1E68q13oHhL40gtyWdkpYY5YFsuICTfp7vUQfO/8E7+ XINAIkAiAn937Yl3NTmwll5uKbyAj563yDhc/sRBOW+t1Kcy0pi4COl2XB0IgfUpl8Jk UzYlXzgtebue0no6p5nilzztpj8jApUYz8qqg7RDKufYPS180kd3zawwwL9HOIHtzeiC raNiNO+zx5ATM95CKscmohNUxBHh56AglDChjSsIjBfQcY11f3rcXLzDJSAMZP8PZj/n rb52iOnqhEs3f6r5y9P6/iCKet0c5X8ZGcokuA8myfgxTaarC+kZwj1I0MxeqtP1ElFM 3LAg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1774766882; x=1775371682; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-gg:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=gQqiwkS82VX9Ubj17wq6gcqrqBCzX78+3UD7c/Wp8mc=; b=W7nc2zdcFIFsS1M2Mt7OmGstTVIjXiqtg9AeYPi5KQ8YzYCN98ZRsWBZICeSOKDk2w BODcTEVZCxquk8sBkufS+ppmIu1ROCkQkSPm750QWF8vMXuhPWQD7Sto1T+lwncYk5/9 k3O921BTHKgpIEhEJc/hyoRsPQxIVnX70ydPjJH3JnUqqHExhxU27knCl38S7wytHepA TatZunStOh/V4JbTHMP2O9IPSiFWR6Ziy5MzcU7WF10Ps88aI5hsDb81uurI3eftJSsT pyu9jZ78QDdpiDYgEc2fzEPlRw9UQdBdox7bPW7dj+YriSdK+4YCh155EL6dorfGHCRu 6P1g== X-Forwarded-Encrypted: i=1; AJvYcCWx7Jj3xhGGVcKiK3bPpR0qFILWk/l1ysTziQxURr10g4OIKPUD4KHYfpGz4keGzWYBDphlrLJYUqrr4mo=@vger.kernel.org X-Gm-Message-State: AOJu0YyMXAbU2TsKRmfXUcOJw+kCQ7/KRaTDnMQtVfOYnS2bDuaYd+Hf tXouRzUPta4oMwYBwvhZQ73SMvCjB0OXg7dRbKyELxSqDHoxhPsuvm8D X-Gm-Gg: ATEYQzxXZPYZdh4QEvyJ6PWDK30E1pyK4HSCuP41veeoP24crGUyag8s2wogOYD8y+9 FeqvPTWu31Aw/t8n7hMlroBD7/qzjjoFoqlDwi8u5Sod2pXeWt8CGjv5VkKHCHNzWj8gYb/A5dK sQbYdahup65Ng3jyLAbZ818aW2+NCfGbxqBB9gKyDykdJwiWqUj9uAs4ksRdyoGYpYDm7+qwCNX joIYFjJhwBId9LRYDbj4LtpetGi+nj4QPA362xzK59JRch0MjfTQXgFIMUi5bX9/p56CvQRa+9V GOIVaqxuUT030yvcGbsmkKllQTk0BcL1NQ4r1NmfHtBXQ8HIEQXwTSEQsf6GlU+r/9r0R9xliQe DP+wTBzOo5IMimlgomi61QzRzxk2RdFcgS+5sw4OfAtpaKQPPYlvC1Y+/nFkbGeshUfuvRgVWxP 9PLXPPj6PXef7epK5c1bLC9dF5o/o6agwYKeN24PcO9g3/rBIwVkwQURKMIoE= X-Received: by 2002:a05:620a:40d5:b0:8cd:8a55:510d with SMTP id af79cd13be357-8d01c7a368bmr1071941785a.48.1774766881712; Sat, 28 Mar 2026 23:48:01 -0700 (PDT) Received: from KASONG-MC4 ([101.32.222.185]) by smtp.gmail.com with ESMTPSA id af79cd13be357-8d02807b0e6sm307384485a.39.2026.03.28.23.47.53 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sat, 28 Mar 2026 23:48:00 -0700 (PDT) Date: Sun, 29 Mar 2026 14:47:50 +0800 From: Kairui Song To: linux-mm@kvack.org Cc: Kairui Song , Andrew Morton , Axel Rasmussen , Yuanchu Xie , Wei Xu , Johannes Weiner , David Hildenbrand , Michal Hocko , Qi Zheng , Shakeel Butt , Lorenzo Stoakes , Barry Song , David Stevens , Chen Ridong , Leno Hou , Yafang Shao , Yu Zhao , Zicheng Wang , Kalesh Singh , Suren Baghdasaryan , Chris Li , Vernon Yang , linux-kernel@vger.kernel.org, Qi Zheng , Baolin Wang Subject: Re: [PATCH v2 04/12] mm/mglru: restructure the reclaim loop Message-ID: References: <20260329-mglru-reclaim-v2-0-b53a3678513c@tencent.com> <20260329-mglru-reclaim-v2-4-b53a3678513c@tencent.com> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20260329-mglru-reclaim-v2-4-b53a3678513c@tencent.com> So sashiko provided some review, I'll post it below: https://sashiko.dev/#/patchset/20260329-mglru-reclaim-v2-0-b53a3678513c%40tencent.com?patch=12986 > From sashiko: > > > diff --git a/mm/vmscan.c b/mm/vmscan.c > > --- a/mm/vmscan.c > > +++ b/mm/vmscan.c > > @@ -4913,49 +4913,40 @@ static int evict_folios(unsigned long nr_to_scan, struct lruvec *lruvec, > > } > > > > static bool should_run_aging(struct lruvec *lruvec, unsigned long max_seq, > > - int swappiness, unsigned long *nr_to_scan) > > + struct scan_control *sc, int swappiness) > > { > > DEFINE_MIN_SEQ(lruvec); > > > > - *nr_to_scan = 0; > > /* have to run aging, since eviction is not possible anymore */ > > if (evictable_min_seq(min_seq, swappiness) + MIN_NR_GENS > max_seq) > > return true; > > > > - *nr_to_scan = lruvec_evictable_size(lruvec, swappiness); > > + /* try to get away with not aging at the default priority */ > > + if (sc->priority == DEF_PRIORITY) > > + return false; > > Is this a regression that unconditionally forces aging even at DEF_PRIORITY? > In the original code, if eviction wasn't possible, it set need_aging to > true, but the subsequent check against DEF_PRIORITY could still bypass aging > and allow MGLRU to skip memcgs without cold pages. > With the new logic, the mandatory aging check precedes the DEF_PRIORITY > check, returning true immediately. Will this cause try_to_shrink_lruvec() > to inappropriately force hot pages into older generations on the gentlest > reclaim pass instead of trying other memcgs first? Right, this part is a bit confusing indeed, I tried to split it into two patches but it seem could be more messy that way. This is more of an intentional change. When eviction is not possible (fewer than MIN_NR_GENS generations), aging is the only way to make progress. The old code always skip aging at DEF_PRIORITY. For single cgroup setup, this is not helpful in anyway, it waste a reclaim iteration only to come back at a higher priority and age then. And it causes over reclaim of slab. For multi cgroup setup, it may seems reasonable that defer the aging and reclaim other cgroup a good idea, but we will have to proceed with aging anyway, potentially when all cgroups are run out of gens, causing a larger jitter, and break reclaim balance. And the escalated priority also may cause over reclaim. And right now we still have 3 gens as the buffer. Testing the multi cgroup setup using JaveScript workload in the cover letter showed that after this series, multi cgroup reclaim balance and latency are good, the total throughput seems better. Testing the reclaim balance of cgroup tree (see below) also seems better. I think to avoid the latency of aging, we really need async aging, not deferred aging. Async aging might be easier to be integrated here now after this series. Could add some word on that in the commit message. > From sashiko: > > > -static long get_nr_to_scan(struct lruvec *lruvec, struct scan_control *sc, int swappiness) > > +static long get_nr_to_scan(struct lruvec *lruvec, struct scan_control *sc, > > + struct mem_cgroup *memcg, int swappiness) > > { > > - bool need_aging; > > unsigned long nr_to_scan; > > - struct mem_cgroup *memcg = lruvec_memcg(lruvec); > > - DEFINE_MAX_SEQ(lruvec); > > - > > - if (mem_cgroup_below_min(sc->target_mem_cgroup, memcg)) > > - return -1; > > - > > - need_aging = should_run_aging(lruvec, max_seq, swappiness, &nr_to_scan); > > > > + nr_to_scan = lruvec_evictable_size(lruvec, swappiness); > > /* try to scrape all its memory if this memcg was deleted */ > > - if (nr_to_scan && !mem_cgroup_online(memcg)) > > + if (!mem_cgroup_online(memcg)) > > return nr_to_scan; > > Will this cause a regression resulting in a permanent memory leak for > offline memcgs that have zero evictable folios? > If an offline memcg has all its folios in the unevictable youngest > generation, lruvec_evictable_size() returns 0. The early return here > then evaluates to 0. > When try_to_shrink_lruvec() receives 0, its while (nr_to_scan > 0) loop > is completely bypassed. Since the aging logic was moved inside that loop, > the offline memcg is never aged, meaning its folios will remain in the > unevictable youngest generation indefinitely. Seems hallucination. lruvec_evictable_size returns evictable folios across gen, not excluding the youngest one since aging may make then evictable. Perhaps we can add some comment for this, if it confuses AI, it may confuse human too. And aging is not out of the loop, it's just moved to a different place helper. > From sashiko: > > > > nr_to_scan = apply_proportional_protection(memcg, sc, nr_to_scan); > > > > - /* try to get away with not aging at the default priority */ > > - if (!need_aging || sc->priority == DEF_PRIORITY) > > - return nr_to_scan >> sc->priority; > > - > > - /* stop scanning this lruvec as it's low on cold folios */ > > - return try_to_inc_max_seq(lruvec, max_seq, swappiness, false) ? -1 : 0; > > + /* > > + * Always respect scan priority, minimally target > > + * SWAP_CLUSTER_MAX pages to keep reclaim moving forwards. > > + */ > > + nr_to_scan >>= sc->priority; > > + return max(nr_to_scan, SWAP_CLUSTER_MAX); > > } > > Does this regression break proportional reclaim and memory protection > semantics for small memcgs by forcing a minimum scan size? Actually this change was inspired by sashiko's review for V1: https://sashiko.dev/#/patchset/20260318-mglru-reclaim-v1-0-2c46f9eb0508%40tencent.com?patch=2909 Without this, for cgroups smaller than 16M, def priority scan will just do nothing. It's still OK but to make it more efficient I added a minimal batch. But now think again, it better be: if (!nr_to_scan) nr_to_scan = min(lruvec_evictable_size, SWAP_CLUSTER_MAX); Using max() here could get very small cgroups over reclaimed. I did test V2 using test_memcg_min suggested by af827e090489: Before: Proportional reclaim results: c[0] actual= 29069312 (27M) ideal= 30408704 (29M) err=4.4% c[1] actual= 23257088 (22M) ideal= 22020096 (21M) err=5.6% c[2] actual= 1552384 (1M) (expected ~0) c[3] actual= 0 (0M) (expected =0) After: Proportional reclaim results: c[0] actual= 31391744 (29M) ideal= 30408704 (29M) err=3.2% c[1] actual= 21028864 (20M) ideal= 22020096 (21M) err=4.5% c[2] actual= 1515520 (1M) (expected ~0) c[3] actual= 0 (0M) (expected =0) In both case the result is somehow not very stable, I run the test 7 times using the medium stable result, after this series it seems sometimes the result is even better but likely just noisy. And didn't see a regression. The 32 folios minimal batch seems already small enough for typical usage, but min(evictable_size, SWAP_CLUSTER_MAX) is definitely better. Will send a V3 to update this. I think non of the benchmark or test would be effected by this.