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

[Christian König <christian.koenig@xxxxxxx>]

Am 16.02.24 um 15:47 schrieb Matthew Auld:
> On 16/02/2024 14:02, Christian König wrote:
> > 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@xxxxxxx>
> > > > > > Suggested-by: Matthew Auld <matthew.auld@xxxxxxxxx>
> > > > > > ---
> > > > > >   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?
>
> Ah, right. At the very least we need to do something similar to this 
> at fini(), just to ensure we properly merge everything back together 
> so we can correctly tear down the mm. Outside of that the thinking was 
> that it might be useful to call when allocating larger min page-sizes. 
> You might now be failing the allocation due to fragmentation, and so 
> in some cases might be better off running some kind of defrag step 
> first, instead of failing the allocation and trying to evict stuff. 
> Anyway, if that is not a concern for amdgpu, then we just need to 
> handle the fini() case and can keep this internal.

Ah, yes that makes more sense.

So you basically force merge the pages before fini to avoid warnings 
that the buddy isn't empty.

Thanks, that answers my curiosity. But I unfortunately still don't have 
time to dig deep enough into this for a review.

Thanks,
Christian.

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