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

[Jan Beulich <jbeulich@xxxxxxxx>]

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:
>>> On 23.09.19 17:41, Jan Beulich wrote:
>>>> On 14.09.2019 10:52, Juergen Gross wrote:
>>>>> @@ -1851,7 +1852,7 @@ void sched_context_switched(struct vcpu *vprev, 
>>>>> struct vcpu *vnext)
>>>>>                while ( atomic_read(&next->rendezvous_out_cnt) )
>>>>>                    cpu_relax();
>>>>>        }
>>>>> -    else if ( vprev != vnext )
>>>>> +    else if ( vprev != vnext && sched_granularity == 1 )
>>>>>            context_saved(vprev);
>>>>>    }
>>>>
>>>> Would you mind helping me with understanding why this call is
>>>> needed with a granularity of 1 only?
>>>
>>> Otherwise it is done just a few lines up (granularity 1 will result
>>> in rendezvous_out_cnt being zero).
>>
>> I.e. if is rendezvous_out_cnt is zero and vprev != vnext but
>> sched_granularity > 1 the call isn't needed? Why? At the end of
> 
> I can add an ASSERT() here. This should never happen.

Ah yes, this would address my question.

>> the series vcpu_context_saved() gets called in all cases; what's
>> conditional upon granularity being 1 there is the call to
>> unit_context_saved().
> 
> vcpu_context_saved() at the end of the series is testing vprev != vnext.

But that part of the conditional was not under question.

>>> 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.

>> As to that construct - why is the parameter named "i" rather than "u"?
>> And why "e" in for_each_sched_unit()?
> 
> "i" like "item" (somehow this survived the big rename). Can change it.
> "e" like "element". I think this is another relic. Can change it, too.

I'd appreciate this, as it would get this more in line with the other
similar macros.

Jan

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