From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from out-187.mta1.migadu.com (out-187.mta1.migadu.com [95.215.58.187]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 4693137186F for ; Tue, 3 Mar 2026 16:26:51 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=95.215.58.187 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1772555212; cv=none; b=AgOyRFhCTYJuKkgNG2iIPiUgFrPhCLZyQJMFbp0u8DMkBE8JSKcz2wMFwi2fBh5K3pS6syewODlpD6X4QOi8CCfNxd9Gfe9TmoUoEMjovizxClz/6RLzC9KaL7leCm4/AaLl7tXsJbZItKx9O+mFJlRkfav/p0zblSKjBEORbIE= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1772555212; c=relaxed/simple; bh=3mi18L2XAhtNJP4w0YftC8HpJpTk85/bCiQBpe/fGzY=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=RL/2OTyTowe6kV6CGWHTCeUSWRt8Tg13BUZZlL7CNP+paslm46+SZwRyv5VZJWbw0gH1utjvUEL6fTCgWjdVugjIz/rbv5gpQWBKSYRiRZmQuVmlWStmOzBIu4QDDVgBZzKOM1RnfUH7qlWPhEyYNYk8AG3NFt06sZQi13qXvJ0= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.dev; spf=pass smtp.mailfrom=linux.dev; dkim=pass (1024-bit key) header.d=linux.dev header.i=@linux.dev header.b=cTct34xt; arc=none smtp.client-ip=95.215.58.187 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.dev Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=linux.dev Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=linux.dev header.i=@linux.dev header.b="cTct34xt" Date: Tue, 3 Mar 2026 08:26:31 -0800 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.dev; s=key1; t=1772555199; 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=KWZ2Q88mCamuBtdqha7mUqDv9x9r6pDkpjHI+V+2Iuk=; b=cTct34xtiw6UR8JCa5DZiQShjiTVn3S5vutC2R8hjpvh0fvfPB7sIcEkVQf5PVaPUOsDgw GqfOywqGUWr+JlDAE+MQRWf2kugQdKM6+VZtboockpUKIh1M73Pijxkld7/AO6m/7+azjf xB1dcAQPS3aOKgl2l9uuAU7qOxT+suc= X-Report-Abuse: Please report any abuse attempt to abuse@migadu.com and include these headers. From: Shakeel Butt To: Johannes Weiner Cc: "Vlastimil Babka (SUSE)" , Hao Li , Andrew Morton , Michal Hocko , Roman Gushchin , Vlastimil Babka , Harry Yoo , linux-mm@kvack.org, cgroups@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 5/5] mm: memcg: separate slab stat accounting from objcg charge cache Message-ID: References: <20260302195305.620713-1-hannes@cmpxchg.org> <20260302195305.620713-6-hannes@cmpxchg.org> <541a6661-7bfe-4517-a32c-5839002c61e5@kernel.org> 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: X-Migadu-Flow: FLOW_OUT On Tue, Mar 03, 2026 at 10:43:29AM -0500, Johannes Weiner wrote: > On Tue, Mar 03, 2026 at 05:45:18AM -0800, Shakeel Butt wrote: > > On Tue, Mar 03, 2026 at 11:42:31AM +0100, Vlastimil Babka (SUSE) wrote: > > > On 3/3/26 09:54, Hao Li wrote: > > > > On Mon, Mar 02, 2026 at 02:50:18PM -0500, Johannes Weiner wrote: > > > >> > > > >> +static void refill_obj_stock(struct obj_cgroup *objcg, > > > >> + unsigned int nr_bytes, > > > >> + bool allow_uncharge) > > > >> +{ > > > >> + struct obj_stock_pcp *stock = trylock_stock(); > > > >> + __refill_obj_stock(objcg, stock, nr_bytes, allow_uncharge); > > > >> + unlock_stock(stock); > > > > > > > > Hi Johannes, > > > > > > > > I noticed that after this patch, obj_cgroup_uncharge_pages() is now inside > > > > the obj_stock.lock critical section. Since obj_cgroup_uncharge_pages() calls > > > > refill_stock(), which seems non-trivial, this might increase the lock hold time. > > > > In particular, could that lead to more failed trylocks for IRQ handlers on > > > > non-RT kernel (or for tasks that preempt others on RT kernel)? > > Good catch. I did ponder this, but forgot by the time I wrote the > changelog. > > > > Yes, it also seems a bit self-defeating? (at least in theory) > > > > > > refill_obj_stock() > > > trylock_stock() > > > __refill_obj_stock() > > > obj_cgroup_uncharge_pages() > > > refill_stock() > > > local_trylock() -> nested, will fail > > > > Not really as the local_locks are different i.e. memcg_stock.lock in > > refill_stock() and obj_stock.lock in refill_obj_stock(). > > Right, refilling the *byte* stock could produce enough excess that we > refill the *page* stock. Which in turn could produce enough excess > that we drain that back to the page counters (shared atomics). > > > However Hao's concern is valid and I think it can be easily fixed by > > moving obj_cgroup_uncharge_pages() out of obj_stock.lock. > > Note that we now have multiple callsites of __refill_obj_stock(). Do > we care enough to move this to the caller? > > There are a few other places with a similar pattern: > > - drain_obj_stock(): calls memcg_uncharge() under the lock > - drain_stock(): calls memcg_uncharge() under the lock > - refill_stock(): still does full drain_stock() > > All of these could be more intentional about only updating the per-cpu > data under the lock and the page counters outside of it. > > Given that IRQ allocations/frees are rare, nested ones even rarer, and > the "slowpath" is a few extra atomics, I'm not sure it's worth the > code complication. At least until proven otherwise. > > What do you think? Yes this makes sense. We already have at least one evidence (bug Hao fixed) that these are very rare, so optimizing for such cases will just increase complexity without real benefit.