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

[Christophe Leroy <christophe.leroy@csgroup.eu>]

Le 13/03/2024 à 15:48, Edgecombe, Rick P a écrit :
> 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.

Yes. Maybe the best would be to have a preparation patch to churn this 
function a bit so that when it comes to the conservion it is trivial.

Something like:

	if (pde->proc_ops->proc_get_unmapped_area)
		return pde->proc_ops->proc_get_unmapped_area(file, orig_addr, len, 
pgoff, flags);

#ifdef CONFIG_MMU
	return current->mm->get_unmapped_area(file, orig_addr, len, pgoff, flags);
#endif
	return orig_addr;


Note that a length of 100 chars is now tolerated when it eases reading 
so you should avoid those 3 lines.

And the else inside #ifdef CONFIG_MMU is not needed because 'if' has 
returned.

> 
> 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 missed the setting of get_unmapped_area by each arch, you are right it 
might be complicated at the end.

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

Right.