Re: [Xen-devel] [PATCH] xen/sched: fix credit2 smt idle handling

[Juergen Gross <jgross@xxxxxxxx>]

On 23/03/2019 03:32, Dario Faggioli wrote:
> On Fri, 2019-03-22 at 10:04 +0100, Juergen Gross wrote:
>> Credit2's smt_idle_mask_set() and smt_idle_mask_clear() are used to
>> identify idle cores where vcpus can be moved to. A core is thought to
>> be idle when all siblings are known to have the idle vcpu running on
>> them.
>>
>> Unfortunately the information of a vcpu running on a cpu is per
>> runqueue. So in case not all siblings are in the same runqueue a core
>> will never be regarded to be idle, as the sibling not in the runqueue
>> is never known to run the idle vcpu.
>>
> Good catch.
> 
> I apparently forgot to take care of this, when introduced the
> possibility of having per single CPU runqueue (which, in an SMT enabled
> system, would mean per-thread runqs).
> 
>> This problem can be solved by and-ing the core's sibling cpumask with
>> the runqueue's active mask before doing the idle test.
>>
> Right. There's one thing, though. Using one runq per CPU, in this
> scheduler, is a really poor choice, and I basically would recommend it
> only for testing or debugging (and this should probably be highlighted
> a lot better in the docs).
> 
> Therefore, I'm a bit reluctant at adding another cpumask bitwise
> operation, in hot paths, just for taking care of it.
> 
> Note that this also applies to cpupools, i.e., I also consider a very
> poor choice putting two sibling hyperthreads in different pools. If you
> recall, I even sent a patch to forbid doing that (which is still
> blocked on a series of yours for passing parameters from the tools to
> the hypervisor).
> 
> The only case I care, is a CPU being off-lined.

In my core scheduling solution we only ever have one active sibling per
core.

> So, one thing that we could do is to put credit2_runqueue=cpu inside
> such #ifdef-s too (and I can prepare a patch to that effect myself, if
> you want).
> 
> To properly deal with offline CPUs, I think we can change the logic a
> little, i.e., we initialize the smt_idle mask to all-1 (all CPUs idle),
> and we also make sure that we set the CPU bit (instead of learing it)
> in smt_idle, when we remove the CPU from the scheduler.

How does that help?

Only if all siblings are marked as idle in rqd->idle we set any bits
in rqd->smt_idle (all siblings).

Or did you mean rqd->idle instead?

This might be problematic in case of runqueue per cpu, though.

Another idea: we could introduce a credit2 pcpu data cpumask pointer
for replacement of the cpu_sibling_mask. For runqueue per cpu it would
pount to cpumask_of(cpu), for the "normal case" it would point to the
correct cpu_sibling_mask, and for special cases we could allocate a
mask if needed.


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel