Re: evbarm hang

[Manuel Bouyer <bouyer%antioche.eu.org@localhost>]

On Sun, Apr 21, 2019 at 07:31:00AM +0100, Nick Hudson wrote:
> 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.

Sure, no problems. Then can you rename it as part of your changes ?

I found the name of this lock, used to lock a single page across pmaps,
qyite confusing ...

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

I didn't see Jason's message. I guess he suggested making kpm->pm_obj_lock
an IPL_VM lock. I can make that, and indeed it'll avoid a test in
pmap_{acquire,release}_pmap_lock. But then we'll need a separate IPL_NONE
lock for pmap_growkernel() (we can't use a IPL_VM lock here because
pmap_grow_map() may sleep).

> 
> 
> > It also includes the patch I posted earlier today to workaround deadlocks
> > in ddb.
> 
> Can this go in separately and a pullup requested, please?

Sure.
But both changes probably needs to be pulled up; netbsd-8's pmap.c
also use KERNEL_LOCK for kernel's pmap.


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

Ha yes, It'll be better.

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

No, the lock used here has to be a IPL_NONE (see above). If we make
pm_lock IPL_VM (which is probably a good idea), we'll need a different
lock here. But anyway the splvm has to stay.

I'll come up with an updated patch later today

-- 
Manuel Bouyer <bouyer%antioche.eu.org@localhost>
     NetBSD: 26 ans d'experience feront toujours la difference
--