Re: [Xen-devel] [PATCH 2/5] xen: credit2: never consider CPUs outside of our cpupool.

[Juergen Gross <jgross@xxxxxxxx>]

On 17/01/17 18:26, Dario Faggioli wrote:
> In fact, relying on the mask of what pCPUs belong to
> which Credit2 runqueue is not enough. If we only do that,
> when Credit2 is the boot scheduler, we may ASSERT() or
> panic when moving a pCPU from Pool-0 to another cpupool.
> 
> This is because pCPUs outside of any pool are considered
> part of cpupool0. This puts us at risk of crash when those
> same pCPUs are added to another pool and something
> different than the idle domain is found to be running
> on them.
> 
> Note that, even if we prevent the above to happen (which
> is the purpose of this patch), this is still pretty bad,
> in fact, when we remove a pCPU from Pool-0:
> - in Credit1, as we do *not* update prv->ncpus and
>   prv->credit, which means we're considering the wrong
>   total credits when doing accounting;
> - in Credit2, the pCPU remains part of one runqueue,
>   and is hence at least considered during load balancing,
>   even if no vCPU should really run there.
> 
> In Credit1, this "only" causes skewed accounting and
> no crashes because there is a lot of `cpumask_and`ing
> going on with the cpumask of the domains' cpupool
> (which, BTW, comes at a price).
> 
> A quick and not to involved (and easily backportable)
> solution for Credit2, is to do exactly the same.
> 
> Signed-off-by: Dario Faggioli <dario.faggioli@xxxxxxxxxx
> ---
> Cc: George Dunlap <george.dunlap@xxxxxxxxxxxxx>
> Cc: Juergen Gross <jgross@xxxxxxxx>
> Cc: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> This is a bugfix, and should be backported to 4.8.
> ---
> The proper solution would mean calling deinit_pdata() when removing a pCPU 
> from
> cpupool0, as well as a bit more of code reshuffling.
> 
> And, although worth doing, it certainly will take more work, more time, and
> will probably be hard (well, surely harder than this) to backport.
> 
> Therefore, I'd argue in favor of both taking and backporting this change, 
> which
> at least enables using Credit2 as default scheduler without risking a crash
> when creating a second cpupool.
> 
> Afterwards, a proper solution would be proposed for Xen 4.9.
> 
> Finally, given the wide number of issues similar to this that I've found and
> fixed in the last release cycle, I think it would be good to take a stab at
> whether the interface between cpupools and the schedulers could not be
> simplified. :-O

The first patch version for support of cpupools I sent used an own
scheduler instance for the free cpus. Keir merged this instance with
the one for Pool-0.

I think it might be a good idea to check whether some of the problems
are going away with the free cpu scheduler idea again. Maybe it would
even be a good idea to add a "null-scheduler" for this purpose
supporting only one vcpu per pcpu. This scheduler could be used for
high performance domains, too.


Juergen


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