Re: [Xen-devel] [PATCH 1/4] xen/common: Break domain_unmap_resources() out of domain_kill()

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 19.02.19 at 23:46, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 19/02/2019 22:39, Razvan Cojocaru wrote:
>> On 2/20/19 12:18 AM, Andrew Cooper wrote:
>>> A subsequent change is going to need an x86-specific unmapping step, so take
>>> the opportunity to split the current vcpu unmapping out into a dedicated 
> path.
>>>
>>> No practical change.
>>>
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>>> ---
>>> CC: Jan Beulich <JBeulich@xxxxxxxx>
>>> CC: Wei Liu <wei.liu2@xxxxxxxxxx>
>>> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>>> CC: Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>
>>> CC: Tamas K Lengyel <tamas@xxxxxxxxxxxxx>
>>> CC: Juergen Gross <jgross@xxxxxxxx>
>>> ---
>>>  xen/common/domain.c      | 16 +++++++++++++---
>>>  xen/include/xen/domain.h |  4 ++++
>>>  2 files changed, 17 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/xen/common/domain.c b/xen/common/domain.c
>>> index 32bca8d..e66f7ea 100644
>>> --- a/xen/common/domain.c
>>> +++ b/xen/common/domain.c
>>> @@ -700,10 +700,21 @@ int rcu_lock_live_remote_domain_by_id(domid_t dom, 
> struct domain **d)
>>>      return 0;
>>>  }
>>>  
>>> +static void domain_unmap_resources(struct domain *d)
>>> +{
>>> +    struct vcpu *v;
>>> +
>>> +    for_each_vcpu ( d, v )
>>> +    {
>>> +        unmap_vcpu_info(v);
>>> +
>>> +        arch_vcpu_unmap_resources(v);
>>> +    }
>>> +}
>>> +
>>>  int domain_kill(struct domain *d)
>>>  {
>>>      int rc = 0;
>>> -    struct vcpu *v;
>>>  
>>>      if ( d == current->domain )
>>>          return -EINVAL;
>>> @@ -732,13 +743,12 @@ int domain_kill(struct domain *d)
>>>          d->tmem_client = NULL;
>>>          /* fallthrough */
>>>      case DOMDYING_dying:
>>> +        domain_unmap_resources(d);
>>>          rc = domain_relinquish_resources(d);
>>>          if ( rc != 0 )
>>>              break;
>>>          if ( cpupool_move_domain(d, cpupool0) )
>>>              return -ERESTART;a
>>> -        for_each_vcpu ( d, v )
>>> -            unmap_vcpu_info(v);
>> So before this change there was a leak of some sort here? I see that
>> before this patch unmap_vcpu_info() was called only if rc == 0, but it
>> is now called unconditionally _before_ domain_relinquish_resources().
>>
>> This does appear to change the behaviour of the code in the error case.
>>
>> If that's intended:
>> Reviewed-by: Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>
> 
> domain_kill() is a very long running hypercall, and takes about 15
> minutes of wallclock time for domains with 1T of RAM (before the idle
> scrubbing changes went in).
> 
> It will frequently break out with -ERESTART for continuation purposes,
> but doesn't fail outright.
> 
> However, with hindsight it would probably be better just to put the
> altp2m case in domain_relinquish_resources() and do away with this patch
> entirely.

While this patch could have my ack, I'd clearly prefer you going the
described alternative route.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel