[PATCH v6 3/3] drm/buddy: Add defragmentation support
Matthew Auld
matthew.auld at intel.com
Fri Feb 16 13:21:21 UTC 2024
On 16/02/2024 12:33, Christian König wrote:
> 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 at amd.com>
>>> Suggested-by: Matthew Auld <matthew.auld at 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?
The fragmentation is due to pages now being tracked as dirty/clear.
Should the allocator merge together a page that is dirty with a page
that is cleared? When should it do that? User wants to be able to keep
the two separate if possible. For example, freeing one single dirty page
can dirty a huge swathe of your already cleared pages if they are merged
together. Or do you have some some other ideas here?
>
> 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,
>
More information about the amd-gfx
mailing list