Re: [Xen-devel] [PATCH v1 1/3] xen:rtds: towards work conserving RTDS

[Meng Xu <xumengpanda@xxxxxxxxx>]

On Tue, Aug 8, 2017 at 10:57 AM, Dario Faggioli
<dario.faggioli@xxxxxxxxxx> wrote:
> On Sun, 2017-08-06 at 12:22 -0400, Meng Xu wrote:
>> Make RTDS scheduler work conserving without breaking the real-time
>> guarantees.
>>
>> VCPU model:
>> Each real-time VCPU is extended to have an extratime flag
>> and a priority_level field.
>> When a VCPU's budget is depleted in the current period,
>> if it has extratime flag set,
>> its priority_level will increase by 1 and its budget will be
>> refilled;
>> othewrise, the VCPU will be moved to the depletedq.
>>
>> Scheduling policy is modified global EDF:
>> A VCPU v1 has higher priority than another VCPU v2 if
>> (i) v1 has smaller priority_leve; or
>> (ii) v1 has the same priority_level but has a smaller deadline
>>
>> Queue management:
>> Run queue holds VCPUs with extratime flag set and VCPUs with
>> remaining budget. Run queue is sorted in increasing order of VCPUs
>> priorities.
>> Depleted queue holds VCPUs which have extratime flag cleared and
>> depleted budget.
>> Replenished queue is not modified.
>>
>> Signed-off-by: Meng Xu <mengxu@xxxxxxxxxxxxx>
>>
> This looks mostly good to me.
>
> There are only a couple of things left, in addition to the
> changlog+comment mention to how the 'spare bandwidth' distribution
> works, that we agreed upon in the other thread.
>
>> --- a/xen/common/sched_rt.c
>> +++ b/xen/common/sched_rt.c
>> @@ -245,6 +258,11 @@ static inline struct list_head *rt_replq(const
>> struct scheduler *ops)
>>      return &rt_priv(ops)->replq;
>>  }
>>
>> +static inline bool has_extratime(const struct rt_vcpu *svc)
>> +{
>> +    return (svc->flags & RTDS_extratime) ? 1 : 0;
>> +}
>> +
>>
> Cool... I like 'has_extratime()' soo much better as a name than what it
> was before! Thanks. :-)
>

:-)

>>  /*
>>   * Helper functions for manipulating the runqueue, the depleted
>> queue,
>>   * and the replenishment events queue.
>> @@ -274,6 +292,21 @@ vcpu_on_replq(const struct rt_vcpu *svc)
>>  }
>>
>>  /*
>> + * If v1 priority >= v2 priority, return value > 0
>> + * Otherwise, return value < 0
>> + */
>> +static s_time_t
>> +compare_vcpu_priority(const struct rt_vcpu *v1, const struct rt_vcpu
>> *v2)
>> +{
>> +    int prio = v2->priority_level - v1->priority_level;
>> +
>> +    if ( prio == 0 )
>> +    return v2->cur_deadline - v1->cur_deadline;
>> +
> Indentation.

Oh, sorry. Will correct it.

>
>> @@ -423,15 +459,18 @@ rt_update_deadline(s_time_t now, struct rt_vcpu
>> *svc)
>>       */
>>      svc->last_start = now;
>>      svc->cur_budget = svc->budget;
>> +    svc->priority_level = 0;
>>
>>      /* TRACE */
>>      {
>>          struct __packed {
>>              unsigned vcpu:16, dom:16;
>> +            unsigned priority_level;
>>              uint64_t cur_deadline, cur_budget;
>>          } d;
>>
> Can you please, and in this very comment, update
> tools/xentrace/xenalyze.c and tools/xentrace/formats as well, to take
> into account this new field?

Sure. Will do in the next version.

>
>> diff --git a/xen/include/public/domctl.h
>> b/xen/include/public/domctl.h
>> index 0669c31..ba5daa9 100644
>> --- a/xen/include/public/domctl.h
>> +++ b/xen/include/public/domctl.h
>> @@ -360,6 +360,9 @@ typedef struct xen_domctl_sched_credit2 {
>>  typedef struct xen_domctl_sched_rtds {
>>      uint32_t period;
>>      uint32_t budget;
>> +#define _XEN_DOMCTL_SCHED_RTDS_extratime 0
>> +#define
>> XEN_DOMCTL_SCHED_RTDS_extratime  (1U<<_XEN_DOMCTL_SCHED_RTDS_extratim
>> e)
>> +    uint32_t flags;
>>
> I'd add a one liner comment above the flag definition, as, for
> instance, how things are done in createdomain:

Sure.

How about comment:
/* Does this VCPU get extratime beyond reserved time? */

>
> struct xen_domctl_createdomain {
>     /* IN parameters */
>     uint32_t ssidref;
>     xen_domain_handle_t handle;
>  /* Is this an HVM guest (as opposed to a PVH or PV guest)? */
> #define _XEN_DOMCTL_CDF_hvm_guest     0
> #define XEN_DOMCTL_CDF_hvm_guest      (1U<<_XEN_DOMCTL_CDF_hvm_guest)
>  /* Use hardware-assisted paging if available? */
> #define _XEN_DOMCTL_CDF_hap           1
> #define XEN_DOMCTL_CDF_hap            (1U<<_XEN_DOMCTL_CDF_hap)
>
> Also, consider shortening the name (e.g., by contracting the SCHED_RTDS
> part; it does not matter if it's not 100% equal to what's in
> sched_rt.c, I think).


How about shorten it to XEN_DOMCTL_RTDS_extra or XEN_DOMCTL_RTDS_extratime?

>
> This, of course, is just my opinion, and final say belongs to
> maintainers of this public interface, which I think means 'THE REST',
> and most of them are not Cc-ed. Let me do that...

Thank you very much!

Best,

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