Re: [PATCH 03/10] drm/ttm: remove ttm_lru_walk_ops

["Christian König" <christian.koenig@amd.com>]

On 7/6/26 14:34, Thomas Hellström wrote:
> Hi,
> 
> On Fri, 2026-07-03 at 18:31 +0200, Natalie Vock wrote:
>> From: Christian König <christian.koenig@amd.com>
>>
>> It's just another layer of indirection.
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> Signed-off-by: Natalie Vock <natalie.vock@gmx.de>
> 
> Personally I don't have a strong opinion on this, but the reason for
> separating out the ops is that adding function pointers in the walk
> iterator itself was once pushed back on quite forcefully by Linus when
> I tried to do that in mm/pagewalk. Claiming for various reasons the
> standard way of doing that in Linux is using a const ops struct that
> ends up in unmodifiable memory.

Ah! I was already wondering why the extra indirection was used.

I'm perfectly fine to keep it. It just looked a bit odd.

Regards,
Christian.

> 
> /Thomas
> 
> 
>> ---
>>  drivers/gpu/drm/ttm/ttm_bo.c      | 12 ++----------
>>  drivers/gpu/drm/ttm/ttm_bo_util.c |  2 +-
>>  include/drm/ttm/ttm_bo.h          | 34 ++++++++++++++---------------
>> -----
>>  3 files changed, 17 insertions(+), 31 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c
>> b/drivers/gpu/drm/ttm/ttm_bo.c
>> index 2b470c1746f60..1fb8c53da0362 100644
>> --- a/drivers/gpu/drm/ttm/ttm_bo.c
>> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
>> @@ -545,10 +545,6 @@ static s64 ttm_bo_evict_cb(struct ttm_lru_walk
>> *walk, struct ttm_buffer_object *
>>  	return lret;
>>  }
>>  
>> -static const struct ttm_lru_walk_ops ttm_evict_walk_ops = {
>> -	.process_bo = ttm_bo_evict_cb,
>> -};
>> -
>>  static int ttm_bo_evict_alloc(struct ttm_device *bdev,
>>  			      struct ttm_resource_manager *man,
>>  			      const struct ttm_place *place,
>> @@ -560,7 +556,7 @@ static int ttm_bo_evict_alloc(struct ttm_device
>> *bdev,
>>  {
>>  	struct ttm_bo_evict_walk evict_walk = {
>>  		.walk = {
>> -			.ops = &ttm_evict_walk_ops,
>> +			.process_bo = ttm_bo_evict_cb,
>>  			.arg = {
>>  				.ctx = ctx,
>>  				.ticket = ticket,
>> @@ -1187,10 +1183,6 @@ ttm_bo_swapout_cb(struct ttm_lru_walk *walk,
>> struct ttm_buffer_object *bo)
>>  	return ret;
>>  }
>>  
>> -const struct ttm_lru_walk_ops ttm_swap_ops = {
>> -	.process_bo = ttm_bo_swapout_cb,
>> -};
>> -
>>  /**
>>   * ttm_bo_swapout() - Swap out buffer objects on the LRU list to
>> shmem.
>>   * @bdev: The ttm device.
>> @@ -1209,7 +1201,7 @@ s64 ttm_bo_swapout(struct ttm_device *bdev,
>> struct ttm_operation_ctx *ctx,
>>  {
>>  	struct ttm_bo_swapout_walk swapout_walk = {
>>  		.walk = {
>> -			.ops = &ttm_swap_ops,
>> +			.process_bo = ttm_bo_swapout_cb,
>>  			.arg = {
>>  				.ctx = ctx,
>>  				.trylock_only = true,
>> diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c
>> b/drivers/gpu/drm/ttm/ttm_bo_util.c
>> index 7ed085adf1c9b..29f068944a972 100644
>> --- a/drivers/gpu/drm/ttm/ttm_bo_util.c
>> +++ b/drivers/gpu/drm/ttm/ttm_bo_util.c
>> @@ -919,7 +919,7 @@ s64 ttm_lru_walk_for_evict(struct ttm_lru_walk
>> *walk, struct ttm_device *bdev,
>>  	s64 lret;
>>  
>>  	ttm_bo_lru_for_each_reserved_guarded(&cursor, man, &walk-
>>> arg, bo) {
>> -		lret = walk->ops->process_bo(walk, bo);
>> +		lret = walk->process_bo(walk, bo);
>>  		if (lret == -EBUSY || lret == -EALREADY)
>>  			lret = 0;
>>  		progress = (lret < 0) ? lret : progress + lret;
>> diff --git a/include/drm/ttm/ttm_bo.h b/include/drm/ttm/ttm_bo.h
>> index 1eae9eea5ff32..0fcd5082a7080 100644
>> --- a/include/drm/ttm/ttm_bo.h
>> +++ b/include/drm/ttm/ttm_bo.h
>> @@ -189,24 +189,6 @@ struct ttm_operation_ctx {
>>  	uint64_t bytes_moved;
>>  };
>>  
>> -struct ttm_lru_walk;
>> -
>> -/** struct ttm_lru_walk_ops - Operations for a LRU walk. */
>> -struct ttm_lru_walk_ops {
>> -	/**
>> -	 * process_bo - Process this bo.
>> -	 * @walk: struct ttm_lru_walk describing the walk.
>> -	 * @bo: A locked and referenced buffer object.
>> -	 *
>> -	 * Return: Negative error code on error, User-defined
>> positive value
>> -	 * (typically, but not always, size of the processed bo) on
>> success.
>> -	 * On success, the returned values are summed by the walk
>> and the
>> -	 * walk exits when its target is met.
>> -	 * 0 also indicates success, -EBUSY means this bo was
>> skipped.
>> -	 */
>> -	s64 (*process_bo)(struct ttm_lru_walk *walk, struct
>> ttm_buffer_object *bo);
>> -};
>> -
>>  /**
>>   * struct ttm_lru_walk_arg - Common part for the variants of BO LRU
>> walk.
>>   */
>> @@ -223,8 +205,20 @@ struct ttm_lru_walk_arg {
>>   * struct ttm_lru_walk - Structure describing a LRU walk.
>>   */
>>  struct ttm_lru_walk {
>> -	/** @ops: Pointer to the ops structure. */
>> -	const struct ttm_lru_walk_ops *ops;
>> +	/**
>> +	 * process_bo - Process this bo.
>> +	 * @walk: struct ttm_lru_walk describing the walk.
>> +	 * @bo: A locked and referenced buffer object.
>> +	 *
>> +	 * Return: Negative error code on error, User-defined
>> positive value
>> +	 * (typically, but not always, size of the processed bo) on
>> success.
>> +	 * On success, the returned values are summed by the walk
>> and the
>> +	 * walk exits when its target is met.
>> +	 * 0 also indicates success, -EBUSY means this bo was
>> skipped.
>> +	 */
>> +	s64 (*process_bo)(struct ttm_lru_walk *walk,
>> +			  struct ttm_buffer_object *bo);
>> +
>>  	/** @arg: Common bo LRU walk arguments. */
>>  	struct ttm_lru_walk_arg arg;
>>  };