On Wed, Dec 18, 2024 at 7:37 AM Liam R. Howlett <Liam.Howlett@xxxxxxxxxx> wrote: > > * Peter Zijlstra <peterz@xxxxxxxxxxxxx> [241218 05:06]: > > 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. > > Yes, that's correct. I tried to make this clear with a gather/complete > naming like other areas of the mm. I hope that helped. > > Also, the comments for the function state that's what's going on: > > * vms_gather_munmap_vmas() - Put all VMAs within a range into a maple tree > * for removal at a later date. Handles splitting first and last if necessary > * and marking the vmas as isolated. > > ... might be worth updating with new information. > > > > > > > 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 :/ > > This is also correct. The maple tree is a b-tree variant that has > internal nodes. When you write to it, including nulls, they are tracked > and may need to allocate. This is a cost for rcu lookups; we will use > the same or less memory in the end but must maintain a consistent view > of the ranges. > > But to put this into perspective, we get 16 nodes per 4k page, most > writes will use 1 or 3 of these from a kmem_cache, so we are talking > about a very unlikely possibility. Except when syzbot decides to fail > random allocations. > > We could preallocate for the write, but this section of the code is > GFP_KERNEL, so we don't. Preallocation is an option to simplify the > failure path though... which is what you did below. > > > > > > > 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. > > The split needs to start the write on the vma to avoid anyone reading it > while it's being altered. > > > > > That too should probably come after you've passes all the fail/unwind > > spots. > > Do you mean the split? I'd like to move the split later as well.. > tracking that is a pain and may need an extra vma for when one vma is > split twice before removing the middle part. > > Actually, I think we need to allocate two (or at least one) vmas in this > case and just pass one through to unmap (written only to the mas_detach > tree?). It would be nice to find a way to NOT need to do that even.. I > had tried to use a vma on the stack years ago, which didn't work out. > > > > > 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_set(mas_detach, 0); > > > + 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); > Drop the mas_set here. > > > - 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; > > > > The preallocation needs to know the range being stored to know what's > going to happen. > > vma_iter_config(vmi, start, end); > > > + error = mas_preallocate(vmi->mas); > > We haven't had a need to have a vma iterator preallocate for storing a > null, but we can add one for this. > > > + 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; > > > > Drop this stuff. > > error = vma_iter_clear_gfp(vmi, start, end, GFP_KERNEL); > > - if (error) > > - goto clear_tree_failed; > > + VM_WARN_ON(error); > > Do this instead > vma_iter_config(vmi, start, end); > vma_iter_clear(vmi); Thanks for the input, Liam. Let me try to make a patch from these suggestions and see where we end up and what might blow up. > > > > > /* Point of no return */ > > vms_complete_munmap_vmas(&vms, &mas_detach);
