Re: [RFC PATCH 0/7] Reviving the slab destructor to tackle the percpu allocator scalability problem

[Pedro Falcato <pfalcato@suse.de>]

On Fri, Apr 25, 2025 at 07:12:02PM +0900, Harry Yoo wrote:
> On Thu, Apr 24, 2025 at 12:28:37PM +0100, Pedro Falcato wrote:
> > On Thu, Apr 24, 2025 at 05:07:48PM +0900, Harry Yoo wrote:
> > > Overview
> > > ========
> > > 
> > > The slab destructor feature existed in early days of slab allocator(s).
> > > It was removed by the commit c59def9f222d ("Slab allocators: Drop support
> > > for destructors") in 2007 due to lack of serious use cases at that time.
> > > 
> > > Eighteen years later, Mateusz Guzik proposed [1] re-introducing a slab
> > > constructor/destructor pair to mitigate the global serialization point
> > > (pcpu_alloc_mutex) that occurs when each slab object allocates and frees
> > > percpu memory during its lifetime.
> > > 
> > > Consider mm_struct: it allocates two percpu regions (mm_cid and rss_stat),
> > > so each allocate–free cycle requires two expensive acquire/release on
> > > that mutex.
> > > 
> > > We can mitigate this contention by retaining the percpu regions after
> > > the object is freed and releasing them only when the backing slab pages
> > > are freed.
> > > 
> > > How to do this with slab constructors and destructors: the constructor
> > > allocates percpu memory, and the destructor frees it when the slab pages
> > > are reclaimed; this slightly alters the constructor’s semantics,
> > > as it can now fail.
> > > 
> > 
> > I really really really really don't like this. We're opening a pandora's box
> > of locking issues for slab deadlocks and other subtle issues. IMO the best
> > solution there would be, what, failing dtors? which says a lot about the whole
> > situation...
> > 
> > Case in point:
> 
> <...snip...>
> 
> > Then there are obviously other problems like: whatever you're calling must
> > not ever require the slab allocator (directly or indirectly) and must not
> > do direct reclaim (ever!), at the risk of a deadlock. The pcpu allocator
> > is a no-go (AIUI!) already because of such issues.
> 
> Could you please elaborate more on this?

Well, as discussed multiple-times both on-and-off-list, the pcpu allocator is
not a problem here because the freeing path takes a spinlock, not a mutex. But
obviously you can see the fun locking horror dependency chains we're creating
with this patchset. ->ctor() needs to be super careful calling things, avoiding
any sort of loop. ->dtor() needs to be super careful calling things, avoiding
_any_ sort of direct reclaim possibilities. You also now need to pass a gfp_t
to both ->ctor and ->dtor.

With regards to "leaf locks", I still don't really understand what you/Mateusz
mean or how that's even enforceable from the get-go.

So basically:
- ->ctor takes more args, can fail, can do fancier things (multiple allocations,
  lock holding, etc, can be hidden with a normal kmem_cache_alloc; certain
  caches become GFP_ATOMIC-incompatible)

- ->dtor *will* do fancy things like recursing back onto the slab allocator and
  grabbing locks

- a normal kmem_cache_free can suddenly attempt to grab !SLUB locks as it tries
  to dispose of slabs. It can also uncontrollably do $whatever.

- a normal kmem_cache_alloc can call vast swaths of code, uncontrollably, due to
  ->ctor. It can also set off direct reclaim, and thus run into all sorts of kmem_
  cache_free/slab disposal issues

- a normal, possibly-unrelated GFP_KERNEL allocation can also run into all of these
  issues by purely starting up shrinkers on direct reclaim as well.

- the whole original "Slab object caching allocator" idea from 1992 is extremely
  confusing and works super poorly with various debugging features (like, e.g,
  KASAN). IMO it should really be reserved (in a limited capacity!) for stuff like
  TYPESAFE_BY_RCU, that we *really* need.

These are basically my issues with the whole idea. I highly disagree that we should
open this pandora's box for problems in *other places*.

-- 
Pedro