Re: uao references & uao_swap_off() cleanup
On 23/06/21(Wed) 23:03, Jonathan Matthew wrote: > 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. Thanks, I'll do that. > 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()? I prefer to keep the KERNEL_ASSERT_LOCKED() where it is needed and not spread it to all the callers. My current plan is to trade those assert by assertions on the vmobjlock so I don't want to add new ones.
Re: uao references & uao_swap_off() cleanup
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 - 1.98 > > +++ uvm/uvm_aobj.c 16 Jun 2021 09:20:26 - > > @@ -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); > > +
Re: uao references & uao_swap_off() cleanup
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? > > 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.c16 Jun 2021 09:02:21 - 1.98 > +++ uvm/uvm_aobj.c16 Jun 2021 09:20:26 - > @@ -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 recursio
uao references & uao_swap_off() cleanup
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? 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 - 1.98 +++ uvm/uvm_aobj.c 16 Jun 2021 09:20:26 - @@ -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. -*/ -