Re: [Xen-devel] [PATCH v2 13/48] xen/sched: add is_running indicator to struct sched_unit

[Juergen Gross <jgross@suse.com>]

On 04.09.19 17:06, Jan Beulich wrote:
> On 09.08.2019 16:57, Juergen Gross wrote:
>> --- a/xen/common/schedule.c
>> +++ b/xen/common/schedule.c
>> @@ -407,6 +407,8 @@ int sched_init_vcpu(struct vcpu *v, unsigned int processor)
>>       {
>>           get_sched_res(v->processor)->curr = unit;
>>           v->is_running = 1;
>> +        unit->is_running = 1;
>> +        unit->state_entry_time = NOW();
>>       }
>>       else
>>       {
> 
> Are idle vCPU-s also going to get grouped into units (I'm sorry,
> I don't recall)? If so, just like further down I'd be putting
> under question the multiple writing of the field. I'd kind of
> expect the unit and all vCPU-s within it to get an identical
> state entry time stored.

When an idle vcpu is being created its cpu is in no cpupool yet
(a free cpu). Those cpus are always subject to cpu scheduling
(granularity 1). Only when cpus are put into a cpupool the
granularity is adjusted accordingly and the idle-vcpus are
possibly grouped in units (see patch 45).

Regarding the state entry time: different vcpus of a unit can be
in different states (blocked/running, etc.), so their state entry
times will generally differ. A unit's state entry time will
reflect its scheduling events (being scheduled/descheduled).

> Also both here and further down I'm puzzled to see that the
> unit's field doesn't get set at the same place in code where
> the vCPU's field gets set.

Right. The states of a vcpu and the unit it is belonging to are
coupled, but not identical.

> 
>> @@ -1663,8 +1666,10 @@ static void schedule(void)
>>        * switch, else lost_records resume will not work properly.
>>        */
>>   
>> -    ASSERT(!next->is_running);
>> +    ASSERT(!next->sched_unit->is_running);
>>       next->is_running = 1;
>> +    next->sched_unit->is_running = 1;
>> +    next->sched_unit->state_entry_time = now;
> 
> Isn't the ASSERT() you delete still supposed to be true? In which case
> wouldn't it be worthwhile to retain it?

No, it is not true any longer. All but one vcpus of a unit can be
blocked resulting in the related idle vcpus to be running. This means
that even with one unit being replaced by another one the vcpu can be
the same.

> 
>> --- a/xen/include/xen/sched.h
>> +++ b/xen/include/xen/sched.h
>> @@ -272,6 +272,11 @@ struct sched_unit {
>>       struct sched_resource *res;
>>       int                    unit_id;
>>   
>> +    /* Last time unit got (de-)scheduled. */
>> +    uint64_t               state_entry_time;
>> +
>> +    /* Currently running on a CPU? */
>> +    bool                   is_running;
>>       /* Does soft affinity actually play a role (given hard affinity)? */
>>       bool                   soft_aff_effective;
>>       /* Bitmask of CPUs on which this VCPU may run. */
> 
> I'm noticing this here, but it may well have been an issue earlier
> already (and there may well be later adjustments invalidating my
> remark, and of course it's the end result of this series which
> matters in the long run): Could you see about adding/removing
> fields to this struct (and generally of course also others)
> minimizing the number / overall size of holes?

Hmm, yes.


Juergen

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