On Wed, Nov 13, 2024 at 11:05 AM Suren Baghdasaryan <surenb@xxxxxxxxxx> wrote: > > On Wed, Nov 13, 2024 at 7:47 AM Suren Baghdasaryan <surenb@xxxxxxxxxx> wrote: > > > > On Wed, Nov 13, 2024 at 7:29 AM Liam R. Howlett <Liam.Howlett@xxxxxxxxxx> wrote: > > > > > > * Suren Baghdasaryan <surenb@xxxxxxxxxx> [241113 10:25]: > > > > On Wed, Nov 13, 2024 at 7:23 AM 'Liam R. Howlett' via kernel-team > > > > <kernel-team@xxxxxxxxxxx> wrote: > > > > > > > > > > * Matthew Wilcox <willy@xxxxxxxxxxxxx> [241113 08:57]: > > > > > > On Wed, Nov 13, 2024 at 07:38:02AM -0500, Liam R. Howlett wrote: > > > > > > > > Hi, I was wondering if we actually need the detached flag. Couldn't > > > > > > > > "detached" simply mean vma->vm_mm == NULL and we save 4 bytes? Do we ever > > > > > > > > need a vma that's detached but still has a mm pointer? I'd hope the places > > > > > > > > that set detached to false have the mm pointer around so it's not inconvenient. > > > > > > > > > > > > > > I think the gate vmas ruin this plan. > > > > > > > > > > > > But the gate VMAs aren't to be found in the VMA tree. Used to be that > > > > > > was because the VMA tree was the injective RB tree and so VMAs could > > > > > > only be in one tree at a time. We could change that now! > > > > > > > > > > \o/ > > > > > > > > > > > > > > > > > Anyway, we could use (void *)1 instead of NULL to indicate a "detached" > > > > > > VMA if we need to distinguish between a detached VMA and a gate VMA. > > > > > > > > > > I was thinking a pointer to itself vma->vm_mm = vma, then a check for > > > > > this, instead of null like we do today. > > > > > > > > The motivation for having a separate detached flag was that vma->vm_mm > > > > is used when read/write locking the vma, so it has to stay valid even > > > > when vma gets detached. Maybe we can be more cautious in > > > > vma_start_read()/vma_start_write() about it but I don't recall if > > > > those were the only places that was an issue. > > > > > > We have the mm form the callers though, so it could be passed in? > > > > Let me try and see if something else blows up. When I was implementing > > per-vma locks I thought about using vma->vm_mm to indicate detached > > state but there were some issues that caused me reconsider. > > Yeah, a quick change reveals the first mine explosion: > > [ 2.838900] BUG: kernel NULL pointer dereference, address: 0000000000000480 > [ 2.840671] #PF: supervisor read access in kernel mode > [ 2.841958] #PF: error_code(0x0000) - not-present page > [ 2.843248] PGD 800000010835a067 P4D 800000010835a067 PUD 10835b067 PMD 0 > [ 2.844920] Oops: Oops: 0000 [#1] PREEMPT SMP PTI > [ 2.846078] CPU: 2 UID: 0 PID: 1 Comm: init Not tainted > 6.12.0-rc6-00258-ga587fcd91b06-dirty #111 > [ 2.848277] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), > BIOS 1.16.3-debian-1.16.3-2 04/01/2014 > [ 2.850673] RIP: 0010:unmap_vmas+0x84/0x190 > [ 2.851717] Code: 00 00 00 00 48 c7 44 24 48 00 00 00 00 48 c7 44 > 24 18 00 00 00 00 48 89 44 24 28 4c 89 44 24 38 e8 b1 c0 d1 00 48 8b > 44 24 28 <48> 83 b8 80 04 00 00 00 0f 85 dd 00 00 00 45 0f b6 ed 49 83 > ec 01 > [ 2.856287] RSP: 0000:ffffa298c0017a18 EFLAGS: 00010246 > [ 2.857599] RAX: 0000000000000000 RBX: 00007f48ccbb4000 RCX: 00007f48ccbb4000 > [ 2.859382] RDX: ffff8918c26401e0 RSI: ffffa298c0017b98 RDI: ffffa298c0017ab0 > [ 2.861156] RBP: 00007f48ccdb6000 R08: 00007f48ccdb6000 R09: 0000000000000001 > [ 2.862941] R10: 0000000000000040 R11: ffff8918c2637108 R12: 0000000000000001 > [ 2.864719] R13: 0000000000000001 R14: ffff8918c26401e0 R15: ffffa298c0017b98 > [ 2.866472] FS: 0000000000000000(0000) GS:ffff8927bf080000(0000) > knlGS:0000000000000000 > [ 2.868439] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > [ 2.869877] CR2: 0000000000000480 CR3: 000000010263e000 CR4: 0000000000750ef0 > [ 2.871661] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 > [ 2.873419] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 > [ 2.875185] PKRU: 55555554 > [ 2.875871] Call Trace: > [ 2.876503] <TASK> > [ 2.877047] ? __die+0x1e/0x60 > [ 2.877824] ? page_fault_oops+0x17b/0x4a0 > [ 2.878857] ? exc_page_fault+0x6b/0x150 > [ 2.879841] ? asm_exc_page_fault+0x26/0x30 > [ 2.880886] ? unmap_vmas+0x84/0x190 > [ 2.881783] ? unmap_vmas+0x7f/0x190 > [ 2.882680] vms_clear_ptes+0x106/0x160 > [ 2.883621] vms_complete_munmap_vmas+0x53/0x170 > [ 2.884762] do_vmi_align_munmap+0x15e/0x1d0 > [ 2.885838] do_vmi_munmap+0xcb/0x160 > [ 2.886760] __vm_munmap+0xa4/0x150 > [ 2.887637] elf_load+0x1c4/0x250 > [ 2.888473] load_elf_binary+0xabb/0x1680 > [ 2.889476] ? __kernel_read+0x111/0x320 > [ 2.890458] ? load_misc_binary+0x1bc/0x2c0 > [ 2.891510] bprm_execve+0x23e/0x5e0 > [ 2.892408] kernel_execve+0xf3/0x140 > [ 2.893331] ? __pfx_kernel_init+0x10/0x10 > [ 2.894356] kernel_init+0xe5/0x1c0 > [ 2.895241] ret_from_fork+0x2c/0x50 > [ 2.896141] ? __pfx_kernel_init+0x10/0x10 > [ 2.897164] ret_from_fork_asm+0x1a/0x30 > [ 2.898148] </TASK> > > Looks like we are detaching VMAs and then unmapping them, where > vms_clear_ptes() uses vms->vma->vm_mm. I'll try to clean up this and > other paths and will see how many changes are required to make this > work. Ok, my vma->detached deprecation effort got to the point that my QEMU boots. The change is not pretty and I'm quite sure I did not cover all cases yet (like hugepages): arch/arm/kernel/process.c | 2 +- arch/arm64/kernel/vdso.c | 4 +- arch/loongarch/kernel/vdso.c | 2 +- arch/powerpc/kernel/vdso.c | 2 +- arch/powerpc/platforms/pseries/vas.c | 2 +- arch/riscv/kernel/vdso.c | 4 +- arch/s390/kernel/vdso.c | 2 +- arch/s390/mm/gmap.c | 2 +- arch/x86/entry/vdso/vma.c | 2 +- arch/x86/kernel/cpu/sgx/encl.c | 2 +- arch/x86/um/mem_32.c | 2 +- drivers/android/binder_alloc.c | 2 +- drivers/gpu/drm/i915/i915_mm.c | 4 +- drivers/infiniband/core/uverbs_main.c | 4 +- drivers/misc/sgi-gru/grumain.c | 2 +- fs/exec.c | 2 +- fs/hugetlbfs/inode.c | 3 +- include/linux/mm.h | 111 +++++++++++++++++--------- include/linux/mm_types.h | 6 -- kernel/bpf/arena.c | 2 +- kernel/fork.c | 6 +- mm/debug_vm_pgtable.c | 2 +- mm/internal.h | 2 +- mm/madvise.c | 4 +- mm/memory.c | 39 ++++----- mm/mmap.c | 9 +-- mm/nommu.c | 6 +- mm/oom_kill.c | 2 +- mm/vma.c | 62 +++++++------- mm/vma.h | 2 +- net/ipv4/tcp.c | 4 +- 31 files changed, 164 insertions(+), 136 deletions(-) Many of the unmap_* and zap_* functions should get an `mm` parameter to make this work. So, if we take this route, it should definitely be a separate patch, which will likely cause some instability issues for some time until all the edge cases are ironed out. I would like to proceed with this patch series first before attempting to deprecate vma->detached. Let me know if you have objections to this plan. > > > > > > > > > > > > > > > > > > > > Either way, we should make it a function so it's easier to reuse for > > > > > whatever we need in the future, wdyt? > > > > > > > > > > To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@xxxxxxxxxxx. > > > > >
