Re: [Xen-devel] [PATCH RFC v1] xen:rtds: towards work conserving RTDS

[Meng Xu <mengxu@xxxxxxxxxxxxx>]

On Wed, Aug 2, 2017 at 1:46 PM, Dario Faggioli
<dario.faggioli@xxxxxxxxxx> wrote:
> Hey, Meng!
>
> It's really cool to see progress on this... There was quite a bit of
> interest in scheduling in general at the Summit in Budapest, and one
> important thing for making sure RTDS will be really useful, is for it
> to have a work conserving mode! :-)

Glad to hear that. :-)

>
> On Tue, 2017-08-01 at 14:13 -0400, Meng Xu wrote:
>> Make RTDS scheduler work conserving to utilize the idle resource,
>> without breaking the real-time guarantees.
>
> Just kill the "to utilize the idle resource". We can expect that people
>  that are interested in this commit, also know what 'work conserving'
> means. :-)

Got it. Will do.

>
>> VCPU model:
>> Each real-time VCPU is extended to have a work conserving flag
>> and a priority_level field.
>> When a VCPU's budget is depleted in the current period,
>> if it has work conserving flag set,
>> its priority_level will increase by 1 and its budget will be
>> refilled;
>> othewrise, the VCPU will be moved to the depletedq.
>>
> Mmm... Ok. But is the budget burned, while the vCPU executes at
> priority_level 1? If yes, doesn't this mean we risk having less budget
> when we get back to priority_lvevel 0?
>
> Oh, wait, maybe it's the case that, when we get back to priority_level
> 0, we also get another replenishment, is that the case? If yes, I
> actually think it's fine...

It's the latter case: the vcpu will get another replenishment when it
gets back to priority_level 0.

>
>> diff --git a/xen/common/sched_rt.c b/xen/common/sched_rt.c
>> index 39f6bee..740a712 100644
>> --- a/xen/common/sched_rt.c
>> +++ b/xen/common/sched_rt.c
>> @@ -191,6 +195,7 @@ struct rt_vcpu {
>>      /* VCPU parameters, in nanoseconds */
>>      s_time_t period;
>>      s_time_t budget;
>> +    bool_t is_work_conserving;   /* is vcpu work conserving */
>>
>>      /* VCPU current infomation in nanosecond */
>>      s_time_t cur_budget;         /* current budget */
>> @@ -201,6 +206,8 @@ struct rt_vcpu {
>>      struct rt_dom *sdom;
>>      struct vcpu *vcpu;
>>
>> +    unsigned priority_level;
>> +
>>      unsigned flags;              /* mark __RTDS_scheduled, etc.. */
>>
> So, since we've got a 'flags' field already, can the flag be one of its
> bit, instead of adding a new bool in the struct:
>
> /*
>  * RTDS_work_conserving: Can the vcpu run in the time that is
>  * not part of any real-time reservation, and would therefore
>  * be otherwise left idle?
>  */
> __RTDS_work_conserving       4
> #define RTDS_work_conserving (1<<__RTDS_work_conserving)

Thank you very much for the suggestion! I will modify based on your suggestion.

Actually, I was not very comfortable with the is_work_conserving field either.
It makes the structure verbose and mess up the struct's the cache_line
alignment.

>
>> @@ -245,6 +252,11 @@ static inline struct list_head *rt_replq(const
>> struct scheduler *ops)
>>      return &rt_priv(ops)->replq;
>>  }
>>
>> +static inline bool_t is_work_conserving(const struct rt_vcpu *svc)
>> +{
>>
> Use bool.

OK.

>
>> @@ -273,6 +285,20 @@ vcpu_on_replq(const struct rt_vcpu *svc)
>>      return !list_empty(&svc->replq_elem);
>>  }
>>
>> +/* If v1 priority >= v2 priority, return value > 0
>> + * Otherwise, return value < 0
>> + */
>>
> Comment style.

Got it. Will make it as:
/*
 * If v1 priority >= v2 priority, return value > 0
 * Otherwise, return value < 0
 */

>
> Apart from that, do you want this to return >0 if v1 should have
> priority over v2, and <0 if vice-versa, right? If yes...

Yes.

>
>> +static int
>> +compare_vcpu_priority(const struct rt_vcpu *v1, const struct rt_vcpu
>> *v2)
>> +{
>> +    if ( v1->priority_level < v2->priority_level ||
>> +         ( v1->priority_level == v2->priority_level &&
>> +             v1->cur_deadline <= v2->cur_deadline ) )
>> +            return 1;
>> +    else
>> +        return -1;
>>
>   int prio = v2->priority_level - v1->priority_level;
>
>   if ( prio == 0 )
>     return v2->cur_deadline - v1->cur_deadline;
>
>   return prio;
>
> Return type has to become s_time_t, and there's a chance that it'll
> return 0, if they are at the same level, and have the same absolute
> deadline. But I think you can deal with this in the caller.

OK. Will do.

>
>> @@ -966,8 +1001,16 @@ burn_budget(const struct scheduler *ops, struct
>> rt_vcpu *svc, s_time_t now)
>>
>>      if ( svc->cur_budget <= 0 )
>>      {
>> -        svc->cur_budget = 0;
>> -        __set_bit(__RTDS_depleted, &svc->flags);
>> +        if ( is_work_conserving(svc) )
>> +        {
>> +            svc->priority_level++;
>>
>                ASSERT(svc->priority_level <= 1);
>
>> +            svc->cur_budget = svc->budget;
>> +        }
>> +        else
>> +        {
>> +            svc->cur_budget = 0;
>> +            __set_bit(__RTDS_depleted, &svc->flags);
>> +        }
>>      }
>>
> The rest looks good to me.

Thank you very much for the review!

I will revise it and combine this patch into the series of the RTDS
work-conserving patches.
Once I receive your comments on the rest of patches, I will send
another version of this patch set.

Thanks and best regards,

Meng

-----------
Meng Xu
PhD Candidate in Computer and Information Science
University of Pennsylvania
http://www.cis.upenn.edu/~mengxu/

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel