Re: [Xen-devel] [PATCH v3 33/47] xen/sched: add a percpu resource index

[Jan Beulich <jbeulich@xxxxxxxx>]

On 24.09.2019 17:20, Jürgen Groß wrote:
> On 24.09.19 12:05, Jan Beulich wrote:
>> On 14.09.2019 10:52, Juergen Gross wrote:
>>> --- a/xen/common/schedule.c
>>> +++ b/xen/common/schedule.c
>>> @@ -71,6 +71,7 @@ static void poll_timer_fn(void *data);
>>>   /* This is global for now so that private implementations can reach it */
>>>   DEFINE_PER_CPU(struct scheduler *, scheduler);
>>>   DEFINE_PER_CPU_READ_MOSTLY(struct sched_resource *, sched_res);
>>> +static DEFINE_PER_CPU_READ_MOSTLY(unsigned int, sched_res_idx);
>>
>> It's of course not very helpful that this variable (right here) doesn't
>> ever get written to, and hence one can't judge where / how this is to
>> be done (without going look elsewhere). But I guess that calculation
>> can't be moved into this patch (accepting that it would always yield
>> zero for now)?
> 
> Not easily. I can set sched_res_idx to 0 explicitly when initializing
> a new cpu if you like that better.

No, not really. Then better leave it as is.

>>> @@ -2008,7 +2015,7 @@ static void sched_slave(void)
>>>   
>>>       pcpu_schedule_unlock_irq(lock, cpu);
>>>   
>>> -    sched_context_switch(vprev, next->vcpu_list, now);
>>> +    sched_context_switch(vprev, sched_unit2vcpu_cpu(next, cpu), now);
>>>   }
>>
>> By the end of the series this will be
>>
>>      sched_context_switch(vprev, sched_unit2vcpu_cpu(next, cpu),
>>                           is_idle_unit(next) && !is_idle_unit(prev), now);
>>
>> clarifying (I think) that "next" can indeed be an idle unit. I'm having
>> difficulty seeing how can produce the correct result in all three cases
>> - all vCPU-s in the unit belong to a real domain
>> - all vCPU-s in the unit belong to the idle domain
>> - there's a mix of "real" and (filler) "idle" vCPU-s
>> My main question is what "next" is in the last of the 3 cases above.
> 
> next points to the real domain's unit.
> 
>> Considering that scheduling happens in terms of units, I'd expect it to
>> be a real domain's unit, yet then I don't see how you'd get at the
>> correct (idle) vCPU when calling sched_unit2vcpu_cpu(next, cpu).
> 
> What is the problem?
> 
> sched_unit2vcpu_cpu() will first get the real domain's vcpu for that cpu
> (if exisiting) and replace it by the idle vcpu of that cpu if the real
> domain's vcpu is either not existing or won't be running. The main trick
> is that we use idle_vcpu[cpu] (which is existing today already).

Yeah, I guess this has become more clear by looking at the next
patch. Without seeing the final shape sched_unit2vcpu_cpu() will
take (i.e. including the trick you talk about) it wasn't really
clear to me what is (supposed to be) going on here. (Makes me
wonder once again whether that function couldn't have taken its
final shape right away. But anyway.)

Jan

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