Received-SPF: pass (google.com: domain of linux-kernel+bounces-72237-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <f88de15e-508f-f651-0164-346845d23e85@linux.dev>
Date: Tue, 20 Feb 2024 11:00:30 +0800
Precedence: bulk
MIME-Version: 1.0
Subject: Re: [PATCH] mm/mmap: Add case 9 in vma_merge()
Content-Language: en-US
To: Lorenzo Stoakes <lstoakes@gmail.com>
Cc: akpm@linux-foundation.org, Liam.Howlett@oracle.com, linux-mm@kvack.org,
 linux-kernel@vger.kernel.org, vbabka@suse.cz
References: <20240218085028.3294332-1-yajun.deng@linux.dev>
 <a5cd692e-34e3-4bc1-a8fa-f6bb56f04e8a@lucifer.local>
From: Yajun Deng <yajun.deng@linux.dev>
In-Reply-To: <a5cd692e-34e3-4bc1-a8fa-f6bb56f04e8a@lucifer.local>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit


On 2024/2/19 07:03, Lorenzo Stoakes wrote:
> On Sun, Feb 18, 2024 at 04:50:28PM +0800, Yajun Deng wrote:
>> If the prev vma exists and the end is less than the end of prev, we
>> can return NULL immediately. This reduces unnecessary operations.
>>
>> Signed-off-by: Yajun Deng <yajun.deng@linux.dev>
> Adding Vlastimil, while get_maintainers.pl might not show it very clearly,
> myself, Vlastimil and Liam often work with vma_merge() so it's handy to cc
> us on these if you can!
Okay.
>> ---
>>   mm/mmap.c | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/mm/mmap.c b/mm/mmap.c
>> index 8f176027583c..b738849321c0 100644
>> --- a/mm/mmap.c
>> +++ b/mm/mmap.c
>> @@ -827,7 +827,7 @@ can_vma_merge_after(struct vm_area_struct *vma, unsigned long vm_flags,
>>    *
>>    *     ****             ****                   ****
>>    *    PPPPPPNNNNNN    PPPPPPNNNNNN       PPPPPPCCCCCC
>> - *    cannot merge    might become       might become
>> + *    cannot merge 9  might become       might become
> While I welcome your interest here :) I am not a fan of the 'case' approach
> to this function as-is and plan to heavily refactor this when I get a chance.
>
> But at any rate, an early-exit situation is not a merge case, merge cases
> describe cases where we _can_ merge, so we can drop this case 9 stuff (this
> is not your fault, it's understandable why you would label this, this
> function is just generally unclear).

Yes, it's not a merge case. I label this to make it easier to understand.

>>    *                    PPNNNNNNNNNN       PPPPPPPPPPCC
>>    *    mmap, brk or    case 4 below       case 5 below
>>    *    mremap move:
>> @@ -890,6 +890,9 @@ static struct vm_area_struct
>>   	if (vm_flags & VM_SPECIAL)
>>   		return NULL;
>>
>> +	if (prev && end < prev->vm_end) /* case 9 */
>> +		return NULL;
>> +
> I need to get back into vma_merge() head space, but I don't actually think
> a caller that's behaving correctly should ever do this. I know the ASCII
> diagram above lists it as a thing that can happen, but I think we
> implicitly avoid this from the way we invoke callers. Either prev == vma as
> per vma_merge_extend(), or the loops that invoke vma_merge_new_vma()
> wouldn't permit this to occur.
No, it will actually happen. That's why I submitted this patch.
> Let me look into it more deeply + reply again a bit later, I mean we could
> perhaps do with asserting this somehow, but I don't think it's useful to do
> an early exit for something that ostensibly _shouldn't_ happen.
>
>>   	/* Does the input range span an existing VMA? (cases 5 - 8) */
>>   	curr = find_vma_intersection(mm, prev ? prev->vm_end : 0, end);
>>
>> --
>> 2.25.1
>>