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

[Christian König <ckoenig.leichtzumerken@xxxxxxxxx>]

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?

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,