Re: [Xen-devel] [PATCH v2 1/2] xen: fix a (latent) cpupool-related race during domain destroy

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

On Fri, 2016-07-15 at 14:52 +0200, Juergen Gross wrote:
> On 15/07/16 13:52, Dario Faggioli wrote:
> > 
> > On Fri, 2016-07-15 at 12:36 +0200, Juergen Gross wrote:
> > > 
> > > On 15/07/16 12:14, Dario Faggioli wrote:
> > > > 
> > > > In particular, I'm probably not fully understanding, from that
> > > > commit
> > > > changelog, what is the set of operations/command that I should
> > > > run
> > > > to
> > > > check whether or not I reintroduced the issue back.
> > > You need to create a domain in a cpupool and destroy it again
> > > while
> > > some dom0 process still is holding a reference to it (resulting
> > > in a
> > > zombie domain). Then try to destroy the cpupool.
> > > 
> > Ah, I see. I wasn't get the fact that it needed to be a zombie
> > domain
> > from anywhere.
> > 
> > > 
> > > > 
> > > > What am I missing?
> > > The domain being a zombie domain might change the picture. Moving
> > > it
> > > to
> > > cpupool0 was failing before my patch and it might do so again
> > > with
> > > your
> > > patch applied.
> > > 
> > Mmmm... I don't immediately see the reason why moving a zombie
> > domain
> > fails either, but I guess I'll have to try.
> Searching through the history I found commit
> 934e7baa6c12d19cfaf24e8f8e27d6c6a8b8c5e4 which might has removed the
> problematic condition (cpupool->n_dom being non-zero while d->cpupool
> was NULL already).
> 
Yeah.. And there's also this:

/*
 * unassign a specific cpu from a cpupool
 * we must be sure not to run on the cpu to be unassigned! to achieve this
 * the main functionality is performed via continue_hypercall_on_cpu on a
 * specific cpu.
 * if the cpu to be removed is the last one of the cpupool no active domain
 * must be bound to the cpupool. dying domains are moved to cpupool0 as they
 * might be zombies.
 * possible failures:
 * - last cpu and still active domains in cpupool
 * - cpu just being unplugged
 */
static int cpupool_unassign_cpu(struct cpupool *c, unsigned int cpu)
{
    int work_cpu;
    int ret;
    struct domain *d;

    cpupool_dprintk("cpupool_unassign_cpu(pool=%d,cpu=%d)\n",
                    c->cpupool_id, cpu);
    [...]
        for_each_domain_in_cpupool(d, c)
        {
            if ( !d->is_dying )
            {
                ret = -EBUSY;
                break;
            }
            ret = cpupool_move_domain_locked(d, cpupool0);
            if ( ret )
                break;
        }
    [...]

So it really looks like it ought to be possible to move zombies to
cpupool0 these days. :-)

> > Therefore, I still think this patch is correct, but I'm up for
> > investigating further and finding a way to solve the "zombie in
> > cpupool" issue as well.
> I'm not saying your patch is wrong. I just wanted to give you a hint
> about the history of the stuff you are changing. :-)
> 
Sure, and that's much appreciated! :-)

> If it is working I'd really prefer it over the current situation.
> 
Right. I've got to leave now. But I'll produce some zombies on Monday,
and will see if they move. :-D

Thanks and Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

Attachment: signature.asc
Description: This is a digitally signed message part

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