Re: [PATCH v3 01/12] mm: Switch mm->get_unmapped_area() to a flag

[Edgecombe, Rick P]

On Wed, 2024-03-13 at 07:19 +0000, Christophe Leroy wrote:
> This patch is quite big and un-easy to follow. Would be worth
> splitting 
> in several patches if possible. Some of the changes seem to go
> further 
> than just switching mm->get_unmapped_area() to a flag.
> 
> First patch could add the new flag and necessary helpers, then
> following 
> patches could convert sub-systems one by one then last patch would 
> remove mm->get_unmapped_area() once all users are converted.

So you are saying to do the tracking in both the new flag and mm-
>get_unmapped_area() during the conversion process and then remove the
pointer at the end? I guess it could be broken out, but most of the
conversions are trivial one line changes. Hmm, I'm not sure.

[snip]
> 
> >    #ifdef CONFIG_MMU
> > -       if (!get_area)
> > -               get_area = current->mm->get_unmapped_area;
> > +       else
> > +               return mm_get_unmapped_area(current->mm, file,
> > orig_addr,
> > +                                           len, pgoff, flags);
> >    #endif
> > -       if (get_area)
> > -               return get_area(file, orig_addr, len, pgoff,
> > flags);
> > +
> >         return orig_addr;
> >    }
> 
> The change looks unclear at first look. Ok after looking a second
> time 
> it seems to simplify things, but would be better as a separate patch.
> Don't know.

Hmm. I think the only way to do it in smaller chunks is to do both
methods of tracking the direction during the conversion process. And
then the smaller pieces would be really small. So it would probably
help for changes like this, but otherwise would generate a lot of
patches with small changes.

The steps are basically:
1. Introduce flag and helpers
2. convert arch's to use it one by one
3. convert callers to use mm_get_unmapped_area() one by one
4. remove setting get_unmapped_area in each arch
5. remove get_unmapped_area

Step 3 is where the few non-oneline changes would be, but most would
still be one liners. 1, 2, 4 and 5 seem simpler as a tree wide patch
because of the one line changes.

I don't know any other variations are a ton simpler. Hopefully others
will weigh in.



[snip]
> >    
> > +unsigned long
> > +mm_get_unmapped_area(struct mm_struct *mm, struct file *file,
> > +                    unsigned long addr, unsigned long len,
> > +                    unsigned long pgoff, unsigned long flags)
> > +{
> > +       if (test_bit(MMF_TOPDOWN, &mm->flags))
> > +               return arch_get_unmapped_area_topdown(file, addr,
> > len, pgoff, flags);
> > +       return arch_get_unmapped_area(file, addr, len, pgoff,
> > flags);
> > +}
> 
> This function seems quite simple, wouldn't it be better to make it a 
> static inline ?

Then all of the arch_get_unmapped_area() and
arch_get_unmapped_area_topdown() would need to be exported. I think it
is better to only export the higher level functions.