On Wed, Jun 23, 2021 at 09:37:10AM +0200, Martin Pieuchot wrote: > On 16/06/21(Wed) 11:26, Martin Pieuchot wrote: > > Diff below does two things: > > > > - Use atomic operations for incrementing/decrementing references of > > anonymous objects. This allows us to manipulate them without holding > > the KERNEL_LOCK(). > > > > - Rewrite the loop from uao_swap_off() to only keep a reference to the > > next item in the list. This is imported from NetBSD and is necessary > > to introduce locking around uao_pagein(). > > > > ok? > > Anyone?
uao_reference_locked() and uao_detach_locked() are prototyped in uvm_extern.h, so they should be removed here too. It doesn't look like uao_detach() is safe to call without the kernel lock; it calls uao_dropswap() for each page, which calls uao_set_swslot(), which includes a KERNEL_ASSERT_LOCKED(). Should we keep the KERNEL_ASSERT_LOCKED() in uao_detach()? ok jmatthew@ otherwise > > > > > Index: uvm/uvm_aobj.c > > =================================================================== > > RCS file: /cvs/src/sys/uvm/uvm_aobj.c,v > > retrieving revision 1.98 > > diff -u -p -r1.98 uvm_aobj.c > > --- uvm/uvm_aobj.c 16 Jun 2021 09:02:21 -0000 1.98 > > +++ uvm/uvm_aobj.c 16 Jun 2021 09:20:26 -0000 > > @@ -779,19 +779,11 @@ uao_init(void) > > void > > uao_reference(struct uvm_object *uobj) > > { > > - KERNEL_ASSERT_LOCKED(); > > - uao_reference_locked(uobj); > > -} > > - > > -void > > -uao_reference_locked(struct uvm_object *uobj) > > -{ > > - > > /* Kernel object is persistent. */ > > if (UVM_OBJ_IS_KERN_OBJECT(uobj)) > > return; > > > > - uobj->uo_refs++; > > + atomic_inc_int(&uobj->uo_refs); > > } > > > > > > @@ -801,34 +793,19 @@ uao_reference_locked(struct uvm_object * > > void > > uao_detach(struct uvm_object *uobj) > > { > > - KERNEL_ASSERT_LOCKED(); > > - uao_detach_locked(uobj); > > -} > > - > > - > > -/* > > - * uao_detach_locked: drop a reference to an aobj > > - * > > - * => aobj may freed upon return. > > - */ > > -void > > -uao_detach_locked(struct uvm_object *uobj) > > -{ > > struct uvm_aobj *aobj = (struct uvm_aobj *)uobj; > > struct vm_page *pg; > > > > /* > > * Detaching from kernel_object is a NOP. > > */ > > - if (UVM_OBJ_IS_KERN_OBJECT(uobj)) { > > + if (UVM_OBJ_IS_KERN_OBJECT(uobj)) > > return; > > - } > > > > /* > > * Drop the reference. If it was the last one, destroy the object. > > */ > > - uobj->uo_refs--; > > - if (uobj->uo_refs) { > > + if (atomic_dec_int_nv(&uobj->uo_refs) > 0) { > > return; > > } > > > > @@ -1265,68 +1242,54 @@ uao_dropswap(struct uvm_object *uobj, in > > boolean_t > > uao_swap_off(int startslot, int endslot) > > { > > - struct uvm_aobj *aobj, *nextaobj, *prevaobj = NULL; > > + struct uvm_aobj *aobj; > > > > /* > > - * Walk the list of all anonymous UVM objects. > > + * Walk the list of all anonymous UVM objects. Grab the first. > > */ > > mtx_enter(&uao_list_lock); > > + if ((aobj = LIST_FIRST(&uao_list)) == NULL) { > > + mtx_leave(&uao_list_lock); > > + return FALSE; > > + } > > + uao_reference(&aobj->u_obj); > > > > - for (aobj = LIST_FIRST(&uao_list); > > - aobj != NULL; > > - aobj = nextaobj) { > > + do { > > + struct uvm_aobj *nextaobj; > > boolean_t rv; > > > > /* > > - * add a ref to the aobj so it doesn't disappear > > - * while we're working. > > - */ > > - uao_reference_locked(&aobj->u_obj); > > - > > - /* > > - * now it's safe to unlock the uao list. > > - * note that lock interleaving is alright with IPL_NONE mutexes. > > + * Prefetch the next object and immediately hold a reference > > + * on it, so neither the current nor the next entry could > > + * disappear while we are iterating. > > */ > > - mtx_leave(&uao_list_lock); > > - > > - if (prevaobj) { > > - uao_detach_locked(&prevaobj->u_obj); > > - prevaobj = NULL; > > + if ((nextaobj = LIST_NEXT(aobj, u_list)) != NULL) { > > + uao_reference(&nextaobj->u_obj); > > } > > + mtx_leave(&uao_list_lock); > > > > /* > > - * page in any pages in the swslot range. > > - * if there's an error, abort and return the error. > > + * Page in all pages in the swap slot range. > > */ > > rv = uao_pagein(aobj, startslot, endslot); > > + > > + /* Drop the reference of the current object. */ > > + uao_detach(&aobj->u_obj); > > if (rv) { > > - uao_detach_locked(&aobj->u_obj); > > + if (nextaobj) { > > + uao_detach(&nextaobj->u_obj); > > + } > > return rv; > > } > > > > - /* > > - * we're done with this aobj. > > - * relock the list and drop our ref on the aobj. > > - */ > > + aobj = nextaobj; > > mtx_enter(&uao_list_lock); > > - nextaobj = LIST_NEXT(aobj, u_list); > > - /* > > - * prevaobj means that we have an object that we need > > - * to drop a reference for. We can't just drop it now with > > - * the list locked since that could cause lock recursion in > > - * the case where we reduce the refcount to 0. It will be > > - * released the next time we drop the list lock. > > - */ > > - prevaobj = aobj; > > - } > > + } while (aobj); > > > > /* > > * done with traversal, unlock the list > > */ > > mtx_leave(&uao_list_lock); > > - if (prevaobj) { > > - uao_detach_locked(&prevaobj->u_obj); > > - } > > return FALSE; > > } > > > > >