xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v9 for Xen 4.7 0/4] Enable per-VCPU parameter for RTDS
@ 2016-04-01  4:59 Chong Li
  2016-04-01  4:59 ` [PATCH v9 for Xen 4.7 1/4] xen: enable " Chong Li
                   ` (3 more replies)
  0 siblings, 4 replies; 32+ messages in thread
From: Chong Li @ 2016-04-01  4:59 UTC (permalink / raw)
  To: xen-devel
  Cc: wei.liu2, george.dunlap, dario.faggioli, ian.jackson,
	ian.campbell, mengxu, jbeulich, lichong659, dgolomb

[Goal]
The current xl sched-rtds tool can only set the VCPUs of a domain 
to the same parameter although the scheduler supports VCPUs with 
different parameters. This patchset is to enable xl sched-rtds 
tool to configure the VCPUs of a domain with different parameters.

This per-VCPU settings can be used in many scenarios. For example,
based on Dario's statement in our pervious discussion
(http://lists.xen.org/archives/html/xen-devel/2014-09/msg00423.html), 
if there are two real-time applications, which have different timing 
requirements, running in a multi-VCPU guest domain, it is beneficial 
to pin these two applications to two seperate VCPUs with different 
scheduling parameters.

What this patchset includes is a wanted and planned feature for RTDS 
scheudler(http://wiki.xenproject.org/wiki/RTDS-Based-Scheduler) in 
Xen 4.7. The interface design of the xl sched-rtds tool is based on 
Meng's previous discussion with Dario, George and Wei
(http://lists.xen.org/archives/html/xen-devel/2015-02/msg02606.html).
Basically, there are two main changes:

1) in xl, we create an array that records all VCPUs whose parameters 
are about to modify or output.

2) in libxl, we receive the array and call different xc functions to 
handle it.

3) in xen and libxc, we use 
XEN_DOMCTL_SCHEDOP_getvcpuinfo/putvcpuinfo(introduced by this
patchset) as the hypercall for per-VCPU operations(get/set method).


[Usage]
With this patchset in use, xl sched-rtds tool can:

1) show the budget and period of each VCPU of each domain, 
by using "xl sched-rtds -v all" command. An example would be like:

# xl sched-rtds -v all
Cpupool Pool-0: sched=RTDS
Name                                ID VCPU    Period    Budget
Domain-0                             0    0     10000      4000
vm1                                  1    0       300       150
vm1                                  1    1       400       200
vm1                                  1    2     10000      4000
vm1                                  1    3      1000       500
vm2                                  2    0     10000      4000
vm2                                  2    1     10000      4000

Using "xl sched-rtds" will output the default scheduling parameters
for each domain. An example would be like:

# xl sched-rtds
Cpupool Pool-0: sched=RTDS
Name                                ID    Period    Budget
Domain-0                             0     10000      4000
vm1                                  1     10000      4000
vm2                                  2     10000      4000


2) show the budget and period of each VCPU of a specific domain, 
by using, e.g., "xl sched-rtds -d vm1 -v all" command. The output 
would be like:

# xl sched-rtds -d vm1 -v all
Name                                ID VCPU    Period    Budget
vm1                                  1    0       300       150
vm1                                  1    1       400       200
vm1                                  1    2     10000      4000
vm1                                  1    3      1000       500

To show a subset of the parameters of the VCPUs of a specific domain, 
please use, e.g.,"xl sched-rtds -d vm1 -v 0 -v 3" command. 
The output would be:

# xl sched-rtds -d vm1 -v 0 -v 3
Name                                ID VCPU    Period    Budget
vm1                                  1    0       300       150
vm1                                  1    3      1000       500

Using command, e.g., "xl sched-rtds -d vm1" will output the default
scheduling parameters of vm1. An example would be like:

# xl sched-rtds -d vm1
Name                                ID    Period    Budget
vm1                                  1     10000      4000


3) Users can set the budget and period of multiple VCPUs of a 
specific domain with only one command, 
e.g., "xl sched-rtds -d vm1 -v 0 -p 100 -b 50 -v 3 -p 300 -b 150".

Users can set all VCPUs with the same parameters, by one command.
e.g., "xl sched-rtds -d vm1 -v all -p 500 -b 250".


The github repo for this patch is at:

https://github.com/Chong-Li/xen.git

And the branch name is:

vcpu-rtds

(https://github.com/Chong-Li/xen/tree/vcpu-rtds)

---



CC: <dario.faggioli@citrix.com>
CC: <george.dunlap@eu.citrix.com>
CC: <dgolomb@seas.upenn.edu>
CC: <mengxu@cis.upenn.edu>
CC: <jbeulich@suse.com>
CC: <wei.liu2@citrix.com>
CC: <lichong659@gmail.com>
CC: <ian.jackson@eu.citrix.com>
CC: <ian.campbell@eu.citrix.com>

Chong Li (4):
  xen: enable per-VCPU parameter for RTDS
  libxc: enable per-VCPU parameter for RTDS
  libxl: enable per-VCPU parameter for RTDS
  xl: enable per-VCPU parameter for RTDS

 docs/man/xl.pod.1             |  67 ++++++++
 tools/libxc/include/xenctrl.h |   8 +
 tools/libxc/xc_rt.c           |  68 ++++++++
 tools/libxl/libxl.c           | 386 +++++++++++++++++++++++++++++++++++++++---
 tools/libxl/libxl.h           |  41 +++++
 tools/libxl/libxl_types.idl   |  14 ++
 tools/libxl/xl_cmdimpl.c      | 365 +++++++++++++++++++++++++++++++++++----
 tools/libxl/xl_cmdtable.c     |   4 +-
 xen/common/sched_credit.c     |  14 +-
 xen/common/sched_credit2.c    |  13 +-
 xen/common/sched_rt.c         |  93 ++++++++--
 xen/common/schedule.c         |  15 +-
 xen/include/public/domctl.h   |  63 +++++--
 13 files changed, 1052 insertions(+), 99 deletions(-)

-- 
1.9.1


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

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

* [PATCH v9 for Xen 4.7 1/4] xen: enable per-VCPU parameter for RTDS
  2016-04-01  4:59 [PATCH v9 for Xen 4.7 0/4] Enable per-VCPU parameter for RTDS Chong Li
@ 2016-04-01  4:59 ` Chong Li
  2016-04-01  9:35   ` Jan Beulich
                     ` (2 more replies)
  2016-04-01  4:59 ` [PATCH v9 for Xen 4.7 2/4] libxc: " Chong Li
                   ` (2 subsequent siblings)
  3 siblings, 3 replies; 32+ messages in thread
From: Chong Li @ 2016-04-01  4:59 UTC (permalink / raw)
  To: xen-devel
  Cc: Chong Li, Sisu Xi, george.dunlap, dario.faggioli, Meng Xu,
	jbeulich, lichong659, dgolomb

Add XEN_DOMCTL_SCHEDOP_getvcpuinfo and _putvcpuinfo hypercalls
to independently get and set the scheduling parameters of each
vCPU of a domain.

Also fix a bug in XEN_DOMCTL_SCHEDOP_getinfo, where PERIOD and
BUDGET are not divided by MICROSECS(1) before being retruned
to the caller.

Signed-off-by: Chong Li <chong.li@wustl.edu>
Signed-off-by: Meng Xu <mengxu@cis.upenn.edu>
Signed-off-by: Sisu Xi <xisisu@gmail.com>

---
Changes on PATCH v8:
1) Move BUG fix description before SoB.
2) Remove the unuseful padding in struct xen_domctl_schedparam_vcpu.

Changes on PATCH v7:
1) A bug in case XEN_DOMCTL_SCHEDOP_getinfo (in Xen 4.6) is fixed:
The default PERIOD or BUDGET should be divided by MICROSECS(1),
before returned to upper caller.

2) In the *cntl() functions in sched_credit.c and sched_credit2.c, remove
two unhandled cases in "switch".


Changes on PATCH v6:
1) Add explain for nr_vcpus in struct xen_domctl_scheduler_op, because it is used
in both IN and OUT ways.
2) Remove the check and warning for vcpu settings with low budget or budget. Making this
feature "per-domain" or "per-operation" is one of the future work.
3) In the *cntl() functions in sched_credit.c and sched_credit2.c, change the "if-else"
structure to "switch" structure.
4) In rt_dom_cntl(), use copy_to_guest* instead of __copy_to_guest*, because the latter one
requires lock protection.


Changes on PATCH v5:
1) When processing XEN_DOMCTL_SCHEDOP_get/putvcpuinfo, we do
preemption check in a similar way to XEN_SYSCTL_pcitopoinfo


Changes on PATCH v4:
1) Add uint32_t vcpu_index to struct xen_domctl_scheduler_op.
When processing XEN_DOMCTL_SCHEDOP_get/putvcpuinfo, we call
hypercall_preemption_check in case the current hypercall lasts
too long. If we decide to preempt the current hypercall, we record
the index of the most-recent finished vcpu into the vcpu_index of
struct xen_domctl_scheduler_op. So when we resume the hypercall after
preemption, we start processing from the posion specified by vcpu_index,
and don't need to repeat the work that has already been done in the
hypercall before the preemption.
(This design is based on the do_grant_table_op() in grant_table.c)

2) Coding style changes


Changes on PATCH v3:
1) Remove struct xen_domctl_schedparam_t. 

2) Change struct xen_domctl_scheduler_op.

3) Check if period/budget is within a validated range


Changes on PATCH v2:
1) Change struct xen_domctl_scheduler_op, for transferring per-vcpu parameters
between libxc and hypervisor.

2) Handler of XEN_DOMCTL_SCHEDOP_getinfo now just returns the default budget and 
period values of RTDS scheduler.

3) Handler of XEN_DOMCTL_SCHEDOP_getvcpuinfo now can return a random subset of 
the parameters of the VCPUs of a specific domain

CC: <dario.faggioli@citrix.com>
CC: <george.dunlap@eu.citrix.com>
CC: <dgolomb@seas.upenn.edu>
CC: <mengxu@cis.upenn.edu>
CC: <jbeulich@suse.com>
CC: <lichong659@gmail.com>
---
 xen/common/sched_credit.c   | 14 +++----
 xen/common/sched_credit2.c  | 13 ++++---
 xen/common/sched_rt.c       | 93 ++++++++++++++++++++++++++++++++++++++-------
 xen/common/schedule.c       | 15 ++++++--
 xen/include/public/domctl.h | 63 ++++++++++++++++++++++++------
 5 files changed, 156 insertions(+), 42 deletions(-)

diff --git a/xen/common/sched_credit.c b/xen/common/sched_credit.c
index 305889a..e5d15d8 100644
--- a/xen/common/sched_credit.c
+++ b/xen/common/sched_credit.c
@@ -1080,15 +1080,13 @@ csched_dom_cntl(
      * lock. Runq lock not needed anywhere in here. */
     spin_lock_irqsave(&prv->lock, flags);
 
-    if ( op->cmd == XEN_DOMCTL_SCHEDOP_getinfo )
+    switch ( op->cmd )
     {
+    case XEN_DOMCTL_SCHEDOP_getinfo:
         op->u.credit.weight = sdom->weight;
         op->u.credit.cap = sdom->cap;
-    }
-    else
-    {
-        ASSERT(op->cmd == XEN_DOMCTL_SCHEDOP_putinfo);
-
+        break;
+    case XEN_DOMCTL_SCHEDOP_putinfo:
         if ( op->u.credit.weight != 0 )
         {
             if ( !list_empty(&sdom->active_sdom_elem) )
@@ -1101,7 +1099,9 @@ csched_dom_cntl(
 
         if ( op->u.credit.cap != (uint16_t)~0U )
             sdom->cap = op->u.credit.cap;
-
+        break;
+    default:
+        return -EINVAL;
     }
 
     spin_unlock_irqrestore(&prv->lock, flags);
diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c
index 7ddad38..d48ed5a 100644
--- a/xen/common/sched_credit2.c
+++ b/xen/common/sched_credit2.c
@@ -1421,14 +1421,12 @@ csched2_dom_cntl(
      * runq lock to update csvcs. */
     spin_lock_irqsave(&prv->lock, flags);
 
-    if ( op->cmd == XEN_DOMCTL_SCHEDOP_getinfo )
+    switch ( op->cmd )
     {
+    case XEN_DOMCTL_SCHEDOP_getinfo:
         op->u.credit2.weight = sdom->weight;
-    }
-    else
-    {
-        ASSERT(op->cmd == XEN_DOMCTL_SCHEDOP_putinfo);
-
+        break;
+    case XEN_DOMCTL_SCHEDOP_putinfo:
         if ( op->u.credit2.weight != 0 )
         {
             struct vcpu *v;
@@ -1457,6 +1455,9 @@ csched2_dom_cntl(
                 vcpu_schedule_unlock(lock, svc->vcpu);
             }
         }
+        break;
+    default:
+        return -EINVAL;
     }
 
     spin_unlock_irqrestore(&prv->lock, flags);
diff --git a/xen/common/sched_rt.c b/xen/common/sched_rt.c
index d98bfb6..321b0a5 100644
--- a/xen/common/sched_rt.c
+++ b/xen/common/sched_rt.c
@@ -90,6 +90,22 @@
 #define RTDS_DEFAULT_PERIOD     (MICROSECS(10000))
 #define RTDS_DEFAULT_BUDGET     (MICROSECS(4000))
 
+/*
+ * Max period: max delta of time type, because period is added to the time
+ * a vcpu activates, so this must not overflow.
+ * Min period: 10 us, considering the scheduling overhead (when period is
+ * too low, scheduling is invoked too frequently, causing high overhead).
+ */
+#define RTDS_MAX_PERIOD     (STIME_DELTA_MAX)
+#define RTDS_MIN_PERIOD     (MICROSECS(10))
+
+/*
+ * Min budget: 10 us, considering the scheduling overhead (when budget is
+ * consumed too fast, scheduling is invoked too frequently, causing
+ * high overhead).
+ */
+#define RTDS_MIN_BUDGET     (MICROSECS(10))
+
 #define UPDATE_LIMIT_SHIFT      10
 
 /*
@@ -1266,24 +1282,16 @@ rt_dom_cntl(
     struct vcpu *v;
     unsigned long flags;
     int rc = 0;
+    xen_domctl_schedparam_vcpu_t local_sched;
+    s_time_t period, budget;
+    uint32_t index = 0;
 
     switch ( op->cmd )
     {
     case XEN_DOMCTL_SCHEDOP_getinfo:
-        if ( d->max_vcpus > 0 )
-        {
-            spin_lock_irqsave(&prv->lock, flags);
-            svc = rt_vcpu(d->vcpu[0]);
-            op->u.rtds.period = svc->period / MICROSECS(1);
-            op->u.rtds.budget = svc->budget / MICROSECS(1);
-            spin_unlock_irqrestore(&prv->lock, flags);
-        }
-        else
-        {
-            /* If we don't have vcpus yet, let's just return the defaults. */
-            op->u.rtds.period = RTDS_DEFAULT_PERIOD;
-            op->u.rtds.budget = RTDS_DEFAULT_BUDGET;
-        }
+        /* Return the default parameters. */
+        op->u.rtds.period = RTDS_DEFAULT_PERIOD / MICROSECS(1);
+        op->u.rtds.budget = RTDS_DEFAULT_BUDGET / MICROSECS(1);
         break;
     case XEN_DOMCTL_SCHEDOP_putinfo:
         if ( op->u.rtds.period == 0 || op->u.rtds.budget == 0 )
@@ -1300,6 +1308,63 @@ rt_dom_cntl(
         }
         spin_unlock_irqrestore(&prv->lock, flags);
         break;
+    case XEN_DOMCTL_SCHEDOP_getvcpuinfo:
+    case XEN_DOMCTL_SCHEDOP_putvcpuinfo:
+        while ( index < op->u.v.nr_vcpus )
+        {
+            if ( copy_from_guest_offset(&local_sched,
+                                        op->u.v.vcpus, index, 1) )
+            {
+                rc = -EFAULT;
+                break;
+            }
+            if ( local_sched.vcpuid >= d->max_vcpus ||
+                 d->vcpu[local_sched.vcpuid] == NULL )
+            {
+                rc = -EINVAL;
+                break;
+            }
+
+            if ( op->cmd == XEN_DOMCTL_SCHEDOP_getvcpuinfo )
+            {
+                spin_lock_irqsave(&prv->lock, flags);
+                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);
+                spin_unlock_irqrestore(&prv->lock, flags);
+
+                if ( copy_to_guest_offset(op->u.v.vcpus, index,
+                                          &local_sched, 1) )
+                {
+                    rc = -EFAULT;
+                    break;
+                }
+            }
+            else
+            {
+                period = MICROSECS(local_sched.u.rtds.period);
+                budget = MICROSECS(local_sched.u.rtds.budget);
+                if ( period > RTDS_MAX_PERIOD || budget < RTDS_MIN_BUDGET ||
+                     budget > period || period < RTDS_MIN_PERIOD )
+                {
+                    rc = -EINVAL;
+                    break;
+                }
+
+                spin_lock_irqsave(&prv->lock, flags);
+                svc = rt_vcpu(d->vcpu[local_sched.vcpuid]);
+                svc->period = period;
+                svc->budget = budget;
+                spin_unlock_irqrestore(&prv->lock, flags);
+            }
+            /* Process a most 64 vCPUs without checking for preemptions. */
+            if ( (++index > 63) && hypercall_preempt_check() )
+                break;
+        }
+        if ( !rc )
+            /* notify upper caller how many vcpus have been processed. */
+            op->u.v.nr_vcpus = index;
+        break;
     }
 
     return rc;
diff --git a/xen/common/schedule.c b/xen/common/schedule.c
index 0627eb5..b7dee16 100644
--- a/xen/common/schedule.c
+++ b/xen/common/schedule.c
@@ -1235,11 +1235,20 @@ long sched_adjust(struct domain *d, struct xen_domctl_scheduler_op *op)
     if ( ret )
         return ret;
 
-    if ( (op->sched_id != DOM2OP(d)->sched_id) ||
-         ((op->cmd != XEN_DOMCTL_SCHEDOP_putinfo) &&
-          (op->cmd != XEN_DOMCTL_SCHEDOP_getinfo)) )
+    if ( op->sched_id != DOM2OP(d)->sched_id )
         return -EINVAL;
 
+    switch ( op->cmd )
+    {
+    case XEN_DOMCTL_SCHEDOP_putinfo:
+    case XEN_DOMCTL_SCHEDOP_getinfo:
+    case XEN_DOMCTL_SCHEDOP_putvcpuinfo:
+    case XEN_DOMCTL_SCHEDOP_getvcpuinfo:
+        break;
+    default:
+        return -EINVAL;
+    }
+
     /* NB: the pluggable scheduler code needs to take care
      * of locking by itself. */
     if ( (ret = SCHED_OP(DOM2OP(d), adjust, d, op)) == 0 )
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index 076b1ae..2457698 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -350,24 +350,63 @@ DEFINE_XEN_GUEST_HANDLE(xen_domctl_max_vcpus_t);
 #define XEN_SCHEDULER_ARINC653 7
 #define XEN_SCHEDULER_RTDS     8
 
-/* Set or get info? */
+typedef struct xen_domctl_sched_credit {
+    uint16_t weight;
+    uint16_t cap;
+} xen_domctl_sched_credit_t;
+
+typedef struct xen_domctl_sched_credit2 {
+    uint16_t weight;
+} xen_domctl_sched_credit2_t;
+
+typedef struct xen_domctl_sched_rtds {
+    uint32_t period;
+    uint32_t budget;
+} xen_domctl_sched_rtds_t;
+
+typedef struct xen_domctl_schedparam_vcpu {
+    union {
+        xen_domctl_sched_credit_t credit;
+        xen_domctl_sched_credit2_t credit2;
+        xen_domctl_sched_rtds_t rtds;
+    } u;
+    uint32_t vcpuid;
+} xen_domctl_schedparam_vcpu_t;
+DEFINE_XEN_GUEST_HANDLE(xen_domctl_schedparam_vcpu_t);
+
+/*
+ * Set or get info?
+ * For schedulers supporting per-vcpu settings (e.g., RTDS):
+ *  XEN_DOMCTL_SCHEDOP_putinfo sets params for all vcpus;
+ *  XEN_DOMCTL_SCHEDOP_getinfo gets default params;
+ *  XEN_DOMCTL_SCHEDOP_put(get)vcpuinfo sets (gets) params of vcpus;
+ *
+ * For schedulers not supporting per-vcpu settings:
+ *  XEN_DOMCTL_SCHEDOP_putinfo sets params for all vcpus;
+ *  XEN_DOMCTL_SCHEDOP_getinfo gets domain-wise params;
+ *  XEN_DOMCTL_SCHEDOP_put(get)vcpuinfo returns error;
+ */
 #define XEN_DOMCTL_SCHEDOP_putinfo 0
 #define XEN_DOMCTL_SCHEDOP_getinfo 1
+#define XEN_DOMCTL_SCHEDOP_putvcpuinfo 2
+#define XEN_DOMCTL_SCHEDOP_getvcpuinfo 3
 struct xen_domctl_scheduler_op {
     uint32_t sched_id;  /* XEN_SCHEDULER_* */
     uint32_t cmd;       /* XEN_DOMCTL_SCHEDOP_* */
+    /* IN/OUT */
     union {
-        struct xen_domctl_sched_credit {
-            uint16_t weight;
-            uint16_t cap;
-        } credit;
-        struct xen_domctl_sched_credit2 {
-            uint16_t weight;
-        } credit2;
-        struct xen_domctl_sched_rtds {
-            uint32_t period;
-            uint32_t budget;
-        } rtds;
+        xen_domctl_sched_credit_t credit;
+        xen_domctl_sched_credit2_t credit2;
+        xen_domctl_sched_rtds_t rtds;
+        struct {
+            XEN_GUEST_HANDLE_64(xen_domctl_schedparam_vcpu_t) vcpus;
+            /*
+             * IN: Number of elements in vcpus array.
+             * OUT: Number of processed elements of vcpus array.
+             */
+            uint32_t nr_vcpus;
+            uint32_t padding;
+        } v;
     } u;
 };
 typedef struct xen_domctl_scheduler_op xen_domctl_scheduler_op_t;
-- 
1.9.1


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

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

* [PATCH v9 for Xen 4.7 2/4] libxc: enable per-VCPU parameter for RTDS
  2016-04-01  4:59 [PATCH v9 for Xen 4.7 0/4] Enable per-VCPU parameter for RTDS Chong Li
  2016-04-01  4:59 ` [PATCH v9 for Xen 4.7 1/4] xen: enable " Chong Li
@ 2016-04-01  4:59 ` Chong Li
  2016-04-01  4:59 ` [PATCH v9 for Xen 4.7 3/4] libxl: " Chong Li
  2016-04-01  4:59 ` [PATCH v9 for Xen 4.7 4/4] xl: " Chong Li
  3 siblings, 0 replies; 32+ messages in thread
From: Chong Li @ 2016-04-01  4:59 UTC (permalink / raw)
  To: xen-devel
  Cc: Chong Li, wei.liu2, Sisu Xi, george.dunlap, dario.faggioli,
	Meng Xu, lichong659, dgolomb

Add xc_sched_rtds_vcpu_get/set functions to interact with
Xen to get/set a domain's per-VCPU parameters.

Signed-off-by: Chong Li <chong.li@wustl.edu>
Signed-off-by: Meng Xu <mengxu@cis.upenn.edu>
Signed-off-by: Sisu Xi <xisisu@gmail.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>
Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com>
---
Changes on PATCH v7:
1) rc in xc_sched_rtds_vcpu_set() should not be initialized to 0. It
is fixed.

Changes on PATCH v6:
1) Resolve some coding sytle issues

Changes on PATCH v5:
1) In xc_sched_rtds_vcpu_get/set, re-issueing the hypercall
if it is preempted.


Changes on PATCH v4:
1) Minor modifications on the function parameters.


Changes on PATCH v2:
1) Minor modifications due to the change of struct xen_domctl_scheduler_op.

CC: <dario.faggioli@citrix.com>
CC: <george.dunlap@eu.citrix.com>
CC: <dgolomb@seas.upenn.edu>
CC: <mengxu@cis.upenn.edu>
CC: <wei.liu2@citrix.com>
CC: <lichong659@gmail.com>
---
 tools/libxc/include/xenctrl.h |  8 +++++
 tools/libxc/xc_rt.c           | 68 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 76 insertions(+)

diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index a9e4dc1..f430ef9 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -878,6 +878,14 @@ int xc_sched_rtds_domain_set(xc_interface *xch,
 int xc_sched_rtds_domain_get(xc_interface *xch,
                             uint32_t domid,
                             struct xen_domctl_sched_rtds *sdom);
+int xc_sched_rtds_vcpu_set(xc_interface *xch,
+                           uint32_t domid,
+                           struct xen_domctl_schedparam_vcpu *vcpus,
+                           uint32_t num_vcpus);
+int xc_sched_rtds_vcpu_get(xc_interface *xch,
+                           uint32_t domid,
+                           struct xen_domctl_schedparam_vcpu *vcpus,
+                           uint32_t num_vcpus);
 
 int
 xc_sched_arinc653_schedule_set(
diff --git a/tools/libxc/xc_rt.c b/tools/libxc/xc_rt.c
index d59e5ce..221d17f 100644
--- a/tools/libxc/xc_rt.c
+++ b/tools/libxc/xc_rt.c
@@ -62,3 +62,71 @@ int xc_sched_rtds_domain_get(xc_interface *xch,
 
     return rc;
 }
+
+int xc_sched_rtds_vcpu_set(xc_interface *xch,
+                           uint32_t domid,
+                           struct xen_domctl_schedparam_vcpu *vcpus,
+                           uint32_t num_vcpus)
+{
+    int rc;
+    unsigned processed = 0;
+    DECLARE_DOMCTL;
+    DECLARE_HYPERCALL_BOUNCE(vcpus, sizeof(*vcpus) * num_vcpus,
+                             XC_HYPERCALL_BUFFER_BOUNCE_IN);
+
+    if ( xc_hypercall_bounce_pre(xch, vcpus) )
+        return -1;
+
+    domctl.cmd = XEN_DOMCTL_scheduler_op;
+    domctl.domain = (domid_t) domid;
+    domctl.u.scheduler_op.sched_id = XEN_SCHEDULER_RTDS;
+    domctl.u.scheduler_op.cmd = XEN_DOMCTL_SCHEDOP_putvcpuinfo;
+
+    while ( processed < num_vcpus )
+    {
+        domctl.u.scheduler_op.u.v.nr_vcpus = num_vcpus - processed;
+        set_xen_guest_handle_offset(domctl.u.scheduler_op.u.v.vcpus, vcpus,
+                                    processed);
+        if ( (rc = do_domctl(xch, &domctl)) != 0 )
+            break;
+        processed += domctl.u.scheduler_op.u.v.nr_vcpus;
+    }
+
+    xc_hypercall_bounce_post(xch, vcpus);
+
+    return rc;
+}
+
+int xc_sched_rtds_vcpu_get(xc_interface *xch,
+                           uint32_t domid,
+                           struct xen_domctl_schedparam_vcpu *vcpus,
+                           uint32_t num_vcpus)
+{
+    int rc;
+    unsigned processed = 0;
+    DECLARE_DOMCTL;
+    DECLARE_HYPERCALL_BOUNCE(vcpus, sizeof(*vcpus) * num_vcpus,
+                             XC_HYPERCALL_BUFFER_BOUNCE_BOTH);
+
+    if ( xc_hypercall_bounce_pre(xch, vcpus) )
+        return -1;
+
+    domctl.cmd = XEN_DOMCTL_scheduler_op;
+    domctl.domain = (domid_t) domid;
+    domctl.u.scheduler_op.sched_id = XEN_SCHEDULER_RTDS;
+    domctl.u.scheduler_op.cmd = XEN_DOMCTL_SCHEDOP_getvcpuinfo;
+
+    while ( processed < num_vcpus )
+    {
+        domctl.u.scheduler_op.u.v.nr_vcpus = num_vcpus - processed;
+        set_xen_guest_handle_offset(domctl.u.scheduler_op.u.v.vcpus, vcpus,
+                                    processed);
+        if ( (rc = do_domctl(xch, &domctl)) != 0 )
+            break;
+        processed += domctl.u.scheduler_op.u.v.nr_vcpus;
+    }
+
+    xc_hypercall_bounce_post(xch, vcpus);
+
+    return rc;
+}
-- 
1.9.1


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

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

* [PATCH v9 for Xen 4.7 3/4] libxl: enable per-VCPU parameter for RTDS
  2016-04-01  4:59 [PATCH v9 for Xen 4.7 0/4] Enable per-VCPU parameter for RTDS Chong Li
  2016-04-01  4:59 ` [PATCH v9 for Xen 4.7 1/4] xen: enable " Chong Li
  2016-04-01  4:59 ` [PATCH v9 for Xen 4.7 2/4] libxc: " Chong Li
@ 2016-04-01  4:59 ` Chong Li
  2016-04-01 12:53   ` Dario Faggioli
  2016-04-01 13:40   ` Wei Liu
  2016-04-01  4:59 ` [PATCH v9 for Xen 4.7 4/4] xl: " Chong Li
  3 siblings, 2 replies; 32+ messages in thread
From: Chong Li @ 2016-04-01  4:59 UTC (permalink / raw)
  To: xen-devel
  Cc: Chong Li, wei.liu2, Sisu Xi, george.dunlap, dario.faggioli,
	ian.jackson, ian.campbell, Meng Xu, lichong659, dgolomb

Add libxl_vcpu_sched_params_get/set and sched_rtds_vcpu_get/set
functions to support per-VCPU settings.

Signed-off-by: Chong Li <chong.li@wustl.edu>
Signed-off-by: Meng Xu <mengxu@cis.upenn.edu>
Signed-off-by: Sisu Xi <xisisu@gmail.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>
Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com>
---
Changes on PATCH v8:
1) Add libxl_vcpu_sched_params_get_all() and
sched_rtds_vcpu_get_all() to output all vcpus of a domain.

Changes on PATCH v7:
1) Fix a mistake in sched_rtds_domain_set()

Changes on PATCH v6:
1) Resolve some coding style issues
2) Change sched_rtds_validate_params()
3) Small changes for sched_rtds_vcpus_params_set(all) functions

Changes on PATCH v5:
1) Add a seperate function, sched_rtds_vcpus_params_set_all(), to set 
the parameters of all vcpus of a domain.

2) Add libxl_vcpu_sched_params_set_all() to invoke the above function.

3) Coding style changes. (I didn't find the indentation rules for function
calls with long parameters (still 4 spaces?), so I just imitated the 
indentation style of some existing functions)


Changes on PATCH v4:
1) Coding style changes


Changes on PATCH v3:
1) Add sanity check on vcpuid

2) Add comments on per-domain and per-vcpu functions for libxl
users


Changes on PATCH v2:
1) New data structure (libxl_vcpu_sched_params and libxl_sched_params) 
to help per-VCPU settings.

2) sched_rtds_vcpu_get now can return a random subset of the parameters 
of the VCPUs of a specific domain.


CC: <dario.faggioli@citrix.com>
CC: <george.dunlap@eu.citrix.com>
CC: <dgolomb@seas.upenn.edu>
CC: <mengxu@cis.upenn.edu>
CC: <wei.liu2@citrix.com>
CC: <lichong659@gmail.com>
CC: <ian.jackson@eu.citrix.com>
CC: <ian.campbell@eu.citrix.com>
---
 tools/libxl/libxl.c         | 386 +++++++++++++++++++++++++++++++++++++++++---
 tools/libxl/libxl.h         |  41 +++++
 tools/libxl/libxl_types.idl |  14 ++
 3 files changed, 420 insertions(+), 21 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index ac4d1f6..7d6ea91 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -5779,6 +5779,236 @@ static int sched_credit2_domain_set(libxl__gc *gc, uint32_t domid,
     return 0;
 }
 
+static int sched_rtds_validate_params(libxl__gc *gc, int period, int budget)
+{
+    int rc;
+
+    if (period < 1) {
+        LOG(ERROR, "Invalid VCPU period of %d (it should be >= 1)", period);
+        rc = ERROR_INVAL;
+        goto out;
+    }
+
+    if (budget < 1) {
+        LOG(ERROR, "Invalid VCPU budget of %d (it should be >= 1)", budget);
+        rc = ERROR_INVAL;
+        goto out;
+    }
+
+    if (budget > period) {
+        LOG(ERROR, "VCPU budget must be smaller than or equal to period, "
+                   "but %d > %d", budget, period);
+        rc = ERROR_INVAL;
+        goto out;
+    }
+    rc = 0;
+out:
+    return rc;
+}
+
+/* Get the RTDS scheduling parameters of vcpu(s) */
+static int sched_rtds_vcpu_get(libxl__gc *gc, uint32_t domid,
+                               libxl_vcpu_sched_params *scinfo)
+{
+    uint32_t num_vcpus;
+    int i, r, rc;
+    xc_dominfo_t info;
+    struct xen_domctl_schedparam_vcpu *vcpus;
+
+    r = xc_domain_getinfo(CTX->xch, domid, 1, &info);
+    if (r < 0) {
+        LOGE(ERROR, "getting domain info");
+        rc = ERROR_FAIL;
+        goto out;
+    }
+
+    if (scinfo->num_vcpus <= 0) {
+        rc = ERROR_INVAL;
+        goto out;
+    } else {
+        num_vcpus = scinfo->num_vcpus;
+        GCNEW_ARRAY(vcpus, num_vcpus);
+        for (i = 0; i < num_vcpus; i++) {
+            if (scinfo->vcpus[i].vcpuid < 0 ||
+                scinfo->vcpus[i].vcpuid > info.max_vcpu_id) {
+                LOG(ERROR, "VCPU index is out of range, "
+                           "valid values are within range from 0 to %d",
+                           info.max_vcpu_id);
+                rc = ERROR_INVAL;
+                goto out;
+            }
+            vcpus[i].vcpuid = scinfo->vcpus[i].vcpuid;
+        }
+    }
+
+    r = xc_sched_rtds_vcpu_get(CTX->xch, domid, vcpus, num_vcpus);
+    if (r != 0) {
+        LOGE(ERROR, "getting vcpu sched rtds");
+        rc = ERROR_FAIL;
+        goto out;
+    }
+    scinfo->sched = LIBXL_SCHEDULER_RTDS;
+    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;
+        scinfo->vcpus[i].vcpuid = vcpus[i].vcpuid;
+    }
+    rc = 0;
+out:
+    return rc;
+}
+
+/* Get the RTDS scheduling parameters of all vcpus of a domain */
+static int sched_rtds_vcpu_get_all(libxl__gc *gc, uint32_t domid,
+                                   libxl_vcpu_sched_params *scinfo)
+{
+    uint32_t num_vcpus;
+    int i, r, rc;
+    xc_dominfo_t info;
+    struct xen_domctl_schedparam_vcpu *vcpus;
+
+    r = xc_domain_getinfo(CTX->xch, domid, 1, &info);
+    if (r < 0) {
+        LOGE(ERROR, "getting domain info");
+        rc = ERROR_FAIL;
+        goto out;
+    }
+
+    if (scinfo->num_vcpus > 0) {
+        rc = ERROR_INVAL;
+        goto out;
+    } else {
+        num_vcpus = info.max_vcpu_id + 1;
+        GCNEW_ARRAY(vcpus, num_vcpus);
+        for (i = 0; i < num_vcpus; i++)
+            vcpus[i].vcpuid = i;
+    }
+
+    r = xc_sched_rtds_vcpu_get(CTX->xch, domid, vcpus, num_vcpus);
+    if (r != 0) {
+        LOGE(ERROR, "getting vcpu sched rtds");
+        rc = ERROR_FAIL;
+        goto out;
+    }
+    scinfo->sched = LIBXL_SCHEDULER_RTDS;
+    scinfo->num_vcpus = num_vcpus;
+    scinfo->vcpus = libxl__calloc(NOGC, num_vcpus,
+                                  sizeof(libxl_sched_params));
+
+    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;
+        scinfo->vcpus[i].vcpuid = vcpus[i].vcpuid;
+    }
+    rc = 0;
+out:
+    return rc;
+}
+
+/* Set the RTDS scheduling parameters of vcpu(s) */
+static int sched_rtds_vcpu_set(libxl__gc *gc, uint32_t domid,
+                               const libxl_vcpu_sched_params *scinfo)
+{
+    int r, rc;
+    int i;
+    uint16_t max_vcpuid;
+    xc_dominfo_t info;
+    struct xen_domctl_schedparam_vcpu *vcpus;
+
+    r = xc_domain_getinfo(CTX->xch, domid, 1, &info);
+    if (r < 0) {
+        LOGE(ERROR, "getting domain info");
+        rc = ERROR_FAIL;
+        goto out;
+    }
+    max_vcpuid = info.max_vcpu_id;
+
+    if (scinfo->num_vcpus <= 0) {
+        rc = ERROR_INVAL;
+        goto out;
+    }
+    for (i = 0; i < scinfo->num_vcpus; i++) {
+        if (scinfo->vcpus[i].vcpuid < 0 ||
+            scinfo->vcpus[i].vcpuid > max_vcpuid) {
+            LOG(ERROR, "Invalid VCPU %d: valid range is [0, %d]",
+                       scinfo->vcpus[i].vcpuid, max_vcpuid);
+            rc = ERROR_INVAL;
+            goto out;
+        }
+        rc = sched_rtds_validate_params(gc, scinfo->vcpus[i].period,
+                                        scinfo->vcpus[i].budget);
+        if (rc) {
+            rc = ERROR_INVAL;
+            goto out;
+        }
+    }
+    GCNEW_ARRAY(vcpus, scinfo->num_vcpus);
+    for (i = 0; i < scinfo->num_vcpus; i++) {
+        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;
+    }
+
+    r = xc_sched_rtds_vcpu_set(CTX->xch, domid,
+                               vcpus, scinfo->num_vcpus);
+    if (r != 0) {
+        LOGE(ERROR, "setting vcpu sched rtds");
+        rc = ERROR_FAIL;
+        goto out;
+    }
+    rc = 0;
+out:
+    return rc;
+}
+
+/* Set the RTDS scheduling parameters of all vcpus of a domain */
+static int sched_rtds_vcpu_set_all(libxl__gc *gc, uint32_t domid,
+                                   const libxl_vcpu_sched_params *scinfo)
+{
+    int r, rc;
+    int i;
+    uint16_t max_vcpuid;
+    xc_dominfo_t info;
+    struct xen_domctl_schedparam_vcpu *vcpus;
+    uint32_t num_vcpus;
+
+    r = xc_domain_getinfo(CTX->xch, domid, 1, &info);
+    if (r < 0) {
+        LOGE(ERROR, "getting domain info");
+        rc = ERROR_FAIL;
+        goto out;
+    }
+    max_vcpuid = info.max_vcpu_id;
+
+    if (scinfo->num_vcpus != 1) {
+        rc = ERROR_INVAL;
+        goto out;
+    }
+    if (sched_rtds_validate_params(gc, scinfo->vcpus[0].period,
+                                   scinfo->vcpus[0].budget)) {
+        rc = ERROR_INVAL;
+        goto out;
+    }
+    num_vcpus = max_vcpuid + 1;
+    GCNEW_ARRAY(vcpus, num_vcpus);
+    for (i = 0; i < num_vcpus; i++) {
+        vcpus[i].vcpuid = i;
+        vcpus[i].u.rtds.period = scinfo->vcpus[0].period;
+        vcpus[i].u.rtds.budget = scinfo->vcpus[0].budget;
+    }
+
+    r = xc_sched_rtds_vcpu_set(CTX->xch, domid,
+                               vcpus, num_vcpus);
+    if (r != 0) {
+        LOGE(ERROR, "setting vcpu sched rtds");
+        rc = ERROR_FAIL;
+        goto out;
+    }
+    rc = 0;
+out:
+    return rc;
+}
+
 static int sched_rtds_domain_get(libxl__gc *gc, uint32_t domid,
                                libxl_domain_sched_params *scinfo)
 {
@@ -5811,30 +6041,12 @@ static int sched_rtds_domain_set(libxl__gc *gc, uint32_t domid,
         LOGE(ERROR, "getting domain sched rtds");
         return ERROR_FAIL;
     }
-
-    if (scinfo->period != LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT) {
-        if (scinfo->period < 1) {
-            LOG(ERROR, "VCPU period is not set or out of range, "
-                       "valid values are larger than 1");
-            return ERROR_INVAL;
-        }
+    if (scinfo->period != LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT)
         sdom.period = scinfo->period;
-    }
-
-    if (scinfo->budget != LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT) {
-        if (scinfo->budget < 1) {
-            LOG(ERROR, "VCPU budget is not set or out of range, "
-                       "valid values are larger than 1");
-            return ERROR_INVAL;
-        }
+    if (scinfo->budget != LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT)
         sdom.budget = scinfo->budget;
-    }
-
-    if (sdom.budget > sdom.period) {
-        LOG(ERROR, "VCPU budget is larger than VCPU period, "
-                   "VCPU budget should be no larger than VCPU period");
+    if (sched_rtds_validate_params(gc, sdom.period, sdom.budget))
         return ERROR_INVAL;
-    }
 
     rc = xc_sched_rtds_domain_set(CTX->xch, domid, &sdom);
     if (rc < 0) {
@@ -5882,6 +6094,74 @@ int libxl_domain_sched_params_set(libxl_ctx *ctx, uint32_t domid,
     return ret;
 }
 
+int libxl_vcpu_sched_params_set(libxl_ctx *ctx, uint32_t domid,
+                                const libxl_vcpu_sched_params *scinfo)
+{
+    GC_INIT(ctx);
+    libxl_scheduler sched = scinfo->sched;
+    int rc;
+
+    if (sched == LIBXL_SCHEDULER_UNKNOWN)
+        sched = libxl__domain_scheduler(gc, domid);
+
+    switch (sched) {
+    case LIBXL_SCHEDULER_SEDF:
+        LOG(ERROR, "SEDF scheduler no longer available");
+        rc = ERROR_FEATURE_REMOVED;
+        break;
+    case LIBXL_SCHEDULER_CREDIT:
+    case LIBXL_SCHEDULER_CREDIT2:
+    case LIBXL_SCHEDULER_ARINC653:
+        LOG(ERROR, "per-VCPU parameter setting not supported for this scheduler");
+        rc = ERROR_INVAL;
+        break;
+    case LIBXL_SCHEDULER_RTDS:
+        rc = sched_rtds_vcpu_set(gc, domid, scinfo);
+        break;
+    default:
+        LOG(ERROR, "Unknown scheduler");
+        rc = ERROR_INVAL;
+        break;
+    }
+
+    GC_FREE;
+    return rc;
+}
+
+int libxl_vcpu_sched_params_set_all(libxl_ctx *ctx, uint32_t domid,
+                                    const libxl_vcpu_sched_params *scinfo)
+{
+    GC_INIT(ctx);
+    libxl_scheduler sched = scinfo->sched;
+    int rc;
+
+    if (sched == LIBXL_SCHEDULER_UNKNOWN)
+        sched = libxl__domain_scheduler(gc, domid);
+
+    switch (sched) {
+    case LIBXL_SCHEDULER_SEDF:
+        LOG(ERROR, "SEDF scheduler no longer available");
+        rc = ERROR_FEATURE_REMOVED;
+        break;
+    case LIBXL_SCHEDULER_CREDIT:
+    case LIBXL_SCHEDULER_CREDIT2:
+    case LIBXL_SCHEDULER_ARINC653:
+        LOG(ERROR, "per-VCPU parameter setting not supported for this scheduler");
+        rc = ERROR_INVAL;
+        break;
+    case LIBXL_SCHEDULER_RTDS:
+        rc = sched_rtds_vcpu_set_all(gc, domid, scinfo);
+        break;
+    default:
+        LOG(ERROR, "Unknown scheduler");
+        rc = ERROR_INVAL;
+        break;
+    }
+
+    GC_FREE;
+    return rc;
+}
+
 int libxl_domain_sched_params_get(libxl_ctx *ctx, uint32_t domid,
                                   libxl_domain_sched_params *scinfo)
 {
@@ -5916,6 +6196,70 @@ int libxl_domain_sched_params_get(libxl_ctx *ctx, uint32_t domid,
     return ret;
 }
 
+int libxl_vcpu_sched_params_get(libxl_ctx *ctx, uint32_t domid,
+                                libxl_vcpu_sched_params *scinfo)
+{
+    GC_INIT(ctx);
+    int rc;
+
+    scinfo->sched = libxl__domain_scheduler(gc, domid);
+
+    switch (scinfo->sched) {
+    case LIBXL_SCHEDULER_SEDF:
+        LOG(ERROR, "SEDF scheduler is no longer available");
+        rc = ERROR_FEATURE_REMOVED;
+        break;
+    case LIBXL_SCHEDULER_CREDIT:
+    case LIBXL_SCHEDULER_CREDIT2:
+    case LIBXL_SCHEDULER_ARINC653:
+        LOG(ERROR, "per-VCPU parameter getting not supported for this scheduler");
+        rc = ERROR_INVAL;
+        break;
+    case LIBXL_SCHEDULER_RTDS:
+        rc = sched_rtds_vcpu_get(gc, domid, scinfo);
+        break;
+    default:
+        LOG(ERROR, "Unknown scheduler");
+        rc = ERROR_INVAL;
+        break;
+    }
+
+    GC_FREE;
+    return rc;
+}
+
+int libxl_vcpu_sched_params_get_all(libxl_ctx *ctx, uint32_t domid,
+                                    libxl_vcpu_sched_params *scinfo)
+{
+    GC_INIT(ctx);
+    int rc;
+
+    scinfo->sched = libxl__domain_scheduler(gc, domid);
+
+    switch (scinfo->sched) {
+    case LIBXL_SCHEDULER_SEDF:
+        LOG(ERROR, "SEDF scheduler is no longer available");
+        rc = ERROR_FEATURE_REMOVED;
+        break;
+    case LIBXL_SCHEDULER_CREDIT:
+    case LIBXL_SCHEDULER_CREDIT2:
+    case LIBXL_SCHEDULER_ARINC653:
+        LOG(ERROR, "per-VCPU parameter getting not supported for this scheduler");
+        rc = ERROR_INVAL;
+        break;
+    case LIBXL_SCHEDULER_RTDS:
+        rc = sched_rtds_vcpu_get_all(gc, domid, scinfo);
+        break;
+    default:
+        LOG(ERROR, "Unknown scheduler");
+        rc = ERROR_INVAL;
+        break;
+    }
+
+    GC_FREE;
+    return rc;
+}
+
 static int libxl__domain_s3_resume(libxl__gc *gc, int domid)
 {
     int rc = 0;
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 83d7cd3..4c36a69 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -218,6 +218,17 @@
 #define LIBXL_HAVE_DEVICE_MODEL_USER 1
 
 /*
+ * libxl_vcpu_sched_params is used to store per-vcpu params.
+ */
+#define LIBXL_HAVE_VCPU_SCHED_PARAMS 1
+
+/*
+ * LIBXL_HAVE_SCHED_RTDS_VCPU_PARAMS indicates RTDS scheduler
+ * now supports per-vcpu settings.
+ */
+#define LIBXL_HAVE_SCHED_RTDS_VCPU_PARAMS 1
+
+/*
  * libxl_domain_build_info has the arm.gic_version field.
  */
 #define LIBXL_HAVE_BUILDINFO_ARM_GIC_VERSION 1
@@ -1849,11 +1860,41 @@ int libxl_sched_credit_params_set(libxl_ctx *ctx, uint32_t poolid,
 #define LIBXL_DOMAIN_SCHED_PARAM_EXTRATIME_DEFAULT -1
 #define LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT    -1
 
+/* Per-VCPU parameters */
+#define LIBXL_SCHED_PARAM_VCPU_INDEX_DEFAULT   -1
+
+/* Get the per-domain scheduling parameters.
+ * For schedulers that support per-vcpu settings (e.g., RTDS),
+ * calling *_domain_get functions will get default scheduling
+ * parameters.
+ */
 int libxl_domain_sched_params_get(libxl_ctx *ctx, uint32_t domid,
                                   libxl_domain_sched_params *params);
+
+/* Set the per-domain scheduling parameters.
+ * For schedulers that support per-vcpu settings (e.g., RTDS),
+ * calling *_domain_set functions will set all vcpus with the same
+ * scheduling parameters.
+ */
 int libxl_domain_sched_params_set(libxl_ctx *ctx, uint32_t domid,
                                   const libxl_domain_sched_params *params);
 
+/* Get the per-vcpu scheduling parameters */
+int libxl_vcpu_sched_params_get(libxl_ctx *ctx, uint32_t domid,
+                                libxl_vcpu_sched_params *params);
+
+/* Get the per-vcpu scheduling parameters of all vcpus of a domain */
+int libxl_vcpu_sched_params_get_all(libxl_ctx *ctx, uint32_t domid,
+                                    libxl_vcpu_sched_params *params);
+
+/* Set the per-vcpu scheduling parameters */
+int libxl_vcpu_sched_params_set(libxl_ctx *ctx, uint32_t domid,
+                                const libxl_vcpu_sched_params *params);
+
+/* Set the per-vcpu scheduling parameters of all vcpus of a domain */
+int libxl_vcpu_sched_params_set_all(libxl_ctx *ctx, uint32_t domid,
+                                    const libxl_vcpu_sched_params *params);
+
 int libxl_send_trigger(libxl_ctx *ctx, uint32_t domid,
                        libxl_trigger trigger, uint32_t vcpuid);
 int libxl_send_sysrq(libxl_ctx *ctx, uint32_t domid, char sysrq);
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 59b183c..d33607e 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -387,6 +387,20 @@ libxl_domain_restore_params = Struct("domain_restore_params", [
     ("stream_version", uint32, {'init_val': '1'}),
     ])
 
+libxl_sched_params = Struct("sched_params",[
+    ("vcpuid",       integer, {'init_val': 'LIBXL_SCHED_PARAM_VCPU_INDEX_DEFAULT'}),
+    ("weight",       integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_WEIGHT_DEFAULT'}),
+    ("cap",          integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_CAP_DEFAULT'}),
+    ("period",       integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT'}),
+    ("extratime",    integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_EXTRATIME_DEFAULT'}),
+    ("budget",       integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT'}),
+    ])
+
+libxl_vcpu_sched_params = Struct("vcpu_sched_params",[
+    ("sched",        libxl_scheduler),
+    ("vcpus",        Array(libxl_sched_params, "num_vcpus")),
+    ])
+
 libxl_domain_sched_params = Struct("domain_sched_params",[
     ("sched",        libxl_scheduler),
     ("weight",       integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_WEIGHT_DEFAULT'}),
-- 
1.9.1


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

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

* [PATCH v9 for Xen 4.7 4/4] xl: enable per-VCPU parameter for RTDS
  2016-04-01  4:59 [PATCH v9 for Xen 4.7 0/4] Enable per-VCPU parameter for RTDS Chong Li
                   ` (2 preceding siblings ...)
  2016-04-01  4:59 ` [PATCH v9 for Xen 4.7 3/4] libxl: " Chong Li
@ 2016-04-01  4:59 ` Chong Li
  2016-04-01 13:15   ` Dario Faggioli
  3 siblings, 1 reply; 32+ messages in thread
From: Chong Li @ 2016-04-01  4:59 UTC (permalink / raw)
  To: xen-devel
  Cc: Chong Li, wei.liu2, Sisu Xi, george.dunlap, dario.faggioli,
	Meng Xu, lichong659, dgolomb

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

Signed-off-by: Chong Li <chong.li@wustl.edu>
Signed-off-by: Meng Xu <mengxu@cis.upenn.edu>
Signed-off-by: Sisu Xi <xisisu@gmail.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>
---
Changes on PATCH v8:
1) Improve the example in xl.pod.1
2) Add sched_rtds_vcpu_output_all() and sched_vcpu_get_all()
to output all vcpus of a domain
3) Fix some coding style issues

Changes on PATCH v7:
1) Add example to xl.pod.1

Changes on PATCH v6:
1) More explain in xl.pod.1 and cmdtable.c
2) Resolve some coding sytle issues


Changes on PATCH v5:
1) Add sched_vcpu_set_all() for the cases that all vcpus of a
domain need to be changed together.


Changes on PATCH v4:
1) Coding style changes


Changes on PATCH v3:
1) Support commands, e.g., "xl sched-rtds -d vm1" to output the
default scheduling parameters


Changes on PATCH v2:
1) Remove per-domain output functions for RTDS scheduler.

2) Users now use '-v all' to specify all VCPUs.

3) Support outputting a subset of the parameters of the VCPUs 
of a specific domain.

4) When setting all VCPUs with the same parameters (by only one 
command), no per-domain function is invoked.

CC: <dario.faggioli@citrix.com>
CC: <george.dunlap@eu.citrix.com>
CC: <dgolomb@seas.upenn.edu>
CC: <mengxu@cis.upenn.edu>
CC: <wei.liu2@citrix.com>
CC: <lichong659@gmail.com>

---
 docs/man/xl.pod.1         |  67 +++++++++
 tools/libxl/xl_cmdimpl.c  | 365 +++++++++++++++++++++++++++++++++++++++++-----
 tools/libxl/xl_cmdtable.c |   4 +-
 3 files changed, 400 insertions(+), 36 deletions(-)

diff --git a/docs/man/xl.pod.1 b/docs/man/xl.pod.1
index 4d4b333..9e89e0a 100644
--- a/docs/man/xl.pod.1
+++ b/docs/man/xl.pod.1
@@ -1056,6 +1056,10 @@ B<OPTIONS>
 Specify domain for which scheduler parameters are to be modified or retrieved.
 Mandatory for modifying scheduler parameters.
 
+=item B<-v VCPUID/all>, B<--vcpuid=VCPUID/all>
+
+Specify vcpu for which scheduler parameters are to be modified or retrieved.
+
 =item B<-p PERIOD>, B<--period=PERIOD>
 
 Period of time, in microseconds, over which to replenish the budget.
@@ -1071,6 +1075,69 @@ Restrict output to domains in the specified cpupool.
 
 =back
 
+B<EXAMPLE>
+
+=over 4
+
+1) Use B<-v all> to see the budget and period of all the VCPUs of
+all the domains:
+
+    xl sched-rtds -v all
+    Cpupool Pool-0: sched=RTDS
+    Name                        ID VCPU    Period    Budget
+    Domain-0                     0    0     10000      4000
+    vm1                          1    0       300       150
+    vm1                          1    1       400       200
+    vm1                          1    2     10000      4000
+    vm1                          1    3      1000       500
+    vm2                          2    0     10000      4000
+    vm2                          2    1     10000      4000
+
+Without any arguments, it will output the default scheduing
+parameters for each domain:
+
+    xl sched-rtds
+    Cpupool Pool-0: sched=RTDS
+    Name                        ID    Period    Budget
+    Domain-0                     0     10000      4000
+    vm1                          1     10000      4000
+    vm2                          2     10000      4000
+
+
+2) Use, for instance B<-d vm1, -v all> to see the budget and
+period of all VCPUs of a specific domain (B<vm1>):
+
+    xl sched-rtds -d vm1 -v all
+    Name                        ID VCPU    Period    Budget
+    vm1                          1    0       300       150
+    vm1                          1    1       400       200
+    vm1                          1    2     10000      4000
+    vm1                          1    3      1000       500
+
+To see the parameters of a subset of the VCPUs of a domain, use:
+
+    xl sched-rtds -d vm1 -v 0 -v 3
+    Name                        ID VCPU    Period    Budget
+    vm1                          1    0       300       150
+    vm1                          1    3      1000       500
+
+If no B<-v> is speficified, the default scheduling parameter for the
+domain are shown:
+
+    xl sched-rtds -d vm1
+    Name                        ID    Period    Budget
+    vm1                          1     10000      4000
+
+
+3) Users can set the budget and period of multiple VCPUs of a
+specific domain with only one command,
+e.g., "xl sched-rtds -d vm1 -v 0 -p 100 -b 50 -v 3 -p 300 -b 150".
+
+To change the parameters of all the VCPUs of a domain, use B<-v all>,
+e.g., "xl sched-rtds -d vm1 -v all -p 500 -b 250".
+
+=back
+
 =back
 
 =head1 CPUPOOLS COMMANDS
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 7750995..dc77689 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -6159,6 +6159,72 @@ static int sched_domain_set(int domid, const libxl_domain_sched_params *scinfo)
     return 0;
 }
 
+static int sched_vcpu_get(libxl_scheduler sched, int domid,
+                          libxl_vcpu_sched_params *scinfo)
+{
+    int rc;
+
+    rc = libxl_vcpu_sched_params_get(ctx, domid, scinfo);
+    if (rc) {
+        fprintf(stderr, "libxl_vcpu_sched_params_get failed.\n");
+        exit(EXIT_FAILURE);
+    }
+    if (scinfo->sched != sched) {
+        fprintf(stderr, "libxl_vcpu_sched_params_get returned %s not %s.\n",
+                libxl_scheduler_to_string(scinfo->sched),
+                libxl_scheduler_to_string(sched));
+        return 1;
+    }
+
+    return 0;
+}
+
+static int sched_vcpu_get_all(libxl_scheduler sched, int domid,
+                              libxl_vcpu_sched_params *scinfo)
+{
+    int rc;
+
+    rc = libxl_vcpu_sched_params_get_all(ctx, domid, scinfo);
+    if (rc) {
+        fprintf(stderr, "libxl_vcpu_sched_params_get_all failed.\n");
+        exit(EXIT_FAILURE);
+    }
+    if (scinfo->sched != sched) {
+        fprintf(stderr, "libxl_vcpu_sched_params_get_all returned %s not %s.\n",
+                libxl_scheduler_to_string(scinfo->sched),
+                libxl_scheduler_to_string(sched));
+        return 1;
+    }
+
+    return 0;
+}
+
+static int sched_vcpu_set(int domid, const libxl_vcpu_sched_params *scinfo)
+{
+    int rc;
+
+    rc = libxl_vcpu_sched_params_set(ctx, domid, scinfo);
+    if (rc) {
+        fprintf(stderr, "libxl_vcpu_sched_params_set failed.\n");
+        exit(EXIT_FAILURE);
+    }
+
+    return 0;
+}
+
+static int sched_vcpu_set_all(int domid, const libxl_vcpu_sched_params *scinfo)
+{
+    int rc;
+
+    rc = libxl_vcpu_sched_params_set_all(ctx, domid, scinfo);
+    if (rc) {
+        fprintf(stderr, "libxl_vcpu_sched_params_set_all failed.\n");
+        exit(EXIT_FAILURE);
+    }
+
+    return 0;
+}
+
 static int sched_credit_params_set(int poolid, libxl_sched_credit_params *scinfo)
 {
     if (libxl_sched_credit_params_set(ctx, poolid, scinfo)) {
@@ -6278,6 +6344,66 @@ static int sched_rtds_domain_output(
     return 0;
 }
 
+static int sched_rtds_vcpu_output(int domid, libxl_vcpu_sched_params *scinfo)
+{
+    char *domname;
+    int rc = 0;
+    int i;
+
+    if (domid < 0) {
+        printf("%-33s %4s %4s %9s %9s\n", "Name", "ID",
+               "VCPU", "Period", "Budget");
+        return 0;
+    }
+
+    rc = sched_vcpu_get(LIBXL_SCHEDULER_RTDS, domid, scinfo);
+    if (rc)
+        return 1;
+
+    domname = libxl_domid_to_name(ctx, domid);
+    for ( i = 0; i < scinfo->num_vcpus; i++ ) {
+        printf("%-33s %4d %4d %9"PRIu32" %9"PRIu32"\n",
+               domname,
+               domid,
+               scinfo->vcpus[i].vcpuid,
+               scinfo->vcpus[i].period,
+               scinfo->vcpus[i].budget);
+    }
+    free(domname);
+    return 0;
+}
+
+static int sched_rtds_vcpu_output_all(int domid,
+                                      libxl_vcpu_sched_params *scinfo)
+{
+    char *domname;
+    int rc = 0;
+    int i;
+
+    if (domid < 0) {
+        printf("%-33s %4s %4s %9s %9s\n", "Name", "ID",
+               "VCPU", "Period", "Budget");
+        return 0;
+    }
+
+    scinfo->num_vcpus = 0;
+    rc = sched_vcpu_get_all(LIBXL_SCHEDULER_RTDS, domid, scinfo);
+    if (rc)
+        return 1;
+
+    domname = libxl_domid_to_name(ctx, domid);
+    for ( i = 0; i < scinfo->num_vcpus; i++ ) {
+        printf("%-33s %4d %4d %9"PRIu32" %9"PRIu32"\n",
+               domname,
+               domid,
+               scinfo->vcpus[i].vcpuid,
+               scinfo->vcpus[i].period,
+               scinfo->vcpus[i].budget);
+    }
+    free(domname);
+    return 0;
+}
+
 static int sched_rtds_pool_output(uint32_t poolid)
 {
     char *poolname;
@@ -6351,6 +6477,61 @@ static int sched_domain_output(libxl_scheduler sched, int (*output)(int),
     return 0;
 }
 
+static int sched_vcpu_output(libxl_scheduler sched,
+                             int (*output)(int, libxl_vcpu_sched_params *),
+                             int (*pooloutput)(uint32_t), const char *cpupool)
+{
+    libxl_dominfo *info;
+    libxl_cpupoolinfo *poolinfo = NULL;
+    uint32_t poolid;
+    int nb_domain, n_pools = 0, i, p;
+    int rc = 0;
+
+    if (cpupool) {
+        if (libxl_cpupool_qualifier_to_cpupoolid(ctx, cpupool, &poolid, NULL)
+            || !libxl_cpupoolid_is_valid(ctx, poolid)) {
+            fprintf(stderr, "unknown cpupool \'%s\'\n", cpupool);
+            return 1;
+        }
+    }
+
+    info = libxl_list_domain(ctx, &nb_domain);
+    if (!info) {
+        fprintf(stderr, "libxl_list_domain failed.\n");
+        return 1;
+    }
+    poolinfo = libxl_list_cpupool(ctx, &n_pools);
+    if (!poolinfo) {
+        fprintf(stderr, "error getting cpupool info\n");
+        libxl_dominfo_list_free(info, nb_domain);
+        return 1;
+    }
+
+    for (p = 0; !rc && (p < n_pools); p++) {
+        if ((poolinfo[p].sched != sched) ||
+            (cpupool && (poolid != poolinfo[p].poolid)))
+            continue;
+
+        pooloutput(poolinfo[p].poolid);
+
+        output(-1, NULL);
+        for (i = 0; i < nb_domain; i++) {
+            libxl_vcpu_sched_params scinfo;
+            if (info[i].cpupool != poolinfo[p].poolid)
+                continue;
+            libxl_vcpu_sched_params_init(&scinfo);
+            rc = output(info[i].domid, &scinfo);
+            libxl_vcpu_sched_params_dispose(&scinfo);
+            if (rc)
+                break;
+        }
+    }
+
+    libxl_cpupoolinfo_list_free(poolinfo, n_pools);
+    libxl_dominfo_list_free(info, nb_domain);
+    return 0;
+}
+
 /* 
  * <nothing>             : List all domain params and sched params from all pools
  * -d [domid]            : List domain params for domain
@@ -6554,84 +6735,198 @@ int main_sched_credit2(int argc, char **argv)
 
 /*
  * <nothing>            : List all domain paramters and sched params
- * -d [domid]           : List domain params for domain
+ * -d [domid]           : List default domain params for domain
  * -d [domid] [params]  : Set domain params for domain
+ * -d [domid] -v [vcpuid 1] -v [vcpuid 2] ...  :
+ * List per-VCPU params for domain
+ * -d [domid] -v all  : List all per-VCPU params for domain
+ * -v all  : List all per-VCPU params for all domains
+ * -d [domid] -v [vcpuid 1] [params] -v [vcpuid 2] [params] ...  :
+ * Set per-VCPU params for domain
+ * -d [domid] -v all [params]  : Set all per-VCPU params for domain
  */
 int main_sched_rtds(int argc, char **argv)
 {
     const char *dom = NULL;
     const char *cpupool = NULL;
-    int period = 0; /* period is in microsecond */
-    int budget = 0; /* budget is in microsecond */
+    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 */
+    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 v_index = 0; /* index in vcpus array */
+    int p_index =0; /* index in periods array */
+    int b_index =0; /* index for in budgets array */
     bool opt_p = false;
     bool opt_b = false;
-    int opt, rc;
+    bool opt_v = false;
+    bool opt_all = false; /* output per-dom parameters */
+    int opt, i, rc, r;
     static struct option opts[] = {
         {"domain", 1, 0, 'd'},
         {"period", 1, 0, 'p'},
         {"budget", 1, 0, 'b'},
+        {"vcpuid",1, 0, 'v'},
         {"cpupool", 1, 0, 'c'},
         COMMON_LONG_OPTS
     };
 
-    SWITCH_FOREACH_OPT(opt, "d:p:b:c:", opts, "sched-rtds", 0) {
+    SWITCH_FOREACH_OPT(opt, "d:p:b:v:c", opts, "sched-rtds", 0) {
     case 'd':
         dom = optarg;
         break;
     case 'p':
-        period = strtol(optarg, NULL, 10);
-        opt_p = true;
+        if (p_index >= p_size) {
+            /*
+             * periods array is full
+             * double the array size for new elements
+             */
+            p_size *= 2;
+            periods = xrealloc(periods, p_size);
+        }
+        periods[p_index++] = strtol(optarg, NULL, 10);
+        opt_p = 1;
         break;
     case 'b':
-        budget = strtol(optarg, NULL, 10);
-        opt_b = true;
+        if (b_index >= b_size) { /* budgets array is full */
+            b_size *= 2;
+            budgets = xrealloc(budgets, b_size);
+        }
+        budgets[b_index++] = strtol(optarg, NULL, 10);
+        opt_b = 1;
+        break;
+    case 'v':
+        if (!strcmp(optarg, "all")) { /* get or set all vcpus of a domain */
+            opt_all = 1;
+            break;
+        }
+        if (v_index >= v_size) { /* vcpus array is full */
+            v_size *= 2;
+            vcpus = xrealloc(vcpus, v_size);
+        }
+        vcpus[v_index++] = strtol(optarg, NULL, 10);
+        opt_v = 1;
         break;
     case 'c':
         cpupool = optarg;
         break;
     }
 
-    if (cpupool && (dom || opt_p || opt_b)) {
+    if (cpupool && (dom || opt_p || opt_b || opt_v || opt_all)) {
         fprintf(stderr, "Specifying a cpupool is not allowed with "
                 "other options.\n");
-        return EXIT_FAILURE;
+        r = EXIT_FAILURE;
+        goto out;
     }
-    if (!dom && (opt_p || opt_b)) {
-        fprintf(stderr, "Must specify a domain.\n");
-        return EXIT_FAILURE;
+    if (!dom && (opt_p || opt_b || opt_v)) {
+        fprintf(stderr, "Missing parameters.\n");
+        r = EXIT_FAILURE;
+        goto out;
     }
-    if (opt_p != opt_b) {
-        fprintf(stderr, "Must specify period and budget\n");
-        return EXIT_FAILURE;
+    if (dom && !opt_v && !opt_all && (opt_p || opt_b)) {
+        fprintf(stderr, "Must specify VCPU.\n");
+        r = EXIT_FAILURE;
+        goto out;
+    }
+    if (opt_v && opt_all) {
+        fprintf(stderr, "Incorrect VCPU IDs.\n");
+        r = EXIT_FAILURE;
+        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");
+        r = EXIT_FAILURE;
+        goto out;
     }
 
-    if (!dom) { /* list all domain's rt scheduler info */
-        if (sched_domain_output(LIBXL_SCHEDULER_RTDS,
-                                sched_rtds_domain_output,
+    if ((!dom) && opt_all) {
+        /* get all domain's per-vcpu rtds scheduler parameters */
+        rc = -sched_vcpu_output(LIBXL_SCHEDULER_RTDS,
+                                sched_rtds_vcpu_output_all,
                                 sched_rtds_pool_output,
-                                cpupool))
-            return EXIT_FAILURE;
+                                cpupool);
+        if (rc) {
+            r = EXIT_FAILURE;
+            goto out;
+        }
+    } else if (!dom && !opt_all) {
+        /* list all domain's default scheduling parameters */
+        rc = -sched_domain_output(LIBXL_SCHEDULER_RTDS,
+                                  sched_rtds_domain_output,
+                                  sched_rtds_pool_output,
+                                  cpupool);
+        if (rc) {
+            r = EXIT_FAILURE;
+            goto out;
+        }
     } else {
         uint32_t domid = find_domain(dom);
-        if (!opt_p && !opt_b) { /* output rt scheduler info */
+        if (!opt_v && !opt_all) { /* output default scheduling parameters */
             sched_rtds_domain_output(-1);
-            if (sched_rtds_domain_output(domid))
-                return EXIT_FAILURE;
-        } else { /* set rt scheduler paramaters */
-            libxl_domain_sched_params scinfo;
-            libxl_domain_sched_params_init(&scinfo);
+            rc = -sched_rtds_domain_output(domid);
+            if (rc) {
+                r = EXIT_FAILURE;
+                goto out;
+            }
+        } else if (!opt_p && !opt_b) {
+            /* get per-vcpu rtds scheduling parameters */
+            libxl_vcpu_sched_params scinfo;
+            libxl_vcpu_sched_params_init(&scinfo);
+            sched_rtds_vcpu_output(-1, &scinfo);
+            scinfo.num_vcpus = v_index;
+            if (v_index > 0) {
+                scinfo.vcpus = (libxl_sched_params *)
+                               xmalloc(sizeof(libxl_sched_params) * (v_index));
+                for (i = 0; i < v_index; i++)
+                    scinfo.vcpus[i].vcpuid = vcpus[i];
+                rc = -sched_rtds_vcpu_output(domid, &scinfo);
+            } else /* get params for all vcpus */
+                rc = -sched_rtds_vcpu_output_all(domid, &scinfo);
+            libxl_vcpu_sched_params_dispose(&scinfo);
+            if (rc) {
+                r = EXIT_FAILURE;
+                goto out;
+            }
+    } else if (opt_v || opt_all) {
+            /* set per-vcpu rtds scheduling parameters */
+            libxl_vcpu_sched_params scinfo;
+            libxl_vcpu_sched_params_init(&scinfo);
             scinfo.sched = LIBXL_SCHEDULER_RTDS;
-            scinfo.period = period;
-            scinfo.budget = budget;
+            if (v_index > 0) {
+                scinfo.num_vcpus = v_index;
+                scinfo.vcpus = (libxl_sched_params *)
+                               xmalloc(sizeof(libxl_sched_params) * (v_index));
+                for (i = 0; i < v_index; i++) {
+                    scinfo.vcpus[i].vcpuid = vcpus[i];
+                    scinfo.vcpus[i].period = periods[i];
+                    scinfo.vcpus[i].budget = budgets[i];
+                }
+                rc = sched_vcpu_set(domid, &scinfo);
+            } else { /* set params for all vcpus */
+                scinfo.num_vcpus = 1;
+                scinfo.vcpus = (libxl_sched_params *)
+                               xmalloc(sizeof(libxl_sched_params));
+                scinfo.vcpus[0].period = periods[0];
+                scinfo.vcpus[0].budget = budgets[0];
+                rc = sched_vcpu_set_all(domid, &scinfo);
+            }
 
-            rc = sched_domain_set(domid, &scinfo);
-            libxl_domain_sched_params_dispose(&scinfo);
-            if (rc)
-                return EXIT_FAILURE;
+            libxl_vcpu_sched_params_dispose(&scinfo);
+            if (rc) {
+                r = EXIT_FAILURE;
+                goto out;
+            }
         }
     }
 
-    return EXIT_SUCCESS;
+    r = EXIT_SUCCESS;
+out:
+    free(vcpus);
+    free(periods);
+    free(budgets);
+    return r;
 }
 
 int main_domid(int argc, char **argv)
diff --git a/tools/libxl/xl_cmdtable.c b/tools/libxl/xl_cmdtable.c
index 7880a9c..859af78 100644
--- a/tools/libxl/xl_cmdtable.c
+++ b/tools/libxl/xl_cmdtable.c
@@ -269,8 +269,10 @@ struct cmd_spec cmd_table[] = {
     { "sched-rtds",
       &main_sched_rtds, 0, 1,
       "Get/set rtds scheduler parameters",
-      "[-d <Domain> [-p[=PERIOD]] [-b[=BUDGET]]]",
+      "[-d <Domain> [-v[=VCPUID/all]] [-p[=PERIOD]] [-b[=BUDGET]]]",
       "-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"
     },
-- 
1.9.1


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

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

* Re: [PATCH v9 for Xen 4.7 1/4] xen: enable per-VCPU parameter for RTDS
  2016-04-01  4:59 ` [PATCH v9 for Xen 4.7 1/4] xen: enable " Chong Li
@ 2016-04-01  9:35   ` Jan Beulich
  2016-04-01 12:37   ` Dario Faggioli
  2016-04-04 15:14   ` [hypervisor deadlock] " Andrew Cooper
  2 siblings, 0 replies; 32+ messages in thread
From: Jan Beulich @ 2016-04-01  9:35 UTC (permalink / raw)
  To: Chong Li
  Cc: Chong Li, Sisu Xi, george.dunlap, dario.faggioli, xen-devel,
	Meng Xu, dgolomb

>>> On 01.04.16 at 06:59, <lichong659@gmail.com> wrote:
> Add XEN_DOMCTL_SCHEDOP_getvcpuinfo and _putvcpuinfo hypercalls
> to independently get and set the scheduling parameters of each
> vCPU of a domain.
> 
> Also fix a bug in XEN_DOMCTL_SCHEDOP_getinfo, where PERIOD and
> BUDGET are not divided by MICROSECS(1) before being retruned
> to the caller.
> 
> Signed-off-by: Chong Li <chong.li@wustl.edu>
> Signed-off-by: Meng Xu <mengxu@cis.upenn.edu>
> Signed-off-by: Sisu Xi <xisisu@gmail.com>

Public interface change
Acked-by: Jan Beulich <jbeulich@suse.com>


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

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

* Re: [PATCH v9 for Xen 4.7 1/4] xen: enable per-VCPU parameter for RTDS
  2016-04-01  4:59 ` [PATCH v9 for Xen 4.7 1/4] xen: enable " Chong Li
  2016-04-01  9:35   ` Jan Beulich
@ 2016-04-01 12:37   ` Dario Faggioli
  2016-04-04 15:14   ` [hypervisor deadlock] " Andrew Cooper
  2 siblings, 0 replies; 32+ messages in thread
From: Dario Faggioli @ 2016-04-01 12:37 UTC (permalink / raw)
  To: Chong Li, xen-devel
  Cc: Chong Li, Sisu Xi, george.dunlap, Meng Xu, jbeulich, dgolomb


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

On Thu, 2016-03-31 at 23:59 -0500, Chong Li wrote:
> Add XEN_DOMCTL_SCHEDOP_getvcpuinfo and _putvcpuinfo hypercalls
> to independently get and set the scheduling parameters of each
> vCPU of a domain.
> 
> Also fix a bug in XEN_DOMCTL_SCHEDOP_getinfo, where PERIOD and
> BUDGET are not divided by MICROSECS(1) before being retruned
> to the caller.
> 
> Signed-off-by: Chong Li <chong.li@wustl.edu>
> Signed-off-by: Meng Xu <mengxu@cis.upenn.edu>
> Signed-off-by: Sisu Xi <xisisu@gmail.com>
>
Acked-by: Dario Faggioli <dario.faggioli@citrix.com>

Thanks and 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: 181 bytes --]

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

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

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

* Re: [PATCH v9 for Xen 4.7 3/4] libxl: enable per-VCPU parameter for RTDS
  2016-04-01  4:59 ` [PATCH v9 for Xen 4.7 3/4] libxl: " Chong Li
@ 2016-04-01 12:53   ` Dario Faggioli
  2016-04-01 13:40   ` Wei Liu
  1 sibling, 0 replies; 32+ messages in thread
From: Dario Faggioli @ 2016-04-01 12:53 UTC (permalink / raw)
  To: Chong Li, xen-devel
  Cc: Chong Li, wei.liu2, Sisu Xi, george.dunlap, ian.jackson, Meng Xu,
	dgolomb


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

On Thu, 2016-03-31 at 23:59 -0500, Chong Li wrote:
> Add libxl_vcpu_sched_params_get/set and sched_rtds_vcpu_get/set
> functions to support per-VCPU settings.
> 
> Signed-off-by: Chong Li <chong.li@wustl.edu>
> Signed-off-by: Meng Xu <mengxu@cis.upenn.edu>
> Signed-off-by: Sisu Xi <xisisu@gmail.com>
> 
> Acked-by: Wei Liu <wei.liu2@citrix.com>
> Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com>
>
You added a new function, so you probably should have dropped the tags.

Anyway, I'll re-issue mine:

Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com>

There are still things I'd like being done better. There's a certain
amount of code duplication, and there still are two instances of this
pattern (the 'else' is superfluous):

> +    if (scinfo->num_vcpus > 0) {
> +        rc = ERROR_INVAL;
> +        goto out;
> +    } else {
> +        num_vcpus = info.max_vcpu_id + 1;
> +        GCNEW_ARRAY(vcpus, num_vcpus);
> +        for (i = 0; i < num_vcpus; i++)
> +            vcpus[i].vcpuid = i;
> +    }
> +

But Wei said (a few series versions ago) he is kinda fine with this,
and I also don't think the series should be blocked because of it. We
can improve and refactor things at a later point (adding this to my
TODO list).

Thanks and 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: 181 bytes --]

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

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

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

* Re: [PATCH v9 for Xen 4.7 4/4] xl: enable per-VCPU parameter for RTDS
  2016-04-01  4:59 ` [PATCH v9 for Xen 4.7 4/4] xl: " Chong Li
@ 2016-04-01 13:15   ` Dario Faggioli
  2016-04-01 13:43     ` Wei Liu
  0 siblings, 1 reply; 32+ messages in thread
From: Dario Faggioli @ 2016-04-01 13:15 UTC (permalink / raw)
  To: Chong Li, xen-devel
  Cc: Chong Li, wei.liu2, Sisu Xi, george.dunlap, Meng Xu, dgolomb


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

On Thu, 2016-03-31 at 23:59 -0500, Chong Li wrote:
> Change main_sched_rtds and related output functions to support
> per-VCPU settings.
> 
> Signed-off-by: Chong Li <chong.li@wustl.edu>
> Signed-off-by: Meng Xu <mengxu@cis.upenn.edu>
> Signed-off-by: Sisu Xi <xisisu@gmail.com>
> 
> Acked-by: Wei Liu <wei.liu2@citrix.com>
>
This Acked-by, I'm should have been removed. In fact, Wei sai it was
subject to me saying Acked-by or Reviewed-by myself to v8. I did not
say anything like that, and in fact I asked for changes, and Wei said
himself that, at least some of those were necessary.

Now, you've addressed those comments, and I'm happy about how the patch
is looking now, so:

Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com>

But I really think Wei (or Ian) should (re-)look and (re-)Ack.

I've found only a couple of style issues. I don't feel like blocking
the series or asking to resend... perhaps they can be fixed during
commit? Or I'm up for sending a cleanup patch myself (which would not
introduce any functional change), as soon as this hit the repo.

Here they are:

> @@ -6554,84 +6735,198 @@ int main_sched_credit2(int argc, char
> **argv)
>  
>  /*
>   * <nothing>            : List all domain paramters and sched params
> - * -d [domid]           : List domain params for domain
> + * -d [domid]           : List default domain params for domain
>   * -d [domid] [params]  : Set domain params for domain
> + * -d [domid] -v [vcpuid 1] -v [vcpuid 2] ...  :
> + * List per-VCPU params for domain
> + * -d [domid] -v all  : List all per-VCPU params for domain
> + * -v all  : List all per-VCPU params for all domains
> + * -d [domid] -v [vcpuid 1] [params] -v [vcpuid 2] [params] ...  :
> + * Set per-VCPU params for domain
> + * -d [domid] -v all [params]  : Set all per-VCPU params for domain
>   */
>  int main_sched_rtds(int argc, char **argv)
>  {
>      const char *dom = NULL;
>      const char *cpupool = NULL;
> -    int period = 0; /* period is in microsecond */
> -    int budget = 0; /* budget is in microsecond */
> +    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 */
> +    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 v_index = 0; /* index in vcpus array */
> +    int p_index =0; /* index in periods array */
> +    int b_index =0; /* index for in budgets array */
>      bool opt_p = false;
>      bool opt_b = false;
> -    int opt, rc;
> +    bool opt_v = false;
> +    bool opt_all = false; /* output per-dom parameters */
> +    int opt, i, rc, r;
>      static struct option opts[] = {
>          {"domain", 1, 0, 'd'},
>          {"period", 1, 0, 'p'},
>          {"budget", 1, 0, 'b'},
> +        {"vcpuid",1, 0, 'v'},
>          {"cpupool", 1, 0, 'c'},
>          COMMON_LONG_OPTS
>      };
>  
> -    SWITCH_FOREACH_OPT(opt, "d:p:b:c:", opts, "sched-rtds", 0) {
> +    SWITCH_FOREACH_OPT(opt, "d:p:b:v:c", opts, "sched-rtds", 0) {
>      case 'd':
>          dom = optarg;
>          break;
>      case 'p':
> -        period = strtol(optarg, NULL, 10);
> -        opt_p = true;
> +        if (p_index >= p_size) {
> +            /*
> +             * periods array is full
> +             * double the array size for new elements
> +             */
> +            p_size *= 2;
> +            periods = xrealloc(periods, p_size);
> +        }
> +        periods[p_index++] = strtol(optarg, NULL, 10);
> +        opt_p = 1;
>          break;
>      case 'b':
> -        budget = strtol(optarg, NULL, 10);
> -        opt_b = true;
> +        if (b_index >= b_size) { /* budgets array is full */
> +            b_size *= 2;
> +            budgets = xrealloc(budgets, b_size);
> +        }
> +        budgets[b_index++] = strtol(optarg, NULL, 10);
> +        opt_b = 1;
> +        break;
> +    case 'v':
> +        if (!strcmp(optarg, "all")) { /* get or set all vcpus of a
> domain */
> +            opt_all = 1;
> +            break;
> +        }
> +        if (v_index >= v_size) { /* vcpus array is full */
> +            v_size *= 2;
> +            vcpus = xrealloc(vcpus, v_size);
> +        }
> +        vcpus[v_index++] = strtol(optarg, NULL, 10);
> +        opt_v = 1;
>          break;
>      case 'c':
>          cpupool = optarg;
>          break;
>      }
>  
> -    if (cpupool && (dom || opt_p || opt_b)) {
> +    if (cpupool && (dom || opt_p || opt_b || opt_v || opt_all)) {
>          fprintf(stderr, "Specifying a cpupool is not allowed with "
>                  "other options.\n");
> -        return EXIT_FAILURE;
> +        r = EXIT_FAILURE;
> +        goto out;
>      }
> -    if (!dom && (opt_p || opt_b)) {
> -        fprintf(stderr, "Must specify a domain.\n");
> -        return EXIT_FAILURE;
> +    if (!dom && (opt_p || opt_b || opt_v)) {
> +        fprintf(stderr, "Missing parameters.\n");
> +        r = EXIT_FAILURE;
> +        goto out;
>      }
> -    if (opt_p != opt_b) {
> -        fprintf(stderr, "Must specify period and budget\n");
> -        return EXIT_FAILURE;
> +    if (dom && !opt_v && !opt_all && (opt_p || opt_b)) {
> +        fprintf(stderr, "Must specify VCPU.\n");
> +        r = EXIT_FAILURE;
> +        goto out;
> +    }
> +    if (opt_v && opt_all) {
> +        fprintf(stderr, "Incorrect VCPU IDs.\n");
> +        r = EXIT_FAILURE;
> +        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");
> +        r = EXIT_FAILURE;
> +        goto out;
>      }
>  
> -    if (!dom) { /* list all domain's rt scheduler info */
> -        if (sched_domain_output(LIBXL_SCHEDULER_RTDS,
> -                                sched_rtds_domain_output,
> +    if ((!dom) && opt_all) {
> +        /* get all domain's per-vcpu rtds scheduler parameters */
> +        rc = -sched_vcpu_output(LIBXL_SCHEDULER_RTDS,
> +                                sched_rtds_vcpu_output_all,
>                                  sched_rtds_pool_output,
> -                                cpupool))
> -            return EXIT_FAILURE;
> +                                cpupool);
> +        if (rc) {
> +            r = EXIT_FAILURE;
> +            goto out;
> +        }
> +    } else if (!dom && !opt_all) {
> +        /* list all domain's default scheduling parameters */
> +        rc = -sched_domain_output(LIBXL_SCHEDULER_RTDS,
> +                                  sched_rtds_domain_output,
> +                                  sched_rtds_pool_output,
> +                                  cpupool);
> +        if (rc) {
> +            r = EXIT_FAILURE;
> +            goto out;
> +        }
>      } else {
>          uint32_t domid = find_domain(dom);
> -        if (!opt_p && !opt_b) { /* output rt scheduler info */
> +        if (!opt_v && !opt_all) { /* output default scheduling
> parameters */
>              sched_rtds_domain_output(-1);
> -            if (sched_rtds_domain_output(domid))
> -                return EXIT_FAILURE;
> -        } else { /* set rt scheduler paramaters */
> -            libxl_domain_sched_params scinfo;
> -            libxl_domain_sched_params_init(&scinfo);
> +            rc = -sched_rtds_domain_output(domid);
> +            if (rc) {
> +                r = EXIT_FAILURE;
> +                goto out;
> +            }
> +        } else if (!opt_p && !opt_b) {
> +            /* get per-vcpu rtds scheduling parameters */
> +            libxl_vcpu_sched_params scinfo;
> +            libxl_vcpu_sched_params_init(&scinfo);
The variable declaration is placed properly now, but there should be a
blank line between the declaration itself, and actual code.

There is another instance of this at [1].

> +            sched_rtds_vcpu_output(-1, &scinfo);
> +            scinfo.num_vcpus = v_index;
> +            if (v_index > 0) {
> +                scinfo.vcpus = (libxl_sched_params *)
> +                               xmalloc(sizeof(libxl_sched_params) *
> (v_index));
> +                for (i = 0; i < v_index; i++)
> +                    scinfo.vcpus[i].vcpuid = vcpus[i];
> +                rc = -sched_rtds_vcpu_output(domid, &scinfo);
> +            } else /* get params for all vcpus */
> +                rc = -sched_rtds_vcpu_output_all(domid, &scinfo);
> +            libxl_vcpu_sched_params_dispose(&scinfo);
> +            if (rc) {
> +                r = EXIT_FAILURE;
> +                goto out;
> +            }
> +    } else if (opt_v || opt_all) {
>
The indentation is wrong. There should be 4 more spaces.

> +            /* set per-vcpu rtds scheduling parameters */
> +            libxl_vcpu_sched_params scinfo;
> +            libxl_vcpu_sched_params_init(&scinfo);
>
[1].

Thanks and 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: 181 bytes --]

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

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

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

* Re: [PATCH v9 for Xen 4.7 3/4] libxl: enable per-VCPU parameter for RTDS
  2016-04-01  4:59 ` [PATCH v9 for Xen 4.7 3/4] libxl: " Chong Li
  2016-04-01 12:53   ` Dario Faggioli
@ 2016-04-01 13:40   ` Wei Liu
  2016-04-06 14:32     ` Ian Jackson
  1 sibling, 1 reply; 32+ messages in thread
From: Wei Liu @ 2016-04-01 13:40 UTC (permalink / raw)
  To: Chong Li
  Cc: Chong Li, wei.liu2, Sisu Xi, george.dunlap, dario.faggioli,
	ian.jackson, xen-devel, ian.campbell, Meng Xu, dgolomb

On Thu, Mar 31, 2016 at 11:59:45PM -0500, Chong Li wrote:
> Add libxl_vcpu_sched_params_get/set and sched_rtds_vcpu_get/set
> functions to support per-VCPU settings.
> 
> Signed-off-by: Chong Li <chong.li@wustl.edu>
> Signed-off-by: Meng Xu <mengxu@cis.upenn.edu>
> Signed-off-by: Sisu Xi <xisisu@gmail.com>
> 
> Acked-by: Wei Liu <wei.liu2@citrix.com>

This should have been dropped.

> Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com>
> ---
> Changes on PATCH v8:
> 1) Add libxl_vcpu_sched_params_get_all() and
> sched_rtds_vcpu_get_all() to output all vcpus of a domain.
> 
[...]
>  
> +int libxl_vcpu_sched_params_get(libxl_ctx *ctx, uint32_t domid,
> +                                libxl_vcpu_sched_params *scinfo)
> +{
> +    GC_INIT(ctx);
> +    int rc;
> +
> +    scinfo->sched = libxl__domain_scheduler(gc, domid);
> +
> +    switch (scinfo->sched) {
> +    case LIBXL_SCHEDULER_SEDF:
> +        LOG(ERROR, "SEDF scheduler is no longer available");
> +        rc = ERROR_FEATURE_REMOVED;
> +        break;
> +    case LIBXL_SCHEDULER_CREDIT:
> +    case LIBXL_SCHEDULER_CREDIT2:
> +    case LIBXL_SCHEDULER_ARINC653:
> +        LOG(ERROR, "per-VCPU parameter getting not supported for this scheduler");
> +        rc = ERROR_INVAL;
> +        break;
> +    case LIBXL_SCHEDULER_RTDS:
> +        rc = sched_rtds_vcpu_get(gc, domid, scinfo);
> +        break;
> +    default:
> +        LOG(ERROR, "Unknown scheduler");
> +        rc = ERROR_INVAL;
> +        break;
> +    }
> +
> +    GC_FREE;
> +    return rc;
> +}
> +
> +int libxl_vcpu_sched_params_get_all(libxl_ctx *ctx, uint32_t domid,
> +                                    libxl_vcpu_sched_params *scinfo)
> +{
> +    GC_INIT(ctx);
> +    int rc;
> +
> +    scinfo->sched = libxl__domain_scheduler(gc, domid);
> +
> +    switch (scinfo->sched) {
> +    case LIBXL_SCHEDULER_SEDF:
> +        LOG(ERROR, "SEDF scheduler is no longer available");
> +        rc = ERROR_FEATURE_REMOVED;
> +        break;
> +    case LIBXL_SCHEDULER_CREDIT:
> +    case LIBXL_SCHEDULER_CREDIT2:
> +    case LIBXL_SCHEDULER_ARINC653:
> +        LOG(ERROR, "per-VCPU parameter getting not supported for this scheduler");
> +        rc = ERROR_INVAL;
> +        break;
> +    case LIBXL_SCHEDULER_RTDS:
> +        rc = sched_rtds_vcpu_get_all(gc, domid, scinfo);
> +        break;
> +    default:
> +        LOG(ERROR, "Unknown scheduler");
> +        rc = ERROR_INVAL;
> +        break;
> +    }
> +
> +    GC_FREE;
> +    return rc;
> +}
> +

These two functions LGTM, so I shall ack this patch again:

  Acked-by: Wei Liu <wei.liu2@citrix.com>

Wei.

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

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

* Re: [PATCH v9 for Xen 4.7 4/4] xl: enable per-VCPU parameter for RTDS
  2016-04-01 13:15   ` Dario Faggioli
@ 2016-04-01 13:43     ` Wei Liu
  2016-04-01 13:51       ` Dario Faggioli
  0 siblings, 1 reply; 32+ messages in thread
From: Wei Liu @ 2016-04-01 13:43 UTC (permalink / raw)
  To: Dario Faggioli
  Cc: Chong Li, wei.liu2, Sisu Xi, george.dunlap, xen-devel, Meng Xu,
	Chong Li, dgolomb

On Fri, Apr 01, 2016 at 03:15:18PM +0200, Dario Faggioli wrote:
> On Thu, 2016-03-31 at 23:59 -0500, Chong Li wrote:
> > Change main_sched_rtds and related output functions to support
> > per-VCPU settings.
> > 
> > Signed-off-by: Chong Li <chong.li@wustl.edu>
> > Signed-off-by: Meng Xu <mengxu@cis.upenn.edu>
> > Signed-off-by: Sisu Xi <xisisu@gmail.com>
> > 
> > Acked-by: Wei Liu <wei.liu2@citrix.com>
> >
> This Acked-by, I'm should have been removed. In fact, Wei sai it was
> subject to me saying Acked-by or Reviewed-by myself to v8. I did not
> say anything like that, and in fact I asked for changes, and Wei said
> himself that, at least some of those were necessary.
> 
> Now, you've addressed those comments, and I'm happy about how the patch
> is looking now, so:
> 
> Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com>
> 
> But I really think Wei (or Ian) should (re-)look and (re-)Ack.
> 

I'm happy with this patch, too. So

  Acked-by: Wei Liu <wei.liu2@citrix.com>

> I've found only a couple of style issues. I don't feel like blocking
> the series or asking to resend... perhaps they can be fixed during
> commit? Or I'm up for sending a cleanup patch myself (which would not
> introduce any functional change), as soon as this hit the repo.
> 

IMHO a separate patch is better. Adjusting during committing in such
large patch is error prone, plus there is huge backlog to be applied at
the moment.

Wei.


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

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

* Re: [PATCH v9 for Xen 4.7 4/4] xl: enable per-VCPU parameter for RTDS
  2016-04-01 13:43     ` Wei Liu
@ 2016-04-01 13:51       ` Dario Faggioli
  0 siblings, 0 replies; 32+ messages in thread
From: Dario Faggioli @ 2016-04-01 13:51 UTC (permalink / raw)
  To: Wei Liu
  Cc: Chong Li, Sisu Xi, george.dunlap, xen-devel, Meng Xu, Chong Li, dgolomb


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

On Fri, 2016-04-01 at 14:43 +0100, Wei Liu wrote:
> On Fri, Apr 01, 2016 at 03:15:18PM +0200, Dario Faggioli wrote:
> > 
> > I've found only a couple of style issues. I don't feel like
> > blocking
> > the series or asking to resend... perhaps they can be fixed during
> > commit? Or I'm up for sending a cleanup patch myself (which would
> > not
> > introduce any functional change), as soon as this hit the repo.
> > 
> IMHO a separate patch is better. Adjusting during committing in such
> large patch is error prone, plus there is huge backlog to be applied
> at
> the moment.
> 
Ok, it makes sense.

I'll queue this up.

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: 181 bytes --]

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

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

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

* [hypervisor deadlock] Re: [PATCH v9 for Xen 4.7 1/4] xen: enable per-VCPU parameter for RTDS
  2016-04-01  4:59 ` [PATCH v9 for Xen 4.7 1/4] xen: enable " Chong Li
  2016-04-01  9:35   ` Jan Beulich
  2016-04-01 12:37   ` Dario Faggioli
@ 2016-04-04 15:14   ` Andrew Cooper
  2016-04-04 15:58     ` Chong Li
  2 siblings, 1 reply; 32+ messages in thread
From: Andrew Cooper @ 2016-04-04 15:14 UTC (permalink / raw)
  To: Chong Li, xen-devel
  Cc: Chong Li, Wei Liu, Sisu Xi, george.dunlap, dario.faggioli,
	Meng Xu, jbeulich, dgolomb

On 01/04/16 05:59, Chong Li wrote:
> diff --git a/xen/common/sched_credit.c b/xen/common/sched_credit.c
> index 305889a..e5d15d8 100644
> --- a/xen/common/sched_credit.c
> +++ b/xen/common/sched_credit.c
> @@ -1080,15 +1080,13 @@ csched_dom_cntl(
>       * lock. Runq lock not needed anywhere in here. */
>      spin_lock_irqsave(&prv->lock, flags);
>  
> -    if ( op->cmd == XEN_DOMCTL_SCHEDOP_getinfo )
> +    switch ( op->cmd )
>      {
> +    case XEN_DOMCTL_SCHEDOP_getinfo:
>          op->u.credit.weight = sdom->weight;
>          op->u.credit.cap = sdom->cap;
> -    }
> -    else
> -    {
> -        ASSERT(op->cmd == XEN_DOMCTL_SCHEDOP_putinfo);
> -
> +        break;
> +    case XEN_DOMCTL_SCHEDOP_putinfo:
>          if ( op->u.credit.weight != 0 )
>          {
>              if ( !list_empty(&sdom->active_sdom_elem) )
> @@ -1101,7 +1099,9 @@ csched_dom_cntl(
>  
>          if ( op->u.credit.cap != (uint16_t)~0U )
>              sdom->cap = op->u.credit.cap;
> -
> +        break;
> +    default:
> +        return -EINVAL;

This path returns without unlocking prv->lock.

>      }
>  
>      spin_unlock_irqrestore(&prv->lock, flags);
> diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c
> index 7ddad38..d48ed5a 100644
> --- a/xen/common/sched_credit2.c
> +++ b/xen/common/sched_credit2.c
> @@ -1421,14 +1421,12 @@ csched2_dom_cntl(
>       * runq lock to update csvcs. */
>      spin_lock_irqsave(&prv->lock, flags);
>  
> -    if ( op->cmd == XEN_DOMCTL_SCHEDOP_getinfo )
> +    switch ( op->cmd )
>      {
> +    case XEN_DOMCTL_SCHEDOP_getinfo:
>          op->u.credit2.weight = sdom->weight;
> -    }
> -    else
> -    {
> -        ASSERT(op->cmd == XEN_DOMCTL_SCHEDOP_putinfo);
> -
> +        break;
> +    case XEN_DOMCTL_SCHEDOP_putinfo:
>          if ( op->u.credit2.weight != 0 )
>          {
>              struct vcpu *v;
> @@ -1457,6 +1455,9 @@ csched2_dom_cntl(
>                  vcpu_schedule_unlock(lock, svc->vcpu);
>              }
>          }
> +        break;
> +    default:
> +        return -EINVAL;

As does this.

Please submit a bugfix ASAP.  This will become a security vulnerability
if Xen 4.7 is shipped without it being fixed.

>      }
>  
>      spin_unlock_irqrestore(&prv->lock, flags);


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

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

* Re: [hypervisor deadlock] Re: [PATCH v9 for Xen 4.7 1/4] xen: enable per-VCPU parameter for RTDS
  2016-04-04 15:14   ` [hypervisor deadlock] " Andrew Cooper
@ 2016-04-04 15:58     ` Chong Li
  2016-04-04 16:05       ` George Dunlap
  0 siblings, 1 reply; 32+ messages in thread
From: Chong Li @ 2016-04-04 15:58 UTC (permalink / raw)
  To: Andrew Cooper
  Cc: Chong Li, Wei Liu, Sisu Xi, George Dunlap, dario.faggioli,
	xen-devel, Meng Xu, Jan Beulich, Dagaen Golomb

On Mon, Apr 4, 2016 at 10:14 AM, Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
> On 01/04/16 05:59, Chong Li wrote:
>> diff --git a/xen/common/sched_credit.c b/xen/common/sched_credit.c
>> index 305889a..e5d15d8 100644
>> --- a/xen/common/sched_credit.c
>> +++ b/xen/common/sched_credit.c
>> @@ -1080,15 +1080,13 @@ csched_dom_cntl(
>>       * lock. Runq lock not needed anywhere in here. */
>>      spin_lock_irqsave(&prv->lock, flags);
>>
>> -    if ( op->cmd == XEN_DOMCTL_SCHEDOP_getinfo )
>> +    switch ( op->cmd )
>>      {
>> +    case XEN_DOMCTL_SCHEDOP_getinfo:
>>          op->u.credit.weight = sdom->weight;
>>          op->u.credit.cap = sdom->cap;
>> -    }
>> -    else
>> -    {
>> -        ASSERT(op->cmd == XEN_DOMCTL_SCHEDOP_putinfo);
>> -
>> +        break;
>> +    case XEN_DOMCTL_SCHEDOP_putinfo:
>>          if ( op->u.credit.weight != 0 )
>>          {
>>              if ( !list_empty(&sdom->active_sdom_elem) )
>> @@ -1101,7 +1099,9 @@ csched_dom_cntl(
>>
>>          if ( op->u.credit.cap != (uint16_t)~0U )
>>              sdom->cap = op->u.credit.cap;
>> -
>> +        break;
>> +    default:
>> +        return -EINVAL;
>
> This path returns without unlocking prv->lock.
>
>>      }
>>
>>      spin_unlock_irqrestore(&prv->lock, flags);
>> diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c
>> index 7ddad38..d48ed5a 100644
>> --- a/xen/common/sched_credit2.c
>> +++ b/xen/common/sched_credit2.c
>> @@ -1421,14 +1421,12 @@ csched2_dom_cntl(
>>       * runq lock to update csvcs. */
>>      spin_lock_irqsave(&prv->lock, flags);
>>
>> -    if ( op->cmd == XEN_DOMCTL_SCHEDOP_getinfo )
>> +    switch ( op->cmd )
>>      {
>> +    case XEN_DOMCTL_SCHEDOP_getinfo:
>>          op->u.credit2.weight = sdom->weight;
>> -    }
>> -    else
>> -    {
>> -        ASSERT(op->cmd == XEN_DOMCTL_SCHEDOP_putinfo);
>> -
>> +        break;
>> +    case XEN_DOMCTL_SCHEDOP_putinfo:
>>          if ( op->u.credit2.weight != 0 )
>>          {
>>              struct vcpu *v;
>> @@ -1457,6 +1455,9 @@ csched2_dom_cntl(
>>                  vcpu_schedule_unlock(lock, svc->vcpu);
>>              }
>>          }
>> +        break;
>> +    default:
>> +        return -EINVAL;
>
> As does this.
>
> Please submit a bugfix ASAP.  This will become a security vulnerability
> if Xen 4.7 is shipped without it being fixed.
>
>>      }
>>
>>      spin_unlock_irqrestore(&prv->lock, flags);
>
Thanks for pointing this out.

Dario, do you want to include this bugfix in your cleanup patch, or
let me submit this?

Chong


-- 
Chong Li
Department of Computer Science and Engineering
Washington University in St.louis

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

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

* Re: [hypervisor deadlock] Re: [PATCH v9 for Xen 4.7 1/4] xen: enable per-VCPU parameter for RTDS
  2016-04-04 15:58     ` Chong Li
@ 2016-04-04 16:05       ` George Dunlap
  2016-04-04 16:32         ` Dario Faggioli
  0 siblings, 1 reply; 32+ messages in thread
From: George Dunlap @ 2016-04-04 16:05 UTC (permalink / raw)
  To: Chong Li, Andrew Cooper
  Cc: Chong Li, Wei Liu, Sisu Xi, George Dunlap, dario.faggioli,
	xen-devel, Meng Xu, Jan Beulich, Dagaen Golomb

On 04/04/16 16:58, Chong Li wrote:
> On Mon, Apr 4, 2016 at 10:14 AM, Andrew Cooper
> <andrew.cooper3@citrix.com> wrote:
>> On 01/04/16 05:59, Chong Li wrote:
>>> diff --git a/xen/common/sched_credit.c b/xen/common/sched_credit.c
>>> index 305889a..e5d15d8 100644
>>> --- a/xen/common/sched_credit.c
>>> +++ b/xen/common/sched_credit.c
>>> @@ -1080,15 +1080,13 @@ csched_dom_cntl(
>>>       * lock. Runq lock not needed anywhere in here. */
>>>      spin_lock_irqsave(&prv->lock, flags);
>>>
>>> -    if ( op->cmd == XEN_DOMCTL_SCHEDOP_getinfo )
>>> +    switch ( op->cmd )
>>>      {
>>> +    case XEN_DOMCTL_SCHEDOP_getinfo:
>>>          op->u.credit.weight = sdom->weight;
>>>          op->u.credit.cap = sdom->cap;
>>> -    }
>>> -    else
>>> -    {
>>> -        ASSERT(op->cmd == XEN_DOMCTL_SCHEDOP_putinfo);
>>> -
>>> +        break;
>>> +    case XEN_DOMCTL_SCHEDOP_putinfo:
>>>          if ( op->u.credit.weight != 0 )
>>>          {
>>>              if ( !list_empty(&sdom->active_sdom_elem) )
>>> @@ -1101,7 +1099,9 @@ csched_dom_cntl(
>>>
>>>          if ( op->u.credit.cap != (uint16_t)~0U )
>>>              sdom->cap = op->u.credit.cap;
>>> -
>>> +        break;
>>> +    default:
>>> +        return -EINVAL;
>>
>> This path returns without unlocking prv->lock.
>>
>>>      }
>>>
>>>      spin_unlock_irqrestore(&prv->lock, flags);
>>> diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c
>>> index 7ddad38..d48ed5a 100644
>>> --- a/xen/common/sched_credit2.c
>>> +++ b/xen/common/sched_credit2.c
>>> @@ -1421,14 +1421,12 @@ csched2_dom_cntl(
>>>       * runq lock to update csvcs. */
>>>      spin_lock_irqsave(&prv->lock, flags);
>>>
>>> -    if ( op->cmd == XEN_DOMCTL_SCHEDOP_getinfo )
>>> +    switch ( op->cmd )
>>>      {
>>> +    case XEN_DOMCTL_SCHEDOP_getinfo:
>>>          op->u.credit2.weight = sdom->weight;
>>> -    }
>>> -    else
>>> -    {
>>> -        ASSERT(op->cmd == XEN_DOMCTL_SCHEDOP_putinfo);
>>> -
>>> +        break;
>>> +    case XEN_DOMCTL_SCHEDOP_putinfo:
>>>          if ( op->u.credit2.weight != 0 )
>>>          {
>>>              struct vcpu *v;
>>> @@ -1457,6 +1455,9 @@ csched2_dom_cntl(
>>>                  vcpu_schedule_unlock(lock, svc->vcpu);
>>>              }
>>>          }
>>> +        break;
>>> +    default:
>>> +        return -EINVAL;
>>
>> As does this.
>>
>> Please submit a bugfix ASAP.  This will become a security vulnerability
>> if Xen 4.7 is shipped without it being fixed.
>>
>>>      }
>>>
>>>      spin_unlock_irqrestore(&prv->lock, flags);
>>
> Thanks for pointing this out.
> 
> Dario, do you want to include this bugfix in your cleanup patch, or
> let me submit this?

If you're around and can test it, it's probably better if you can send a
patch right a way.

Thanks,
 -George


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

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

* Re: [hypervisor deadlock] Re: [PATCH v9 for Xen 4.7 1/4] xen: enable per-VCPU parameter for RTDS
  2016-04-04 16:05       ` George Dunlap
@ 2016-04-04 16:32         ` Dario Faggioli
  2016-04-04 16:47           ` Wei Liu
  0 siblings, 1 reply; 32+ messages in thread
From: Dario Faggioli @ 2016-04-04 16:32 UTC (permalink / raw)
  To: George Dunlap, Chong Li, Andrew Cooper
  Cc: Chong Li, Wei Liu, Sisu Xi, George Dunlap, xen-devel, Meng Xu,
	Jan Beulich, Dagaen Golomb


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

On Mon, 2016-04-04 at 17:05 +0100, George Dunlap wrote:
> On 04/04/16 16:58, Chong Li wrote:
> > On Mon, Apr 4, 2016 at 10:14 AM, Andrew Cooper
> > <andrew.cooper3@citrix.com> wrote:
> > > On 01/04/16 05:59, Chong Li wrote:
> > > > 
> > > > --- a/xen/common/sched_credit2.c
> > > > +++ b/xen/common/sched_credit2.c
> > > > @@ -1421,14 +1421,12 @@ csched2_dom_cntl(
> > > >       * runq lock to update csvcs. */
> > > >      spin_lock_irqsave(&prv->lock, flags);
> > > > 
> > > > -    if ( op->cmd == XEN_DOMCTL_SCHEDOP_getinfo )
> > > > +    switch ( op->cmd )
> > > >      {
> > > > +    case XEN_DOMCTL_SCHEDOP_getinfo:
> > > >          op->u.credit2.weight = sdom->weight;
> > > > -    }
> > > > -    else
> > > > -    {
> > > > -        ASSERT(op->cmd == XEN_DOMCTL_SCHEDOP_putinfo);
> > > > -
> > > > +        break;
> > > > +    case XEN_DOMCTL_SCHEDOP_putinfo:
> > > >          if ( op->u.credit2.weight != 0 )
> > > >          {
> > > >              struct vcpu *v;
> > > > @@ -1457,6 +1455,9 @@ csched2_dom_cntl(
> > > >                  vcpu_schedule_unlock(lock, svc->vcpu);
> > > >              }
> > > >          }
> > > > +        break;
> > > > +    default:
> > > > +        return -EINVAL;
> > > As does this.
> > > 
> > > Please submit a bugfix ASAP.  This will become a security
> > > vulnerability
> > > if Xen 4.7 is shipped without it being fixed.
> > > 
> > > > 
> > > >      }
> > > > 
> > > >      spin_unlock_irqrestore(&prv->lock, flags);
> > Thanks for pointing this out.
> > 
> > Dario, do you want to include this bugfix in your cleanup patch, or
> > let me submit this?
> If you're around and can test it, it's probably better if you can
> send a
> patch right a way.
> 
Exactly. In fact:
 - we don't fold bugfixes in clanups,
 - I think I mentioned wanting to cleanup some code duplication in 
   libxl, this is in xen,
 - cleanups are delayed to 4.8, while this must be fixed before 
   release (or the patch/the whole feature be reverted).

So, if you can't work out a fix today or, at most, tomorrow, let me
know and I'll do it myself.

Sorry for not catching this during review... :-/

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: 181 bytes --]

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

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

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

* Re: [hypervisor deadlock] Re: [PATCH v9 for Xen 4.7 1/4] xen: enable per-VCPU parameter for RTDS
  2016-04-04 16:32         ` Dario Faggioli
@ 2016-04-04 16:47           ` Wei Liu
  2016-04-04 17:22             ` Chong Li
  0 siblings, 1 reply; 32+ messages in thread
From: Wei Liu @ 2016-04-04 16:47 UTC (permalink / raw)
  To: Dario Faggioli
  Cc: Chong Li, Wei Liu, Sisu Xi, George Dunlap, Andrew Cooper,
	George Dunlap, xen-devel, Meng Xu, Jan Beulich, Chong Li,
	Dagaen Golomb

On Mon, Apr 04, 2016 at 06:32:48PM +0200, Dario Faggioli wrote:
> On Mon, 2016-04-04 at 17:05 +0100, George Dunlap wrote:
> > On 04/04/16 16:58, Chong Li wrote:
> > > On Mon, Apr 4, 2016 at 10:14 AM, Andrew Cooper
> > > <andrew.cooper3@citrix.com> wrote:
> > > > On 01/04/16 05:59, Chong Li wrote:
> > > > > 
> > > > > --- a/xen/common/sched_credit2.c
> > > > > +++ b/xen/common/sched_credit2.c
> > > > > @@ -1421,14 +1421,12 @@ csched2_dom_cntl(
> > > > >       * runq lock to update csvcs. */
> > > > >      spin_lock_irqsave(&prv->lock, flags);
> > > > > 
> > > > > -    if ( op->cmd == XEN_DOMCTL_SCHEDOP_getinfo )
> > > > > +    switch ( op->cmd )
> > > > >      {
> > > > > +    case XEN_DOMCTL_SCHEDOP_getinfo:
> > > > >          op->u.credit2.weight = sdom->weight;
> > > > > -    }
> > > > > -    else
> > > > > -    {
> > > > > -        ASSERT(op->cmd == XEN_DOMCTL_SCHEDOP_putinfo);
> > > > > -
> > > > > +        break;
> > > > > +    case XEN_DOMCTL_SCHEDOP_putinfo:
> > > > >          if ( op->u.credit2.weight != 0 )
> > > > >          {
> > > > >              struct vcpu *v;
> > > > > @@ -1457,6 +1455,9 @@ csched2_dom_cntl(
> > > > >                  vcpu_schedule_unlock(lock, svc->vcpu);
> > > > >              }
> > > > >          }
> > > > > +        break;
> > > > > +    default:
> > > > > +        return -EINVAL;
> > > > As does this.
> > > > 
> > > > Please submit a bugfix ASAP.  This will become a security
> > > > vulnerability
> > > > if Xen 4.7 is shipped without it being fixed.
> > > > 
> > > > > 
> > > > >      }
> > > > > 
> > > > >      spin_unlock_irqrestore(&prv->lock, flags);
> > > Thanks for pointing this out.
> > > 
> > > Dario, do you want to include this bugfix in your cleanup patch, or
> > > let me submit this?
> > If you're around and can test it, it's probably better if you can
> > send a
> > patch right a way.
> > 
> Exactly. In fact:
>  - we don't fold bugfixes in clanups,
>  - I think I mentioned wanting to cleanup some code duplication in 
>    libxl, this is in xen,
>  - cleanups are delayed to 4.8, while this must be fixed before 
>    release (or the patch/the whole feature be reverted).
> 
> So, if you can't work out a fix today or, at most, tomorrow, let me
> know and I'll do it myself.
> 

Yes, please fix this by tomorrow. I think it is quite straightforward to
fix anyway.

Wei.

> Sorry for not catching this during review... :-/
> 
> 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)
> 



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

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

* Re: [hypervisor deadlock] Re: [PATCH v9 for Xen 4.7 1/4] xen: enable per-VCPU parameter for RTDS
  2016-04-04 16:47           ` Wei Liu
@ 2016-04-04 17:22             ` Chong Li
  0 siblings, 0 replies; 32+ messages in thread
From: Chong Li @ 2016-04-04 17:22 UTC (permalink / raw)
  To: Wei Liu
  Cc: Chong Li, Sisu Xi, George Dunlap, Andrew Cooper, Dario Faggioli,
	George Dunlap, xen-devel, Meng Xu, Jan Beulich, Dagaen Golomb

On Mon, Apr 4, 2016 at 11:47 AM, Wei Liu <wei.liu2@citrix.com> wrote:
> On Mon, Apr 04, 2016 at 06:32:48PM +0200, Dario Faggioli wrote:
>> On Mon, 2016-04-04 at 17:05 +0100, George Dunlap wrote:
>> > On 04/04/16 16:58, Chong Li wrote:
>> > > On Mon, Apr 4, 2016 at 10:14 AM, Andrew Cooper
>> > > <andrew.cooper3@citrix.com> wrote:
>> > > > On 01/04/16 05:59, Chong Li wrote:
>> > > > >
>> > > > > --- a/xen/common/sched_credit2.c
>> > > > > +++ b/xen/common/sched_credit2.c
>> > > > > @@ -1421,14 +1421,12 @@ csched2_dom_cntl(
>> > > > >       * runq lock to update csvcs. */
>> > > > >      spin_lock_irqsave(&prv->lock, flags);
>> > > > >
>> > > > > -    if ( op->cmd == XEN_DOMCTL_SCHEDOP_getinfo )
>> > > > > +    switch ( op->cmd )
>> > > > >      {
>> > > > > +    case XEN_DOMCTL_SCHEDOP_getinfo:
>> > > > >          op->u.credit2.weight = sdom->weight;
>> > > > > -    }
>> > > > > -    else
>> > > > > -    {
>> > > > > -        ASSERT(op->cmd == XEN_DOMCTL_SCHEDOP_putinfo);
>> > > > > -
>> > > > > +        break;
>> > > > > +    case XEN_DOMCTL_SCHEDOP_putinfo:
>> > > > >          if ( op->u.credit2.weight != 0 )
>> > > > >          {
>> > > > >              struct vcpu *v;
>> > > > > @@ -1457,6 +1455,9 @@ csched2_dom_cntl(
>> > > > >                  vcpu_schedule_unlock(lock, svc->vcpu);
>> > > > >              }
>> > > > >          }
>> > > > > +        break;
>> > > > > +    default:
>> > > > > +        return -EINVAL;
>> > > > As does this.
>> > > >
>> > > > Please submit a bugfix ASAP.  This will become a security
>> > > > vulnerability
>> > > > if Xen 4.7 is shipped without it being fixed.
>> > > >
>> > > > >
>> > > > >      }
>> > > > >
>> > > > >      spin_unlock_irqrestore(&prv->lock, flags);
>> > > Thanks for pointing this out.
>> > >
>> > > Dario, do you want to include this bugfix in your cleanup patch, or
>> > > let me submit this?
>> > If you're around and can test it, it's probably better if you can
>> > send a
>> > patch right a way.
>> >
>> Exactly. In fact:
>>  - we don't fold bugfixes in clanups,
>>  - I think I mentioned wanting to cleanup some code duplication in
>>    libxl, this is in xen,
>>  - cleanups are delayed to 4.8, while this must be fixed before
>>    release (or the patch/the whole feature be reverted).
>>
>> So, if you can't work out a fix today or, at most, tomorrow, let me
>> know and I'll do it myself.
>>
>
> Yes, please fix this by tomorrow. I think it is quite straightforward to
> fix anyway.
>
> Wei.
>

Will do.



-- 
Chong Li
Department of Computer Science and Engineering
Washington University in St.louis

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

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

* Re: [PATCH v9 for Xen 4.7 3/4] libxl: enable per-VCPU parameter for RTDS
  2016-04-01 13:40   ` Wei Liu
@ 2016-04-06 14:32     ` Ian Jackson
  2016-04-06 14:55       ` Ian Jackson
  0 siblings, 1 reply; 32+ messages in thread
From: Ian Jackson @ 2016-04-06 14:32 UTC (permalink / raw)
  To: Wei Liu
  Cc: Chong Li, Sisu Xi, george.dunlap, dario.faggioli, xen-devel,
	ian.campbell, Meng Xu, Chong Li, dgolomb

Wei Liu writes ("Re: [PATCH v9 for Xen 4.7 3/4] libxl: enable per-VCPU parameter for RTDS"):
> On Thu, Mar 31, 2016 at 11:59:45PM -0500, Chong Li wrote:
> > Add libxl_vcpu_sched_params_get/set and sched_rtds_vcpu_get/set
> > functions to support per-VCPU settings.
> > 
> > Signed-off-by: Chong Li <chong.li@wustl.edu>
> > Signed-off-by: Meng Xu <mengxu@cis.upenn.edu>
> > Signed-off-by: Sisu Xi <xisisu@gmail.com>
> > 
> > Acked-by: Wei Liu <wei.liu2@citrix.com>
> 
> This should have been dropped.
...
> 
> These two functions LGTM, so I shall ack this patch again:
> 
>   Acked-by: Wei Liu <wei.liu2@citrix.com>

I have queued this patch.

Ian.

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

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

* Re: [PATCH v9 for Xen 4.7 3/4] libxl: enable per-VCPU parameter for RTDS
  2016-04-06 14:32     ` Ian Jackson
@ 2016-04-06 14:55       ` Ian Jackson
  2016-04-06 15:04         ` Wei Liu
  0 siblings, 1 reply; 32+ messages in thread
From: Ian Jackson @ 2016-04-06 14:55 UTC (permalink / raw)
  To: Chong Li, Chong Li
  Cc: Wei Liu, Sisu Xi, george.dunlap, dario.faggioli, xen-devel,
	ian.campbell, Meng Xu, dgolomb

Ian Jackson writes ("Re: [PATCH v9 for Xen 4.7 3/4] libxl: enable per-VCPU parameter for RTDS"):
> Wei Liu writes ("Re: [PATCH v9 for Xen 4.7 3/4] libxl: enable per-VCPU parameter for RTDS"):
> > These two functions LGTM, so I shall ack this patch again:
> > 
> >   Acked-by: Wei Liu <wei.liu2@citrix.com>
> 
> I have queued this patch.

It broke the build, see below.  Was this patch build-tested ?
I have dropped it.

Ian.

libxl.c: In function 'sched_rtds_vcpu_get':
libxl.c:5980:5: error: implicit declaration of function 'xc_sched_rtds_vcpu_get' [-Werror=implicit-function-declaration]
libxl.c: In function 'sched_rtds_vcpu_set':
libxl.c:6088:5: error: implicit declaration of function 'xc_sched_rtds_vcpu_set' [-Werror=implicit-function-declaration]

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

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

* Re: [PATCH v9 for Xen 4.7 3/4] libxl: enable per-VCPU parameter for RTDS
  2016-04-06 14:55       ` Ian Jackson
@ 2016-04-06 15:04         ` Wei Liu
  2016-04-06 15:22           ` Ian Jackson
  0 siblings, 1 reply; 32+ messages in thread
From: Wei Liu @ 2016-04-06 15:04 UTC (permalink / raw)
  To: Ian Jackson
  Cc: Chong Li, Wei Liu, Sisu Xi, george.dunlap, dario.faggioli,
	xen-devel, ian.campbell, Meng Xu, Chong Li, dgolomb

On Wed, Apr 06, 2016 at 03:55:25PM +0100, Ian Jackson wrote:
> Ian Jackson writes ("Re: [PATCH v9 for Xen 4.7 3/4] libxl: enable per-VCPU parameter for RTDS"):
> > Wei Liu writes ("Re: [PATCH v9 for Xen 4.7 3/4] libxl: enable per-VCPU parameter for RTDS"):
> > > These two functions LGTM, so I shall ack this patch again:
> > > 
> > >   Acked-by: Wei Liu <wei.liu2@citrix.com>
> > 
> > I have queued this patch.
> 
> It broke the build, see below.  Was this patch build-tested ?
> I have dropped it.
> 
> Ian.
> 
> libxl.c: In function 'sched_rtds_vcpu_get':
> libxl.c:5980:5: error: implicit declaration of function 'xc_sched_rtds_vcpu_get' [-Werror=implicit-function-declaration]
> libxl.c: In function 'sched_rtds_vcpu_set':
> libxl.c:6088:5: error: implicit declaration of function 'xc_sched_rtds_vcpu_set' [-Werror=implicit-function-declaration]

Hi Chong,

I could have fixed this for you, but I think it would be useful to you
if you address this problem.

I suggest you:

1. Fold in Reviewed-by and Acked-by tags to your patches.
2. Rebase these three patches on top of staging branch.
3. Use the following rune to build test every single commit and fix the
   problem if it fails.
   
   $ git rebase -i HEAD~3 --exec "make -j4 clean && ./configure && \
        make -j4 dist"

It will stop rebasing if one commit doesn't build. This is what I
normally do when developing.

Note that the deadline for applying patches is Friday, so if you can
refresh this by tomorrow that would be good.


Wei.

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

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

* Re: [PATCH v9 for Xen 4.7 3/4] libxl: enable per-VCPU parameter for RTDS
  2016-04-06 15:04         ` Wei Liu
@ 2016-04-06 15:22           ` Ian Jackson
  2016-04-06 15:38             ` Ian Jackson
  0 siblings, 1 reply; 32+ messages in thread
From: Ian Jackson @ 2016-04-06 15:22 UTC (permalink / raw)
  To: Wei Liu
  Cc: Chong Li, Sisu Xi, george.dunlap, dario.faggioli, xen-devel,
	ian.campbell, Meng Xu, Chong Li, dgolomb

Wei Liu writes ("Re: [PATCH v9 for Xen 4.7 3/4] libxl: enable per-VCPU parameter for RTDS"):
> On Wed, Apr 06, 2016 at 03:55:25PM +0100, Ian Jackson wrote:
> > libxl.c: In function 'sched_rtds_vcpu_get':
> > libxl.c:5980:5: error: implicit declaration of function 'xc_sched_rtds_vcpu_get' [-Werror=implicit-function-declaration]
> > libxl.c: In function 'sched_rtds_vcpu_set':
> > libxl.c:6088:5: error: implicit declaration of function 'xc_sched_rtds_vcpu_set' [-Werror=implicit-function-declaration]
> 
...
> I could have fixed this for you, but I think it would be useful to you
> if you address this problem.
> 
> I suggest you:
> 
> 1. Fold in Reviewed-by and Acked-by tags to your patches.
> 2. Rebase these three patches on top of staging branch.
> 3. Use the following rune to build test every single commit and fix the
>    problem if it fails.
>    
>    $ git rebase -i HEAD~3 --exec "make -j4 clean && ./configure && \
>         make -j4 dist"
> 
> It will stop rebasing if one commit doesn't build. This is what I
> normally do when developing.
> 
> Note that the deadline for applying patches is Friday, so if you can
> refresh this by tomorrow that would be good.

Dario points out on irc that perhaps the problem is that I didn't
apply 2/4.  I wasn't CC'd on 2/4, so I foolishly assumed it was a
hypervisor patch (and the HV parts are already in tree).

I will check my view of the xen-devel list.

Ian.

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

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

* Re: [PATCH v9 for Xen 4.7 3/4] libxl: enable per-VCPU parameter for RTDS
  2016-04-06 15:22           ` Ian Jackson
@ 2016-04-06 15:38             ` Ian Jackson
  2016-04-06 16:36               ` Dario Faggioli
  0 siblings, 1 reply; 32+ messages in thread
From: Ian Jackson @ 2016-04-06 15:38 UTC (permalink / raw)
  To: Wei Liu, Chong Li, Chong Li, xen-devel, Meng Xu, Sisu Xi,
	dario.faggioli, george.dunlap, dgolomb, ian.campbell

Ian Jackson writes ("Re: [PATCH v9 for Xen 4.7 3/4] libxl: enable per-VCPU parameter for RTDS"):
> Dario points out on irc that perhaps the problem is that I didn't
> apply 2/4.  I wasn't CC'd on 2/4, so I foolishly assumed it was a
> hypervisor patch (and the HV parts are already in tree).
> 
> I will check my view of the xen-devel list.

Indeed.  With 2/4 it builds.  4/4 was also not CC'd to me.  I used a
copy from the list.

I have pushed all four.

Ian.

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

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

* Re: [PATCH v9 for Xen 4.7 3/4] libxl: enable per-VCPU parameter for RTDS
  2016-04-06 15:38             ` Ian Jackson
@ 2016-04-06 16:36               ` Dario Faggioli
  2016-04-06 16:41                 ` Chong Li
  0 siblings, 1 reply; 32+ messages in thread
From: Dario Faggioli @ 2016-04-06 16:36 UTC (permalink / raw)
  To: Ian Jackson, Chong Lee
  Cc: Chong Li, Wei Liu, Sisu Xi, george.dunlap, xen-devel, Meng Xu, dgolomb


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

On Wed, 2016-04-06 at 16:38 +0100, Ian Jackson wrote:
> Ian Jackson writes ("Re: [PATCH v9 for Xen 4.7 3/4] libxl: enable
> per-VCPU parameter for RTDS"):
> > 
> > Dario points out on irc that perhaps the problem is that I didn't
> > apply 2/4.  I wasn't CC'd on 2/4, so I foolishly assumed it was a
> > hypervisor patch (and the HV parts are already in tree).
> > 
> > I will check my view of the xen-devel list.
> Indeed.  With 2/4 it builds.  4/4 was also not CC'd to me.  I used a
> copy from the list.
> 
> I have pushed all four.
> 
Thanks Ian!

So, Chong, clearly, the build failure was not your fault (as there is
no actual build failure), but please, always double check (even when
sending new versions of a series) that the appropriate maintainers are
Cc-ed... This would help limiting problems like this one we've seen
here.

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: 181 bytes --]

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

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

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

* Re: [PATCH v9 for Xen 4.7 3/4] libxl: enable per-VCPU parameter for RTDS
  2016-04-06 16:36               ` Dario Faggioli
@ 2016-04-06 16:41                 ` Chong Li
  2016-04-06 18:54                   ` Andrew Cooper
  0 siblings, 1 reply; 32+ messages in thread
From: Chong Li @ 2016-04-06 16:41 UTC (permalink / raw)
  To: Dario Faggioli
  Cc: Chong Li, Wei Liu, Sisu Xi, George Dunlap, Ian Jackson,
	xen-devel, Meng Xu, Dagaen Golomb

On Wed, Apr 6, 2016 at 11:36 AM, Dario Faggioli
<dario.faggioli@citrix.com> wrote:
> On Wed, 2016-04-06 at 16:38 +0100, Ian Jackson wrote:
>> Ian Jackson writes ("Re: [PATCH v9 for Xen 4.7 3/4] libxl: enable
>> per-VCPU parameter for RTDS"):
>> >
>> > Dario points out on irc that perhaps the problem is that I didn't
>> > apply 2/4.  I wasn't CC'd on 2/4, so I foolishly assumed it was a
>> > hypervisor patch (and the HV parts are already in tree).
>> >
>> > I will check my view of the xen-devel list.
>> Indeed.  With 2/4 it builds.  4/4 was also not CC'd to me.  I used a
>> copy from the list.
>>
>> I have pushed all four.
>>
> Thanks Ian!
>
> So, Chong, clearly, the build failure was not your fault (as there is
> no actual build failure), but please, always double check (even when
> sending new versions of a series) that the appropriate maintainers are
> Cc-ed... This would help limiting problems like this one we've seen
> here.
>
Yes, I'll.

Thanks for your help on this.
Chong



-- 
Chong Li
Department of Computer Science and Engineering
Washington University in St.louis

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

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

* Re: [PATCH v9 for Xen 4.7 3/4] libxl: enable per-VCPU parameter for RTDS
  2016-04-06 16:41                 ` Chong Li
@ 2016-04-06 18:54                   ` Andrew Cooper
  2016-04-06 19:20                     ` Chong Li
  0 siblings, 1 reply; 32+ messages in thread
From: Andrew Cooper @ 2016-04-06 18:54 UTC (permalink / raw)
  To: Chong Li, Dario Faggioli
  Cc: Chong Li, Wei Liu, Sisu Xi, George Dunlap, Ian Jackson,
	xen-devel, Meng Xu, Dagaen Golomb

On 06/04/16 17:41, Chong Li wrote:
> On Wed, Apr 6, 2016 at 11:36 AM, Dario Faggioli
> <dario.faggioli@citrix.com> wrote:
>> On Wed, 2016-04-06 at 16:38 +0100, Ian Jackson wrote:
>>> Ian Jackson writes ("Re: [PATCH v9 for Xen 4.7 3/4] libxl: enable
>>> per-VCPU parameter for RTDS"):
>>>> Dario points out on irc that perhaps the problem is that I didn't
>>>> apply 2/4.  I wasn't CC'd on 2/4, so I foolishly assumed it was a
>>>> hypervisor patch (and the HV parts are already in tree).
>>>>
>>>> I will check my view of the xen-devel list.
>>> Indeed.  With 2/4 it builds.  4/4 was also not CC'd to me.  I used a
>>> copy from the list.
>>>
>>> I have pushed all four.
>>>
>> Thanks Ian!
>>
>> So, Chong, clearly, the build failure was not your fault (as there is
>> no actual build failure), but please, always double check (even when
>> sending new versions of a series) that the appropriate maintainers are
>> Cc-ed... This would help limiting problems like this one we've seen
>> here.
>>
> Yes, I'll.
>
> Thanks for your help on this.
> Chong

Yet another build failure on CentOS.

xc_rt.c: In function 'xc_sched_rtds_vcpu_set':
xc_rt.c:71:9: error: 'rc' may be used uninitialized in this function
[-Werror=maybe-uninitialized]
     int rc;
         ^
xc_rt.c: In function 'xc_sched_rtds_vcpu_get':
xc_rt.c:105:9: error: 'rc' may be used uninitialized in this function
[-Werror=maybe-uninitialized]
     int rc;
         ^
cc1: all warnings being treated as errors

In both cases, if your while loop doesn't execute (i.e. the user passes
num_vcpus = 0), rc is genuinely uninialised when used at the end of the
function.

~Andrew

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

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

* Re: [PATCH v9 for Xen 4.7 3/4] libxl: enable per-VCPU parameter for RTDS
  2016-04-06 18:54                   ` Andrew Cooper
@ 2016-04-06 19:20                     ` Chong Li
  2016-04-06 19:23                       ` Andrew Cooper
  2016-04-06 19:30                       ` Wei Liu
  0 siblings, 2 replies; 32+ messages in thread
From: Chong Li @ 2016-04-06 19:20 UTC (permalink / raw)
  To: Andrew Cooper
  Cc: Chong Li, Wei Liu, Sisu Xi, George Dunlap, Dario Faggioli,
	Ian Jackson, xen-devel, Meng Xu, Dagaen Golomb

On Wed, Apr 6, 2016 at 1:54 PM, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> On 06/04/16 17:41, Chong Li wrote:
>> On Wed, Apr 6, 2016 at 11:36 AM, Dario Faggioli
>> <dario.faggioli@citrix.com> wrote:
>>> On Wed, 2016-04-06 at 16:38 +0100, Ian Jackson wrote:
>>>> Ian Jackson writes ("Re: [PATCH v9 for Xen 4.7 3/4] libxl: enable
>>>> per-VCPU parameter for RTDS"):
>>>>> Dario points out on irc that perhaps the problem is that I didn't
>>>>> apply 2/4.  I wasn't CC'd on 2/4, so I foolishly assumed it was a
>>>>> hypervisor patch (and the HV parts are already in tree).
>>>>>
>>>>> I will check my view of the xen-devel list.
>>>> Indeed.  With 2/4 it builds.  4/4 was also not CC'd to me.  I used a
>>>> copy from the list.
>>>>
>>>> I have pushed all four.
>>>>
>>> Thanks Ian!
>>>
>>> So, Chong, clearly, the build failure was not your fault (as there is
>>> no actual build failure), but please, always double check (even when
>>> sending new versions of a series) that the appropriate maintainers are
>>> Cc-ed... This would help limiting problems like this one we've seen
>>> here.
>>>
>> Yes, I'll.
>>
>> Thanks for your help on this.
>> Chong
>
> Yet another build failure on CentOS.
>
> xc_rt.c: In function 'xc_sched_rtds_vcpu_set':
> xc_rt.c:71:9: error: 'rc' may be used uninitialized in this function
> [-Werror=maybe-uninitialized]
>      int rc;
>          ^
> xc_rt.c: In function 'xc_sched_rtds_vcpu_get':
> xc_rt.c:105:9: error: 'rc' may be used uninitialized in this function
> [-Werror=maybe-uninitialized]
>      int rc;
>          ^
> cc1: all warnings being treated as errors
>
> In both cases, if your while loop doesn't execute (i.e. the user passes
> num_vcpus = 0), rc is genuinely uninialised when used at the end of the
> function.
>
> ~Andrew

I see. I can do a sanity check on num_vcpus before the while loop.

Do I have to re-send the whole patch series? Or maybe just something
like a bug fix patch?

Chong

-- 
Chong Li
Department of Computer Science and Engineering
Washington University in St.louis

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

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

* Re: [PATCH v9 for Xen 4.7 3/4] libxl: enable per-VCPU parameter for RTDS
  2016-04-06 19:20                     ` Chong Li
@ 2016-04-06 19:23                       ` Andrew Cooper
  2016-04-06 19:30                       ` Wei Liu
  1 sibling, 0 replies; 32+ messages in thread
From: Andrew Cooper @ 2016-04-06 19:23 UTC (permalink / raw)
  To: Chong Li
  Cc: Chong Li, Wei Liu, Sisu Xi, George Dunlap, Dario Faggioli,
	Ian Jackson, xen-devel, Meng Xu, Dagaen Golomb

On 06/04/16 20:20, Chong Li wrote:
> On Wed, Apr 6, 2016 at 1:54 PM, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> On 06/04/16 17:41, Chong Li wrote:
>>> On Wed, Apr 6, 2016 at 11:36 AM, Dario Faggioli
>>> <dario.faggioli@citrix.com> wrote:
>>>> On Wed, 2016-04-06 at 16:38 +0100, Ian Jackson wrote:
>>>>> Ian Jackson writes ("Re: [PATCH v9 for Xen 4.7 3/4] libxl: enable
>>>>> per-VCPU parameter for RTDS"):
>>>>>> Dario points out on irc that perhaps the problem is that I didn't
>>>>>> apply 2/4.  I wasn't CC'd on 2/4, so I foolishly assumed it was a
>>>>>> hypervisor patch (and the HV parts are already in tree).
>>>>>>
>>>>>> I will check my view of the xen-devel list.
>>>>> Indeed.  With 2/4 it builds.  4/4 was also not CC'd to me.  I used a
>>>>> copy from the list.
>>>>>
>>>>> I have pushed all four.
>>>>>
>>>> Thanks Ian!
>>>>
>>>> So, Chong, clearly, the build failure was not your fault (as there is
>>>> no actual build failure), but please, always double check (even when
>>>> sending new versions of a series) that the appropriate maintainers are
>>>> Cc-ed... This would help limiting problems like this one we've seen
>>>> here.
>>>>
>>> Yes, I'll.
>>>
>>> Thanks for your help on this.
>>> Chong
>> Yet another build failure on CentOS.
>>
>> xc_rt.c: In function 'xc_sched_rtds_vcpu_set':
>> xc_rt.c:71:9: error: 'rc' may be used uninitialized in this function
>> [-Werror=maybe-uninitialized]
>>      int rc;
>>          ^
>> xc_rt.c: In function 'xc_sched_rtds_vcpu_get':
>> xc_rt.c:105:9: error: 'rc' may be used uninitialized in this function
>> [-Werror=maybe-uninitialized]
>>      int rc;
>>          ^
>> cc1: all warnings being treated as errors
>>
>> In both cases, if your while loop doesn't execute (i.e. the user passes
>> num_vcpus = 0), rc is genuinely uninialised when used at the end of the
>> function.
>>
>> ~Andrew
> I see. I can do a sanity check on num_vcpus before the while loop.
>
> Do I have to re-send the whole patch series? Or maybe just something
> like a bug fix patch?

The series has been committed.

Please send a new patch based on top of the existing `staging` branch.

~Andrew

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

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

* Re: [PATCH v9 for Xen 4.7 3/4] libxl: enable per-VCPU parameter for RTDS
  2016-04-06 19:20                     ` Chong Li
  2016-04-06 19:23                       ` Andrew Cooper
@ 2016-04-06 19:30                       ` Wei Liu
  2016-04-06 19:41                         ` Chong Li
  1 sibling, 1 reply; 32+ messages in thread
From: Wei Liu @ 2016-04-06 19:30 UTC (permalink / raw)
  To: Chong Li
  Cc: Chong Li, Wei Liu, Sisu Xi, George Dunlap, Andrew Cooper,
	Dario Faggioli, Ian Jackson, xen-devel, Meng Xu, Dagaen Golomb

On Wed, Apr 06, 2016 at 02:20:55PM -0500, Chong Li wrote:
> On Wed, Apr 6, 2016 at 1:54 PM, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> > On 06/04/16 17:41, Chong Li wrote:
> >> On Wed, Apr 6, 2016 at 11:36 AM, Dario Faggioli
> >> <dario.faggioli@citrix.com> wrote:
> >>> On Wed, 2016-04-06 at 16:38 +0100, Ian Jackson wrote:
> >>>> Ian Jackson writes ("Re: [PATCH v9 for Xen 4.7 3/4] libxl: enable
> >>>> per-VCPU parameter for RTDS"):
> >>>>> Dario points out on irc that perhaps the problem is that I didn't
> >>>>> apply 2/4.  I wasn't CC'd on 2/4, so I foolishly assumed it was a
> >>>>> hypervisor patch (and the HV parts are already in tree).
> >>>>>
> >>>>> I will check my view of the xen-devel list.
> >>>> Indeed.  With 2/4 it builds.  4/4 was also not CC'd to me.  I used a
> >>>> copy from the list.
> >>>>
> >>>> I have pushed all four.
> >>>>
> >>> Thanks Ian!
> >>>
> >>> So, Chong, clearly, the build failure was not your fault (as there is
> >>> no actual build failure), but please, always double check (even when
> >>> sending new versions of a series) that the appropriate maintainers are
> >>> Cc-ed... This would help limiting problems like this one we've seen
> >>> here.
> >>>
> >> Yes, I'll.
> >>
> >> Thanks for your help on this.
> >> Chong
> >
> > Yet another build failure on CentOS.
> >
> > xc_rt.c: In function 'xc_sched_rtds_vcpu_set':
> > xc_rt.c:71:9: error: 'rc' may be used uninitialized in this function
> > [-Werror=maybe-uninitialized]
> >      int rc;
> >          ^
> > xc_rt.c: In function 'xc_sched_rtds_vcpu_get':
> > xc_rt.c:105:9: error: 'rc' may be used uninitialized in this function
> > [-Werror=maybe-uninitialized]
> >      int rc;
> >          ^
> > cc1: all warnings being treated as errors
> >
> > In both cases, if your while loop doesn't execute (i.e. the user passes
> > num_vcpus = 0), rc is genuinely uninialised when used at the end of the
> > function.
> >
> > ~Andrew
> 
> I see. I can do a sanity check on num_vcpus before the while loop.
> 

Not sure what kind of sanity check you were thinking about. But you can
just set rc = 0 at the beginning of each function. That semantics should
be sensible enough. What do you think?

> Do I have to re-send the whole patch series? Or maybe just something
> like a bug fix patch?
> 

Please send a patch on top of staging branch. This series has already
been committed.

Wei.

> Chong
> 
> -- 
> Chong Li
> Department of Computer Science and Engineering
> Washington University in St.louis

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

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

* Re: [PATCH v9 for Xen 4.7 3/4] libxl: enable per-VCPU parameter for RTDS
  2016-04-06 19:30                       ` Wei Liu
@ 2016-04-06 19:41                         ` Chong Li
  2016-04-06 19:49                           ` Wei Liu
  0 siblings, 1 reply; 32+ messages in thread
From: Chong Li @ 2016-04-06 19:41 UTC (permalink / raw)
  To: Wei Liu
  Cc: Chong Li, Sisu Xi, George Dunlap, Andrew Cooper, Dario Faggioli,
	Ian Jackson, xen-devel, Meng Xu, Dagaen Golomb

On Wed, Apr 6, 2016 at 2:30 PM, Wei Liu <wei.liu2@citrix.com> wrote:
> On Wed, Apr 06, 2016 at 02:20:55PM -0500, Chong Li wrote:
>> On Wed, Apr 6, 2016 at 1:54 PM, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> > On 06/04/16 17:41, Chong Li wrote:
>> >> On Wed, Apr 6, 2016 at 11:36 AM, Dario Faggioli
>> >> <dario.faggioli@citrix.com> wrote:
>> >>> On Wed, 2016-04-06 at 16:38 +0100, Ian Jackson wrote:
>> >>>> Ian Jackson writes ("Re: [PATCH v9 for Xen 4.7 3/4] libxl: enable
>> >>>> per-VCPU parameter for RTDS"):
>> >>>>> Dario points out on irc that perhaps the problem is that I didn't
>> >>>>> apply 2/4.  I wasn't CC'd on 2/4, so I foolishly assumed it was a
>> >>>>> hypervisor patch (and the HV parts are already in tree).
>> >>>>>
>> >>>>> I will check my view of the xen-devel list.
>> >>>> Indeed.  With 2/4 it builds.  4/4 was also not CC'd to me.  I used a
>> >>>> copy from the list.
>> >>>>
>> >>>> I have pushed all four.
>> >>>>
>> >>> Thanks Ian!
>> >>>
>> >>> So, Chong, clearly, the build failure was not your fault (as there is
>> >>> no actual build failure), but please, always double check (even when
>> >>> sending new versions of a series) that the appropriate maintainers are
>> >>> Cc-ed... This would help limiting problems like this one we've seen
>> >>> here.
>> >>>
>> >> Yes, I'll.
>> >>
>> >> Thanks for your help on this.
>> >> Chong
>> >
>> > Yet another build failure on CentOS.
>> >
>> > xc_rt.c: In function 'xc_sched_rtds_vcpu_set':
>> > xc_rt.c:71:9: error: 'rc' may be used uninitialized in this function
>> > [-Werror=maybe-uninitialized]
>> >      int rc;
>> >          ^
>> > xc_rt.c: In function 'xc_sched_rtds_vcpu_get':
>> > xc_rt.c:105:9: error: 'rc' may be used uninitialized in this function
>> > [-Werror=maybe-uninitialized]
>> >      int rc;
>> >          ^
>> > cc1: all warnings being treated as errors
>> >
>> > In both cases, if your while loop doesn't execute (i.e. the user passes
>> > num_vcpus = 0), rc is genuinely uninialised when used at the end of the
>> > function.
>> >
>> > ~Andrew
>>
>> I see. I can do a sanity check on num_vcpus before the while loop.
>>
>
> Not sure what kind of sanity check you were thinking about. But you can
> just set rc = 0 at the beginning of each function. That semantics should
> be sensible enough. What do you think?
Yes, I can do this change. But didn't you or Dario say that rc should not be
initialized at the beginning of a function?

>
>> Do I have to re-send the whole patch series? Or maybe just something
>> like a bug fix patch?
>>
>
> Please send a patch on top of staging branch. This series has already
> been committed.
>
Sure.



-- 
Chong Li
Department of Computer Science and Engineering
Washington University in St.louis

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

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

* Re: [PATCH v9 for Xen 4.7 3/4] libxl: enable per-VCPU parameter for RTDS
  2016-04-06 19:41                         ` Chong Li
@ 2016-04-06 19:49                           ` Wei Liu
  2016-04-06 20:01                             ` Dario Faggioli
  0 siblings, 1 reply; 32+ messages in thread
From: Wei Liu @ 2016-04-06 19:49 UTC (permalink / raw)
  To: Chong Li
  Cc: Chong Li, Wei Liu, Sisu Xi, George Dunlap, Andrew Cooper,
	Dario Faggioli, Ian Jackson, xen-devel, Meng Xu, Dagaen Golomb

On Wed, Apr 06, 2016 at 02:41:31PM -0500, Chong Li wrote:
> On Wed, Apr 6, 2016 at 2:30 PM, Wei Liu <wei.liu2@citrix.com> wrote:
> > On Wed, Apr 06, 2016 at 02:20:55PM -0500, Chong Li wrote:
> >> On Wed, Apr 6, 2016 at 1:54 PM, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> >> > On 06/04/16 17:41, Chong Li wrote:
> >> >> On Wed, Apr 6, 2016 at 11:36 AM, Dario Faggioli
> >> >> <dario.faggioli@citrix.com> wrote:
> >> >>> On Wed, 2016-04-06 at 16:38 +0100, Ian Jackson wrote:
> >> >>>> Ian Jackson writes ("Re: [PATCH v9 for Xen 4.7 3/4] libxl: enable
> >> >>>> per-VCPU parameter for RTDS"):
> >> >>>>> Dario points out on irc that perhaps the problem is that I didn't
> >> >>>>> apply 2/4.  I wasn't CC'd on 2/4, so I foolishly assumed it was a
> >> >>>>> hypervisor patch (and the HV parts are already in tree).
> >> >>>>>
> >> >>>>> I will check my view of the xen-devel list.
> >> >>>> Indeed.  With 2/4 it builds.  4/4 was also not CC'd to me.  I used a
> >> >>>> copy from the list.
> >> >>>>
> >> >>>> I have pushed all four.
> >> >>>>
> >> >>> Thanks Ian!
> >> >>>
> >> >>> So, Chong, clearly, the build failure was not your fault (as there is
> >> >>> no actual build failure), but please, always double check (even when
> >> >>> sending new versions of a series) that the appropriate maintainers are
> >> >>> Cc-ed... This would help limiting problems like this one we've seen
> >> >>> here.
> >> >>>
> >> >> Yes, I'll.
> >> >>
> >> >> Thanks for your help on this.
> >> >> Chong
> >> >
> >> > Yet another build failure on CentOS.
> >> >
> >> > xc_rt.c: In function 'xc_sched_rtds_vcpu_set':
> >> > xc_rt.c:71:9: error: 'rc' may be used uninitialized in this function
> >> > [-Werror=maybe-uninitialized]
> >> >      int rc;
> >> >          ^
> >> > xc_rt.c: In function 'xc_sched_rtds_vcpu_get':
> >> > xc_rt.c:105:9: error: 'rc' may be used uninitialized in this function
> >> > [-Werror=maybe-uninitialized]
> >> >      int rc;
> >> >          ^
> >> > cc1: all warnings being treated as errors
> >> >
> >> > In both cases, if your while loop doesn't execute (i.e. the user passes
> >> > num_vcpus = 0), rc is genuinely uninialised when used at the end of the
> >> > function.
> >> >
> >> > ~Andrew
> >>
> >> I see. I can do a sanity check on num_vcpus before the while loop.
> >>
> >
> > Not sure what kind of sanity check you were thinking about. But you can
> > just set rc = 0 at the beginning of each function. That semantics should
> > be sensible enough. What do you think?
> Yes, I can do this change. But didn't you or Dario say that rc should not be
> initialized at the beginning of a function?
> 

I'm fine with bending the rules a bit to make life easier. For a simple
function like this I won't argue one way or another. Besides there isn't
really a CODING_STYLE file in libxc.

But really what I care about is to not return an uninitialised rc. If
you don't want to set rc to 0 at the beginning of the function, you can
do it before the loop -- that should both fix the error and make Dario
happy. :-)

Wei.

> >
> >> Do I have to re-send the whole patch series? Or maybe just something
> >> like a bug fix patch?
> >>
> >
> > Please send a patch on top of staging branch. This series has already
> > been committed.
> >
> Sure.
> 
> 
> 
> -- 
> Chong Li
> Department of Computer Science and Engineering
> Washington University in St.louis

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

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

* Re: [PATCH v9 for Xen 4.7 3/4] libxl: enable per-VCPU parameter for RTDS
  2016-04-06 19:49                           ` Wei Liu
@ 2016-04-06 20:01                             ` Dario Faggioli
  0 siblings, 0 replies; 32+ messages in thread
From: Dario Faggioli @ 2016-04-06 20:01 UTC (permalink / raw)
  To: Wei Liu, Chong Li
  Cc: Chong Li, Sisu Xi, George Dunlap, Andrew Cooper, Ian Jackson,
	xen-devel, Meng Xu, Dagaen Golomb


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

On Wed, 2016-04-06 at 20:49 +0100, Wei Liu wrote:
> On Wed, Apr 06, 2016 at 02:41:31PM -0500, Chong Li wrote:
> > On Wed, Apr 6, 2016 at 2:30 PM, Wei Liu <wei.liu2@citrix.com>
> > wrote:
> > > On Wed, Apr 06, 2016 at 02:20:55PM -0500, Chong Li wrote:
> > > Not sure what kind of sanity check you were thinking about. But
> > > you can
> > > just set rc = 0 at the beginning of each function. That semantics
> > > should
> > > be sensible enough. What do you think?
> > Yes, I can do this change. But didn't you or Dario say that rc
> > should not be
> > initialized at the beginning of a function?
> > 
> I'm fine with bending the rules a bit to make life easier. For a
> simple
> function like this I won't argue one way or another. Besides there
> isn't
> really a CODING_STYLE file in libxc.
> 
I did say it, but it wasn't because of coding style, especially
considering, as Wei says, that we're in libxc. I did because it looked
a pointless (and hence avoidable) initialization.

Of course, I was wrong, as I did not see the issue in case we get
passed 0. Also, weirdly enough, my compiler did not spot this (and I
build on Debian unstable, so with a fairly recent gcc).

In any case, I don't think it make much sense to deal with num_vcpus=0,
and in fact, in libxl, we reject it. But as a matter of fact, the
easiest and cleanest way of "not dealing" with it, here in libxc, is
really to just initialize rc to zero, let the loop be skipped and carry
on.

More advanced sanity checking is better done at both higher and lower
levels, and both are happening already, so we're fine.

> But really what I care about is to not return an uninitialised rc. If
> you don't want to set rc to 0 at the beginning of the function, you
> can
> do it before the loop -- that should both fix the error and make
> Dario
> happy. :-)
> 
Thanks for this effort of making me happy. :-)

Jokes apart, just initialize it and get done with this. Again, it's the
cleanest and simplest way, and I _am_ happy with it.

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: 181 bytes --]

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

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

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

end of thread, other threads:[~2016-04-06 20:01 UTC | newest]

Thread overview: 32+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-04-01  4:59 [PATCH v9 for Xen 4.7 0/4] Enable per-VCPU parameter for RTDS Chong Li
2016-04-01  4:59 ` [PATCH v9 for Xen 4.7 1/4] xen: enable " Chong Li
2016-04-01  9:35   ` Jan Beulich
2016-04-01 12:37   ` Dario Faggioli
2016-04-04 15:14   ` [hypervisor deadlock] " Andrew Cooper
2016-04-04 15:58     ` Chong Li
2016-04-04 16:05       ` George Dunlap
2016-04-04 16:32         ` Dario Faggioli
2016-04-04 16:47           ` Wei Liu
2016-04-04 17:22             ` Chong Li
2016-04-01  4:59 ` [PATCH v9 for Xen 4.7 2/4] libxc: " Chong Li
2016-04-01  4:59 ` [PATCH v9 for Xen 4.7 3/4] libxl: " Chong Li
2016-04-01 12:53   ` Dario Faggioli
2016-04-01 13:40   ` Wei Liu
2016-04-06 14:32     ` Ian Jackson
2016-04-06 14:55       ` Ian Jackson
2016-04-06 15:04         ` Wei Liu
2016-04-06 15:22           ` Ian Jackson
2016-04-06 15:38             ` Ian Jackson
2016-04-06 16:36               ` Dario Faggioli
2016-04-06 16:41                 ` Chong Li
2016-04-06 18:54                   ` Andrew Cooper
2016-04-06 19:20                     ` Chong Li
2016-04-06 19:23                       ` Andrew Cooper
2016-04-06 19:30                       ` Wei Liu
2016-04-06 19:41                         ` Chong Li
2016-04-06 19:49                           ` Wei Liu
2016-04-06 20:01                             ` Dario Faggioli
2016-04-01  4:59 ` [PATCH v9 for Xen 4.7 4/4] xl: " Chong Li
2016-04-01 13:15   ` Dario Faggioli
2016-04-01 13:43     ` Wei Liu
2016-04-01 13:51       ` Dario Faggioli

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).