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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 01.09.17 at 18:28, <andrew.cooper3@xxxxxxxxxx> 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@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?

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


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