Re: [PATCH v8 4/6] drm/ttm: Hook up a cgroup-aware reclaim callback for the dmem controller

On Thu, 2026-07-23 at 14:02 +0200, Maarten Lankhorst wrote:
> Hey,
> 
> On 7/23/26 12:03, Thomas Hellström wrote:
> > Add ttm_bo_evict_cgroup() to evict buffer objects charged to a
> > specific
> > dmem cgroup pool from a resource manager's LRU until a byte target
> > is
> > met.  Add ttm_resource_manager_set_dmem_region() to associate a
> > dmem
> > cgroup region with a resource manager; drivers supply their own
> > dmem_cgroup_ops with ttm_resource_manager_dmem_reclaim as the
> > reclaim
> > function and the manager pointer as reclaim_priv in the
> > dmem_cgroup_init
> > to wire up TTM eviction as the reclaim callback.
> > 
> > The eviction context is interruptible; signals abort the operation
> > and
> > propagate back through the write() syscall.
> > 
> > Introduce a new mode for the bo LRU walker so that sleeping locks
> > can be taken. This can be used when the caller doesn't hold any
> > previous dma_resv locks, and where it intends to hold at most
> > one lock at a time.
> > 
> > Like the rest of the TTM eviction this should sooner than later
> > be converted to full WW transactions.
> > 
> > v3:
> > - Fix ttm_resource_manager_set_dmem_region() storing an error
> > pointer
> >   in man->cg unconditionally. (Sashiko-bot)
> > - Fix kernel-doc function name format for ttm_bo_evict_cgroup() and
> >   ttm_resource_manager_set_dmem_region().
> > 
> > v5:
> > - Rebased on the introduction of struct dmem_cgroup_init.
> > - Handle NULL region in ttm_resource_manager_set_dmem_region() to
> > clear
> >   the reclaim callback, preventing use-after-free when the manager
> > is
> >   torn down while the dmem region outlives it. (Sashiko-bot)
> > - Return 0 on any progress (even partial eviction), -ENOSPC only
> > when
> >   nothing was freed; fixes callers that expected 0 on partial
> > success.
> > - Document that the reclaim callback should return 0 if some
> > progress
> >   was made, -ENOSPC if no progress at all, or another error for
> > fatal
> >   failures.
> > 
> > v8:
> > - Fix ttm_resource_manager_set_dmem_region() using
> > IS_ERR_OR_NULL(),
> >   which skipped the assignment for a NULL region and thus never
> >   cleared man->cg. Use IS_ERR() so that a NULL region detaches the
> >   region as the kernel-doc and the v5 changelog intended. (Sashiko-
> > bot)
> > 
> > Assisted-by: GitHub_Copilot:claude-sonnet-4.6
> > Signed-off-by: Thomas Hellström <[email protected]>
> > Reviewed-by: Maarten Lankhorst <[email protected]>
> > #v7
> > ---
> >  drivers/gpu/drm/ttm/ttm_bo.c       | 95
> > +++++++++++++++++++++++++++++-
> >  drivers/gpu/drm/ttm/ttm_bo_util.c  |  3 +-
> >  drivers/gpu/drm/ttm/ttm_resource.c | 52 ++++++++++++++++
> >  include/drm/ttm/ttm_bo.h           | 10 ++++
> >  include/drm/ttm/ttm_resource.h     |  7 +++
> >  5 files changed, 163 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/ttm/ttm_bo.c
> > b/drivers/gpu/drm/ttm/ttm_bo.c
> > index 3980f376e3ba..b2bbbb69add3 100644
> > --- a/drivers/gpu/drm/ttm/ttm_bo.c
> > +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> > @@ -515,12 +515,20 @@ static s64 ttm_bo_evict_cb(struct
> > ttm_lru_walk *walk, struct ttm_buffer_object *
> >  {
> >  	struct ttm_bo_evict_walk *evict_walk =
> >  		container_of(walk, typeof(*evict_walk), walk);
> > +	/* Capture size before eviction in case res is cleared. */
> > +	s64 bo_size = bo->base.size;
> 
> I just noticed this comment, a bo's size should be fixed, even if the
> backing store is gone?

Yeah, correct. Looks like an old leftover. I'll remove.
I'll post a v9 as well, since initialization seems a bit tricky to get
right.

Thanks,
Thomas

> 
> >  	s64 lret;
> >  
> >  	if (!dmem_cgroup_state_evict_valuable(evict_walk-
> > >limit_pool, bo->resource->css,
> >  					      evict_walk->try_low,
> > &evict_walk->hit_low))
> >  		return 0;
> >  
> > +	/*
> > +	 * evict_walk->place is NULL in cgroup drain mode. 
> > Drivers'
> > +	 * eviction_valuable() callbacks must handle a NULL place,
> > treating it
> > +	 * as "any placement": the TTM base implementation already
> > does so via
> > +	 * ttm_resource_intersects().
> > +	 */
> >  	if (bo->pin_count || !bo->bdev->funcs-
> > >eviction_valuable(bo, evict_walk->place))
> >  		return 0;
> >  
> > @@ -536,11 +544,15 @@ static s64 ttm_bo_evict_cb(struct
> > ttm_lru_walk *walk, struct ttm_buffer_object *
> >  		goto out;
> >  
> >  	evict_walk->evicted++;
> > -	if (evict_walk->res)
> > +	if (evict_walk->res) {
> >  		lret = ttm_resource_alloc(evict_walk->evictor,
> > evict_walk->place,
> >  					  evict_walk->res, NULL);
> > -	if (lret == 0)
> > -		return 1;
> > +		if (lret == 0)
> > +			return 1;
> > +	} else {
> > +		/* Cgroup drain: return bytes freed for byte-
> > denominated progress. */
> > +		return bo_size;
> > +	}
> >  out:
> >  	/* Errors that should terminate the walk. */
> >  	if (lret == -ENOSPC)
> > @@ -614,6 +626,83 @@ static int ttm_bo_evict_alloc(struct
> > ttm_device *bdev,
> >  	return 0;
> >  }
> >  
> > +/**
> > + * ttm_bo_evict_cgroup() - Evict buffer objects charged to a
> > specific cgroup.
> > + * @bdev: The TTM device.
> > + * @man: The resource manager whose LRU to walk.
> > + * @limit_pool: The cgroup pool state whose members should be
> > evicted.
> > + * @target_bytes: Number of bytes to free.
> > + * @ctx: The TTM operation context.
> > + *
> > + * Walk the LRU of @man and evict buffer objects that are charged
> > to the
> > + * cgroup identified by @limit_pool, until at least @target_bytes
> > have been
> > + * freed.  Mirrors the two-pass (trylock -> sleeping-lock, low-
> > watermark)
> > + * strategy used by ttm_bo_evict_alloc().
> > + *
> > + * Return: >= @target_bytes on full success, 0..target_bytes-1 if
> > partial,
> > + *         negative error code on fatal error.
> > + */
> > +s64 ttm_bo_evict_cgroup(struct ttm_device *bdev,
> > +			struct ttm_resource_manager *man,
> > +			struct dmem_cgroup_pool_state *limit_pool,
> > +			s64 target_bytes,
> > +			struct ttm_operation_ctx *ctx)
> > +{
> > +	struct ttm_bo_evict_walk evict_walk = {
> > +		.walk = {
> > +			.ops = &ttm_evict_walk_ops,
> > +			.arg = { .ctx = ctx },
> > +		},
> > +		.limit_pool = limit_pool,
> > +		/* place, evictor, res left NULL: selects cgroup
> > drain mode */
> > +	};
> > +	s64 lret, pass;
> > +
> > +	evict_walk.walk.arg.trylock_only = true;
> > +	lret = ttm_lru_walk_for_evict(&evict_walk.walk, bdev, man,
> > target_bytes);
> > +	if (lret < 0 || lret >= target_bytes)
> > +		return lret;
> > +
> > +	/* Second pass: also evict BOs at the low watermark. */
> > +	if (evict_walk.hit_low) {
> > +		evict_walk.try_low = true;
> > +		pass = ttm_lru_walk_for_evict(&evict_walk.walk,
> > bdev, man,
> > +					      target_bytes -
> > lret);
> > +		if (pass < 0)
> > +			return pass;
> > +		lret += pass;
> > +		if (lret >= target_bytes)
> > +			return lret;
> > +	}
> > +
> > +	/* Full sleeping-lock pass for remaining target. */
> > +	evict_walk.try_low = evict_walk.hit_low = false;
> > +	evict_walk.walk.arg.trylock_only = false;
> > +
> > +retry:
> > +	evict_walk.walk.arg.sleeping_lock = true;
> > +	do {
> > +		evict_walk.evicted = 0;
> > +		pass = ttm_lru_walk_for_evict(&evict_walk.walk,
> > bdev, man,
> > +					      target_bytes -
> > lret);
> > +		if (pass < 0) {
> > +			lret = pass;
> > +			goto out;
> > +		}
> > +		lret += pass;
> > +	} while (lret < target_bytes && evict_walk.evicted);
> > +
> > +	/* One more attempt if we hit the low limit during
> > sleeping-lock pass. */
> > +	if (lret < target_bytes && evict_walk.hit_low &&
> > !evict_walk.try_low) {
> > +		evict_walk.try_low = true;
> > +		goto retry;
> > +	}
> > +
> > +out:
> > +	return lret;
> > +}
> > +EXPORT_SYMBOL(ttm_bo_evict_cgroup);
> > +
> >  /**
> >   * ttm_bo_pin - Pin the buffer object.
> >   * @bo: The buffer object to pin
> > diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c
> > b/drivers/gpu/drm/ttm/ttm_bo_util.c
> > index 3e3c201a0222..bd0b23ac2cc4 100644
> > --- a/drivers/gpu/drm/ttm/ttm_bo_util.c
> > +++ b/drivers/gpu/drm/ttm/ttm_bo_util.c
> > @@ -999,7 +999,8 @@ __ttm_bo_lru_cursor_next(struct
> > ttm_bo_lru_cursor *curs)
> >  		bo = res->bo;
> >  		if (ttm_lru_walk_trylock(curs, bo))
> >  			bo_locked = true;
> > -		else if (!arg->ticket || arg->ctx->no_wait_gpu ||
> > arg->trylock_only)
> > +		else if ((!arg->ticket && !arg->sleeping_lock) ||
> > arg->ctx->no_wait_gpu ||
> > +			 arg->trylock_only)
> >  			continue;
> >  
> >  		if (!ttm_bo_get_unless_zero(bo)) {
> > diff --git a/drivers/gpu/drm/ttm/ttm_resource.c
> > b/drivers/gpu/drm/ttm/ttm_resource.c
> > index 154d6739256f..1ff4a470b083 100644
> > --- a/drivers/gpu/drm/ttm/ttm_resource.c
> > +++ b/drivers/gpu/drm/ttm/ttm_resource.c
> > @@ -953,3 +953,55 @@ void
> > ttm_resource_manager_create_debugfs(struct ttm_resource_manager
> > *man,
> >  #endif
> >  }
> >  EXPORT_SYMBOL(ttm_resource_manager_create_debugfs);
> > +
> > +/**
> > + * ttm_resource_manager_dmem_reclaim() - dmem cgroup reclaim
> > callback for TTM
> > + *                                       resource managers.
> > + * @pool: The dmem cgroup pool state for the cgroup being
> > reclaimed.
> > + * @target_bytes: Number of bytes to try to free.
> > + * @priv: The &ttm_resource_manager pointer, passed as
> > @init.reclaim_priv to
> > + *        dmem_cgroup_register_region().
> > + *
> > + * Drivers should use this as the @reclaim member of their own
> > + * &struct dmem_cgroup_ops, with the &ttm_resource_manager pointer
> > as
> > + * @init.reclaim_priv.
> > + *
> > + * Return: 0 if some memory was freed, -ENOSPC if nothing was
> > freed, or
> > + *         another negative error code on fatal failure.
> > + */
> > +int ttm_resource_manager_dmem_reclaim(struct
> > dmem_cgroup_pool_state *pool,
> > +				      u64 target_bytes, void
> > *priv)
> > +{
> > +	struct ttm_resource_manager *man = priv;
> > +	struct ttm_operation_ctx ctx = { .interruptible = true };
> > +	s64 freed;
> > +
> > +	freed = ttm_bo_evict_cgroup(man->bdev, man, pool,
> > target_bytes, &ctx);
> > +	if (freed < 0)
> > +		return freed;
> > +
> > +	return freed > 0 ? 0 : -ENOSPC;
> > +}
> > +EXPORT_SYMBOL(ttm_resource_manager_dmem_reclaim);
> > +
> > +/**
> > + * ttm_resource_manager_set_dmem_region() - Associate a dmem
> > cgroup region with a
> > + *                                        resource manager.
> > + * @man: The resource manager.
> > + * @region: The dmem cgroup region to associate, may be NULL or
> > IS_ERR().
> > + *
> > + * When @region is valid, stores it in @man->cg so that TTM can
> > look up the
> > + * associated pool during charging and eviction-target selection. 
> > When
> > + * @region is %NULL, clears @man->cg to detach the region before
> > teardown.
> > + * An IS_ERR() @region is ignored, leaving @man->cg unchanged.
> > + * The reclaim callback must be wired up using
> > ttm_resource_manager_dmem_reclaim()
> > + * in the driver's own &struct dmem_cgroup_ops, with the manager
> > pointer as
> > + * @init.reclaim_priv.
> > + */
> > +void ttm_resource_manager_set_dmem_region(struct
> > ttm_resource_manager *man,
> > +					  struct
> > dmem_cgroup_region *region)
> > +{
> > +	if (!IS_ERR(region))
> > +		man->cg = region;
> > +}
> > +EXPORT_SYMBOL(ttm_resource_manager_set_dmem_region);
> > diff --git a/include/drm/ttm/ttm_bo.h b/include/drm/ttm/ttm_bo.h
> > index 8310bc3d55f9..32791c4db2a9 100644
> > --- a/include/drm/ttm/ttm_bo.h
> > +++ b/include/drm/ttm/ttm_bo.h
> > @@ -226,6 +226,11 @@ struct ttm_lru_walk_arg {
> >  	struct ww_acquire_ctx *ticket;
> >  	/** @trylock_only: Only use trylock for locking. */
> >  	bool trylock_only;
> > +	/**
> > +	 * @sleeping_lock: Use sleeping locks even with %NULL
> > @ticket.
> > +	 * @trylock_only has precedence over this field.
> > +	 */
> > +	bool sleeping_lock;
> >  };
> >  
> >  /**
> > @@ -431,6 +436,11 @@ void ttm_bo_unpin(struct ttm_buffer_object
> > *bo);
> >  int ttm_bo_evict_first(struct ttm_device *bdev,
> >  		       struct ttm_resource_manager *man,
> >  		       struct ttm_operation_ctx *ctx);
> > +s64 ttm_bo_evict_cgroup(struct ttm_device *bdev,
> > +			struct ttm_resource_manager *man,
> > +			struct dmem_cgroup_pool_state *limit_pool,
> > +			s64 target_bytes,
> > +			struct ttm_operation_ctx *ctx);
> >  int ttm_bo_access(struct ttm_buffer_object *bo, unsigned long
> > offset,
> >  		  void *buf, int len, int write);
> >  vm_fault_t ttm_bo_vm_reserve(struct ttm_buffer_object *bo,
> > diff --git a/include/drm/ttm/ttm_resource.h
> > b/include/drm/ttm/ttm_resource.h
> > index a5d386583fb6..32e485fdce9a 100644
> > --- a/include/drm/ttm/ttm_resource.h
> > +++ b/include/drm/ttm/ttm_resource.h
> > @@ -39,6 +39,7 @@
> >  
> >  struct dentry;
> >  struct dmem_cgroup_device;
> > +struct dmem_cgroup_region;
> >  struct drm_printer;
> >  struct ttm_device;
> >  struct ttm_resource_manager;
> > @@ -477,6 +478,12 @@ void ttm_resource_manager_init(struct
> > ttm_resource_manager *man,
> >  			       struct ttm_device *bdev,
> >  			       uint64_t size);
> >  
> > +void ttm_resource_manager_set_dmem_region(struct
> > ttm_resource_manager *man,
> > +					  struct
> > dmem_cgroup_region *region);
> > +
> > +int ttm_resource_manager_dmem_reclaim(struct
> > dmem_cgroup_pool_state *pool,
> > +				      u64 target_bytes, void
> > *priv);
> > +
> >  int ttm_resource_manager_evict_all(struct ttm_device *bdev,
> >  				   struct ttm_resource_manager
> > *man);
> >