Re: [Xen-devel] [PATCH v2 30/48] xen/sched: introduce unit_runnable_state()

[Juergen Gross <jgross@suse.com>]

On 11.09.19 12:30, Jan Beulich wrote:
> On 09.08.2019 16:58, Juergen Gross wrote:
>> --- a/xen/common/schedule.c
>> +++ b/xen/common/schedule.c
>> @@ -266,7 +266,7 @@ static inline void sched_unit_runstate_change(struct sched_unit *unit,
>>       struct vcpu *v = unit->vcpu_list;
>>   
>>       if ( running )
>> -        vcpu_runstate_change(v, RUNSTATE_running, new_entry_time);
>> +        vcpu_runstate_change(v, v->new_state, new_entry_time);
> 
> Strictly speaking this is wrong when there's no actual state
> change, as the state entry time then shouldn't change. Quite
> possibly this would be merely a cosmetic issue though.

This will be changed in vcpu_runstate_change() with patch 31 when this
situation is actually possible. With only one vcpu in a unit the state
will always change here, while after the next patch
vcpu_runstate_change() will return early in case the state isn't
changing.

> 
>> --- a/xen/include/xen/sched-if.h
>> +++ b/xen/include/xen/sched-if.h
>> @@ -75,6 +75,20 @@ static inline bool unit_runnable(const struct sched_unit *unit)
>>       return vcpu_runnable(unit->vcpu_list);
>>   }
>>   
>> +static inline bool unit_runnable_state(const struct sched_unit *unit)
>> +{
>> +    struct vcpu *v;
>> +    bool runnable;
>> +
>> +    v = unit->vcpu_list;
>> +    runnable = vcpu_runnable(v);
>> +
>> +    v->new_state = runnable ? RUNSTATE_running
>> +                            : (v->pause_flags & VPF_blocked)
>> +                              ? RUNSTATE_blocked : RUNSTATE_offline;
>> +    return runnable;
>> +}
> 
> Especially for understanding the (correctness of the) credit1
> changes it would be rather helpful if once again this function
> actually iterated over all vCPU-s right away (even if there's
> only one per unit right now), to see how their varying states
> get combined.

Okay, will move it.

> 
>> --- a/xen/include/xen/sched.h
>> +++ b/xen/include/xen/sched.h
>> @@ -174,6 +174,7 @@ struct vcpu
>>           XEN_GUEST_HANDLE(vcpu_runstate_info_compat_t) compat;
>>       } runstate_guest; /* guest address */
>>   #endif
>> +    int              new_state;
> 
> I realize its counterpart (wrongly) is plain int in the public
> interface - I think it should be unsigned int here and uint32_t
> there. I'm pondering whether to do a swipe across all public
> headers to replace all uses of plain int (and alike) with
> fixed width types.

The list for cleanups is becoming longer...

So are you fine with me not changing anything in this regard right now?


Juergen

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