On Thu, Jan 09, 2025 at 10:49:32PM GMT, Mark Kettenis wrote:
> > Date: Thu, 9 Jan 2025 17:29:38 +0100
> > From: Marcus Glocker <[email protected]>
> >
> > On Thu, Jan 09, 2025 at 05:05:14PM GMT, Mark Kettenis wrote:
> >
> > > > Date: Thu, 9 Jan 2025 15:48:29 +0100
> > > > From: Marcus Glocker <[email protected]>
> > > >
> > > > Hello bugs@, Martin,
> > > >
> > > > Since a while I am noticing processes hanging on my Samsung Galaxy Book4
> > > > Edge (arm64/snapdragon-x/12-cores/16gb ram) machine. Those hangs appear
> > > > very frequent, which makes it hard to work on the machine since things
> > > > like xterm, ssh, man, etc. just suddenly start to hang. If this
> > > > happens,
> > > > executing another process would immediatly release the hanging/waiting
> > > > process.
> > > >
> > > > I've discussed this behavior today on icb, which has lead to the
> > > > following conversation:
> > > >
> > > > 11:39 < mglocker> 5344 hacki -18 0 1436K 392K idle flt_pmf
> > > > 0:00 0.00% man
> > > > 11:41 < mglocker> uvm_wait("flt_pmfail1");
> > > > 11:42 < mglocker> uvm_wait("flt_pmfail2");
> > > > 11:43 < mglocker> 49811 hacki -18 0 8144K 112K sleep/0 flt_pmf
> > > > 0:00 0.00% xterm
> > > > 11:54 < mglocker> ok, the process hang is always at
> > > > uvm/uvm_fault.c:1879 -> uvm_wait("flt_pmfail2")
> > > >
> > > > 12:17 < kettenis> so that's pmap_enter() failing
> > > > 12:19 < kettenis> which means a pool allocation failure
> > > > 12:20 < kettenis> what does vmstat -m say about the "pted" and "vp"
> > > > pools?
> > > > 12:28 < mglocker> Name Size Requests Fail InUse Pgreq Pgrel
> > > > Npage Hiwat Minpg Maxpg Idle
> > > > 12:29 < mglocker> pted 40 962117 0 42480 1582 0
> > > > 1582 1582 1 8 0
> > > > 12:29 < mglocker> vp 8192 47009 102 5676 7830 1100
> > > > 6730 7830 20 8 20
> > > > 12:30 < mglocker> vp 102 fails?
> > > > 12:37 < mglocker> it keeps increasing on those hangs
> > > > 12:46 < mglocker> so pmap_enter_vp() fails for
> > > > 12:46 < mglocker> vp2 = pool_get()
> > > > 12:46 < mglocker> and
> > > > 12:47 < mglocker> vp3 = pool_get()
> > > > 13:00 < mglocker> i booted again with a fresh single processor kernel.
> > > > there no vp fails.
> > > > 13:09 < claudio> didn't we switch the vp pool to use per-cpu caches
> > > > exactly because of this?
> > > > 14:02 < kettenis> I believe so
> > > > 14:03 < kettenis> the problem is that pmap_enter(9) isn't supposed to
> > > > sleep
> > > > 14:03 < kettenis> so the pool allocations are done with PR_NOWAIT
> > > > 14:04 < kettenis> but that means that kd_trylock gets set
> > > > 14:04 < kettenis> which means that the allocations fail if there is
> > > > contention on the pool lock
> > > > 14:04 < claudio> yes, I remeber this strange behaviour.
> > > > 14:06 < kettenis> uvm things this means we're out of physmem
> > > > 14:06 < kettenis> so it'll sleep until something else pokes the
> > > > pagedaemon
> > > > 14:06 < kettenis> the per-cpu mitigated the issue somewhat
> > > > 14:07 < kettenis> but didn't solve things completely
> > > > 14:07 < kettenis> and now that mpi pushed back the locks in uvm again,
> > > > the problem is back
> > > > 14:09 < kettenis> so we need a real solution for this problem...
> > > > 14:12 < kettenis> a potential solution would be to make pmap_enter(9)
> > > > return a different error for this case
> > > > 14:13 < kettenis> and then handle that case different in
> > > > uvm_fault_{upper|lower}
> > > > 14:15 < kettenis> the problem there is that pool_get() doesn't actually
> > > > tell us why it failed
> > > > 14:37 < kettenis> s/contention on the pool lock/contention on the
> > > > kernal map/
> > > >
> > > > Any proposal on how we could proceed to find a solution for this issue?
> > >
> > > The following hack fixes the issue for me. I don't think this is a
> > > proper solution, but it may be a starting point. Or a temporary fix.
> > >
> > > The issue really is that we can't tell whether pmap_enter(9) failed
> > > because we're out of physical memory, or if it failed for some other
> > > reason. In the case at hand we failt because of contention on the
> > > kernel map lock. But we could also be failing because we have completely
> > > run out of KVA.
> >
> > Works for me as well!
> >
> > > We can't sleep while holding all those uvm locks. I'm not sure the
> > > free memory check this does is right. Or whether we want such a check
> > > at all. The vm_map_lock()/vm_map_unlock() dance is necessary to make
> > > sure we don't spin too quickly if the kernel map lock is contended.
> > >
> > > A better fix would perhaps be to have a new pmap function that we
> > > could call at this spot that would sleep until the necessary resources
> > > are available. On arm64 this would populate the page tables using
> > > pool allocations that use PR_WAITOK, but would not actually enter a
> > > valid mapping. I'm going to explore that idea a bit.
>
> This seems to work. Even a little bit better I think as the number of
> vp pool fails seems to be a bit smaller with this diff.
>
> Thoughts?
The patch still works fine on my Samsung, and I can also confirm that
the vp failures I'm seeing are bit lower than with the first patch.
Unfortunately I don't have the required knowledge in that area to come
up with feedback on the semantics. I hope that people with a better
understanding about this topic will jump in.
> Index: arch/arm64/arm64/pmap.c
> ===================================================================
> RCS file: /cvs/src/sys/arch/arm64/arm64/pmap.c,v
> diff -u -p -r1.105 pmap.c
> --- arch/arm64/arm64/pmap.c 9 Nov 2024 12:58:29 -0000 1.105
> +++ arch/arm64/arm64/pmap.c 9 Jan 2025 21:44:41 -0000
> @@ -443,6 +443,67 @@ pmap_vp_enter(pmap_t pm, vaddr_t va, str
> return 0;
> }
>
> +void
> +pmap_vp_populate(pmap_t pm, vaddr_t va)
> +{
> + struct pte_desc *pted;
> + struct pmapvp1 *vp1;
> + struct pmapvp2 *vp2;
> + struct pmapvp3 *vp3;
> + void *vp;
> +
> + pted = pool_get(&pmap_pted_pool, PR_WAITOK | PR_ZERO);
> + vp = pool_get(&pmap_vp_pool, PR_WAITOK | PR_ZERO);
> +
> + pmap_lock(pm);
> +
> + if (pm->have_4_level_pt) {
> + vp1 = pm->pm_vp.l0->vp[VP_IDX0(va)];
> + if (vp1 == NULL) {
> + vp1 = vp; vp = NULL;
> + pmap_set_l1(pm, va, vp1);
> + }
> + } else {
> + vp1 = pm->pm_vp.l1;
> + }
> +
> + if (vp == NULL) {
> + pmap_unlock(pm);
> + vp = pool_get(&pmap_vp_pool, PR_WAITOK | PR_ZERO);
> + pmap_lock(pm);
> + }
> +
> + vp2 = vp1->vp[VP_IDX1(va)];
> + if (vp2 == NULL) {
> + vp2 = vp; vp = NULL;
> + pmap_set_l2(pm, va, vp1, vp2);
> + }
> +
> + if (vp == NULL) {
> + pmap_unlock(pm);
> + vp = pool_get(&pmap_vp_pool, PR_WAITOK | PR_ZERO);
> + pmap_lock(pm);
> + }
> +
> + vp3 = vp2->vp[VP_IDX2(va)];
> + if (vp3 == NULL) {
> + vp3 = vp; vp = NULL;
> + pmap_set_l3(pm, va, vp2, vp3);
> + }
> +
> + if (vp3->vp[VP_IDX3(va)] == NULL) {
> + vp3->vp[VP_IDX3(va)] = pted;
> + pted = NULL;
> + }
> +
> + pmap_unlock(pm);
> +
> + if (vp)
> + pool_put(&pmap_vp_pool, vp);
> + if (pted)
> + pool_put(&pmap_pted_pool, pted);
> +}
> +
> void *
> pmap_vp_page_alloc(struct pool *pp, int flags, int *slowdown)
> {
> @@ -616,6 +677,11 @@ out:
> return error;
> }
>
> +void
> +pmap_populate(pmap_t pm, vaddr_t va)
> +{
> + pmap_vp_populate(pm, va);
> +}
>
> /*
> * Remove the given range of mapping entries.
> Index: arch/arm64/include/pmap.h
> ===================================================================
> RCS file: /cvs/src/sys/arch/arm64/include/pmap.h,v
> diff -u -p -r1.25 pmap.h
> --- arch/arm64/include/pmap.h 11 Dec 2023 22:12:53 -0000 1.25
> +++ arch/arm64/include/pmap.h 9 Jan 2025 21:44:41 -0000
> @@ -123,6 +123,7 @@ struct pv_entry;
> int pmap_fault_fixup(pmap_t, vaddr_t, vm_prot_t);
>
> #define __HAVE_PMAP_MPSAFE_ENTER_COW
> +#define __HAVE_PMAP_POPULATE
>
> #endif /* _KERNEL && !_LOCORE */
>
> Index: uvm/uvm_fault.c
> ===================================================================
> RCS file: /cvs/src/sys/uvm/uvm_fault.c,v
> diff -u -p -r1.159 uvm_fault.c
> --- uvm/uvm_fault.c 3 Jan 2025 15:31:48 -0000 1.159
> +++ uvm/uvm_fault.c 9 Jan 2025 21:44:41 -0000
> @@ -1105,8 +1105,12 @@ uvm_fault_upper(struct uvm_faultinfo *uf
> /* XXX instrumentation */
> return ENOMEM;
> }
> +#ifdef __HAVE_PMAP_POPULATE
> + pmap_populate(ufi->orig_map->pmap, ufi->orig_rvaddr);
> +#else
> /* XXX instrumentation */
> uvm_wait("flt_pmfail1");
> +#endif
> return ERESTART;
> }
>
> @@ -1457,8 +1461,12 @@ uvm_fault_lower(struct uvm_faultinfo *uf
> /* XXX instrumentation */
> return (ENOMEM);
> }
> +#ifdef __HAVE_PMAP_POPULATE
> + pmap_populate(ufi->orig_map->pmap, ufi->orig_rvaddr);
> +#else
> /* XXX instrumentation */
> uvm_wait("flt_pmfail2");
> +#endif
> return ERESTART;
> }
>
> Index: uvm/uvm_pmap.h
> ===================================================================
> RCS file: /cvs/src/sys/uvm/uvm_pmap.h,v
> diff -u -p -r1.34 uvm_pmap.h
> --- uvm/uvm_pmap.h 3 Apr 2024 18:43:32 -0000 1.34
> +++ uvm/uvm_pmap.h 9 Jan 2025 21:44:41 -0000
> @@ -179,6 +179,10 @@ vaddr_t pmap_steal_memory(vsize_t, vad
> void pmap_virtual_space(vaddr_t *, vaddr_t *);
> #endif
>
> +#if defined(__HAVE_PMAP_POPULATE)
> +void pmap_populate(pmap_t, vaddr_t);
> +#endif
> +
> /* nested pmaps are used in i386/amd64 vmm */
> #ifndef pmap_nested
> #define pmap_nested(pm) 0
>
