On 14/05/25(Wed) 10:39, Dave Voutila wrote:
> Martin Pieuchot <mpi@grenadille.net> writes:
> 
> > Diff below removes the `lockflags' argument from uvm_map_pageable(),
> > grabs the exclusive lock explicitly where necessary and adds locking
> > assertions in uvm_map_pageable_wire() and uvm_map_pageable().
> >
> > As a result uvm_map_pageable_wire() becomes much simpler, the locking
> > requirement becomes obvious and I don't have to think to understand
> > where locks are released.
> >
> > I'd like to get this in before fixing uvm_vslock_device() which suffers
> > from the same race that was present in uvm_vslock().
> >
> > ok?
> 
> I like this and diff looks ok to me with one question about a removal of
> a DIGNOSTIC section below.

Thanks for the review.  See below.

>  [...]
> > @@ -2129,12 +2127,12 @@ uvm_map_pageable_wire(struct vm_map *map
> >  	vm_map_lock(map);
> >  	vm_map_unbusy(map);
> >
> > -	if (error) {
> >  #ifdef DIAGNOSTIC
> > -		if (timestamp_save != map->timestamp)
> > -			panic("uvm_map_pageable_wire: stale map");
> > +	if (timestamp_save != map->timestamp)
> > +		panic("stale map");
> >  #endif
> >
> > +	if (error) {
> >  		/*
> >  		 * first is no longer needed to restart loops.
> >  		 * Use it as iterator to unmap successful mappings.
> > @@ -2162,37 +2160,20 @@ uvm_map_pageable_wire(struct vm_map *map
> >
> >  			iter->wired_count--;
> >  		}
> > -
> > -		if ((lockflags & UVM_LK_EXIT) == 0)
> > -			vm_map_unlock(map);
> > -		return error;
> >  	}
> >
> > -
> > -	if ((lockflags & UVM_LK_EXIT) == 0) {
> > -		vm_map_unlock(map);
> > -	} else {
> > -#ifdef DIAGNOSTIC
> > -		if (timestamp_save != map->timestamp)
> > -			panic("uvm_map_pageable_wire: stale map");
> > -#endif
> > -	}
> > -	return 0;
> > +	return error;
> 
> Did you mean to remove this stale map check bef10;rgb:dcdc/dfdf/e4e4ore return? Or is this
> because it's now checked unconditionally above?

The two branches have been merged.  It has always been unconditionally
checked, but now it is written only once.  This is to ensure no change
on the map happened while it was marked busy and unlocked.

It is easier to understand by looking at the file before and after
applying the diff.