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

[Juergen Gross <juergen.gross@xxxxxxxxxxxxxx>]

On 19.05.2014 18:19, George Dunlap wrote:
> On Mon, May 19, 2014 at 4:34 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
> > > > > On 19.05.14 at 16:57, <dunlapg@xxxxxxxxx> wrote:
> > > 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.
> > No, a crashed domain is merely being reported as crashed to the
> > tool stack. It's the tool stack to then actually invoke the killing of
> > it (or else e.g. "on_crash=preserve" would be rather hard to handle).
> Right, I see.
>
> > > 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.
> > Moving this earlier was a requirement to avoid the race that the
> > earlier (much different) patch tried to address. Also I think the
> > patch's description already addresses that question (see the last
> > sentence of the quoted original mail contents above).
> But we're avoiding that race by instead moving the dying domain to
> cpupool0, which is never going to disappear.
>
> Or, moving the domain to cpupool0 *won't* sufficiently solve the race,
> and this will -- in which case, why are we bothering to move it to
> cpupool0 at all?  Why not just remove it from the cpupool when
> removing it from the domain list?  Wouldn't that also solve the
> original problem?
>
> Regarding the last bit, "...a domain without any cpupool membership is
> implicitly regarded as belonging to cpupool0...":
>
> 1. At a quick glance through the code, I couldn't find any evidence
> that this was the case; I couldn't find an instance where d->cpupool
> == NULL => assumed cpupool0.
xen/common/schedule.c:

#define DOM2OP(_d)    (((_d)->cpupool == NULL) ? &ops : ((_d)->cpupool->sched))

together with:

struct scheduler *scheduler_get_default(void)
{
    return &ops;
}

> 2. If in reality d->cpupool is never (or almost never) actually NULL,
> then the "implicitly belongs to cpupool0" assumption will bitrot.
> Having that kind of assumption without some way of making sure it's
> maintained is a bug waiting to happen.
That's not going to happen: This assumption is tested every time the idle
domain is being referenced by the scheduler...

> > > 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()?
> > No, it would not, because then again we wouldn't be able to deal
> > with potential failure, needing re-invocation of the function.
> Right.
Juergen

--
Juergen Gross                 Principal Developer Operating Systems
PSO PM&D ES&S SWE OS6                  Telephone: +49 (0) 89 62060 2932
Fujitsu                                   e-mail: juergen.gross@xxxxxxxxxxxxxx
Mies-van-der-Rohe-Str. 8                Internet: ts.fujitsu.com
D-80807 Muenchen                 Company details: ts.fujitsu.com/imprint.html

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