Re: [Xen-devel] [PATCH RFC V2 42/45] xen/sched: add fall back to idle vcpu when scheduling item

[Juergen Gross <jgross@xxxxxxxx>]

On 17/05/2019 10:22, Jan Beulich wrote:
>>>> On 17.05.19 at 09:48, <jgross@xxxxxxxx> wrote:
>> On 17/05/2019 08:57, Jan Beulich wrote:
>>>>>> On 17.05.19 at 07:13, <jgross@xxxxxxxx> wrote:
>>>> On 16/05/2019 16:41, Jan Beulich wrote:
>>>>>>>> On 16.05.19 at 15:51, <jgross@xxxxxxxx> wrote:
>>>>>> As with core scheduling we can be sure the other thread is active
>>>>>> (otherwise we would schedule the idle item) and hoping for saving power
>>>>>> by using mwait is moot.
>>>>>
>>>>> Saving power may be indirect, by the CPU re-arranging
>>>>> resource assignment between threads when one goes idle.
>>>>> I have no idea whether they do this when entering C1, or
>>>>> only when entering deeper C states.
>>>>
>>>> SDM Vol. 3 chapter 8.10.1 "HLT instruction":
>>>>
>>>> "Here shared resources that were being used by the halted logical
>>>> processor become available to active logical processors, allowing them
>>>> to execute at greater efficiency."
>>>
>>> To be honest, this is to broad/generic a statement to fully
>>> trust it, judging from other areas of the SDM. And then, as
>>> per above, what about the socket granularity case? Putting
>>> entirely idle cores to sleep is surely worthwhile?
>>
>> Yes, I assume it is. OTOH this might affect context switches badly
>> as the reaction time for the coordinated switch will rise. Maybe a
>> good reason for another sub-option?
> 
> While I agree that fine grained control is useful, I'm seeing an
> increasing risk of there going to be too many controls to actually
> be certain in the end that all possible combinations work
> correctly.

Okay, I think I'll leave it as is for the moment and do some
performance tests later. Depending on the results we can then
decide how to proceed.

> 
>>>>> And anyway - I'm still none the wiser as to the v->is_urgent
>>>>> relationship.
>>>>
>>>> With v->is_urgent set today's idle loop will drop into default_idle().
>>>> I can remove this sentence in case it is just confusing.
>>>
>>> I'd prefer if the connection would become more obvious. One
>>> needs to go from ->is_urgent via ->urgent_count to
>>> sched_has_urgent_vcpu() to find where the described
>>> behavior really lives.
>>>
>>> What's worse though: This won't work as intended on AMD
>>> at all. I don't think it's correct to fall back to default_idle() in
>>> this case. Instead sched_has_urgent_vcpu() returning true
>>> should amount to the same effect as max_cstate being set
>>> to 1. There's
>>> (a) no reason not to use MWAIT on Intel CPUs in this case,
>>> if MWAIT can enter C1, and
>>> (b) a strong need to use MWAIT on (at least) AMD Fam17,
>>> or else it won't be C1 that gets entered.
>>> I'll see about making a patch in due course.
>>
>> Thanks. Would you mind doing it in a way that the caller can specify
>> max_cstate? This would remove the need to call sched_has_urgent_vcpu()
>> deep down the idle handling and I could re-use it for my purpose.
> 
> Hmm, to be honest I'm not fancying giving a parameter to
> default_idle(), pm_idle(), and friends. Conceptually it is not
> the business of the callers to control the C states to be used.
> 
> What about the opposite: You simply mark idle (v)CPUs in
> question as "urgent", thus achieving the intended effect as
> well.

I can do that, of course. IMO letting the caller specify how he wants
idle to behave is cleaner than burying such an implicit dependency deep
down the code, but you are the maintainer of that part.


Juergen

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