Re: [PATCH v6 3/3] drm/buddy: Add defragmentation support

[Matthew Auld <matthew.auld@intel.com>]

On 08/02/2024 15:50, Arunpravin Paneer Selvam wrote:
> Add a function to support defragmentation.
> 
> v1: Defragment the memory beginning from min_order
>      till the required memory space is available.
> 
> Signed-off-by: Arunpravin Paneer Selvam <Arunpravin.PaneerSelvam@amd.com>
> Suggested-by: Matthew Auld <matthew.auld@intel.com>
> ---
>   drivers/gpu/drm/drm_buddy.c | 67 +++++++++++++++++++++++++++++++------
>   include/drm/drm_buddy.h     |  3 ++

No users?

>   2 files changed, 59 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c
> index 33ad0cfbd54c..fac423d2cb73 100644
> --- a/drivers/gpu/drm/drm_buddy.c
> +++ b/drivers/gpu/drm/drm_buddy.c
> @@ -276,10 +276,12 @@ drm_get_buddy(struct drm_buddy_block *block)
>   }
>   EXPORT_SYMBOL(drm_get_buddy);
>   
> -static void __drm_buddy_free(struct drm_buddy *mm,
> -			     struct drm_buddy_block *block)
> +static unsigned int __drm_buddy_free(struct drm_buddy *mm,
> +				     struct drm_buddy_block *block,
> +				     bool defrag)
>   {
>   	struct drm_buddy_block *parent;
> +	unsigned int order;
>   
>   	while ((parent = block->parent)) {
>   		struct drm_buddy_block *buddy;
> @@ -289,12 +291,14 @@ static void __drm_buddy_free(struct drm_buddy *mm,
>   		if (!drm_buddy_block_is_free(buddy))
>   			break;
>   
> -		if (drm_buddy_block_is_clear(block) !=
> -		    drm_buddy_block_is_clear(buddy))
> -			break;
> +		if (!defrag) {
> +			if (drm_buddy_block_is_clear(block) !=
> +			    drm_buddy_block_is_clear(buddy))
> +				break;
>   
> -		if (drm_buddy_block_is_clear(block))
> -			mark_cleared(parent);
> +			if (drm_buddy_block_is_clear(block))
> +				mark_cleared(parent);
> +		}

Maybe check if the two blocks are incompatible and chuck a warn if they 
are not? Main thing is not to hide issues with split blocks that should 
have been merged before.

>   
>   		list_del(&buddy->link);
>   
> @@ -304,8 +308,49 @@ static void __drm_buddy_free(struct drm_buddy *mm,
>   		block = parent;
>   	}
>   
> +	order = drm_buddy_block_order(block);
>   	mark_free(mm, block);
> +
> +	return order;
> +}
> +
> +/**
> + * drm_buddy_defrag - Defragmentation routine
> + *
> + * @mm: DRM buddy manager
> + * @min_order: minimum order in the freelist to begin
> + * the defragmentation process
> + *
> + * Driver calls the defragmentation function when the
> + * requested memory allocation returns -ENOSPC.
> + */
> +void drm_buddy_defrag(struct drm_buddy *mm,
> +		      unsigned int min_order)

Just wondering if we need "full defag" also? We would probably need to 
call this at fini() anyway.

> +{
> +	struct drm_buddy_block *block;
> +	struct list_head *list;
> +	unsigned int order;
> +	int i;
> +
> +	if (min_order > mm->max_order)
> +		return;
> +
> +	for (i = min_order - 1; i >= 0; i--) {

Need to be careful with min_order = 0 ?

> +		list = &mm->free_list[i];
> +		if (list_empty(list))
> +			continue;
> +
> +		list_for_each_entry_reverse(block, list, link) {

Don't we need the safe_reverse() variant here, since this is removing 
from the list?

> +			if (!block->parent)
> +				continue;
> +
> +			order = __drm_buddy_free(mm, block, 1);
> +			if (order >= min_order)
> +				return;
> +		}
> +	}
>   }
> +EXPORT_SYMBOL(drm_buddy_defrag);
>   
>   /**
>    * drm_buddy_free_block - free a block
> @@ -321,7 +366,7 @@ void drm_buddy_free_block(struct drm_buddy *mm,
>   	if (drm_buddy_block_is_clear(block))
>   		mm->clear_avail += drm_buddy_block_size(mm, block);
>   
> -	__drm_buddy_free(mm, block);
> +	__drm_buddy_free(mm, block, 0);
>   }
>   EXPORT_SYMBOL(drm_buddy_free_block);
>   
> @@ -470,7 +515,7 @@ __alloc_range_bias(struct drm_buddy *mm,
>   	if (buddy &&
>   	    (drm_buddy_block_is_free(block) &&
>   	     drm_buddy_block_is_free(buddy)))
> -		__drm_buddy_free(mm, block);
> +		__drm_buddy_free(mm, block, 0);
>   	return ERR_PTR(err);
>   }
>   
> @@ -588,7 +633,7 @@ alloc_from_freelist(struct drm_buddy *mm,
>   
>   err_undo:
>   	if (tmp != order)
> -		__drm_buddy_free(mm, block);
> +		__drm_buddy_free(mm, block, 0);
>   	return ERR_PTR(err);
>   }
>   
> @@ -668,7 +713,7 @@ static int __alloc_range(struct drm_buddy *mm,
>   	if (buddy &&
>   	    (drm_buddy_block_is_free(block) &&
>   	     drm_buddy_block_is_free(buddy)))
> -		__drm_buddy_free(mm, block);
> +		__drm_buddy_free(mm, block, 0);
>   
>   err_free:
>   	if (err == -ENOSPC && total_allocated_on_err) {
> diff --git a/include/drm/drm_buddy.h b/include/drm/drm_buddy.h
> index d81c596dfa38..d0f63e7b5915 100644
> --- a/include/drm/drm_buddy.h
> +++ b/include/drm/drm_buddy.h
> @@ -166,6 +166,9 @@ void drm_buddy_free_list(struct drm_buddy *mm,
>   			 struct list_head *objects,
>   			 unsigned int flags);
>   
> +void drm_buddy_defrag(struct drm_buddy *mm,
> +		      unsigned int min_order);
> +
>   void drm_buddy_print(struct drm_buddy *mm, struct drm_printer *p);
>   void drm_buddy_block_print(struct drm_buddy *mm,
>   			   struct drm_buddy_block *block,