by Vlastimil Babka

[permalink] [raw]

Subject: Re: [PATCH 09/10] mm/mmap: start distinguishing if vma can be removed in mergeability test

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 <[email protected]>

Thanks!

----8<----
From b8072720288491bdc887ebebbb099babcfb108a5 Mon Sep 17 00:00:00 2001
From: Vlastimil Babka <[email protected]>
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 <[email protected]>
---
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

2023-03-20 14:17:02

by Liam R. Howlett

[permalink] [raw]

Subject: Re: [PATCH 00/10] cleanup vma_merge() and improve mergeability tests

Sorry for the late review, I've been away.

For the whole series,
Reviewed-by: Liam R. Howlett <[email protected]>

* Vlastimil Babka <[email protected]> [230309 06:18]:
> Also available in git:
> https://git.kernel.org/vbabka/h/vma_merge_cleanup-v1r2
>
> Changes since RFC:
> - rebased to 6.3-rc1, dropped first patch (urgent fix) that was merged there
> - reindent parameters of mergeability checks (suggested by willy on IRC)
>
> My initial goal here was to try making the check for vm_ops->close in
> is_mergeable_vma() only be applied for vma's that would be truly removed
> as part of the merge (see Patch 9). This would then allow reverting the
> quick fix d014cd7c1c35 ("mm, mremap: fix mremap() expanding for vma's
> with vm_ops->close()"). This was successful enough to allow the revert
> (Patch 10). Checks using can_vma_merge_before() are still pessimistic
> about possible vma removal, and making them precise would probably
> complicate the vma_merge() code too much.
>
> Liam's 6.3-rc1 simplification of vma_merge() and removal of
> __vma_adjust() was very much helpful in understanding the vma_merge()
> implementation and especially when vma removals can happen, which is now
> very obvious. While studing the code, I've found ways to make it
> hopefully even more easy to follow, so that's the patches 1-8. That made
> me also notice a bug that's now already fixed in 6.3-rc1.
>
> Vlastimil Babka (10):
> mm/mmap/vma_merge: use only primary pointers for preparing merge
> mm/mmap/vma_merge: use the proper vma pointer in case 3
> mm/mmap/vma_merge: use the proper vma pointers in cases 1 and 6
> mm/mmap/vma_merge: use the proper vma pointer in case 4
> mm/mmap/vma_merge: initialize mid and next in natural order
> mm/mmap/vma_merge: set mid to NULL if not applicable
> mm/mmap/vma_merge: rename adj_next to adj_start
> mm/mmap/vma_merge: convert mergeability checks to return bool
> mm/mmap: start distinguishing if vma can be removed in mergeability
> test
> mm/mremap: simplify vma expansion again
>
> mm/mmap.c | 142 ++++++++++++++++++++++++++++------------------------
> mm/mremap.c | 20 ++------
> 2 files changed, 80 insertions(+), 82 deletions(-)
>
> --
> 2.39.2
>