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

[Juergen Gross <juergen.gross@ts.fujitsu.com>]

On 19.05.2014 18:19, George Dunlap wrote:
> On Mon, May 19, 2014 at 4:34 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 19.05.14 at 16:57, <dunlapg@umich.edu> wrote:
>>> On Thu, May 15, 2014 at 5:59 AM, Juergen Gross
>>> <juergen.gross@ts.fujitsu.com> 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?

No. sched_destroy_domain() has to be called with the domain in the
correct cpupool. Otherwise the selection of the scheduler to use for freeing
the scheduler data won't be correct, as it will assume the default scheduler
if the domain isn't registered to any cpupool.

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@ts.fujitsu.com
Mies-van-der-Rohe-Str. 8                Internet: ts.fujitsu.com
D-80807 Muenchen                 Company details: ts.fujitsu.com/imprint.html