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.
