Re: [PATCH v2 0/5] Introduce memcg_stock_pcp remote draining

From: Michal Hocko <mhocko@suse.com>
To: Roman Gushchin <roman.gushchin@linux.dev>
Cc: "Marcelo Tosatti" <mtosatti@redhat.com>,
	"Leonardo Brás" <leobras@redhat.com>,
	"Johannes Weiner" <hannes@cmpxchg.org>,
	"Shakeel Butt" <shakeelb@google.com>,
	"Muchun Song" <muchun.song@linux.dev>,
	"Andrew Morton" <akpm@linux-foundation.org>,
	cgroups@vger.kernel.org, linux-mm@kvack.org,
	linux-kernel@vger.kernel.org,
	"Frederic Weisbecker" <fweisbecker@suse.de>
Subject: Re: [PATCH v2 0/5] Introduce memcg_stock_pcp remote draining
Date: Fri, 27 Jan 2023 08:11:04 +0100	[thread overview]
Message-ID: <Y9N5CI8PpsfiaY9c@dhcp22.suse.cz> (raw)
In-Reply-To: <Y9MI42NSLooyVZNu@P9FQF9L96D.corp.robot.car>

[Cc Frederic]

On Thu 26-01-23 15:12:35, Roman Gushchin wrote:
> On Thu, Jan 26, 2023 at 08:41:34AM +0100, Michal Hocko wrote:
[...]
> > > Essentially each cpu will try to grab the remains of the memory quota
> > > and move it locally. I wonder in such circumstances if we need to disable the pcp-caching
> > > on per-cgroup basis.
> > 
> > I think it would be more than sufficient to disable pcp charging on an
> > isolated cpu.
> 
> It might have significant performance consequences.

Is it really significant?

> I'd rather opt out of stock draining for isolated cpus: it might slightly reduce
> the accuracy of memory limits and slightly increase the memory footprint (all
> those dying memcgs...), but the impact will be limited. Actually it is limited
> by the number of cpus.

Hmm, OK, I have misunderstood your proposal. Yes, the overal pcp charges
potentially left behind should be small and that shouldn't really be a
concern for memcg oom situations (unless the limit is very small and
workloads on isolated cpus using small hard limits is way beyond my
imagination).

My first thought was that those charges could be left behind without any
upper bound but in reality sooner or later something should be running
on those cpus and if the memcg is gone the pcp cache would get refilled
and old charges gone.

So yes, this is actually a better and even simpler solution. All we need
is something like this

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index ab457f0394ab..13b84bbd70ba 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -2344,6 +2344,9 @@ static void drain_all_stock(struct mem_cgroup *root_memcg)
 		struct mem_cgroup *memcg;
 		bool flush = false;
 
+		if (cpu_is_isolated(cpu))
+			continue;
+
 		rcu_read_lock();
 		memcg = stock->cached;
 		if (memcg && stock->nr_pages &&

There is no such cpu_is_isolated() AFAICS so we would need a help from
NOHZ and cpuisol people to create one for us. Frederic, would such an
abstraction make any sense from your POV?
-- 
Michal Hocko
SUSE Labs
