Re: evbarm hang

[Nick Hudson <nick.hudson%gmx.co.uk@localhost>]

On 19/04/2019 16:57, Manuel Bouyer wrote:
> [redirected to port-arm]
>
> On Fri, Apr 19, 2019 at 12:47:11PM +0200, Manuel Bouyer wrote:
[snip]


Thanks for fixing 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.

See comment inline.

> 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).

I'd rather stuff didn't get renamed at this point as I have other work
that'll conflict.

Also, Jason suggested initialising kernel_pmap pm_obj_lock appropriately
to help simplify pmap_{acquire,release}_pmap_lock further.


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

Can this go in separately and a pullup requested, please?

> 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.

Thanks,
Nick


> 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
[snip]

> @@ -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


Write old pvs to a list and pool_put after the loop and the lock gets
dropped?



> @@ -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

kpm->pm_lock is IPL_VM so the splvm() call is not required anymore.

Nick