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

>>> +    if ( is_idle_unit(unit) )
>>> +        return true;
>>> +
>>> +    for_each_sched_unit_vcpu ( unit, v )
>>> +        if ( vcpu_runnable(v) )
>>> +            return true;
>>
>> Isn't the loop going to yield true anyway for idle units? If so, is
>> there a particular reason for the special casing of idle units up
>> front here?
> 
> Didn't I explain that before?

Quite possible; a good hint that a code comment wouldn't hurt.

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

As to that construct - why is the parameter named "i" rather than "u"?
And why "e" in for_each_sched_unit()?

Jan

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