Re: [Xen-devel] [PATCH RFC] domctl: improve locking during domain destruction

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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@xxxxxxxx>
> ---
> 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:
>      {
>
>
>


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel