Re: [Xen-devel] [PATCH v3 30/47] xen/sched: add support for multiple vcpus per sched unit where missing

[Jan Beulich <jbeulich@xxxxxxxx>]

On 26.09.2019 15:53, Dario Faggioli wrote:
> On Wed, 2019-09-25 at 15:07 +0200, Jan Beulich wrote:
>> On 25.09.2019 14:40, Jürgen Groß  wrote:
>>> On 24.09.19 17:22, Jan Beulich wrote:
>>>> On 24.09.2019 17:09, Jürgen Groß wrote:
>>>>> On 24.09.19 17:00, Jan Beulich wrote:
>>>>>> On 24.09.2019 16:41, Jürgen Groß wrote:
>>>>>>> for_each_sched_unit_vcpu() for an idle
>>>>>>> unit might end premature when one of the vcpus is running
>>>>>>> in another
>>>>>>> unit (idle_vcpu->sched_unit != idle_unit).
>>>>>>
>>>>>> Oh, that (v)->sched_unit == (i) in the construct is clearly
>>>>>> unexpected.
>>>>>> Is this really still needed by the end of the series? I
>>>>>> realize that
>>>>>> _some_ check is needed, but could this perhaps be arranged in
>>>>>> a way
>>>>>> that you'd still hit all vCPU-s when using it on an idle
>>>>>> unit, no
>>>>>> matter whether they're in use as a substitute in a "real"
>>>>>> unit?
>>>>>
>>>>> I could do that by having another linked list in struct vcpu.
>>>>> This way
>>>>> I can avoid it.
>>>>
>>>> Oh, no, not another list just for this purpose. I was rather
>>>> thinking
>>>> of e.g. a comparison of IDs.
>>>
>>> That would result either in something like:
>>>
>>> (v)->vcpu_id < (u)->unit_id + (u)->res->cpupool->granularity
>>>
>>> requiring to make struct sched_resource public as keyhandler.c
>>> needs
>>> for_each_sched_unit_vcpu() plus being quite expensive,
>>
>> I agree this is not a good option.
>>
>>> or:
>>>
>>> !(u)->next_in_list || (v)->vcpu_id < (u)->next_in_list->unit_id
>>>
>>> which seems to be more expensive as the current variant, too.
>>
>> It's not this much more expensive, and it eliminates unexpected
>> (as I would call it) behavior, so I think I'd go this route. 
>>
> So, I honestly like the way it's currently done in Juergen's pathes.
> 
> However, I'm not sure I understand what it is the issue that Jan thinks
> that has, and in what sense the code/behavior is regarded as
> "unexpected".
> 
> Can you help me see the problem? Maybe, if I realize it, I'd change my
> preference...

The unexpected / surprising behavior is described at the top (i.e.
still visible in context), but I'll quote it again here:

"for_each_sched_unit_vcpu() for an idle unit might end premature
 when one of the vcpus is running in another unit
 (idle_vcpu->sched_unit != idle_unit)"

This started out with me asking about an apparently (but as
Jürgen has clarified not truly) unnecessary special casing of
idle vCPU-s/units/domain ahead of a use of this construct.

Jan

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