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

[Dario Faggioli <dario.faggioli@citrix.com>]

[-- Attachment #1.1: Type: text/plain, Size: 4721 bytes --]

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@cis.upenn.edu>
> 
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.

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

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

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

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

Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

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