Re: [PATCH v6 10/16] mm: replace vm_lock and detached flag with a reference count

Peter Zijlstra
2024-12-18T10:06:13Z

On Wed, Dec 18, 2024 at 10:41:04AM +0100, Peter Zijlstra wrote:
> On Tue, Dec 17, 2024 at 08:27:46AM -0800, Suren Baghdasaryan wrote:
>
> > > So I just replied there, and no, I don't think it makes sense. Just put
> > > the kmem_cache_free() in vma_refcount_put(), to be done on 0.
> >
> > That's very appealing indeed and makes things much simpler. The
> > problem I see with that is the case when we detach a vma from the tree
> > to isolate it, then do some cleanup and only then free it. That's done
> > in vms_gather_munmap_vmas() here:
> > https://elixir.bootlin.com/linux/v6.12.5/source/mm/vma.c#L1240 and we
> > even might reattach detached vmas back:
> > https://elixir.bootlin.com/linux/v6.12.5/source/mm/vma.c#L1312. IOW,
> > detached state is not final and we can't destroy the object that
> > reached this state.
>
> Urgh, so that's the munmap() path, but arguably when that fails, the
> map stays in place.
>
> I think this means you're marking detached too soon; you should only
> mark detached once you reach the point of no return.
>
> That said, once you've reached the point of no return; and are about to
> go remove the page-tables, you very much want to ensure a lack of
> concurrency.
>
> So perhaps waiting for out-standing readers at this point isn't crazy.
>
> Also, I'm having a very hard time reading this maple tree stuff :/
> Afaict vms_gather_munmap_vmas() only adds the VMAs to be removed to a
> second tree, it does not in fact unlink them from the mm yet.
>
> AFAICT it's vma_iter_clear_gfp() that actually wipes the vmas from the
> mm -- and that being able to fail is mind boggling and I suppose is what
> gives rise to much of this insanity :/
>
> Anyway, I would expect remove_vma() to be the one that marks it detached
> (it's already unreachable through vma_lookup() at this point) and there
> you should wait for concurrent readers to bugger off.

Also, I think vma_start_write() in that gather look is too early, you're
not actually going to change the VMA yet -- with obvious exception of
the split cases.

That too should probably come after you've passes all the fail/unwind
spots.

Something like so perhaps? (yeah, I know, I wrecked a bunch)

diff --git a/mm/vma.c b/mm/vma.c
index 8e31b7e25aeb..45d43adcbb36 100644
--- a/mm/vma.c
+++ b/mm/vma.c
@@ -1173,6 +1173,11 @@ static void vms_complete_munmap_vmas(struct vma_munmap_struct *vms,
struct vm_area_struct *vma;
struct mm_struct *mm;

+ mas_for_each(mas_detach, vma, ULONG_MAX) {
+ vma_start_write(next);
+ vma_mark_detached(next, true);
+ }
+
mm = current->mm;
mm->map_count -= vms->vma_count;
mm->locked_vm -= vms->locked_vm;
@@ -1219,9 +1224,6 @@ static void reattach_vmas(struct ma_state *mas_detach)
struct vm_area_struct *vma;

mas_set(mas_detach, 0);
- mas_for_each(mas_detach, vma, ULONG_MAX)
- vma_mark_detached(vma, false);
-
__mt_destroy(mas_detach->tree);
}

@@ -1289,13 +1291,11 @@ static int vms_gather_munmap_vmas(struct vma_munmap_struct *vms,
if (error)
goto end_split_failed;
}
- vma_start_write(next);
mas_set(mas_detach, vms->vma_count++);
error = mas_store_gfp(mas_detach, next, GFP_KERNEL);
if (error)
goto munmap_gather_failed;

- vma_mark_detached(next, true);
nrpages = vma_pages(next);

vms->nr_pages += nrpages;
@@ -1431,14 +1431,17 @@ int do_vmi_align_munmap(struct vma_iterator *vmi, struct vm_area_struct *vma,
struct vma_munmap_struct vms;
int error;

+ error = mas_preallocate(vmi->mas);
+ if (error)
+ goto gather_failed;
+
init_vma_munmap(&vms, vmi, vma, start, end, uf, unlock);
error = vms_gather_munmap_vmas(&vms, &mas_detach);
if (error)
goto gather_failed;

error = vma_iter_clear_gfp(vmi, start, end, GFP_KERNEL);
- if (error)
- goto clear_tree_failed;
+ VM_WARN_ON(error);

/* Point of no return */
vms_complete_munmap_vmas(&vms, &mas_detach);