Message-ID: <74f2ea6c-f1a9-6dd7-260c-25e660f42379@suse.cz>
Date:   Thu, 16 Mar 2023 11:57:01 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.8.0
Subject: Re: [PATCH 09/10] mm/mmap: start distinguishing if vma can be removed
 in mergeability test
Content-Language: en-US
To:     Lorenzo Stoakes <lstoakes@gmail.com>,
        Andrew Morton <akpm@linux-foundation.org>
Cc:     "Liam R. Howlett" <Liam.Howlett@oracle.com>,
        Matthew Wilcox <willy@infradead.org>, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org, patches@lists.linux.dev,
        maple-tree@lists.infradead.org
References: <20230309111258.24079-1-vbabka@suse.cz>
 <20230309111258.24079-10-vbabka@suse.cz>
 <7e0e33a7-4e9b-4714-8d4b-9487c8d1f799@lucifer.local>
From:   Vlastimil Babka <vbabka@suse.cz>
In-Reply-To: <7e0e33a7-4e9b-4714-8d4b-9487c8d1f799@lucifer.local>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 3/15/23 23:05, Lorenzo Stoakes wrote:
> On Thu, Mar 09, 2023 at 12:12:57PM +0100, Vlastimil Babka wrote:
>> --- a/mm/mmap.c
>> +++ b/mm/mmap.c
>> @@ -742,12 +742,13 @@ int vma_shrink(struct vma_iterator *vmi, struct vm_area_struct *vma,
>>
>>  /*
>>   * If the vma has a ->close operation then the driver probably needs to release
>> - * per-vma resources, so we don't attempt to merge those.
>> + * per-vma resources, so we don't attempt to merge those in case the caller
>> + * indicates the current vma may be removed as part of the merge.
> 
> Nit: 'in case the caller indicates' -> 'if the caller indicates'

Ok, -fix patch below.
 
...

>> -	if (is_mergeable_vma(vma, file, vm_flags, vm_userfaultfd_ctx, anon_name) &&
>> +	if (is_mergeable_vma(vma, file, vm_flags, vm_userfaultfd_ctx, anon_name, false) &&
>>  	    is_mergeable_anon_vma(anon_vma, vma->anon_vma, vma)) {
>>  		pgoff_t vm_pglen;
>>  		vm_pglen = vma_pages(vma);
>> --
>> 2.39.2
>>
> 
> I wonder whether this might be moved later into the actual vma_merge() logic so
> we only abort a merge at the point a VMA with ->close() would otherwise be
> removed? But obviously that would probably need some further clean up to make it
> not add yet more complexity to an already very complicated bit of logic.

Yeah, can try that later to cover the remaining cases where
->close prevents merging unnecessarily.

> Otherwise, very nice, clear + conservative so,
> 
> Reviewed-By: Lorenzo Stoakes <lstoakes@gmail.com>

Thanks!

----8<----
From b8072720288491bdc887ebebbb099babcfb108a5 Mon Sep 17 00:00:00 2001
From: Vlastimil Babka <vbabka@suse.cz>
Date: Thu, 16 Mar 2023 11:54:27 +0100
Subject: [PATCH] mm/mmap: start distinguishing if vma can be removed in
 mergeability test-fix

Adjust comment as suggested by Lorenzo.

Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
---
 mm/mmap.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/mm/mmap.c b/mm/mmap.c
index 4259095a2982..b255e6352710 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -742,8 +742,8 @@ int vma_shrink(struct vma_iterator *vmi, struct vm_area_struct *vma,
 
 /*
  * If the vma has a ->close operation then the driver probably needs to release
- * per-vma resources, so we don't attempt to merge those in case the caller
- * indicates the current vma may be removed as part of the merge.
+ * per-vma resources, so we don't attempt to merge those if the caller indicates
+ * the current vma may be removed as part of the merge.
  */
 static inline bool is_mergeable_vma(struct vm_area_struct *vma,
 		struct file *file, unsigned long vm_flags,
-- 
2.39.2