On Tue, Feb 20, 2018 at 08:52:20AM +0100, Mark Kettenis wrote:
> > Date: Mon, 19 Feb 2018 13:49:48 +0100 (CET)
> > From: Mark Kettenis <[email protected]>
> >
> > The diff below attempts to make the arm64 pmap "mpsafe" and enables MP
> > support. This diff survived a full build on my Firefly-RK3399 board.
> > It also seems to work on the Overdrive 1000. It might even work on
> > the Raspberry Pi 3. I'd appreciate it if people could play with this
> > on the Raspberry Pi and other arm64 hardware they have.
> >
> > I tried to follow a strategy for making the pmap "mpsafe" that more
> > closely resembles what we did for our other architectures, although I
> > looked at Dale Rahn's diff as well. It seems I fixed at least some
> > bugs in the process. But I'm sure there are bugs remaining. I may
> > even have introduced new ones. So do expect this to crash and burn,
> > or at least lock up.
> >
> > I'm also interested in people testing the normal GENERIC kernel with
> > this diff, especially building ports. There are some changes in there
> > that could affect non-MP as well.
>
> Here is a slightly better diff that removes a potential lock ordering
> problem, removes a redundant splvm and moves some code around to avoid
> a level of indentation. Gaining confidence, so I'm looking for ok's
> now.
I have been running you diff on my rpi (together with jsg firmware update).
My rpi has hung twice now while doing a make -j4 in the clang part of
the tree.
The last top screen shows it using swap (around 400M) and has three
c++ processes each with resident size around 200M.
I'll try this diff now.
-Otto
>
> Index: arch/arm64/arm64/cpu.c
> ===================================================================
> RCS file: /cvs/src/sys/arch/arm64/arm64/cpu.c,v
> retrieving revision 1.13
> diff -u -p -r1.13 cpu.c
> --- arch/arm64/arm64/cpu.c 31 Jan 2018 10:52:12 -0000 1.13
> +++ arch/arm64/arm64/cpu.c 19 Feb 2018 12:28:05 -0000
> @@ -454,13 +454,8 @@ cpu_start_secondary(struct cpu_info *ci)
>
> spllower(IPL_NONE);
>
> -#ifdef notyet
> SCHED_LOCK(s);
> cpu_switchto(NULL, sched_chooseproc());
> -#else
> - for (;;)
> - __asm volatile("wfe");
> -#endif
> }
>
> void
> Index: arch/arm64/arm64/pmap.c
> ===================================================================
> RCS file: /cvs/src/sys/arch/arm64/arm64/pmap.c,v
> retrieving revision 1.47
> diff -u -p -r1.47 pmap.c
> --- arch/arm64/arm64/pmap.c 31 Jan 2018 23:23:16 -0000 1.47
> +++ arch/arm64/arm64/pmap.c 19 Feb 2018 12:28:05 -0000
> @@ -165,6 +165,19 @@ struct mem_region *pmap_allocated = &pma
> int pmap_cnt_avail, pmap_cnt_allocated;
> uint64_t pmap_avail_kvo;
>
> +static inline void
> +pmap_lock(struct pmap *pmap)
> +{
> + if (pmap != pmap_kernel())
> + mtx_enter(&pmap->pm_mtx);
> +}
> +
> +static inline void
> +pmap_unlock(struct pmap *pmap)
> +{
> + if (pmap != pmap_kernel())
> + mtx_leave(&pmap->pm_mtx);
> +}
>
> /* virtual to physical helpers */
> static inline int
> @@ -362,6 +375,7 @@ pmap_vp_page_alloc(struct pool *pp, int
> void *v;
>
> kd.kd_waitok = ISSET(flags, PR_WAITOK);
> + kd.kd_trylock = ISSET(flags, PR_NOWAIT);
> kd.kd_slowdown = slowdown;
>
> KERNEL_LOCK();
> @@ -439,14 +453,20 @@ pmap_enter_pv(struct pte_desc *pted, str
> if (__predict_false(!pmap_initialized))
> return;
>
> + mtx_enter(&pg->mdpage.pv_mtx);
> LIST_INSERT_HEAD(&(pg->mdpage.pv_list), pted, pted_pv_list);
> pted->pted_va |= PTED_VA_MANAGED_M;
> + mtx_leave(&pg->mdpage.pv_mtx);
> }
>
> void
> pmap_remove_pv(struct pte_desc *pted)
> {
> + struct vm_page *pg = PHYS_TO_VM_PAGE(pted->pted_pte & PTE_RPGN);
> +
> + mtx_enter(&pg->mdpage.pv_mtx);
> LIST_REMOVE(pted, pted_pv_list);
> + mtx_leave(&pg->mdpage.pv_mtx);
> }
>
> int
> @@ -454,7 +474,7 @@ pmap_enter(pmap_t pm, vaddr_t va, paddr_
> {
> struct pte_desc *pted;
> struct vm_page *pg;
> - int s, error;
> + int error;
> int cache = PMAP_CACHE_WB;
> int need_sync = 0;
>
> @@ -464,9 +484,7 @@ pmap_enter(pmap_t pm, vaddr_t va, paddr_
> cache = PMAP_CACHE_DEV;
> pg = PHYS_TO_VM_PAGE(pa);
>
> - /* MP - Acquire lock for this pmap */
> -
> - s = splvm();
> + pmap_lock(pm);
> pted = pmap_vp_lookup(pm, va, NULL);
> if (pted && PTED_VALID(pted)) {
> pmap_remove_pted(pm, pted);
> @@ -535,8 +553,7 @@ pmap_enter(pmap_t pm, vaddr_t va, paddr_
>
> error = 0;
> out:
> - splx(s);
> - /* MP - free pmap lock */
> + pmap_unlock(pm);
> return error;
> }
>
> @@ -550,6 +567,7 @@ pmap_remove(pmap_t pm, vaddr_t sva, vadd
> struct pte_desc *pted;
> vaddr_t va;
>
> + pmap_lock(pm);
> for (va = sva; va < eva; va += PAGE_SIZE) {
> pted = pmap_vp_lookup(pm, va, NULL);
>
> @@ -564,6 +582,7 @@ pmap_remove(pmap_t pm, vaddr_t sva, vadd
> if (PTED_VALID(pted))
> pmap_remove_pted(pm, pted);
> }
> + pmap_unlock(pm);
> }
>
> /*
> @@ -590,11 +609,12 @@ pmap_remove_pted(pmap_t pm, struct pte_d
> pted->pted_va &= ~PTED_VA_EXEC_M;
> }
>
> - pted->pted_pte = 0;
> -
> if (PTED_MANAGED(pted))
> pmap_remove_pv(pted);
>
> + pted->pted_pte = 0;
> + pted->pted_va = 0;
> +
> if (pm != pmap_kernel())
> pool_put(&pmap_pted_pool, pted);
> splx(s);
> @@ -615,10 +635,6 @@ _pmap_kenter_pa(vaddr_t va, paddr_t pa,
> {
> pmap_t pm = pmap_kernel();
> struct pte_desc *pted;
> - int s;
> -
> - /* MP - lock pmap. */
> - s = splvm();
>
> pted = pmap_vp_lookup(pm, va, NULL);
>
> @@ -645,8 +661,6 @@ _pmap_kenter_pa(vaddr_t va, paddr_t pa,
> pmap_pte_insert(pted);
>
> ttlb_flush(pm, va & ~PAGE_MASK);
> -
> - splx(s);
> }
>
> void
> @@ -839,6 +853,9 @@ pmap_create(void)
> pmap_t pmap;
>
> pmap = pool_get(&pmap_pmap_pool, PR_WAITOK | PR_ZERO);
> +
> + mtx_init(&pmap->pm_mtx, IPL_VM);
> +
> pmap_pinit(pmap);
> if (pmap_vp_poolcache == 0) {
> pool_setlowat(&pmap_vp_pool, 20);
> @@ -853,7 +870,7 @@ pmap_create(void)
> void
> pmap_reference(pmap_t pm)
> {
> - pm->pm_refs++;
> + atomic_inc_int(&pm->pm_refs);
> }
>
> /*
> @@ -865,7 +882,7 @@ pmap_destroy(pmap_t pm)
> {
> int refs;
>
> - refs = --pm->pm_refs;
> + refs = atomic_dec_int_nv(&pm->pm_refs);
> if (refs > 0)
> return;
>
> @@ -1384,41 +1401,67 @@ pmap_page_ro(pmap_t pm, vaddr_t va, vm_p
> void
> pmap_page_protect(struct vm_page *pg, vm_prot_t prot)
> {
> - int s;
> struct pte_desc *pted;
> -
> - /* need to lock for this pv */
> - s = splvm();
> + struct pmap *pm;
>
> if (prot == PROT_NONE) {
> - while (!LIST_EMPTY(&(pg->mdpage.pv_list))) {
> + mtx_enter(&pg->mdpage.pv_mtx);
> + while ((pted = LIST_FIRST(&(pg->mdpage.pv_list))) != NULL) {
> + pmap_reference(pted->pted_pmap);
> + pm = pted->pted_pmap;
> + mtx_leave(&pg->mdpage.pv_mtx);
> +
> + pmap_lock(pm);
> +
> + /*
> + * We dropped the pvlist lock before grabbing
> + * the pmap lock to avoid lock ordering
> + * problems. This means we have to check the
> + * pvlist again since somebody else might have
> + * modified it. All we care about is that the
> + * pvlist entry matches the pmap we just
> + * locked. If it doesn't, unlock the pmap and
> + * try again.
> + */
> + mtx_enter(&pg->mdpage.pv_mtx);
> pted = LIST_FIRST(&(pg->mdpage.pv_list));
> - pmap_remove_pted(pted->pted_pmap, pted);
> + if (pted == NULL || pted->pted_pmap != pm) {
> + mtx_leave(&pg->mdpage.pv_mtx);
> + pmap_unlock(pm);
> + pmap_destroy(pm);
> + mtx_enter(&pg->mdpage.pv_mtx);
> + continue;
> + }
> + mtx_leave(&pg->mdpage.pv_mtx);
> + pmap_remove_pted(pm, pted);
> + pmap_unlock(pm);
> + pmap_destroy(pm);
> +
> + mtx_enter(&pg->mdpage.pv_mtx);
> }
> /* page is being reclaimed, sync icache next use */
> atomic_clearbits_int(&pg->pg_flags, PG_PMAP_EXE);
> - splx(s);
> + mtx_leave(&pg->mdpage.pv_mtx);
> return;
> }
>
> + mtx_enter(&pg->mdpage.pv_mtx);
> LIST_FOREACH(pted, &(pg->mdpage.pv_list), pted_pv_list) {
> pmap_page_ro(pted->pted_pmap, pted->pted_va, prot);
> }
> - splx(s);
> + mtx_leave(&pg->mdpage.pv_mtx);
> }
>
> void
> pmap_protect(pmap_t pm, vaddr_t sva, vaddr_t eva, vm_prot_t prot)
> {
> - int s;
> -
> if (prot & (PROT_READ | PROT_EXEC)) {
> - s = splvm();
> + pmap_lock(pm);
> while (sva < eva) {
> pmap_page_ro(pm, sva, 0);
> sva += PAGE_SIZE;
> }
> - splx(s);
> + pmap_unlock(pm);
> return;
> }
> pmap_remove(pm, sva, eva);
> @@ -1446,8 +1489,8 @@ pmap_init(void)
> pool_init(&pmap_pted_pool, sizeof(struct pte_desc), 0, IPL_VM, 0,
> "pted", NULL);
> pool_setlowat(&pmap_pted_pool, 20);
> - pool_init(&pmap_vp_pool, sizeof(struct pmapvp0), PAGE_SIZE, IPL_VM,
> - PR_WAITOK, "vp", &pmap_vp_allocator);
> + pool_init(&pmap_vp_pool, sizeof(struct pmapvp0), PAGE_SIZE, IPL_VM, 0,
> + "vp", &pmap_vp_allocator);
> /* pool_setlowat(&pmap_vp_pool, 20); */
>
> pmap_initialized = 1;
> @@ -1692,12 +1735,10 @@ pmap_clear_modify(struct vm_page *pg)
> {
> struct pte_desc *pted;
> uint64_t *pl3 = NULL;
> - int s;
>
> - s = splvm();
> -
> - pg->pg_flags &= ~PG_PMAP_MOD;
> + atomic_clearbits_int(&pg->pg_flags, PG_PMAP_MOD);
>
> + mtx_enter(&pg->mdpage.pv_mtx);
> LIST_FOREACH(pted, &(pg->mdpage.pv_list), pted_pv_list) {
> if (pmap_vp_lookup(pted->pted_pmap, pted->pted_va & ~PAGE_MASK,
> &pl3) == NULL)
> panic("failed to look up pte\n");
> @@ -1706,7 +1747,7 @@ pmap_clear_modify(struct vm_page *pg)
>
> ttlb_flush(pted->pted_pmap, pted->pted_va & ~PAGE_MASK);
> }
> - splx(s);
> + mtx_leave(&pg->mdpage.pv_mtx);
>
> return 0;
> }
> @@ -1719,18 +1760,16 @@ int
> pmap_clear_reference(struct vm_page *pg)
> {
> struct pte_desc *pted;
> - int s;
> -
> - s = splvm();
>
> - pg->pg_flags &= ~PG_PMAP_REF;
> + atomic_clearbits_int(&pg->pg_flags, PG_PMAP_REF);
>
> + mtx_enter(&pg->mdpage.pv_mtx);
> LIST_FOREACH(pted, &(pg->mdpage.pv_list), pted_pv_list) {
> pted->pted_pte &= ~PROT_MASK;
> pmap_pte_insert(pted);
> ttlb_flush(pted->pted_pmap, pted->pted_va & ~PAGE_MASK);
> }
> - splx(s);
> + mtx_leave(&pg->mdpage.pv_mtx);
>
> return 0;
> }
> Index: arch/arm64/conf/GENERIC.MP
> ===================================================================
> RCS file: arch/arm64/conf/GENERIC.MP
> diff -N arch/arm64/conf/GENERIC.MP
> --- /dev/null 1 Jan 1970 00:00:00 -0000
> +++ arch/arm64/conf/GENERIC.MP 19 Feb 2018 12:28:05 -0000
> @@ -0,0 +1,8 @@
> +# $OpenBSD$
> +
> +include "arch/arm64/conf/GENERIC"
> +
> +option MULTIPROCESSOR
> +#option MP_LOCKDEBUG
> +
> +cpu* at mainbus?
> Index: arch/arm64/include/pmap.h
> ===================================================================
> RCS file: /cvs/src/sys/arch/arm64/include/pmap.h,v
> retrieving revision 1.9
> diff -u -p -r1.9 pmap.h
> --- arch/arm64/include/pmap.h 12 Jan 2018 14:52:55 -0000 1.9
> +++ arch/arm64/include/pmap.h 19 Feb 2018 12:28:05 -0000
> @@ -17,7 +17,9 @@
> #ifndef _ARM64_PMAP_H_
> #define _ARM64_PMAP_H_
>
> -#include <arm64/pte.h>
> +#include <sys/mutex.h>
> +
> +#include <machine/pte.h>
>
> #define PMAP_PA_MASK ~((paddr_t)PAGE_MASK) /* to remove the flags */
> #define PMAP_NOCACHE 0x1 /* non-cacheable memory */
> @@ -66,6 +68,7 @@ void pagezero_cache(vaddr_t);
> * Pmap stuff
> */
> struct pmap {
> + struct mutex pm_mtx;
> union {
> struct pmapvp0 *l0; /* virtual to physical table 4 lvl */
> struct pmapvp1 *l1; /* virtual to physical table 3 lvl */
> @@ -104,12 +107,14 @@ void pmap_map_early(paddr_t, psize_t);
>
> #define __HAVE_VM_PAGE_MD
> struct vm_page_md {
> + struct mutex pv_mtx;
> LIST_HEAD(,pte_desc) pv_list;
> int pvh_attrs; /* page attributes */
> };
>
> #define VM_MDPAGE_INIT(pg) do { \
> - LIST_INIT(&((pg)->mdpage.pv_list)); \
> + mtx_init(&(pg)->mdpage.pv_mtx, IPL_VM); \
> + LIST_INIT(&((pg)->mdpage.pv_list)); \
> (pg)->mdpage.pvh_attrs = 0; \
> } while (0)
>
