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 A3A6EC369D9 for ; Wed, 30 Apr 2025 15:16:51 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 3E2706B008C; Wed, 30 Apr 2025 11:16:50 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 395766B00C2; Wed, 30 Apr 2025 11:16:50 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 234486B00C3; Wed, 30 Apr 2025 11:16:50 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0017.hostedemail.com [216.40.44.17]) by kanga.kvack.org (Postfix) with ESMTP id 0386A6B00C2 for ; Wed, 30 Apr 2025 11:16:49 -0400 (EDT) Received: from smtpin28.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay06.hostedemail.com (Postfix) with ESMTP id 83F24BAC7E for ; Wed, 30 Apr 2025 15:16:50 +0000 (UTC) X-FDA: 83391062580.28.A68CB0A Received: from out-183.mta0.migadu.com (out-183.mta0.migadu.com [91.218.175.183]) by imf28.hostedemail.com (Postfix) with ESMTP id AB08CC0007 for ; Wed, 30 Apr 2025 15:16:48 +0000 (UTC) Authentication-Results: imf28.hostedemail.com; dkim=pass header.d=linux.dev header.s=key1 header.b=tbPh85mq; dmarc=pass (policy=none) header.from=linux.dev; spf=pass (imf28.hostedemail.com: domain of shakeel.butt@linux.dev designates 91.218.175.183 as permitted sender) smtp.mailfrom=shakeel.butt@linux.dev ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1746026209; a=rsa-sha256; cv=none; b=S/2L5X2kyTwqdQ6Up0po5zv2CsbGiXaGjr5HoKc8r0lGL+QUgI1jUTduXGC6cd6BuJ1D1D Yow+Xu5VT27t8EX3wfPD4/eopF9w1C+GU5JY/K9rJ91BSwzVAFXRSy50euSyuiYYTPRF7G yzZyS1NDeQp73qPtBX9fKt+enI+iwyk= ARC-Authentication-Results: i=1; imf28.hostedemail.com; dkim=pass header.d=linux.dev header.s=key1 header.b=tbPh85mq; dmarc=pass (policy=none) header.from=linux.dev; spf=pass (imf28.hostedemail.com: domain of shakeel.butt@linux.dev designates 91.218.175.183 as permitted sender) smtp.mailfrom=shakeel.butt@linux.dev ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1746026209; 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=QuMn9HO2ylHkudtJJgWW+uAsUFq7j0ybDWnjZm1chbA=; b=LPHrWWvhyemtPttcB4xu1yTH9gNW4LxzL1wHIZa64LM3iACL6ImjYu2UShNktbvoPlsEhV CdrsOWVys6PXloY5trWUsF1GYGPY0oXYJrk9RwMeCE82lBPEYrmVCvJpl11ooTzH/Dy2fL Mo4kcmiw4Ba63BgXJBScLItvx0rzF6s= Date: Wed, 30 Apr 2025 08:16:40 -0700 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.dev; s=key1; t=1746026206; h=from:from:reply-to:subject:subject: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=QuMn9HO2ylHkudtJJgWW+uAsUFq7j0ybDWnjZm1chbA=; b=tbPh85mqFuFE68zG9wwsyzvn0w6q9QuM+tyJK8/hQ/ZBmw8hoZyObkmHYHybSvoObi4+cv QGRo+e36qf+yddQwfi4mYAWAiyTM3HNdl9YrzzAL4Hs0r7nwS2o1DdyRTdYMBuCIWmCZNT 3JvPQLMoYKh/+B36R4CstLUfTjk0YDo= X-Report-Abuse: Please report any abuse attempt to abuse@migadu.com and include these headers. From: Shakeel Butt To: Vlastimil Babka Cc: Andrew Morton , Johannes Weiner , Michal Hocko , Roman Gushchin , Muchun Song , Jakub Kicinski , Eric Dumazet , Soheil Hassas Yeganeh , linux-mm@kvack.org, cgroups@vger.kernel.org, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, Meta kernel team , Hugh Dickins Subject: Re: [PATCH] memcg: multi-memcg percpu charge cache Message-ID: References: <20250416180229.2902751-1-shakeel.butt@linux.dev> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Migadu-Flow: FLOW_OUT X-Rspamd-Queue-Id: AB08CC0007 X-Rspam-User: X-Rspamd-Server: rspam07 X-Stat-Signature: a3pr3913n7fon3kwm8hgecxiuzkqz5w4 X-HE-Tag: 1746026208-22182 X-HE-Meta: U2FsdGVkX1+uwqefgWgxSqeUxwZ9v2PThXOO/jJ9OnKw1rwv5csVnyZKne1RDLdBqd0V8OR9r5LXn9v2vyxG4fgPDcPMRIyfzXSPYhnNpe8XWcwOh0KkwDK7c4U9RnoZ89CXiNm/fTkbsfbPbRL0/DyliiNSyAWJevCiCSK8sGRzKGG/IC/2ZdSONNdqEUFSifjO71ZftZvUT84thwOcr+sFgqa9X7ScvP0BY6g3Fc5hNDGUQmz/kV/WME1AD6gEoMbnHwmWgXRC7RGYDNfTP+UcnkR5XvlsRcP9LCtqmSkHhGcGMMUJ3bU9qx9o5cuyyJqps4s2jr1Awh8p8za+xdIhFXBrfmcn2fCMJrVM5Qoz7+fFzA+j1i2n8qumdqdjCFVAsQxi0NZTzbH+6U/C0AgupdDCs/HHBd7XhpOBt7aCB+SXGkJ8ukS57jOPFYWFQgyWineyoXG+Hfs2G4Lam6eFKTZfSucfWZGEkjtbz6sMV7rn5VK5kmGlWkG8lleC0C3rSsKimXs72oXcmIFsl5UmC69ksEedIs6dAJNDDsHARcoUyHK3wQE6mjASr9fIB1vYfxTCOBBwpFI4lNNlIuAzw4BXXJCqYDwmap+FFxBTUgMYTH3PjSOG4Xh/zo8Zwlkpb3iJaabTUL+4P7mK8fSPEmTuwW+8hgV5KPW6HFEaJVl5c6OTRVl4PX2SidoQEo2LHvN69grYAbP/meIvnItbPO2S4Id1PuMSLEF4spfICWrITsxjQqkR9qPVOlROaiy//tLhfk00vkJOxrL6Q3W4+zKNkxjaOeCwY1E3s+ncg2xuJdzJI4vrgShnI6RsLslojXYbFP3H98Vp/AYvxW1zR77CN1XU7UD1RnMFtuB8x9VB4lC7y+Q+jAnzZSMsFRlwnZnWBuAVx+5UeoGMG8cRRwanZpS85Gr3/VuhX5lGJbpDxNc1Uy449OZBAqnuZThMsColbQCYMLztZy0 0RkQAkd9 lISTRad7mGSWvG4FtRERx4wDRf2dMf6nD0eRDb1SKLWUVJpKtzpYdyHOmyLy5O6DXT5A8Ddf2kz7vxGTcleIAUDTBQFKVFaOBj16uXBNStHmoDEUxTVrp+ZrTeh+cSEPT+mEO6rwUeHOPOH5nO9hvxO+QKieXk/Ouwgo7nqymRJSOizxNiSYzuviiug== 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: List-Subscribe: List-Unsubscribe: On Wed, Apr 30, 2025 at 12:05:48PM +0200, Vlastimil Babka wrote: > On 4/25/25 22:18, Shakeel Butt wrote: > > Hi Andrew, > > > > Another fix for this patch. Basically simplification of refill_stock and > > avoiding multiple cached entries of a memcg. > > > > From 6f6f7736799ad8ca5fee48eca7b7038f6c9bb5b9 Mon Sep 17 00:00:00 2001 > > From: Shakeel Butt > > Date: Fri, 25 Apr 2025 13:10:43 -0700 > > Subject: [PATCH] memcg: multi-memcg percpu charge cache - fix 2 > > > > Simplify refill_stock by avoiding goto and doing the operations inline > > and make sure the given memcg is not cached multiple times. > > > > Signed-off-by: Shakeel Butt > > It seems to me you could simplify further based on how cached/nr_pages > arrays are filled from 0 to higher index and thus if you see a NULL it means > all higher indices are also NULL. At least I don't think there's ever a > drain_stock() that would "punch a NULL" in the middle? When it's done in > refill_stock() for the random index, it's immediately reused. > > Of course if that invariant was made official and relied upon, it would need > to be documented and care taken not to break it. > > But then I think: > - refill_stock() could be further simplified > - loops in consume_stop() and is_drain_needed() could stop on first NULL > cached[i] encountered. > > WDYT? > Please see below. > > --- > > mm/memcontrol.c | 27 +++++++++++++++------------ > > 1 file changed, 15 insertions(+), 12 deletions(-) > > > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > > index 997e2da5d2ca..9dfdbb2fcccc 100644 > > --- a/mm/memcontrol.c > > +++ b/mm/memcontrol.c > > @@ -1907,7 +1907,8 @@ static void refill_stock(struct mem_cgroup *memcg, unsigned int nr_pages) > > struct mem_cgroup *cached; > > uint8_t stock_pages; > > unsigned long flags; > > - bool evict = true; > > + bool success = false; > > + int empty_slot = -1; > > int i; > > > > /* > > @@ -1931,26 +1932,28 @@ static void refill_stock(struct mem_cgroup *memcg, unsigned int nr_pages) > > > > stock = this_cpu_ptr(&memcg_stock); > > for (i = 0; i < NR_MEMCG_STOCK; ++i) { > > -again: > > cached = READ_ONCE(stock->cached[i]); > > - if (!cached) { > > - css_get(&memcg->css); > > - WRITE_ONCE(stock->cached[i], memcg); > > - } > > - if (!cached || memcg == READ_ONCE(stock->cached[i])) { > > + if (!cached && empty_slot == -1) > > + empty_slot = i; > > + if (memcg == READ_ONCE(stock->cached[i])) { > > stock_pages = READ_ONCE(stock->nr_pages[i]) + nr_pages; > > WRITE_ONCE(stock->nr_pages[i], stock_pages); > > if (stock_pages > MEMCG_CHARGE_BATCH) > > drain_stock(stock, i); So, this drain_stock() above can punch a NULL hole in the array but I think I do see your point. We can fill this hole by moving the last non-NULL here. For now I plan to keep it as is as I have some followup plans to make this specific drain_stock() conditional on the caller (somewhat similar to commit 5387c90490f7f) and then I will re-check if we can eliminate this NULL hole. Thanks a lot for the reviews and suggestions.