[PATCH v6 3/3] drm/buddy: Add defragmentation support
Christian König
ckoenig.leichtzumerken at gmail.com
Fri Feb 16 14:02:46 UTC 2024
Am 16.02.24 um 14:21 schrieb Matthew Auld:
> 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?
Sorry, that was not what I meant. I should probably have been clearer.
That dirty and clean pages are now kept separated is obvious, but why do
you need to de-fragment them at some point?
Christian.
>
>>
>> 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 Intel-gfx
mailing list