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

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

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>


Thanks,
Razvan

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