Re: [Xen-devel] [PATCH] move domain to cpupool0 before destroying it

[George Dunlap <dunlapg@xxxxxxxxx>]

On Thu, May 15, 2014 at 5:59 AM, Juergen Gross
<juergen.gross@xxxxxxxxxxxxxx> wrote:
> Currently when a domain is destroyed it is removed from the domain_list
> before all of it's resources, including the cpupool membership, are freed.
> This can lead to a situation where the domain is still member of a cpupool
> without for_each_domain_in_cpupool() (or even for_each_domain()) being
> able to find it any more. This in turn can result in rejection of removing
> the last cpu from a cpupool, because there seems to be still a domain in
> the cpupool, even if it can't be found by scanning through all domains.
>
> This situation can be avoided by moving the domain to be destroyed to
> cpupool0 first and then remove it from this cpupool BEFORE deleting it from
> the domain_list. As cpupool0 is always active and a domain without any cpupool
> membership is implicitly regarded as belonging to cpupool0, this poses no
> problem.

I'm a bit unclear why we're doing *both* a sched_move_domain(), *and*
moving the "cpupool_rm_domain()".

The sched_move_domain() only happens in domain_kill(), which is only
initiated (AFAICT) by hypercall: does that mean if a VM dies for some
other reason (i.e., crashes), that you may still have the same race?
If not, then just this change alone should be sufficent.  If it does,
then this change is redundant.

Moving the cpupool_rm_domain() will change things so that there is now
a period of time where the VM is not being listed as being in
cpupool0's pool, but may still be in that pool's scheduler's list of
domains.  Is that OK?  If it is OK, it seems like that change alone
should be sufficient.

I've been trying to trace through the twisty little passages of domain
destruction, and I'm still not quite sure: would it be OK if we just
called sched_move_domain() in domain_destroy() instead of calling
cpupool_rm_domain()?

 -George


>
> Signed-off-by: Juergen Gross <juergen.gross@xxxxxxxxxxxxxx>
> ---
>  xen/common/domain.c |    6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index 4291e29..d4bcf6b 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -593,6 +593,8 @@ int domain_kill(struct domain *d)
>              BUG_ON(rc != -EAGAIN);
>              break;
>          }
> +        if ( sched_move_domain(d, cpupool0) )
> +            return -EAGAIN;
>          for_each_vcpu ( d, v )
>              unmap_vcpu_info(v);
>          d->is_dying = DOMDYING_dead;
> @@ -775,8 +777,6 @@ static void complete_domain_destroy(struct rcu_head *head)
>
>      sched_destroy_domain(d);
>
> -    cpupool_rm_domain(d);
> -
>      /* Free page used by xen oprofile buffer. */
>  #ifdef CONFIG_XENOPROF
>      free_xenoprof_pages(d);
> @@ -823,6 +823,8 @@ void domain_destroy(struct domain *d)
>      if ( _atomic_read(old) != 0 )
>          return;
>
> +    cpupool_rm_domain(d);
> +
>      /* Delete from task list and task hashtable. */
>      TRACE_1D(TRC_SCHED_DOM_REM, d->domain_id);
>      spin_lock(&domlist_update_lock);
> --
> 1.7.10.4
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxx
> http://lists.xen.org/xen-devel

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