All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v1 0/3] Towards work-conserving RTDS
@ 2017-08-06 16:22 Meng Xu
  2017-08-06 16:22 ` [PATCH v1 1/3] xen:rtds: towards work conserving RTDS Meng Xu
                   ` (3 more replies)
  0 siblings, 4 replies; 17+ messages in thread
From: Meng Xu @ 2017-08-06 16:22 UTC (permalink / raw)
  To: xen-devel
  Cc: george.dunlap, dario.faggioli, ian.jackson, xumengpanda, Meng Xu,
	wei.liu

This series of patches make RTDS scheduler work-conserving
without breaking real-time guarantees.
VCPUs with extratime flag set can get extra time
from the unreserved system resource.
System administrators can decide which VCPUs have extratime flag set.

Example:
Set the extratime bit of all VCPUs of domain 1:
# xl sched-rtds -d 1 -v all -p 10000 -b 2000 -e 1
Each VCPU of domain 1 will be guaranteed to have 2000ms every 10000ms
(if the system is schedulable).
If there is a CPU having no work to do,
domain 1's VCPUs will be scheduled onto the CPU,
even though the VCPUs have got 2000ms in 10000ms.

Clear the extra bit of all VCPUs of domain 1:
# xl sched-rtds -d 1 -v all -p 10000 -b 2000 -e 0

Set/Clear the extratime bit of one specific VCPU of domain 1:
# xl sched-rtds -d 1 -v 1 -p 10000 -b 2000 -e 1
# xl sched-rtds -d 1 -v 1 -p 10000 -b 2000 -e 0


The original design of the work-conserving RTDS was discussed in
https://www.mail-archive.com/xen-devel@lists.xen.org/msg77150.html

The series of patch can be found at github:
https://github.com/PennPanda/RT-Xen
under the branch:
xenbits/rtds/work-conserving-v1

Changes from RFC v1
Merge changes in sched_rt.c into one patch;
Minor change in variable name and comments.

Signed-off-by: Meng Xu <mengxu@cis.upenn.edu>

[PATCH v1 1/3] xen:rtds: towards work conserving RTDS
[PATCH v1 2/3] libxl: enable per-VCPU extratime flag for RTDS
[PATCH v1 3/3] xl: enable per-VCPU extratime flag for RTDS

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH v1 1/3] xen:rtds: towards work conserving RTDS
  2017-08-06 16:22 [PATCH v1 0/3] Towards work-conserving RTDS Meng Xu
@ 2017-08-06 16:22 ` Meng Xu
  2017-08-08 14:57   ` Dario Faggioli
  2017-08-06 16:22 ` [PATCH v1 2/3] libxl: enable per-VCPU extratime flag for RTDS Meng Xu
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 17+ messages in thread
From: Meng Xu @ 2017-08-06 16:22 UTC (permalink / raw)
  To: xen-devel
  Cc: george.dunlap, dario.faggioli, ian.jackson, xumengpanda, Meng Xu,
	wei.liu

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>

---
Changes from RFC v1
Rewording comments and commit message
Remove is_work_conserving field from rt_vcpu structure
Use one bit in VCPU's flag to indicate if a VCPU will have extra time
Correct comments style
---
 xen/common/sched_rt.c       | 90 ++++++++++++++++++++++++++++++++++++++-------
 xen/include/public/domctl.h |  3 ++
 2 files changed, 79 insertions(+), 14 deletions(-)

diff --git a/xen/common/sched_rt.c b/xen/common/sched_rt.c
index 39f6bee..4e048b9 100644
--- a/xen/common/sched_rt.c
+++ b/xen/common/sched_rt.c
@@ -49,13 +49,15 @@
  * A PCPU is feasible if the VCPU can run on this PCPU and (the PCPU is idle or
  * has a lower-priority VCPU running on it.)
  *
- * Each VCPU has a dedicated period and budget.
+ * Each VCPU has a dedicated period, budget and a extratime flag
  * The deadline of a VCPU is at the end of each period;
  * A VCPU has its budget replenished at the beginning of each period;
  * While scheduled, a VCPU burns its budget.
  * The VCPU needs to finish its budget before its deadline in each period;
  * The VCPU discards its unused budget at the end of each period.
- * If a VCPU runs out of budget in a period, it has to wait until next period.
+ * When a VCPU runs out of budget in a period, if its extratime flag is set,
+ * the VCPU increases its priority_level by 1 and refills its budget; otherwise,
+ * it has to wait until next period.
  *
  * Each VCPU is implemented as a deferable server.
  * When a VCPU has a task running on it, its budget is continuously burned;
@@ -63,7 +65,8 @@
  *
  * Queue scheme:
  * A global runqueue and a global depletedqueue for each CPU pool.
- * The runqueue holds all runnable VCPUs with budget, sorted by deadline;
+ * The runqueue holds all runnable VCPUs with budget,
+ * sorted by priority_level and deadline;
  * The depletedqueue holds all VCPUs without budget, unsorted;
  *
  * Note: cpumask and cpupool is supported.
@@ -151,6 +154,14 @@
 #define RTDS_depleted (1<<__RTDS_depleted)
 
 /*
+ * RTDS_extratime: Can the vcpu run in the time that is
+ * not part of any real-time reservation, and would therefore
+ * be otherwise left idle?
+ */
+#define __RTDS_extratime    4
+#define RTDS_extratime (1<<__RTDS_extratime)
+
+/*
  * rt tracing events ("only" 512 available!). Check
  * include/public/trace.h for more details.
  */
@@ -201,6 +212,8 @@ struct rt_vcpu {
     struct rt_dom *sdom;
     struct vcpu *vcpu;
 
+    unsigned priority_level;
+
     unsigned flags;              /* mark __RTDS_scheduled, etc.. */
 };
 
@@ -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;
+}
+
 /*
  * 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;
+
+    return prio;
+}
+
+/*
  * Debug related code, dump vcpu/cpu information
  */
 static void
@@ -303,6 +336,7 @@ rt_dump_vcpu(const struct scheduler *ops, const struct rt_vcpu *svc)
     cpulist_scnprintf(keyhandler_scratch, sizeof(keyhandler_scratch), mask);
     printk("[%5d.%-2u] cpu %u, (%"PRI_stime", %"PRI_stime"),"
            " cur_b=%"PRI_stime" cur_d=%"PRI_stime" last_start=%"PRI_stime"\n"
+           " \t\t priority_level=%d has_extratime=%d\n"
            " \t\t onQ=%d runnable=%d flags=%x effective hard_affinity=%s\n",
             svc->vcpu->domain->domain_id,
             svc->vcpu->vcpu_id,
@@ -312,6 +346,8 @@ rt_dump_vcpu(const struct scheduler *ops, const struct rt_vcpu *svc)
             svc->cur_budget,
             svc->cur_deadline,
             svc->last_start,
+            svc->priority_level,
+            has_extratime(svc),
             vcpu_on_q(svc),
             vcpu_runnable(svc->vcpu),
             svc->flags,
@@ -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;
         d.dom = svc->vcpu->domain->domain_id;
         d.vcpu = svc->vcpu->vcpu_id;
+        d.priority_level = svc->priority_level;
         d.cur_deadline = (uint64_t) svc->cur_deadline;
         d.cur_budget = (uint64_t) svc->cur_budget;
         trace_var(TRC_RTDS_BUDGET_REPLENISH, 1,
@@ -454,7 +493,7 @@ rt_update_deadline(s_time_t now, struct rt_vcpu *svc)
  * cases, if the vcpu with the earliest deadline is what we
  * are dealing with).
  */
-static inline bool_t
+static inline bool
 deadline_queue_remove(struct list_head *queue, struct list_head *elem)
 {
     int pos = 0;
@@ -466,7 +505,7 @@ deadline_queue_remove(struct list_head *queue, struct list_head *elem)
     return !pos;
 }
 
-static inline bool_t
+static inline bool
 deadline_queue_insert(struct rt_vcpu * (*qelem)(struct list_head *),
                       struct rt_vcpu *svc, struct list_head *elem,
                       struct list_head *queue)
@@ -477,7 +516,7 @@ deadline_queue_insert(struct rt_vcpu * (*qelem)(struct list_head *),
     list_for_each ( iter, queue )
     {
         struct rt_vcpu * iter_svc = (*qelem)(iter);
-        if ( svc->cur_deadline <= iter_svc->cur_deadline )
+        if ( compare_vcpu_priority(svc, iter_svc) > 0 )
             break;
         pos++;
     }
@@ -537,8 +576,9 @@ runq_insert(const struct scheduler *ops, struct rt_vcpu *svc)
     ASSERT( !vcpu_on_q(svc) );
     ASSERT( vcpu_on_replq(svc) );
 
-    /* add svc to runq if svc still has budget */
-    if ( svc->cur_budget > 0 )
+    /* add svc to runq if svc still has budget or its extratime is set */
+    if ( svc->cur_budget > 0 ||
+         has_extratime(svc) )
         deadline_runq_insert(svc, &svc->q_elem, runq);
     else
         list_add(&svc->q_elem, &prv->depletedq);
@@ -857,6 +897,8 @@ rt_alloc_vdata(const struct scheduler *ops, struct vcpu *vc, void *dd)
     svc->vcpu = vc;
     svc->last_start = 0;
 
+    __set_bit(__RTDS_extratime, &svc->flags);
+    svc->priority_level = 0;
     svc->period = RTDS_DEFAULT_PERIOD;
     if ( !is_idle_vcpu(vc) )
         svc->budget = RTDS_DEFAULT_BUDGET;
@@ -966,8 +1008,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 ( has_extratime(svc) )
+        {
+            svc->priority_level++;
+            svc->cur_budget = svc->budget;
+        }
+        else
+        {
+            svc->cur_budget = 0;
+            __set_bit(__RTDS_depleted, &svc->flags);
+        }
     }
 
     /* TRACE */
@@ -976,11 +1026,15 @@ burn_budget(const struct scheduler *ops, struct rt_vcpu *svc, s_time_t now)
             unsigned vcpu:16, dom:16;
             uint64_t cur_budget;
             int delta;
+            unsigned priority_level;
+            bool has_extratime;
         } d;
         d.dom = svc->vcpu->domain->domain_id;
         d.vcpu = svc->vcpu->vcpu_id;
         d.cur_budget = (uint64_t) svc->cur_budget;
         d.delta = delta;
+        d.priority_level = svc->priority_level;
+        d.has_extratime = svc->flags & RTDS_extratime;
         trace_var(TRC_RTDS_BUDGET_BURN, 1,
                   sizeof(d),
                   (unsigned char *) &d);
@@ -1088,7 +1142,7 @@ rt_schedule(const struct scheduler *ops, s_time_t now, bool_t tasklet_work_sched
              vcpu_runnable(current) &&
              scurr->cur_budget > 0 &&
              ( is_idle_vcpu(snext->vcpu) ||
-               scurr->cur_deadline <= snext->cur_deadline ) )
+               compare_vcpu_priority(scurr, snext) > 0 ) )
             snext = scurr;
     }
 
@@ -1198,13 +1252,13 @@ runq_tickle(const struct scheduler *ops, struct rt_vcpu *new)
         }
         iter_svc = rt_vcpu(iter_vc);
         if ( latest_deadline_vcpu == NULL ||
-             iter_svc->cur_deadline > latest_deadline_vcpu->cur_deadline )
+             compare_vcpu_priority(iter_svc, latest_deadline_vcpu) < 0 )
             latest_deadline_vcpu = iter_svc;
     }
 
     /* 3) candicate has higher priority, kick out lowest priority vcpu */
     if ( latest_deadline_vcpu != NULL &&
-         new->cur_deadline < latest_deadline_vcpu->cur_deadline )
+         compare_vcpu_priority(latest_deadline_vcpu, new) < 0 )
     {
         SCHED_STAT_CRANK(tickled_busy_cpu);
         cpu_to_tickle = latest_deadline_vcpu->vcpu->processor;
@@ -1395,6 +1449,10 @@ rt_dom_cntl(
                 svc = rt_vcpu(d->vcpu[local_sched.vcpuid]);
                 local_sched.u.rtds.budget = svc->budget / MICROSECS(1);
                 local_sched.u.rtds.period = svc->period / MICROSECS(1);
+                if ( has_extratime(svc) )
+                    local_sched.u.rtds.flags |= XEN_DOMCTL_SCHED_RTDS_extratime;
+                else
+                    local_sched.u.rtds.flags &= ~XEN_DOMCTL_SCHED_RTDS_extratime;
                 spin_unlock_irqrestore(&prv->lock, flags);
 
                 if ( copy_to_guest_offset(op->u.v.vcpus, index,
@@ -1419,6 +1477,10 @@ rt_dom_cntl(
                 svc = rt_vcpu(d->vcpu[local_sched.vcpuid]);
                 svc->period = period;
                 svc->budget = budget;
+                if ( local_sched.u.rtds.flags & XEN_DOMCTL_SCHED_RTDS_extratime )
+                    __set_bit(__RTDS_extratime, &svc->flags);
+                else
+                    __clear_bit(__RTDS_extratime, &svc->flags);
                 spin_unlock_irqrestore(&prv->lock, flags);
             }
             /* Process a most 64 vCPUs without checking for preemptions. */
@@ -1493,7 +1555,7 @@ static void repl_timer_handler(void *data){
         {
             struct rt_vcpu *next_on_runq = q_elem(runq->next);
 
-            if ( svc->cur_deadline > next_on_runq->cur_deadline )
+            if ( compare_vcpu_priority(svc, next_on_runq) < 0 )
                 runq_tickle(ops, next_on_runq);
         }
         else if ( __test_and_clear_bit(__RTDS_depleted, &svc->flags) &&
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_extratime)
+    uint32_t flags;
 } xen_domctl_sched_rtds_t;
 
 typedef struct xen_domctl_schedparam_vcpu {
-- 
1.9.1


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

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v1 2/3] libxl: enable per-VCPU extratime flag for RTDS
  2017-08-06 16:22 [PATCH v1 0/3] Towards work-conserving RTDS Meng Xu
  2017-08-06 16:22 ` [PATCH v1 1/3] xen:rtds: towards work conserving RTDS Meng Xu
@ 2017-08-06 16:22 ` Meng Xu
  2017-08-08 15:37   ` Dario Faggioli
  2017-08-06 16:22 ` [PATCH v1 3/3] xl: " Meng Xu
  2017-08-08 22:54 ` [PATCH v1 0/3] Towards work-conserving RTDS Dario Faggioli
  3 siblings, 1 reply; 17+ messages in thread
From: Meng Xu @ 2017-08-06 16:22 UTC (permalink / raw)
  To: xen-devel
  Cc: george.dunlap, dario.faggioli, ian.jackson, xumengpanda, Meng Xu,
	wei.liu

Modify libxl_vcpu_sched_params_get/set and sched_rtds_vcpu_get/set
functions to support per-VCPU extratime flag

Signed-off-by: Meng Xu <mengxu@cis.upenn.edu>

---
Changes from RFC v1
Change work_conserving flag to extratime flag
---
 tools/libxl/libxl_sched.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/tools/libxl/libxl_sched.c b/tools/libxl/libxl_sched.c
index faa604e..4ebed96 100644
--- a/tools/libxl/libxl_sched.c
+++ b/tools/libxl/libxl_sched.c
@@ -558,6 +558,10 @@ static int sched_rtds_vcpu_get_all(libxl__gc *gc, uint32_t domid,
     for (i = 0; i < num_vcpus; i++) {
         scinfo->vcpus[i].period = vcpus[i].u.rtds.period;
         scinfo->vcpus[i].budget = vcpus[i].u.rtds.budget;
+        if ( vcpus[i].u.rtds.flags & XEN_DOMCTL_SCHED_RTDS_extratime )
+           scinfo->vcpus[i].extratime = 1;
+        else
+           scinfo->vcpus[i].extratime = 0;
         scinfo->vcpus[i].vcpuid = vcpus[i].vcpuid;
     }
     rc = 0;
@@ -607,6 +611,10 @@ static int sched_rtds_vcpu_set(libxl__gc *gc, uint32_t domid,
         vcpus[i].vcpuid = scinfo->vcpus[i].vcpuid;
         vcpus[i].u.rtds.period = scinfo->vcpus[i].period;
         vcpus[i].u.rtds.budget = scinfo->vcpus[i].budget;
+        if ( scinfo->vcpus[i].extratime )
+            vcpus[i].u.rtds.flags |= XEN_DOMCTL_SCHED_RTDS_extratime;
+        else
+            vcpus[i].u.rtds.flags &= ~XEN_DOMCTL_SCHED_RTDS_extratime;
     }
 
     r = xc_sched_rtds_vcpu_set(CTX->xch, domid,
@@ -655,6 +663,10 @@ static int sched_rtds_vcpu_set_all(libxl__gc *gc, uint32_t domid,
         vcpus[i].vcpuid = i;
         vcpus[i].u.rtds.period = scinfo->vcpus[0].period;
         vcpus[i].u.rtds.budget = scinfo->vcpus[0].budget;
+        if ( scinfo->vcpus[0].extratime )
+            vcpus[i].u.rtds.flags |= XEN_DOMCTL_SCHED_RTDS_extratime;
+        else
+            vcpus[i].u.rtds.flags &= ~XEN_DOMCTL_SCHED_RTDS_extratime;
     }
 
     r = xc_sched_rtds_vcpu_set(CTX->xch, domid,
-- 
1.9.1


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

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v1 3/3] xl: enable per-VCPU extratime flag for RTDS
  2017-08-06 16:22 [PATCH v1 0/3] Towards work-conserving RTDS Meng Xu
  2017-08-06 16:22 ` [PATCH v1 1/3] xen:rtds: towards work conserving RTDS Meng Xu
  2017-08-06 16:22 ` [PATCH v1 2/3] libxl: enable per-VCPU extratime flag for RTDS Meng Xu
@ 2017-08-06 16:22 ` Meng Xu
  2017-08-07  2:43   ` Meng Xu
  2017-08-08 22:54 ` [PATCH v1 0/3] Towards work-conserving RTDS Dario Faggioli
  3 siblings, 1 reply; 17+ messages in thread
From: Meng Xu @ 2017-08-06 16:22 UTC (permalink / raw)
  To: xen-devel
  Cc: george.dunlap, dario.faggioli, ian.jackson, xumengpanda, Meng Xu,
	wei.liu

Change main_sched_rtds and related output functions to support
per-VCPU extratime flag.

Signed-off-by: Meng Xu <mengxu@cis.upenn.edu>

---
Changes from RFC v1
Changes work_conserving flag to extratime flag
---
 tools/xl/xl_cmdtable.c |  3 ++-
 tools/xl/xl_sched.c    | 56 ++++++++++++++++++++++++++++++++++----------------
 2 files changed, 40 insertions(+), 19 deletions(-)

diff --git a/tools/xl/xl_cmdtable.c b/tools/xl/xl_cmdtable.c
index 2c71a9f..88933a4 100644
--- a/tools/xl/xl_cmdtable.c
+++ b/tools/xl/xl_cmdtable.c
@@ -272,12 +272,13 @@ struct cmd_spec cmd_table[] = {
     { "sched-rtds",
       &main_sched_rtds, 0, 1,
       "Get/set rtds scheduler parameters",
-      "[-d <Domain> [-v[=VCPUID/all]] [-p[=PERIOD]] [-b[=BUDGET]]]",
+      "[-d <Domain> [-v[=VCPUID/all]] [-p[=PERIOD]] [-b[=BUDGET]] [-e[=EXTRATIME]]]",
       "-d DOMAIN, --domain=DOMAIN     Domain to modify\n"
       "-v VCPUID/all, --vcpuid=VCPUID/all    VCPU to modify or output;\n"
       "               Using '-v all' to modify/output all vcpus\n"
       "-p PERIOD, --period=PERIOD     Period (us)\n"
       "-b BUDGET, --budget=BUDGET     Budget (us)\n"
+      "-e EXTRATIME, --extratime=EXTRATIME EXTRATIME (1=yes, 0=no)\n"
     },
     { "domid",
       &main_domid, 0, 0,
diff --git a/tools/xl/xl_sched.c b/tools/xl/xl_sched.c
index 85722fe..5138012 100644
--- a/tools/xl/xl_sched.c
+++ b/tools/xl/xl_sched.c
@@ -251,7 +251,7 @@ static int sched_rtds_domain_output(
     libxl_domain_sched_params scinfo;
 
     if (domid < 0) {
-        printf("%-33s %4s %9s %9s\n", "Name", "ID", "Period", "Budget");
+        printf("%-33s %4s %9s %9s %10s\n", "Name", "ID", "Period", "Budget", "Extra time");
         return 0;
     }
 
@@ -262,11 +262,12 @@ static int sched_rtds_domain_output(
     }
 
     domname = libxl_domid_to_name(ctx, domid);
-    printf("%-33s %4d %9d %9d\n",
+    printf("%-33s %4d %9d %9d %10s\n",
         domname,
         domid,
         scinfo.period,
-        scinfo.budget);
+        scinfo.budget,
+        scinfo.extratime ? "yes" : "no");
     free(domname);
     libxl_domain_sched_params_dispose(&scinfo);
     return 0;
@@ -279,8 +280,8 @@ static int sched_rtds_vcpu_output(int domid, libxl_vcpu_sched_params *scinfo)
     int i;
 
     if (domid < 0) {
-        printf("%-33s %4s %4s %9s %9s\n", "Name", "ID",
-               "VCPU", "Period", "Budget");
+        printf("%-33s %4s %4s %9s %9s %10s\n", "Name", "ID",
+               "VCPU", "Period", "Budget", "Extra time");
         return 0;
     }
 
@@ -290,12 +291,13 @@ static int sched_rtds_vcpu_output(int domid, libxl_vcpu_sched_params *scinfo)
 
     domname = libxl_domid_to_name(ctx, domid);
     for ( i = 0; i < scinfo->num_vcpus; i++ ) {
-        printf("%-33s %4d %4d %9"PRIu32" %9"PRIu32"\n",
+        printf("%-33s %4d %4d %9"PRIu32" %9"PRIu32" %10s\n",
                domname,
                domid,
                scinfo->vcpus[i].vcpuid,
                scinfo->vcpus[i].period,
-               scinfo->vcpus[i].budget);
+               scinfo->vcpus[i].budget,
+               scinfo->vcpus[i].extratime ? "yes" : "no");
     }
     free(domname);
     return 0;
@@ -309,8 +311,8 @@ static int sched_rtds_vcpu_output_all(int domid,
     int i;
 
     if (domid < 0) {
-        printf("%-33s %4s %4s %9s %9s\n", "Name", "ID",
-               "VCPU", "Period", "Budget");
+        printf("%-33s %4s %4s %9s %9s %10s\n", "Name", "ID",
+               "VCPU", "Period", "Budget", "Extra time");
         return 0;
     }
 
@@ -321,12 +323,13 @@ static int sched_rtds_vcpu_output_all(int domid,
 
     domname = libxl_domid_to_name(ctx, domid);
     for ( i = 0; i < scinfo->num_vcpus; i++ ) {
-        printf("%-33s %4d %4d %9"PRIu32" %9"PRIu32"\n",
+        printf("%-33s %4d %4d %9"PRIu32" %9"PRIu32" %10s\n",
                domname,
                domid,
                scinfo->vcpus[i].vcpuid,
                scinfo->vcpus[i].period,
-               scinfo->vcpus[i].budget);
+               scinfo->vcpus[i].budget,
+               scinfo->vcpus[i].extratime ? "yes" : "no");
     }
     free(domname);
     return 0;
@@ -702,14 +705,18 @@ int main_sched_rtds(int argc, char **argv)
     int *vcpus = (int *)xmalloc(sizeof(int)); /* IDs of VCPUs that change */
     int *periods = (int *)xmalloc(sizeof(int)); /* period is in microsecond */
     int *budgets = (int *)xmalloc(sizeof(int)); /* budget is in microsecond */
+    bool *extratimes = (bool *)xmalloc(sizeof(bool)); /* extratime is bool */
     int v_size = 1; /* size of vcpus array */
     int p_size = 1; /* size of periods array */
     int b_size = 1; /* size of budgets array */
+    int e_size = 1; /* size of extratimes array */
     int v_index = 0; /* index in vcpus array */
     int p_index =0; /* index in periods array */
     int b_index =0; /* index for in budgets array */
+    int e_index = 0; /* index in extratimes array */
     bool opt_p = false;
     bool opt_b = false;
+    bool opt_e = false;
     bool opt_v = false;
     bool opt_all = false; /* output per-dom parameters */
     int opt, i, rc, r;
@@ -717,12 +724,13 @@ int main_sched_rtds(int argc, char **argv)
         {"domain", 1, 0, 'd'},
         {"period", 1, 0, 'p'},
         {"budget", 1, 0, 'b'},
+        {"extratime", 1, 0, 'e'},
         {"vcpuid",1, 0, 'v'},
         {"cpupool", 1, 0, 'c'},
         COMMON_LONG_OPTS
     };
 
-    SWITCH_FOREACH_OPT(opt, "d:p:b:v:c", opts, "sched-rtds", 0) {
+    SWITCH_FOREACH_OPT(opt, "d:p:b:e:v:c", opts, "sched-rtds", 0) {
     case 'd':
         dom = optarg;
         break;
@@ -746,6 +754,14 @@ int main_sched_rtds(int argc, char **argv)
         budgets[b_index++] = strtol(optarg, NULL, 10);
         opt_b = 1;
         break;
+    case 'e':
+        if (e_index >= e_size) { /* extratime array is full */
+            e_size *= 2;
+            extratimes = xrealloc(extratimes, e_size);
+        }
+        extratimes[e_index++] = strtol(optarg, NULL, 10);
+        opt_e = 1;
+        break;
     case 'v':
         if (!strcmp(optarg, "all")) { /* get or set all vcpus of a domain */
             opt_all = 1;
@@ -763,18 +779,18 @@ int main_sched_rtds(int argc, char **argv)
         break;
     }
 
-    if (cpupool && (dom || opt_p || opt_b || opt_v || opt_all)) {
+    if (cpupool && (dom || opt_p || opt_b || opt_e || opt_v || opt_all)) {
         fprintf(stderr, "Specifying a cpupool is not allowed with "
                 "other options.\n");
         r = EXIT_FAILURE;
         goto out;
     }
-    if (!dom && (opt_p || opt_b || opt_v)) {
+    if (!dom && (opt_p || opt_b || opt_e || opt_v)) {
         fprintf(stderr, "Missing parameters.\n");
         r = EXIT_FAILURE;
         goto out;
     }
-    if (dom && !opt_v && !opt_all && (opt_p || opt_b)) {
+    if (dom && !opt_v && !opt_all && (opt_p || opt_b || opt_e)) {
         fprintf(stderr, "Must specify VCPU.\n");
         r = EXIT_FAILURE;
         goto out;
@@ -785,8 +801,9 @@ int main_sched_rtds(int argc, char **argv)
         goto out;
     }
     if (((v_index > b_index) && opt_b) || ((v_index > p_index) && opt_p)
-        || p_index != b_index) {
-        fprintf(stderr, "Incorrect number of period and budget\n");
+         || ((v_index > e_index) && opt_e) || p_index != b_index
+         || p_index != e_index || b_index != e_index ) {
+        fprintf(stderr, "Incorrect number of period, budget and extratime\n");
         r = EXIT_FAILURE;
         goto out;
     }
@@ -820,7 +837,7 @@ int main_sched_rtds(int argc, char **argv)
                 r = EXIT_FAILURE;
                 goto out;
             }
-        } else if (!opt_p && !opt_b) {
+        } else if (!opt_p && !opt_b && !opt_e) {
             /* get per-vcpu rtds scheduling parameters */
             libxl_vcpu_sched_params scinfo;
             libxl_vcpu_sched_params_init(&scinfo);
@@ -852,6 +869,7 @@ int main_sched_rtds(int argc, char **argv)
                     scinfo.vcpus[i].vcpuid = vcpus[i];
                     scinfo.vcpus[i].period = periods[i];
                     scinfo.vcpus[i].budget = budgets[i];
+                    scinfo.vcpus[i].extratime = extratimes[i] ? 1 : 0;
                 }
                 rc = sched_vcpu_set(domid, &scinfo);
             } else { /* set params for all vcpus */
@@ -860,6 +878,7 @@ int main_sched_rtds(int argc, char **argv)
                                xmalloc(sizeof(libxl_sched_params));
                 scinfo.vcpus[0].period = periods[0];
                 scinfo.vcpus[0].budget = budgets[0];
+                scinfo.vcpus[0].extratime = extratimes[0] ? 1 : 0;
                 rc = sched_vcpu_set_all(domid, &scinfo);
             }
 
@@ -876,6 +895,7 @@ out:
     free(vcpus);
     free(periods);
     free(budgets);
+    free(extratimes);
     return r;
 }
 
-- 
1.9.1


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

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH v1 3/3] xl: enable per-VCPU extratime flag for RTDS
  2017-08-06 16:22 ` [PATCH v1 3/3] xl: " Meng Xu
@ 2017-08-07  2:43   ` Meng Xu
  2017-08-08 16:09     ` Dario Faggioli
  0 siblings, 1 reply; 17+ messages in thread
From: Meng Xu @ 2017-08-07  2:43 UTC (permalink / raw)
  To: xen-devel
  Cc: George Dunlap, Dario Faggioli, Ian Jackson, Meng Xu, Meng Xu, Wei Liu

On Sun, Aug 6, 2017 at 12:22 PM, Meng Xu <mengxu@cis.upenn.edu> wrote:
> Change main_sched_rtds and related output functions to support
> per-VCPU extratime flag.
>
> Signed-off-by: Meng Xu <mengxu@cis.upenn.edu>
>
> ---
> Changes from RFC v1
> Changes work_conserving flag to extratime flag
> ---
>  tools/xl/xl_cmdtable.c |  3 ++-
>  tools/xl/xl_sched.c    | 56 ++++++++++++++++++++++++++++++++++----------------
>  2 files changed, 40 insertions(+), 19 deletions(-)
>
> diff --git a/tools/xl/xl_cmdtable.c b/tools/xl/xl_cmdtable.c
> index 2c71a9f..88933a4 100644
> --- a/tools/xl/xl_cmdtable.c
> +++ b/tools/xl/xl_cmdtable.c
> @@ -272,12 +272,13 @@ struct cmd_spec cmd_table[] = {
>      { "sched-rtds",
>        &main_sched_rtds, 0, 1,
>        "Get/set rtds scheduler parameters",
> -      "[-d <Domain> [-v[=VCPUID/all]] [-p[=PERIOD]] [-b[=BUDGET]]]",
> +      "[-d <Domain> [-v[=VCPUID/all]] [-p[=PERIOD]] [-b[=BUDGET]] [-e[=EXTRATIME]]]",
>        "-d DOMAIN, --domain=DOMAIN     Domain to modify\n"
>        "-v VCPUID/all, --vcpuid=VCPUID/all    VCPU to modify or output;\n"
>        "               Using '-v all' to modify/output all vcpus\n"
>        "-p PERIOD, --period=PERIOD     Period (us)\n"
>        "-b BUDGET, --budget=BUDGET     Budget (us)\n"
> +      "-e EXTRATIME, --extratime=EXTRATIME EXTRATIME (1=yes, 0=no)\n"

Hi Dario,

I kept the EXTRATIME value for -e option because: (1) it may be more
intuitive for users; (2) it needs much less code change than the input
style that does not need EXTRATIME value.

As to (1), if users want to set some VCPUs with extratime flag set and
some with extratime flag clear, there are two types of input:
(a) xl sched-rtds -d 1 -v 1 -p 10000 -b 4000 -e 0 -v 2 -p 10000 -b
4000 -e 1 -v 5 -p 10000 -b 4000 -e 0
(b) xl sched-rtds -d 1 -v 1 -p 10000 -b 4000 -v 2 -p 10000 -b 4000 -e
1 -v 5 -p 10000 -b 4000
I felt that the style (a) is more intuitive and the input commands
have very static pattern, i.e., each vcpu must have -v -p -b -e
options set.

As to (2), if we go with -e without EXTRATIME, we will have to keep
track of the vcpu that has no -e option. I thought about this option,
we can pre-set the extratime value to false when -v option is
assigned:
    case 'v':
    ...
    extratimes[v_index]  = 0;

and set the extratimes[v_index] = 0 when -e is set.

This approach is not very neat in the code: we have to reallocate
memory for extratimes array when its size is not enough; we also have
to deal with the special case when -e is set before -v, such as the
command "xl sched-rtds -p 10000 -b 4000 -e -v 0"

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@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v1 1/3] xen:rtds: towards work conserving RTDS
  2017-08-06 16:22 ` [PATCH v1 1/3] xen:rtds: towards work conserving RTDS Meng Xu
@ 2017-08-08 14:57   ` Dario Faggioli
  2017-08-08 19:06     ` Meng Xu
  0 siblings, 1 reply; 17+ messages in thread
From: Dario Faggioli @ 2017-08-08 14:57 UTC (permalink / raw)
  To: Meng Xu, xen-devel
  Cc: Stefano Stabellini, Wei Liu, george.dunlap, Andrew Cooper,
	ian.jackson, TimDeegan, xumengpanda, Jan Beulich, wei.liu


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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v1 2/3] libxl: enable per-VCPU extratime flag for RTDS
  2017-08-06 16:22 ` [PATCH v1 2/3] libxl: enable per-VCPU extratime flag for RTDS Meng Xu
@ 2017-08-08 15:37   ` Dario Faggioli
  0 siblings, 0 replies; 17+ messages in thread
From: Dario Faggioli @ 2017-08-08 15:37 UTC (permalink / raw)
  To: Meng Xu, xen-devel; +Cc: george.dunlap, xumengpanda, ian.jackson, wei.liu


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

On Sun, 2017-08-06 at 12:22 -0400, Meng Xu wrote:
> Modify libxl_vcpu_sched_params_get/set and sched_rtds_vcpu_get/set
> functions to support per-VCPU extratime flag
> 
> Signed-off-by: Meng Xu <mengxu@cis.upenn.edu>
> 
Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com>

Of course, if the flag name in domctl.h change, this will have to
change as well. If that's the only thing changing, you can keep the
tag.

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v1 3/3] xl: enable per-VCPU extratime flag for RTDS
  2017-08-07  2:43   ` Meng Xu
@ 2017-08-08 16:09     ` Dario Faggioli
  2017-08-08 19:16       ` Meng Xu
  0 siblings, 1 reply; 17+ messages in thread
From: Dario Faggioli @ 2017-08-08 16:09 UTC (permalink / raw)
  To: Meng Xu, xen-devel; +Cc: George Dunlap, Ian Jackson, Wei Liu, Meng Xu


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

On Sun, 2017-08-06 at 22:43 -0400, Meng Xu wrote:
> On Sun, Aug 6, 2017 at 12:22 PM, Meng Xu <mengxu@cis.upenn.edu>
> wrote:
> > 
> > diff --git a/tools/xl/xl_cmdtable.c b/tools/xl/xl_cmdtable.c
> > index 2c71a9f..88933a4 100644
> > --- a/tools/xl/xl_cmdtable.c
> > +++ b/tools/xl/xl_cmdtable.c
> > @@ -272,12 +272,13 @@ struct cmd_spec cmd_table[] = {
> >      { "sched-rtds",
> >        &main_sched_rtds, 0, 1,
> >        "Get/set rtds scheduler parameters",
> > -      "[-d <Domain> [-v[=VCPUID/all]] [-p[=PERIOD]] [-
> > b[=BUDGET]]]",
> > +      "[-d <Domain> [-v[=VCPUID/all]] [-p[=PERIOD]] [-b[=BUDGET]]
> > [-e[=EXTRATIME]]]",
> >        "-d DOMAIN, --domain=DOMAIN     Domain to modify\n"
> >        "-v VCPUID/all, --vcpuid=VCPUID/all    VCPU to modify or
> > output;\n"
> >        "               Using '-v all' to modify/output all vcpus\n"
> >        "-p PERIOD, --period=PERIOD     Period (us)\n"
> >        "-b BUDGET, --budget=BUDGET     Budget (us)\n"
> > +      "-e EXTRATIME, --extratime=EXTRATIME EXTRATIME (1=yes,
> > 0=no)\n"
> 
> Hi Dario,
> 
> I kept the EXTRATIME value for -e option because: (1) it may be more
> intuitive for users; (2) it needs much less code change than the
> input
> style that does not need EXTRATIME value.
> 
I'm of the opposite view.

But again, it's tools' people views' that count. :-D

> As to (1), if users want to set some VCPUs with extratime flag set
> and
> some with extratime flag clear, there are two types of input:
> (a) xl sched-rtds -d 1 -v 1 -p 10000 -b 4000 -e 0 -v 2 -p 10000 -b
> 4000 -e 1 -v 5 -p 10000 -b 4000 -e 0
> (b) xl sched-rtds -d 1 -v 1 -p 10000 -b 4000 -v 2 -p 10000 -b 4000 -e
> 1 -v 5 -p 10000 -b 4000
> I felt that the style (a) is more intuitive and the input commands
> have very static pattern, i.e., each vcpu must have -v -p -b -e
> options set.
>
Exactly, I do think that (b) is indeed a better user interface.

For instance, what if I want to change period and budget of vcpu 1 of
domain 3, _without_ changing whether or not it can use the extra time. 

With (a), I don't think I can do that. Or at least, I'd have to either
remember or check what extratime is right now, and pass that same value
explicitly to `xl sched-rtds -d 3 -v 1 ...'.

That does not look super user-friendly to me.

> As to (2), if we go with -e without EXTRATIME, we will have to keep
> track of the vcpu that has no -e option. I thought about this option,
> we can pre-set the extratime value to false when -v option is
> assigned:
>     case 'v':
>     ...
>     extratimes[v_index]  = 0;
> 
> and set the extratimes[v_index] = 0 when -e is set.
> 
Yes, something like that. Or, even better, use its current value.

That would require calling libxl_vcpu_sched_params_get() (or, at times,
libxl_vcpu_sched_params_get_all()), which I assumed you were doing
already, while you apparently don't. Mmm...

> This approach is not very neat in the code: we have to reallocate
> memory for extratimes array when its size is not enough; we also have
> to deal with the special case when -e is set before -v, such as the
> command "xl sched-rtds -p 10000 -b 4000 -e -v 0"
> 
Err... sorry, there's code for reallocations in this patch already,
isn't this the case?

Also, it may be me, but I don't understand how this is different from
how -b and -p are dealt with.

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v1 1/3] xen:rtds: towards work conserving RTDS
  2017-08-08 14:57   ` Dario Faggioli
@ 2017-08-08 19:06     ` Meng Xu
  2017-08-08 22:52       ` Dario Faggioli
  0 siblings, 1 reply; 17+ messages in thread
From: Meng Xu @ 2017-08-08 19:06 UTC (permalink / raw)
  To: Dario Faggioli
  Cc: TimDeegan, Stefano Stabellini, Wei Liu, George Dunlap,
	Andrew Cooper, Ian Jackson, xen-devel, Jan Beulich, Wei Liu

On Tue, Aug 8, 2017 at 10:57 AM, Dario Faggioli
<dario.faggioli@citrix.com> 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@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.

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@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v1 3/3] xl: enable per-VCPU extratime flag for RTDS
  2017-08-08 16:09     ` Dario Faggioli
@ 2017-08-08 19:16       ` Meng Xu
  2017-08-08 22:24         ` Dario Faggioli
  0 siblings, 1 reply; 17+ messages in thread
From: Meng Xu @ 2017-08-08 19:16 UTC (permalink / raw)
  To: Dario Faggioli; +Cc: George Dunlap, Ian Jackson, Wei Liu, xen-devel

On Tue, Aug 8, 2017 at 9:09 AM, Dario Faggioli
<dario.faggioli@citrix.com> wrote:
> On Sun, 2017-08-06 at 22:43 -0400, Meng Xu wrote:
>> On Sun, Aug 6, 2017 at 12:22 PM, Meng Xu <mengxu@cis.upenn.edu>
>> wrote:
>> >
>> > diff --git a/tools/xl/xl_cmdtable.c b/tools/xl/xl_cmdtable.c
>> > index 2c71a9f..88933a4 100644
>> > --- a/tools/xl/xl_cmdtable.c
>> > +++ b/tools/xl/xl_cmdtable.c
>> > @@ -272,12 +272,13 @@ struct cmd_spec cmd_table[] = {
>> >      { "sched-rtds",
>> >        &main_sched_rtds, 0, 1,
>> >        "Get/set rtds scheduler parameters",
>> > -      "[-d <Domain> [-v[=VCPUID/all]] [-p[=PERIOD]] [-
>> > b[=BUDGET]]]",
>> > +      "[-d <Domain> [-v[=VCPUID/all]] [-p[=PERIOD]] [-b[=BUDGET]]
>> > [-e[=EXTRATIME]]]",
>> >        "-d DOMAIN, --domain=DOMAIN     Domain to modify\n"
>> >        "-v VCPUID/all, --vcpuid=VCPUID/all    VCPU to modify or
>> > output;\n"
>> >        "               Using '-v all' to modify/output all vcpus\n"
>> >        "-p PERIOD, --period=PERIOD     Period (us)\n"
>> >        "-b BUDGET, --budget=BUDGET     Budget (us)\n"
>> > +      "-e EXTRATIME, --extratime=EXTRATIME EXTRATIME (1=yes,
>> > 0=no)\n"
>>
>> Hi Dario,
>>
>> I kept the EXTRATIME value for -e option because: (1) it may be more
>> intuitive for users; (2) it needs much less code change than the
>> input
>> style that does not need EXTRATIME value.
>>
> I'm of the opposite view.
>
> But again, it's tools' people views' that count. :-D
>
>> As to (1), if users want to set some VCPUs with extratime flag set
>> and
>> some with extratime flag clear, there are two types of input:
>> (a) xl sched-rtds -d 1 -v 1 -p 10000 -b 4000 -e 0 -v 2 -p 10000 -b
>> 4000 -e 1 -v 5 -p 10000 -b 4000 -e 0
>> (b) xl sched-rtds -d 1 -v 1 -p 10000 -b 4000 -v 2 -p 10000 -b 4000 -e
>> 1 -v 5 -p 10000 -b 4000
>> I felt that the style (a) is more intuitive and the input commands
>> have very static pattern, i.e., each vcpu must have -v -p -b -e
>> options set.
>>
> Exactly, I do think that (b) is indeed a better user interface.
>
> For instance, what if I want to change period and budget of vcpu 1 of
> domain 3, _without_ changing whether or not it can use the extra time.

With the approach (b), what I have in my mind was: if users do not use
-e option for a vcpu index, the vcpu will have its extratime flag
cleared.
If not-setting -e option for a VCPU means using the current extratime
value for the VCPU, then how should users clear the extratime flag for
a VCPU? Are you indicating the -e option has three meanings:
If -e option is set without value, keep the extratime value unchanged;
If -e option is set with value 0, clear the extratime value;
If -e option is set with value 1, set the extratime value.


If you look at the -p and -b option for the xl sched-rtds, we will
find that users will have to first read both parameters of a VCPU even
if they only want to change the value for one parameter, either -p or
-b. We don't allow users to specify -p or -b without an input value.

By looking at how -p and -b options are handled, I leaned to the
approach (a): users must input a value for the -e option,  similar to
how  the -p and -b options are handled.

>
> With (a), I don't think I can do that. Or at least, I'd have to either
> remember or check what extratime is right now, and pass that same value
> explicitly to `xl sched-rtds -d 3 -v 1 ...'.
>
> That does not look super user-friendly to me.
>
>> As to (2), if we go with -e without EXTRATIME, we will have to keep
>> track of the vcpu that has no -e option. I thought about this option,
>> we can pre-set the extratime value to false when -v option is
>> assigned:
>>     case 'v':
>>     ...
>>     extratimes[v_index]  = 0;
>>
>> and set the extratimes[v_index] = 0 when -e is set.
>>
> Yes, something like that. Or, even better, use its current value.
>
> That would require calling libxl_vcpu_sched_params_get() (or, at times,
> libxl_vcpu_sched_params_get_all()), which I assumed you were doing
> already, while you apparently don't. Mmm...
>
>> This approach is not very neat in the code: we have to reallocate
>> memory for extratimes array when its size is not enough; we also have
>> to deal with the special case when -e is set before -v, such as the
>> command "xl sched-rtds -p 10000 -b 4000 -e -v 0"
>>
> Err... sorry, there's code for reallocations in this patch already,
> isn't this the case?
>
> Also, it may be me, but I don't understand how this is different from
> how -b and -p are dealt with.
>
> 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)

Thanks,

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@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v1 3/3] xl: enable per-VCPU extratime flag for RTDS
  2017-08-08 19:16       ` Meng Xu
@ 2017-08-08 22:24         ` Dario Faggioli
  2017-08-08 22:55           ` Meng Xu
  0 siblings, 1 reply; 17+ messages in thread
From: Dario Faggioli @ 2017-08-08 22:24 UTC (permalink / raw)
  To: Meng Xu; +Cc: George Dunlap, Ian Jackson, Wei Liu, xen-devel


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

On Tue, 2017-08-08 at 12:16 -0700, Meng Xu wrote:
> On Tue, Aug 8, 2017 at 9:09 AM, Dario Faggioli
> <dario.faggioli@citrix.com> wrote:
> > On Sun, 2017-08-06 at 22:43 -0400, Meng Xu wrote:
> > > 
> > > As to (1), if users want to set some VCPUs with extratime flag
> > > set
> > > and
> > > some with extratime flag clear, there are two types of input:
> > > (a) xl sched-rtds -d 1 -v 1 -p 10000 -b 4000 -e 0 -v 2 -p 10000
> > > -b
> > > 4000 -e 1 -v 5 -p 10000 -b 4000 -e 0
> > > (b) xl sched-rtds -d 1 -v 1 -p 10000 -b 4000 -v 2 -p 10000 -b
> > > 4000 -e
> > > 1 -v 5 -p 10000 -b 4000
> > > I felt that the style (a) is more intuitive and the input
> > > commands
> > > have very static pattern, i.e., each vcpu must have -v -p -b -e
> > > options set.
> > > 
> > 
> > Exactly, I do think that (b) is indeed a better user interface.
> > 
> With the approach (b), what I have in my mind was: if users do not
> use
> -e option for a vcpu index, the vcpu will have its extratime flag
> cleared.
> If not-setting -e option for a VCPU means using the current extratime
> value for the VCPU, then how should users clear the extratime flag
> for
> a VCPU? 
>
Yeah, I know... Well, it's an hard interface to get right.

So, I think, considering how things currently work for budget and
period, I guess I'm fine with the -e switch taking a 0/1 value.

I've checked how it was in SEDF, and it was like that in there too
(see, e.g. commit 1583cdd1fdded49698503a699c5868643051e391).

> If you look at the -p and -b option for the xl sched-rtds, we will
> find that users will have to first read both parameters of a VCPU
> even
> if they only want to change the value for one parameter, either -p or
> -b. We don't allow users to specify -p or -b without an input value.
> 
Yes. Which I now remember as something I've never really liked. But
again, it's an interface which is a bit hard to get right. And it's
certainly not this patch series' job to change it.

So, let's stick with it. Thanks for bearing with me. :-)


I now want to bring something new on the table, though: what should the
default be?

I mean, what do we expect most people to want, e.g., at domain creation
time, if they don't include an 'extratime=1' in their config file
(actually, I don't think it's even possible to do that! :-O) ?

I believe that --kind of unlikely wrt what happens in the real-time
research and papers-- most users would expect a work conserving
scheduler, unless they specify otherwise.

As in, they hopefully will enjoy being able to reserve some CPU
bandwidth in a very precise and deterministic way, for their vCPUs. But
I don't think they see as a good thing the fact that those vCPUs stops
running at some point, even if the system is idle.

Also, I think we really should set dom0 to be in extratime mode.

Therefore, I think I would set extratime as on by default in both Xen
an xl. What do you think?

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v1 1/3] xen:rtds: towards work conserving RTDS
  2017-08-08 19:06     ` Meng Xu
@ 2017-08-08 22:52       ` Dario Faggioli
  2017-08-08 22:56         ` Meng Xu
  0 siblings, 1 reply; 17+ messages in thread
From: Dario Faggioli @ 2017-08-08 22:52 UTC (permalink / raw)
  To: Meng Xu
  Cc: TimDeegan, Stefano Stabellini, Wei Liu, George Dunlap,
	Andrew Cooper, Ian Jackson, xen-devel, Jan Beulich, Wei Liu


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

On Tue, 2017-08-08 at 12:06 -0700, Meng Xu wrote:
> On Tue, Aug 8, 2017 at 10:57 AM, Dario Faggioli
> <dario.faggioli@citrix.com> wrote:
> > On Sun, 2017-08-06 at 12:22 -0400, Meng Xu wrote:
> > > 
> > > 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_extr
> > > atim
> > > 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? */
> 
'Can this vCPU execute beyond its reserved amount of 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?
> 
Personally, I'd go for XEN_DOMCTL_SCHEDRT_extra (or _extratime, or
_extrat).

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v1 0/3] Towards work-conserving RTDS
  2017-08-06 16:22 [PATCH v1 0/3] Towards work-conserving RTDS Meng Xu
                   ` (2 preceding siblings ...)
  2017-08-06 16:22 ` [PATCH v1 3/3] xl: " Meng Xu
@ 2017-08-08 22:54 ` Dario Faggioli
  3 siblings, 0 replies; 17+ messages in thread
From: Dario Faggioli @ 2017-08-08 22:54 UTC (permalink / raw)
  To: Meng Xu, xen-devel; +Cc: george.dunlap, xumengpanda, ian.jackson, wei.liu


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

On Sun, 2017-08-06 at 12:22 -0400, Meng Xu wrote:
> This series of patches make RTDS scheduler work-conserving
> without breaking real-time guarantees.
> VCPUs with extratime flag set can get extra time
> from the unreserved system resource.
> System administrators can decide which VCPUs have extratime flag set.
> 
> Example:
> Set the extratime bit of all VCPUs of domain 1:
> # xl sched-rtds -d 1 -v all -p 10000 -b 2000 -e 1
> Each VCPU of domain 1 will be guaranteed to have 2000ms every 10000ms
> (if the system is schedulable).
> If there is a CPU having no work to do,
> domain 1's VCPUs will be scheduled onto the CPU,
> even though the VCPUs have got 2000ms in 10000ms.
> 
> Clear the extra bit of all VCPUs of domain 1:
> # xl sched-rtds -d 1 -v all -p 10000 -b 2000 -e 0
> 
> Set/Clear the extratime bit of one specific VCPU of domain 1:
> # xl sched-rtds -d 1 -v 1 -p 10000 -b 2000 -e 1
> # xl sched-rtds -d 1 -v 1 -p 10000 -b 2000 -e 0
> 
Oh, BTW, can you please update 'docs/features/sched_rtds.pandoc' too?

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v1 3/3] xl: enable per-VCPU extratime flag for RTDS
  2017-08-08 22:24         ` Dario Faggioli
@ 2017-08-08 22:55           ` Meng Xu
  2017-08-09 10:32             ` Dario Faggioli
  0 siblings, 1 reply; 17+ messages in thread
From: Meng Xu @ 2017-08-08 22:55 UTC (permalink / raw)
  To: Dario Faggioli; +Cc: George Dunlap, Ian Jackson, Wei Liu, xen-devel

On Tue, Aug 8, 2017 at 3:24 PM, Dario Faggioli
<dario.faggioli@citrix.com> wrote:
> On Tue, 2017-08-08 at 12:16 -0700, Meng Xu wrote:
>> On Tue, Aug 8, 2017 at 9:09 AM, Dario Faggioli
>> <dario.faggioli@citrix.com> wrote:
>> > On Sun, 2017-08-06 at 22:43 -0400, Meng Xu wrote:
>> > >
>> > > As to (1), if users want to set some VCPUs with extratime flag
>> > > set
>> > > and
>> > > some with extratime flag clear, there are two types of input:
>> > > (a) xl sched-rtds -d 1 -v 1 -p 10000 -b 4000 -e 0 -v 2 -p 10000
>> > > -b
>> > > 4000 -e 1 -v 5 -p 10000 -b 4000 -e 0
>> > > (b) xl sched-rtds -d 1 -v 1 -p 10000 -b 4000 -v 2 -p 10000 -b
>> > > 4000 -e
>> > > 1 -v 5 -p 10000 -b 4000
>> > > I felt that the style (a) is more intuitive and the input
>> > > commands
>> > > have very static pattern, i.e., each vcpu must have -v -p -b -e
>> > > options set.
>> > >
>> >
>> > Exactly, I do think that (b) is indeed a better user interface.
>> >
>> With the approach (b), what I have in my mind was: if users do not
>> use
>> -e option for a vcpu index, the vcpu will have its extratime flag
>> cleared.
>> If not-setting -e option for a VCPU means using the current extratime
>> value for the VCPU, then how should users clear the extratime flag
>> for
>> a VCPU?
>>
> Yeah, I know... Well, it's an hard interface to get right.
>
> So, I think, considering how things currently work for budget and
> period, I guess I'm fine with the -e switch taking a 0/1 value.
>
> I've checked how it was in SEDF, and it was like that in there too
> (see, e.g. commit 1583cdd1fdded49698503a699c5868643051e391).
>
>> If you look at the -p and -b option for the xl sched-rtds, we will
>> find that users will have to first read both parameters of a VCPU
>> even
>> if they only want to change the value for one parameter, either -p or
>> -b. We don't allow users to specify -p or -b without an input value.
>>
> Yes. Which I now remember as something I've never really liked. But
> again, it's an interface which is a bit hard to get right. And it's
> certainly not this patch series' job to change it.
>
> So, let's stick with it. Thanks for bearing with me. :-)

No problem at all. :-)
I also checked the SEDF scheduler's commands while I was working on
this patch version.
I felt that keeping the same format for the -p, -b and -e options is a
better idea.

>
>
> I now want to bring something new on the table, though: what should the
> default be?
>
> I mean, what do we expect most people to want, e.g., at domain creation
> time, if they don't include an 'extratime=1' in their config file
> (actually, I don't think it's even possible to do that! :-O) ?
>
> I believe that --kind of unlikely wrt what happens in the real-time
> research and papers-- most users would expect a work conserving
> scheduler, unless they specify otherwise.
>
> As in, they hopefully will enjoy being able to reserve some CPU
> bandwidth in a very precise and deterministic way, for their vCPUs. But
> I don't think they see as a good thing the fact that those vCPUs stops
> running at some point, even if the system is idle.
>
> Also, I think we really should set dom0 to be in extratime mode.
>
> Therefore, I think I would set extratime as on by default in both Xen
> an xl. What do you think?
>

Right now, the domain is created with its VCPUs' extratime flag on. So
by default, extratime is set on in Xen.

I'm not sure what do you suggest setting the extratime flag on by default in xl?
Did you mean if users do not input -e option, the extratime flag will
be set as on?
If so, users may get confused IMHO. Some users may think not setting
-e option indicating clear the extratime flag, while some who
carefully read the instruction of the commands know the xl set the
extratime flag by default if -e option is not provided.

Thanks,

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@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v1 1/3] xen:rtds: towards work conserving RTDS
  2017-08-08 22:52       ` Dario Faggioli
@ 2017-08-08 22:56         ` Meng Xu
  0 siblings, 0 replies; 17+ messages in thread
From: Meng Xu @ 2017-08-08 22:56 UTC (permalink / raw)
  To: Dario Faggioli
  Cc: TimDeegan, Stefano Stabellini, Wei Liu, George Dunlap,
	Andrew Cooper, Ian Jackson, xen-devel, Jan Beulich, Wei Liu

On Tue, Aug 8, 2017 at 3:52 PM, Dario Faggioli
<dario.faggioli@citrix.com> wrote:
> On Tue, 2017-08-08 at 12:06 -0700, Meng Xu wrote:
>> On Tue, Aug 8, 2017 at 10:57 AM, Dario Faggioli
>> <dario.faggioli@citrix.com> wrote:
>> > On Sun, 2017-08-06 at 12:22 -0400, Meng Xu wrote:
>> > >
>> > > 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_extr
>> > > atim
>> > > 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? */
>>
> 'Can this vCPU execute beyond its reserved amount of 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?
>>
> Personally, I'd go for XEN_DOMCTL_SCHEDRT_extra (or _extratime, or
> _extrat).

OK. I can go with  XEN_DOMCTL_SCHEDRT_extra.

Thanks,

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@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v1 3/3] xl: enable per-VCPU extratime flag for RTDS
  2017-08-08 22:55           ` Meng Xu
@ 2017-08-09 10:32             ` Dario Faggioli
  2017-08-09 17:12               ` Meng Xu
  0 siblings, 1 reply; 17+ messages in thread
From: Dario Faggioli @ 2017-08-09 10:32 UTC (permalink / raw)
  To: Meng Xu; +Cc: George Dunlap, Ian Jackson, Wei Liu, xen-devel


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

On Tue, 2017-08-08 at 15:55 -0700, Meng Xu wrote:
> On Tue, Aug 8, 2017 at 3:24 PM, Dario Faggioli
> <dario.faggioli@citrix.com> wrote:
> > 
> > Therefore, I think I would set extratime as on by default in both
> > Xen
> > an xl. What do you think?
> > 
> 
> Right now, the domain is created with its VCPUs' extratime flag on.
> So
> by default, extratime is set on in Xen.
> 
> I'm not sure what do you suggest setting the extratime flag on by
> default in xl?
> Did you mean if users do not input -e option, the extratime flag will
> be set as on?
>
No, as I said, I'm ok with the requirement of -e 0/1 always having to
be present, when changing the vCPU(s) parameters with xl.

I'm talking about what happens at domain creation time.

If the default in Xen is already 'extratime on', I think we're mostly
fine.

As for xl/libxl, I think it would probably be good to take care of
extratime, e.g., in sched_rtds_domain_set() (in a similar way to how we
deal with period and budget, i.e., taking advantage of
LIBXL_DOMAIN_SCHED_PARAM_EXTRATIME_DEFAULT).

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v1 3/3] xl: enable per-VCPU extratime flag for RTDS
  2017-08-09 10:32             ` Dario Faggioli
@ 2017-08-09 17:12               ` Meng Xu
  0 siblings, 0 replies; 17+ messages in thread
From: Meng Xu @ 2017-08-09 17:12 UTC (permalink / raw)
  To: Dario Faggioli; +Cc: George Dunlap, Ian Jackson, Wei Liu, xen-devel

On Wed, Aug 9, 2017 at 3:32 AM, Dario Faggioli
<dario.faggioli@citrix.com> wrote:
> On Tue, 2017-08-08 at 15:55 -0700, Meng Xu wrote:
>> On Tue, Aug 8, 2017 at 3:24 PM, Dario Faggioli
>> <dario.faggioli@citrix.com> wrote:
>> >
>> > Therefore, I think I would set extratime as on by default in both
>> > Xen
>> > an xl. What do you think?
>> >
>>
>> Right now, the domain is created with its VCPUs' extratime flag on.
>> So
>> by default, extratime is set on in Xen.
>>
>> I'm not sure what do you suggest setting the extratime flag on by
>> default in xl?
>> Did you mean if users do not input -e option, the extratime flag will
>> be set as on?
>>
> No, as I said, I'm ok with the requirement of -e 0/1 always having to
> be present, when changing the vCPU(s) parameters with xl.
>
> I'm talking about what happens at domain creation time.
>
> If the default in Xen is already 'extratime on', I think we're mostly
> fine.

Yes. It is.

>
> As for xl/libxl, I think it would probably be good to take care of
> extratime, e.g., in sched_rtds_domain_set() (in a similar way to how we
> deal with period and budget, i.e., taking advantage of
> LIBXL_DOMAIN_SCHED_PARAM_EXTRATIME_DEFAULT).
>

OK.

Thanks,

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@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2017-08-09 17:12 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-08-06 16:22 [PATCH v1 0/3] Towards work-conserving RTDS Meng Xu
2017-08-06 16:22 ` [PATCH v1 1/3] xen:rtds: towards work conserving RTDS Meng Xu
2017-08-08 14:57   ` Dario Faggioli
2017-08-08 19:06     ` Meng Xu
2017-08-08 22:52       ` Dario Faggioli
2017-08-08 22:56         ` Meng Xu
2017-08-06 16:22 ` [PATCH v1 2/3] libxl: enable per-VCPU extratime flag for RTDS Meng Xu
2017-08-08 15:37   ` Dario Faggioli
2017-08-06 16:22 ` [PATCH v1 3/3] xl: " Meng Xu
2017-08-07  2:43   ` Meng Xu
2017-08-08 16:09     ` Dario Faggioli
2017-08-08 19:16       ` Meng Xu
2017-08-08 22:24         ` Dario Faggioli
2017-08-08 22:55           ` Meng Xu
2017-08-09 10:32             ` Dario Faggioli
2017-08-09 17:12               ` Meng Xu
2017-08-08 22:54 ` [PATCH v1 0/3] Towards work-conserving RTDS Dario Faggioli

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.