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

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

Am 16.02.24 um 13:23 schrieb Matthew Auld:
> 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?

Other question is how can a buddy allocator fragment in the first place?

Christian.

>
>>   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,