| Message ID | 59A9A4620200007800176A49@prv-mh.provo.novell.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 01/09/17 17:18, Jan Beulich wrote: > There is no need to hold the global domctl lock while across > domain_kill() - the domain lock is fully sufficient here. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > --- > Sadly so far we haven't had any feedback on this change from the people > who observed an issue apparently resulting from heavy contention here. > Hence the RFC. > > Obviously other domctl-s could benefit from similar adjustments, so > this is meant to be just a start. What is the expected ordering of the global domctl lock and perdomain domctl locks? The current uses appear a little ad-hoc. Is there anything we can do to more strongly enforce the expected behaviour? > --- a/xen/common/domain.c > +++ b/xen/common/domain.c > @@ -619,13 +619,16 @@ int domain_kill(struct domain *d) > if ( d == current->domain ) > return -EINVAL; > > - /* Protected by domctl_lock. */ > + /* Protected by d->domain_lock. */ > switch ( d->is_dying ) > { > case DOMDYING_alive: > + domain_unlock(d); > domain_pause(d); > + domain_lock(d); > + if ( d->is_dying != DOMDYING_alive ) > + return domain_kill(d); Comment about recursion safety? > d->is_dying = DOMDYING_dying; > - spin_barrier(&d->domain_lock); > evtchn_destroy(d); > gnttab_release_mappings(d); > tmem_destroy(d->tmem_client); > --- a/xen/common/domctl.c > +++ b/xen/common/domctl.c > @@ -664,11 +664,14 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe > break; > > case XEN_DOMCTL_destroydomain: > + domctl_lock_release(); > + domain_lock(d); > ret = domain_kill(d); > + domain_unlock(d); > if ( ret == -ERESTART ) > ret = hypercall_create_continuation( > __HYPERVISOR_domctl, "h", u_domctl); > - break; > + goto domctl_out_unlock_domonly; > > case XEN_DOMCTL_setnodeaffinity: > { > > >
>>> On 01.09.17 at 18:28, <andrew.cooper3@citrix.com> wrote: > On 01/09/17 17:18, Jan Beulich wrote: >> There is no need to hold the global domctl lock while across >> domain_kill() - the domain lock is fully sufficient here. >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> --- >> Sadly so far we haven't had any feedback on this change from the people >> who observed an issue apparently resulting from heavy contention here. >> Hence the RFC. >> >> Obviously other domctl-s could benefit from similar adjustments, so >> this is meant to be just a start. > > What is the expected ordering of the global domctl lock and perdomain > domctl locks? I'd expect the domain locks to nest inside the global domctl one, but I wouldn't want to spell this out anywhere until we actually have a case where both need to be acquired (and I would hope such a case to never arise). Or (I didn't check this) maybe we already have cases where the per-domain lock is being acquired with the global domctl one being held, even without this suggested adjustment. > The current uses appear a little ad-hoc. Is there anything we can do to > more strongly enforce the expected behaviour? Expected behavior of what? >> --- a/xen/common/domain.c >> +++ b/xen/common/domain.c >> @@ -619,13 +619,16 @@ int domain_kill(struct domain *d) >> if ( d == current->domain ) >> return -EINVAL; >> >> - /* Protected by domctl_lock. */ >> + /* Protected by d->domain_lock. */ >> switch ( d->is_dying ) >> { >> case DOMDYING_alive: >> + domain_unlock(d); >> domain_pause(d); >> + domain_lock(d); >> + if ( d->is_dying != DOMDYING_alive ) >> + return domain_kill(d); > > Comment about recursion safety? I can certainly add one, albeit in the case here I'm not really convinced it is necessary. Would you be happy with /* * With the domain lock dropped, d->is_dying may have changed. Call * ourselves recursively if so, which is safe as then we won't come * back here. */ ? Jan
On 09/04/2017 09:47 AM, Jan Beulich wrote: >>>> On 01.09.17 at 18:28, <andrew.cooper3@citrix.com> wrote: >> On 01/09/17 17:18, Jan Beulich wrote: >>> There is no need to hold the global domctl lock while across >>> domain_kill() - the domain lock is fully sufficient here. >>> >>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >>> --- >>> Sadly so far we haven't had any feedback on this change from the people >>> who observed an issue apparently resulting from heavy contention here. >>> Hence the RFC. >>> >>> Obviously other domctl-s could benefit from similar adjustments, so >>> this is meant to be just a start. >> >> What is the expected ordering of the global domctl lock and perdomain >> domctl locks? > > I'd expect the domain locks to nest inside the global domctl one, but > I wouldn't want to spell this out anywhere until we actually have a > case where both need to be acquired (and I would hope such a > case to never arise). Or (I didn't check this) maybe we already have > cases where the per-domain lock is being acquired with the global > domctl one being held, even without this suggested adjustment. FWIW in the scheduler, we normally nest the global locks inside the per-domain locks. This actually makes more sense, as a typical usage would be: * Grab per-domain lock * Figure out what's going on, what global resource we need * Grab global lock * Grab global resource * Release global lock * Finish assigning resource to current do main, do whatever else needs to be done * Release per-domain lock Or to put it differently: * The more people there are contending for a lock, the shorter the critical section should be * Shorter critical sections should be nested inside larger critical sections * Global locks will have more people contending for them than per-domain locks * So global locks should typically be nested inside of per-domain locks. -George
>>> On 09.10.17 at 13:08, <george.dunlap@citrix.com> wrote: > On 09/04/2017 09:47 AM, Jan Beulich wrote: >>>>> On 01.09.17 at 18:28, <andrew.cooper3@citrix.com> wrote: >>> On 01/09/17 17:18, Jan Beulich wrote: >>>> There is no need to hold the global domctl lock while across >>>> domain_kill() - the domain lock is fully sufficient here. >>>> >>>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >>>> --- >>>> Sadly so far we haven't had any feedback on this change from the people >>>> who observed an issue apparently resulting from heavy contention here. >>>> Hence the RFC. >>>> >>>> Obviously other domctl-s could benefit from similar adjustments, so >>>> this is meant to be just a start. >>> >>> What is the expected ordering of the global domctl lock and perdomain >>> domctl locks? >> >> I'd expect the domain locks to nest inside the global domctl one, but >> I wouldn't want to spell this out anywhere until we actually have a >> case where both need to be acquired (and I would hope such a >> case to never arise). Or (I didn't check this) maybe we already have >> cases where the per-domain lock is being acquired with the global >> domctl one being held, even without this suggested adjustment. > > FWIW in the scheduler, we normally nest the global locks inside the > per-domain locks. This actually makes more sense, as a typical usage > would be: > > * Grab per-domain lock > * Figure out what's going on, what global resource we need > * Grab global lock > * Grab global resource > * Release global lock > * Finish assigning resource to current do main, do whatever else needs > to be done > * Release per-domain lock > > Or to put it differently: > > * The more people there are contending for a lock, the shorter the > critical section should be > * Shorter critical sections should be nested inside larger critical sections > * Global locks will have more people contending for them than per-domain > locks > * So global locks should typically be nested inside of per-domain locks. Ah, yes, for strictly ordered locks (scope wise, I mean) this certainly makes sense. I'm not sure it applies to the domain lock / domctl lock relationship, as the latter would normally be acquired only be the control domain. Anyway - the patch here doesn't introduce any nesting of locks, so I don't think the relative order between the two locks should be spelled out at this time. Jan
--- a/xen/common/domain.c +++ b/xen/common/domain.c @@ -619,13 +619,16 @@ int domain_kill(struct domain *d) if ( d == current->domain ) return -EINVAL; - /* Protected by domctl_lock. */ + /* Protected by d->domain_lock. */ switch ( d->is_dying ) { case DOMDYING_alive: + domain_unlock(d); domain_pause(d); + domain_lock(d); + if ( d->is_dying != DOMDYING_alive ) + return domain_kill(d); d->is_dying = DOMDYING_dying; - spin_barrier(&d->domain_lock); evtchn_destroy(d); gnttab_release_mappings(d); tmem_destroy(d->tmem_client); --- a/xen/common/domctl.c +++ b/xen/common/domctl.c @@ -664,11 +664,14 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe break; case XEN_DOMCTL_destroydomain: + domctl_lock_release(); + domain_lock(d); ret = domain_kill(d); + domain_unlock(d); if ( ret == -ERESTART ) ret = hypercall_create_continuation( __HYPERVISOR_domctl, "h", u_domctl); - break; + goto domctl_out_unlock_domonly; case XEN_DOMCTL_setnodeaffinity: {
There is no need to hold the global domctl lock while across domain_kill() - the domain lock is fully sufficient here. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- Sadly so far we haven't had any feedback on this change from the people who observed an issue apparently resulting from heavy contention here. Hence the RFC. Obviously other domctl-s could benefit from similar adjustments, so this is meant to be just a start.