On Mon, Dec 23, 2024 at 10:06:10AM +0100, Martin Pieuchot wrote:
> On 20/12/24(Fri) 20:16, Martin Pieuchot wrote:
> > Diff below introduces a `lower_lock_type' variable to the context
> > descriptor used in the fault handler.  This variable is currently always
> > set to RW_WRITE to not introduce any change in behavior.
> > 
> > Related assertions are also updated in this diff.
> > 
> > The rw_enter() in uvm_fault_lower_lookup() is the one which is currently
> > highly contended.  The next step will be to grab a RW_READ lock for read
> > faults.
> 
> Anyone?

ok

One small comment:

> > @@ -1412,17 +1417,21 @@ uvm_fault_lower(struct uvm_faultinfo *uf
> >  		}
> >  	}
> >  
> > -	/* note: pg is either the uobjpage or the new page in the new anon */
> > +	/*
> > +	 * anon must be write locked (promotion).  uobj can be either.
> > +	 *
> > +	 * Note: pg is either the uobjpage or the new page in the new anon.
> > +	 */
> > +	KASSERT(amap == NULL ||
> > +	    rw_write_held(amap->am_lock));
> > +	KASSERT(uobj == NULL ||
> > +	    rw_status(uobj->vmobjlock) == flt->lower_lock_type);
> > +	KASSERT(anon == NULL || anon->an_lock == amap->am_lock);

Previously this last KASSERT was only reachable if amap != NULL. So
perhaps this should reflect that and add a NULL check before dereferencing
it? But maybe hitting an assert or crashing because of NULL deref makes
no difference?