Re: [PATCH] mm/mremap.c: refactor finding vma and checking vma is alllowed to expand

[John Hubbard <jhubbard@nvidia.com>]

On 1/2/21 4:24 AM, Li Xinhai wrote:
> On 12/31/20 4:52 AM, John Hubbard wrote:
>> On 12/29/20 11:56 PM, Li Xinhai wrote:
>>> Function vma_to_resize)() is called to find the vma to be remapped and
>>> also check if expand size is allowed or not. This function assume that all
>>> call sites should make sure new_len >= old_len, and currently this
>>> assumption is fullfilled at those two call sites, so no real problem at
>>> present.
>>>
>>> After this patch, we explicitly check new_len < old_len case, and separate
>>> a new function for checking if expand size is allowed or not. Also rename
>>> vma_to_resize to vma_to_remap, since the vma to be remapped would not
>>> always require resize.
>>
>> I don't see any clear motivation for this code churn, either above, or
>> implicitly in the patch itself. The new function names are not an improvement.
>>
>> Probably best to just drop this, unless there is some sort of benefit that
>> I'm missing? >
> The main issue is that in vma_to_size() there are code like below
> 
>      if (new_len == old_len)
>          return vma;
> 
>      ...
>      locked += new_len - old_len;
>      ...
> 
> 
>      unsigned long charged = (new_len - old_len) >> PAGE_SHIFT;
>      ...
> 
> the test didn't cover new_len < old_len case, then just do 'new_len - old_len'. That looks like 
> hiding potential bug. So this need be fixed.

This chain of reasoning doesn't work for me. First of all, the callers of vma_to_resize()
already check that new_len >= old_len, right? So I don't think "this needs to be fixed".

Second, if there is a bug that I'm overlooking here, then I'd like to see a fix that
does not also gratuitously refactor this into an unnecessary subroutine. What is the
minimum clean change that you could make to fix the bug?

Here's a bit more detail, in order to guide your future work:

It is true that breaking something that is long and complex into one or more subroutines
can improve some situations. But in this case, vma_to_resize() is already fairly short and
not too complex, and your new subroutine has a somewhat misleading name. That, plus the act
of splitting it up, please the unreadable documentation, actually makes it much harder to
follow.

Also, spend some time trying to write up what you did and why, in the commit log. If the
log is quite difficult to write, then sometimes it means that it wasn't actually a good
move. :)

thanks,
-- 
John Hubbard
NVIDIA