tech-kern archive

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]

Re: evbarm hang



[redirected to port-arm]

On Fri, Apr 19, 2019 at 12:47:11PM +0200, Manuel Bouyer wrote:
> On Fri, Apr 19, 2019 at 08:33:00PM +1000, matthew green wrote:
> > > So here's our deadlock: cpu 0 holds the kernel lock and wants the pool spin
> > > mutex; cpu 1 holds the spin mutex and wants the kenrel lock.
> > 
> > AFAICT, cpu 1 is at fault here.  this locking order is
> > backwards.
> > 
> > not sure why arm32 pmap operations on the kernel map are
> > with the kernel lock instead of another lock.  the arm64
> > version of this code doesn't take any lock.
> 
> Yes, this looks like it can cause lots of problems.
> 
> Indeed, I suspect the kernel_lock here could be replaced with pm_lock as
> aarch64 does. I will try this.

Here's what I found.
I think a per-pmap lock is needed even for the kernel pmap, because of
the l2_dtable (and related) structures contains statistics that
needs to be keep coherent. For user pmaps, pm_lock protects it.
For the kernel pmap we can't use pm_lock, because as pmap_kenter/pmap_kremove
can be called from interrupt context, it needs a mutex at IPL_VM.
So I added a kpm_lock kmutex, and use it in pmap_acquire_pmap_lock() for
kernel pmap.

Then we need to be carefull not calling a path that could sleep with this mutex
held. This needs some adjustements in pmap_enter() (don't call
pool_get()/pool_put() with the pmap locked)  and pmap_remove()
(defer pool_put() util it is safe to release the pmap lock).

While there, also use splvm() instead of splhigh() in pmap_growkernel(),
as x86 does.
Also, rename pmap_lock to pmap_pg_lock. As this can be used with the
kernel pmap lock held, this also needs to be a IPL_VM kmutex (in all cases).

It also includes the patch I posted earlier today to workaround deadlocks
in ddb.

I've been running a DIAGNOSTIC+LOCKDEBUG kernel on my lime2 allwinner A20
board, and it did build a few packages (using distcc, and a SATA disk for
local storage) without problems, so at this point it looks like
an improvement. I'll keep it running over the week-end and unless I get
negative feedback, I'll commit it early next week.


-- 
Manuel Bouyer <bouyer%antioche.eu.org@localhost>
     NetBSD: 26 ans d'experience feront toujours la difference
--
Index: arm32/pmap.c
===================================================================
RCS file: /cvsroot/src/sys/arch/arm/arm32/pmap.c,v
retrieving revision 1.371
diff -u -p -u -r1.371 pmap.c
--- arm32/pmap.c	28 Oct 2018 14:59:17 -0000	1.371
+++ arm32/pmap.c	19 Apr 2019 15:55:30 -0000
@@ -217,6 +217,10 @@
 
 #include <arm/locore.h>
 
+#ifdef DDB
+#include <arm/db_machdep.h>
+#endif
+
 __KERNEL_RCSID(0, "$NetBSD: pmap.c,v 1.371 2018/10/28 14:59:17 skrll Exp $");
 
 //#define PMAP_DEBUG
@@ -516,7 +520,8 @@ static size_t cnptes;
 #endif
 vaddr_t memhook;			/* used by mem.c & others */
 kmutex_t memlock __cacheline_aligned;	/* used by mem.c & others */
-kmutex_t pmap_lock __cacheline_aligned;
+kmutex_t pmap_pg_lock __cacheline_aligned;
+kmutex_t kpm_lock __cacheline_aligned;
 extern void *msgbufaddr;
 int pmap_kmpages;
 /*
@@ -538,9 +543,14 @@ vaddr_t pmap_directlimit;
 static inline void
 pmap_acquire_pmap_lock(pmap_t pm)
 {
+#if defined(MULTIPROCESSOR) && defined(DDB)
+	if (__predict_false(db_onproc != NULL))
+		return;
+#endif
+	
 	if (pm == pmap_kernel()) {
 #ifdef MULTIPROCESSOR
-		KERNEL_LOCK(1, NULL);
+		mutex_enter(&kpm_lock);
 #endif
 	} else {
 		mutex_enter(pm->pm_lock);
@@ -550,9 +560,13 @@ pmap_acquire_pmap_lock(pmap_t pm)
 static inline void
 pmap_release_pmap_lock(pmap_t pm)
 {
+#if defined(MULTIPROCESSOR) && defined(DDB)
+	if (__predict_false(db_onproc != NULL))
+		return;
+#endif
 	if (pm == pmap_kernel()) {
 #ifdef MULTIPROCESSOR
-		KERNEL_UNLOCK_ONE(NULL);
+		mutex_exit(&kpm_lock);
 #endif
 	} else {
 		mutex_exit(pm->pm_lock);
@@ -562,20 +576,20 @@ pmap_release_pmap_lock(pmap_t pm)
 static inline void
 pmap_acquire_page_lock(struct vm_page_md *md)
 {
-	mutex_enter(&pmap_lock);
+	mutex_enter(&pmap_pg_lock);
 }
 
 static inline void
 pmap_release_page_lock(struct vm_page_md *md)
 {
-	mutex_exit(&pmap_lock);
+	mutex_exit(&pmap_pg_lock);
 }
 
 #ifdef DIAGNOSTIC
 static inline int
 pmap_page_locked_p(struct vm_page_md *md)
 {
-	return mutex_owned(&pmap_lock);
+	return mutex_owned(&pmap_pg_lock);
 }
 #endif
 
@@ -3057,6 +3071,10 @@ pmap_enter(pmap_t pm, vaddr_t va, paddr_
 #else
 	const bool vector_page_p = (va == vector_page);
 #endif
+	struct pmap_page *pp = pmap_pv_tracked(pa);
+	struct pv_entry *new_pv = NULL;
+	struct pv_entry *old_pv = NULL;
+	int error = 0;
 
 	UVMHIST_FUNC(__func__); UVMHIST_CALLED(maphist);
 
@@ -3072,6 +3090,12 @@ pmap_enter(pmap_t pm, vaddr_t va, paddr_
 	 * test for a managed page by checking pg != NULL.
 	 */
 	pg = pmap_initialized ? PHYS_TO_VM_PAGE(pa) : NULL;
+	/*
+	 * if we may need a new pv entry allocate if now, as we can't do it
+	 * with the kernel_pmap locked
+	 */
+	if (pg || pp)
+		new_pv = pool_get(&pmap_pv_pool, PR_NOWAIT);
 
 	nflags = 0;
 	if (prot & VM_PROT_WRITE)
@@ -3095,7 +3119,8 @@ pmap_enter(pmap_t pm, vaddr_t va, paddr_
 	if (l2b == NULL) {
 		if (flags & PMAP_CANFAIL) {
 			pmap_release_pmap_lock(pm);
-			return (ENOMEM);
+			error = ENOMEM;
+			goto free_pv;
 		}
 		panic("pmap_enter: failed to allocate L2 bucket");
 	}
@@ -3118,8 +3143,6 @@ pmap_enter(pmap_t pm, vaddr_t va, paddr_
 	} else
 		opg = NULL;
 
-	struct pmap_page *pp = pmap_pv_tracked(pa);
-
 	if (pg || pp) {
 		KASSERT((pg != NULL) != (pp != NULL));
 		struct vm_page_md *md = (pg != NULL) ? VM_PAGE_TO_MD(pg) :
@@ -3228,9 +3251,10 @@ pmap_enter(pmap_t pm, vaddr_t va, paddr_
 				}
 #endif
 			} else {
-				pmap_release_page_lock(md);
-				pv = pool_get(&pmap_pv_pool, PR_NOWAIT);
+				pv = new_pv;
+				new_pv = NULL;
 				if (pv == NULL) {
+					pmap_release_page_lock(md);
 					pmap_release_pmap_lock(pm);
 					if ((flags & PMAP_CANFAIL) == 0)
 						panic("pmap_enter: "
@@ -3241,7 +3265,6 @@ pmap_enter(pmap_t pm, vaddr_t va, paddr_
 					    0, 0, 0, 0);
 					return (ENOMEM);
 				}
-				pmap_acquire_page_lock(md);
 			}
 
 			pmap_enter_pv(md, pa, pv, pm, va, nflags);
@@ -3278,9 +3301,9 @@ pmap_enter(pmap_t pm, vaddr_t va, paddr_
 			paddr_t opa = VM_PAGE_TO_PHYS(opg);
 
 			pmap_acquire_page_lock(omd);
-			struct pv_entry *pv = pmap_remove_pv(omd, opa, pm, va);
+			old_pv = pmap_remove_pv(omd, opa, pm, va);
 			pmap_vac_me_harder(omd, opa, pm, 0);
-			oflags = pv->pv_flags;
+			oflags = old_pv->pv_flags;
 			pmap_release_page_lock(omd);
 
 #ifdef PMAP_CACHE_VIVT
@@ -3288,7 +3311,6 @@ pmap_enter(pmap_t pm, vaddr_t va, paddr_
 				pmap_cache_wbinv_page(pm, va, true, oflags);
 			}
 #endif
-			pool_put(&pmap_pv_pool, pv);
 		}
 	}
 
@@ -3390,7 +3412,13 @@ pmap_enter(pmap_t pm, vaddr_t va, paddr_
 
 	pmap_release_pmap_lock(pm);
 
-	return (0);
+
+	if (old_pv)
+		pool_put(&pmap_pv_pool, old_pv);
+free_pv:
+	if (new_pv)
+		pool_put(&pmap_pv_pool, new_pv);
+	return (error);
 }
 
 /*
@@ -3425,6 +3453,7 @@ pmap_remove(pmap_t pm, vaddr_t sva, vadd
 	/*
 	 * we lock in the pmap => pv_head direction
 	 */
+
 	pmap_acquire_pmap_lock(pm);
 
 #ifndef ARM_MMU_EXTENDED
@@ -3463,6 +3492,7 @@ pmap_remove(pmap_t pm, vaddr_t sva, vadd
 		for (;sva < next_bucket;
 		     sva += PAGE_SIZE, ptep += PAGE_SIZE / L2_S_SIZE) {
 			pt_entry_t opte = *ptep;
+			struct pv_entry *opv = NULL;
 
 			if (opte == 0) {
 				/* Nothing here, move along */
@@ -3480,17 +3510,15 @@ pmap_remove(pmap_t pm, vaddr_t sva, vadd
 			 */
 			if (pg != NULL) {
 				struct vm_page_md *md = VM_PAGE_TO_MD(pg);
-				struct pv_entry *pv;
 
 				pmap_acquire_page_lock(md);
-				pv = pmap_remove_pv(md, pa, pm, sva);
+				opv = pmap_remove_pv(md, pa, pm, sva);
 				pmap_vac_me_harder(md, pa, pm, 0);
 				pmap_release_page_lock(md);
-				if (pv != NULL) {
+				if (opv != NULL) {
 					if (pm->pm_remove_all == false) {
-						flags = pv->pv_flags;
+						flags = opv->pv_flags;
 					}
-					pool_put(&pmap_pv_pool, pv);
 				}
 			}
 			mappings += PAGE_SIZE / L2_S_SIZE;
@@ -3503,7 +3531,7 @@ pmap_remove(pmap_t pm, vaddr_t sva, vadd
 				 */
 				l2pte_reset(ptep);
 				PTE_SYNC_CURRENT(pm, ptep);
-				continue;
+				goto free_opv;
 			}
 
 #ifdef ARM_MMU_EXTENDED
@@ -3545,6 +3573,12 @@ pmap_remove(pmap_t pm, vaddr_t sva, vadd
 				}
 			}
 #endif
+free_opv:
+			if (opv) {
+				pmap_release_pmap_lock(pm);
+				pool_put(&pmap_pv_pool, opv);
+				pmap_acquire_pmap_lock(pm);
+			}
 		}
 
 #ifndef ARM_MMU_EXTENDED
@@ -3807,7 +3841,6 @@ pmap_kremove(vaddr_t va, vsize_t len)
 #ifdef UVMHIST
 	u_int total_mappings = 0;
 #endif
-
 	PMAPCOUNT(kenter_unmappings);
 
 	UVMHIST_FUNC(__func__); UVMHIST_CALLED(maphist);
@@ -3815,15 +3848,15 @@ pmap_kremove(vaddr_t va, vsize_t len)
 	UVMHIST_LOG(maphist, " (va=%#jx, len=%#jx)", va, len, 0, 0);
 
 	const vaddr_t eva = va + len;
+	pmap_t kpm = pmap_kernel();
 
-	pmap_acquire_pmap_lock(pmap_kernel());
+	pmap_acquire_pmap_lock(kpm);
 
 	while (va < eva) {
 		vaddr_t next_bucket = L2_NEXT_BUCKET_VA(va);
 		if (next_bucket > eva)
 			next_bucket = eva;
 
-		pmap_t kpm = pmap_kernel();
 		struct l2_bucket * const l2b = pmap_get_l2_bucket(kpm, va);
 		KDASSERT(l2b != NULL);
 
@@ -3879,7 +3912,7 @@ pmap_kremove(vaddr_t va, vsize_t len)
 		total_mappings += mappings;
 #endif
 	}
-	pmap_release_pmap_lock(pmap_kernel());
+	pmap_release_pmap_lock(kpm);
 	cpu_cpwait();
 	UVMHIST_LOG(maphist, "  <--- done (%ju mappings removed)",
 	    total_mappings, 0, 0, 0);
@@ -5904,7 +5937,7 @@ pmap_growkernel(vaddr_t maxkvaddr)
 	 * whoops!   we need to add kernel PTPs
 	 */
 
-	s = splhigh();	/* to be safe */
+	s = splvm();	/* to be safe */
 	mutex_enter(kpm->pm_lock);
 
 	/* Map 1MB at a time */
@@ -6147,15 +6180,8 @@ pmap_bootstrap(vaddr_t vstart, vaddr_t v
 #endif
 
 	VPRINTF("locks ");
-#if defined(PMAP_CACHE_VIPT) && !defined(ARM_MMU_EXTENDED)
-	if (arm_cache_prefer_mask != 0) {
-		mutex_init(&pmap_lock, MUTEX_DEFAULT, IPL_VM);
-	} else {
-#endif
-		mutex_init(&pmap_lock, MUTEX_DEFAULT, IPL_NONE);
-#if defined(PMAP_CACHE_VIPT) && !defined(ARM_MMU_EXTENDED)
-	}
-#endif
+	mutex_init(&pmap_pg_lock, MUTEX_DEFAULT, IPL_VM);
+	mutex_init(&kpm_lock, MUTEX_DEFAULT, IPL_VM);
 	mutex_init(&pm->pm_obj_lock, MUTEX_DEFAULT, IPL_NONE);
 	uvm_obj_init(&pm->pm_obj, NULL, false, 1);
 	uvm_obj_setlock(&pm->pm_obj, &pm->pm_obj_lock);


Home | Main Index | Thread Index | Old Index