[PATCH v2 for Xen 4.6 1/4] xen: enabling XL to set per-VCPU parameters of a domain for RTDS scheduler

[PATCH v2 for Xen 4.6 1/4] xen: enabling XL to set per-VCPU parameters of a domain for RTDS scheduler

[Chong Li]

Add two hypercalls(XEN_DOMCTL_SCHEDOP_getvcpuinfo/putvcpuinfo) to get/set a domain's
per-VCPU parameters. Hypercalls are handled by newly added hook (.adjust_vcpu) in the
scheduler interface.

Add a new data structure (struct xen_domctl_scheduler_vcpu_op) for transferring data
between tool and hypervisor.

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>
---
 xen/common/domctl.c         |  5 ++++
 xen/common/sched_rt.c       | 71 ++++++++++++++++++++++++++++++++++++++++++++-
 xen/common/schedule.c       | 24 +++++++++++++++
 xen/include/public/domctl.h | 29 ++++++++++++++++++
 xen/include/xen/sched-if.h  |  2 ++
 xen/include/xen/sched.h     |  1 +
 6 files changed, 131 insertions(+), 1 deletion(-)

diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index 28aea55..8143c44 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -841,6 +841,11 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
         copyback = 1;
         break;
 
+    case XEN_DOMCTL_scheduler_vcpu_op:
+        ret = sched_adjust_vcpu(d, &op->u.scheduler_vcpu_op);
+        copyback = 1;
+        break;
+
     case XEN_DOMCTL_getdomaininfo:
     {
         domid_t dom = op->domain;
diff --git a/xen/common/sched_rt.c b/xen/common/sched_rt.c
index 7c39a9e..524ea8e 100644
--- a/xen/common/sched_rt.c
+++ b/xen/common/sched_rt.c
@@ -1071,7 +1071,7 @@ out:
 }
 
 /*
- * set/get each vcpu info of each domain
+ * set/get per-domain params of a domain
  */
 static int
 rt_dom_cntl(
@@ -1115,6 +1115,74 @@ rt_dom_cntl(
     return rc;
 }
 
+/*
+ * set/get per-vcpu params of a domain
+ */
+static int
+rt_vcpu_cntl(
+    const struct scheduler *ops,
+    struct domain *d,
+    struct xen_domctl_scheduler_vcpu_op *op)
+{
+    struct rt_private *prv = rt_priv(ops);
+    struct rt_dom * const sdom = rt_dom(d);
+    struct rt_vcpu *svc;
+    struct list_head *iter;
+    unsigned long flags;
+    int rc = 0;
+    xen_domctl_sched_rtds_params_t local_sched;
+    unsigned int vcpuid;
+    unsigned int i;
+
+    switch ( op->cmd )
+    {
+    case XEN_DOMCTL_SCHEDOP_getvcpuinfo:
+        spin_lock_irqsave(&prv->lock, flags);
+        list_for_each( iter, &sdom->vcpu )
+        {
+            svc = list_entry(iter, struct rt_vcpu, sdom_elem);
+            vcpuid = svc->vcpu->vcpu_id;
+
+            local_sched.budget = svc->budget / MICROSECS(1);
+            local_sched.period = svc->period / MICROSECS(1);
+            if ( copy_to_guest_offset(op->u.rtds.vcpus, vcpuid,
+                    &local_sched, 1) )
+            {
+                spin_unlock_irqrestore(&prv->lock, flags);
+                return  -EFAULT;
+            }
+            hypercall_preempt_check();
+        }
+        spin_unlock_irqrestore(&prv->lock, flags);
+        break;
+    case XEN_DOMCTL_SCHEDOP_putvcpuinfo:
+        spin_lock_irqsave(&prv->lock, flags);
+        for( i = 0; i < op->u.rtds.nr_vcpus; i++ )
+        {
+            if ( copy_from_guest_offset(&local_sched,
+                    op->u.rtds.vcpus, i, 1) )
+            {
+                spin_unlock_irqrestore(&prv->lock, flags);
+                return -EFAULT;
+            }
+            if ( local_sched.period <= 0 || local_sched.budget <= 0 )
+            {
+                spin_unlock_irqrestore(&prv->lock, flags);
+                return -EINVAL;
+            }
+            svc = rt_vcpu(d->vcpu[local_sched.vcpuid]);
+            svc->period = MICROSECS(local_sched.period);
+            svc->budget = MICROSECS(local_sched.budget);
+            hypercall_preempt_check();
+        }
+        spin_unlock_irqrestore(&prv->lock, flags);
+        break;
+    }
+
+    return rc;
+}
+
+
 static struct rt_private _rt_priv;
 
 const struct scheduler sched_rtds_def = {
@@ -1139,6 +1207,7 @@ const struct scheduler sched_rtds_def = {
     .remove_vcpu    = rt_vcpu_remove,
 
     .adjust         = rt_dom_cntl,
+    .adjust_vcpu    = rt_vcpu_cntl,
 
     .pick_cpu       = rt_cpu_pick,
     .do_schedule    = rt_schedule,
diff --git a/xen/common/schedule.c b/xen/common/schedule.c
index f5a2e55..64b3c11 100644
--- a/xen/common/schedule.c
+++ b/xen/common/schedule.c
@@ -1104,6 +1104,30 @@ long sched_adjust(struct domain *d, struct xen_domctl_scheduler_op *op)
     return ret;
 }
 
+/* Adjust scheduling parameter for the vcpus of a given domain. */
+long sched_adjust_vcpu(
+    struct domain *d,
+    struct xen_domctl_scheduler_vcpu_op *op)
+{
+    long ret;
+
+    ret = xsm_domctl_scheduler_op(XSM_HOOK, d, op->cmd);
+    if ( ret )
+        return ret;
+
+    if ( (op->sched_id != DOM2OP(d)->sched_id) ||
+         ((op->cmd != XEN_DOMCTL_SCHEDOP_putvcpuinfo) &&
+          (op->cmd != XEN_DOMCTL_SCHEDOP_getvcpuinfo)) )
+        return -EINVAL;
+
+    /* NB: the pluggable scheduler code needs to take care
+     * of locking by itself. */
+    if ( (ret = SCHED_OP(DOM2OP(d), adjust_vcpu, d, op)) == 0 )
+        TRACE_1D(TRC_SCHED_ADJDOM, d->domain_id);
+
+    return ret;
+}
+
 long sched_adjust_global(struct xen_sysctl_scheduler_op *op)
 {
     struct cpupool *pool;
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index 10b51ef..13a88a3 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -342,6 +342,15 @@ struct xen_domctl_max_vcpus {
 typedef struct xen_domctl_max_vcpus xen_domctl_max_vcpus_t;
 DEFINE_XEN_GUEST_HANDLE(xen_domctl_max_vcpus_t);
 
+struct xen_domctl_sched_rtds_params {
+    /* vcpus' info */
+    uint32_t period;
+    uint32_t budget;
+    uint16_t vcpuid;
+    uint16_t padding[3];
+};
+typedef struct xen_domctl_sched_rtds_params xen_domctl_sched_rtds_params_t;
+DEFINE_XEN_GUEST_HANDLE(xen_domctl_sched_rtds_params_t);
 
 /* XEN_DOMCTL_scheduler_op */
 /* Scheduler types. */
@@ -354,6 +363,23 @@ DEFINE_XEN_GUEST_HANDLE(xen_domctl_max_vcpus_t);
 /* Set or get info? */
 #define XEN_DOMCTL_SCHEDOP_putinfo 0
 #define XEN_DOMCTL_SCHEDOP_getinfo 1
+#define XEN_DOMCTL_SCHEDOP_getvcpuinfo 2
+#define XEN_DOMCTL_SCHEDOP_putvcpuinfo 3
+
+struct xen_domctl_scheduler_vcpu_op {
+    uint32_t sched_id;  /* XEN_SCHEDULER_* */
+    uint32_t cmd;       /* XEN_DOMCTL_SCHEDOP_{get, put}vcpuinfo */
+    union {
+        struct xen_domctl_sched_rtds_vcpus {
+            XEN_GUEST_HANDLE_64(xen_domctl_sched_rtds_params_t) vcpus;
+            uint16_t nr_vcpus;
+            uint16_t padding[3];
+        } rtds;
+    } u;
+};
+typedef struct xen_domctl_scheduler_op xen_domctl_scheduler_vcpu_op_t;
+DEFINE_XEN_GUEST_HANDLE(xen_domctl_scheduler_vcpu_op_t);
+
 struct xen_domctl_scheduler_op {
     uint32_t sched_id;  /* XEN_SCHEDULER_* */
     uint32_t cmd;       /* XEN_DOMCTL_SCHEDOP_* */
@@ -1118,6 +1144,8 @@ struct xen_domctl {
 #define XEN_DOMCTL_setvnumainfo                  74
 #define XEN_DOMCTL_psr_cmt_op                    75
 #define XEN_DOMCTL_monitor_op                    77
+/* FIXME: put scheduler_vcpu_op here or after scheduler_op */
+#define XEN_DOMCTL_scheduler_vcpu_op                  78
 #define XEN_DOMCTL_gdbsx_guestmemio            1000
 #define XEN_DOMCTL_gdbsx_pausevcpu             1001
 #define XEN_DOMCTL_gdbsx_unpausevcpu           1002
@@ -1139,6 +1167,7 @@ struct xen_domctl {
         struct xen_domctl_getvcpuinfo       getvcpuinfo;
         struct xen_domctl_max_vcpus         max_vcpus;
         struct xen_domctl_scheduler_op      scheduler_op;
+        struct xen_domctl_scheduler_vcpu_op scheduler_vcpu_op;
         struct xen_domctl_setdomainhandle   setdomainhandle;
         struct xen_domctl_setdebugging      setdebugging;
         struct xen_domctl_irq_permission    irq_permission;
diff --git a/xen/include/xen/sched-if.h b/xen/include/xen/sched-if.h
index 7cc25c6..8106310 100644
--- a/xen/include/xen/sched-if.h
+++ b/xen/include/xen/sched-if.h
@@ -156,6 +156,8 @@ struct scheduler {
                                     unsigned int);
     int          (*adjust)         (const struct scheduler *, struct domain *,
                                     struct xen_domctl_scheduler_op *);
+    int          (*adjust_vcpu)         (const struct scheduler *, struct domain *,
+                                    struct xen_domctl_scheduler_vcpu_op *);
     int          (*adjust_global)  (const struct scheduler *,
                                     struct xen_sysctl_scheduler_op *);
     void         (*dump_settings)  (const struct scheduler *);
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 80c6f62..0e72bb6 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -643,6 +643,7 @@ int  sched_init_domain(struct domain *d);
 void sched_destroy_domain(struct domain *d);
 int sched_move_domain(struct domain *d, struct cpupool *c);
 long sched_adjust(struct domain *, struct xen_domctl_scheduler_op *);
+long sched_adjust_vcpu(struct domain *, struct xen_domctl_scheduler_vcpu_op *);
 long sched_adjust_global(struct xen_sysctl_scheduler_op *);
 int  sched_id(void);
 void sched_tick_suspend(void);
-- 
1.9.1

Re: [PATCH v2 for Xen 4.6 1/4] xen: enabling XL to set per-VCPU parameters of a domain for RTDS scheduler

[Jan Beulich]

>>> On 26.05.15 at 02:05, <lichong659@gmail.com> wrote:
> Add two hypercalls(XEN_DOMCTL_SCHEDOP_getvcpuinfo/putvcpuinfo) to get/set a 
> domain's
> per-VCPU parameters. Hypercalls are handled by newly added hook 
> (.adjust_vcpu) in the
> scheduler interface.
> 
> Add a new data structure (struct xen_domctl_scheduler_vcpu_op) for 
> transferring data
> between tool and hypervisor.
> 
> 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>
> ---

Missing brief description of changes in v2 here.

> +static int
> +rt_vcpu_cntl(
> +    const struct scheduler *ops,
> +    struct domain *d,
> +    struct xen_domctl_scheduler_vcpu_op *op)
> +{
> +    struct rt_private *prv = rt_priv(ops);
> +    struct rt_dom * const sdom = rt_dom(d);
> +    struct rt_vcpu *svc;
> +    struct list_head *iter;
> +    unsigned long flags;
> +    int rc = 0;
> +    xen_domctl_sched_rtds_params_t local_sched;
> +    unsigned int vcpuid;
> +    unsigned int i;
> +
> +    switch ( op->cmd )
> +    {
> +    case XEN_DOMCTL_SCHEDOP_getvcpuinfo:
> +        spin_lock_irqsave(&prv->lock, flags);
> +        list_for_each( iter, &sdom->vcpu )
> +        {
> +            svc = list_entry(iter, struct rt_vcpu, sdom_elem);
> +            vcpuid = svc->vcpu->vcpu_id;
> +
> +            local_sched.budget = svc->budget / MICROSECS(1);
> +            local_sched.period = svc->period / MICROSECS(1);
> +            if ( copy_to_guest_offset(op->u.rtds.vcpus, vcpuid,

What makes you think that the caller has provided enough slots?

> +                    &local_sched, 1) )

You're leaking hypervisor stack contents here, but by reading
the structure from guest memory first to honor its vcpuid field
(this making "get" match "put") you'd avoid this anyway.

Also - indentation.

> +            {
> +                spin_unlock_irqrestore(&prv->lock, flags);
> +                return  -EFAULT;

Setting rc to -EFAULT and break-ing would seem better for these
error paths (avoiding the repeated spin_unlock_irqrestore()-s).

> +            }
> +            hypercall_preempt_check();

???

> +        }
> +        spin_unlock_irqrestore(&prv->lock, flags);
> +        break;
> +    case XEN_DOMCTL_SCHEDOP_putvcpuinfo:
> +        spin_lock_irqsave(&prv->lock, flags);
> +        for( i = 0; i < op->u.rtds.nr_vcpus; i++ )
> +        {
> +            if ( copy_from_guest_offset(&local_sched,
> +                    op->u.rtds.vcpus, i, 1) )
> +            {
> +                spin_unlock_irqrestore(&prv->lock, flags);
> +                return -EFAULT;
> +            }
> +            if ( local_sched.period <= 0 || local_sched.budget <= 0 )
> +            {
> +                spin_unlock_irqrestore(&prv->lock, flags);
> +                return -EINVAL;
> +            }
> +            svc = rt_vcpu(d->vcpu[local_sched.vcpuid]);

You mustn't assume local_sched.vcpuid to represent a valid array
index.

> +            svc->period = MICROSECS(local_sched.period);
> +            svc->budget = MICROSECS(local_sched.budget);
> +            hypercall_preempt_check();

Again - ???

> --- a/xen/common/schedule.c
> +++ b/xen/common/schedule.c
> @@ -1104,6 +1104,30 @@ long sched_adjust(struct domain *d, struct xen_domctl_scheduler_op *op)
>      return ret;
>  }
>  
> +/* Adjust scheduling parameter for the vcpus of a given domain. */
> +long sched_adjust_vcpu(
> +    struct domain *d,
> +    struct xen_domctl_scheduler_vcpu_op *op)
> +{
> +    long ret;

I see no reason for this and the function return type to be "long".

Jan

Re: [PATCH v2 for Xen 4.6 1/4] xen: enabling XL to set per-VCPU parameters of a domain for RTDS scheduler

[Chong Li]

On Tue, May 26, 2015 at 4:08 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 26.05.15 at 02:05, <lichong659@gmail.com> wrote:
>> Add two hypercalls(XEN_DOMCTL_SCHEDOP_getvcpuinfo/putvcpuinfo) to get/set a
>> domain's
>> per-VCPU parameters. Hypercalls are handled by newly added hook
>> (.adjust_vcpu) in the
>> scheduler interface.
>>
>> Add a new data structure (struct xen_domctl_scheduler_vcpu_op) for
>> transferring data
>> between tool and hypervisor.
>>
>> 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>
>> ---
>
> Missing brief description of changes in v2 here.

Based on Dario's suggestion in PATCH v1, we think it would be better
to make the per-vcpu get/set functionalities more general, rather than
just serving for rtds scheduler. So, the changes in v2 are:

1) Add a new hypercall, XEN_DOMCTL_scheduler_vcpu_op. Any scheduler
can use it for per-vcpu get/set. There is a new data structure (struct
xen_domctl_scheduler_vcpu_op), which helps transferring data (per-vcpu
params) between tool and hypervisor when the hypercall is invoked.

2) The new hypercall is handled by function sched_adjust_vcpu (in
schedule.c), which would call a newly added hook (named as
.adjust_vcpu, added to the scheduler interface (struct scheduler)).

3) In RTDS scheduler, .adjust_vcpu hooks a function defined in
sched_rt.c, named as rt_vcpu_cntl. This function gets/sets the
per-vcpu params.


>> +
>> +    switch ( op->cmd )
>> +    {
>> +    case XEN_DOMCTL_SCHEDOP_getvcpuinfo:
>> +        spin_lock_irqsave(&prv->lock, flags);
>> +        list_for_each( iter, &sdom->vcpu )
>> +        {
>> +            svc = list_entry(iter, struct rt_vcpu, sdom_elem);
>> +            vcpuid = svc->vcpu->vcpu_id;
>> +
>> +            local_sched.budget = svc->budget / MICROSECS(1);
>> +            local_sched.period = svc->period / MICROSECS(1);
>> +            if ( copy_to_guest_offset(op->u.rtds.vcpus, vcpuid,
>
> What makes you think that the caller has provided enough slots?

This code works in a similar way as the one for XEN_SYSCTL_numainfo.
So if there is no enough slot in guest space, en error code is
returned.
>
>> +                    &local_sched, 1) )
>
> You're leaking hypervisor stack contents here, but by reading
> the structure from guest memory first to honor its vcpuid field
> (this making "get" match "put") you'd avoid this anyway.
>

The structure in guest memory has nothing, and it will hold per-vcpu
params after this XEN_DOMCTL_SCHEDOP_getvcpuinfo hypercall is well
served.
Does "leaking hypervisor stack" mean that I need to call
"local_sched.vcpuid = vcpuid" before copying this local_sched to the
guest memory?

>
>> +            {
>> +                spin_unlock_irqrestore(&prv->lock, flags);
>> +                return  -EFAULT;
>
> Setting rc to -EFAULT and break-ing would seem better for these
> error paths (avoiding the repeated spin_unlock_irqrestore()-s).
>
>> +            }
>> +            hypercall_preempt_check();
>
> ???

We've talked about this in patch v1
(http://lists.xenproject.org/archives/html/xen-devel/2015-05/msg01152.html).
When a domain has too many VCPUs, we need to make sure the spin lock
does not last for too long.
>
>> +        }
>> +        spin_unlock_irqrestore(&prv->lock, flags);
>> +        break;
>> +    case XEN_DOMCTL_SCHEDOP_putvcpuinfo:
>> +        spin_lock_irqsave(&prv->lock, flags);
>> +        for( i = 0; i < op->u.rtds.nr_vcpus; i++ )
>> +        {
>> +            if ( copy_from_guest_offset(&local_sched,
>> +                    op->u.rtds.vcpus, i, 1) )
>> +            {
>> +                spin_unlock_irqrestore(&prv->lock, flags);
>> +                return -EFAULT;
>> +            }
>> +            if ( local_sched.period <= 0 || local_sched.budget <= 0 )
>> +            {
>> +                spin_unlock_irqrestore(&prv->lock, flags);
>> +                return -EINVAL;
>> +            }
>> +            svc = rt_vcpu(d->vcpu[local_sched.vcpuid]);
>
> You mustn't assume local_sched.vcpuid to represent a valid array
> index.

Do you mean that I locate the vcpu by comparing the local_sched.vcpuid
with the IDs of each element in the vcpu array?

>
>
>> --- a/xen/common/schedule.c
>> +++ b/xen/common/schedule.c
>> @@ -1104,6 +1104,30 @@ long sched_adjust(struct domain *d, struct xen_domctl_scheduler_op *op)
>>      return ret;
>>  }
>>
>> +/* Adjust scheduling parameter for the vcpus of a given domain. */
>> +long sched_adjust_vcpu(
>> +    struct domain *d,
>> +    struct xen_domctl_scheduler_vcpu_op *op)
>> +{
>> +    long ret;
>
> I see no reason for this and the function return type to be "long".

The reason is stated in the begining.

>
> Jan
>

Best,
Chong

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

Re: [PATCH v2 for Xen 4.6 1/4] xen: enabling XL to set per-VCPU parameters of a domain for RTDS scheduler

[Chong Li]

On Tue, May 26, 2015 at 12:18 PM, Chong Li <lichong659@gmail.com> wrote:
> On Tue, May 26, 2015 at 4:08 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 26.05.15 at 02:05, <lichong659@gmail.com> wrote:
>>> Add two hypercalls(XEN_DOMCTL_SCHEDOP_getvcpuinfo/putvcpuinfo) to get/set a
>>> domain's
>>> per-VCPU parameters. Hypercalls are handled by newly added hook
>>> (.adjust_vcpu) in the
>>> scheduler interface.
>>>
>>> Add a new data structure (struct xen_domctl_scheduler_vcpu_op) for
>>> transferring data
>>> between tool and hypervisor.
>>>
>>> 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>
>>> ---
>>
>> Missing brief description of changes in v2 here.
>
> Based on Dario's suggestion in PATCH v1, we think it would be better
> to make the per-vcpu get/set functionalities more general, rather than
> just serving for rtds scheduler. So, the changes in v2 are:
>
> 1) Add a new hypercall, XEN_DOMCTL_scheduler_vcpu_op. Any scheduler
> can use it for per-vcpu get/set. There is a new data structure (struct
> xen_domctl_scheduler_vcpu_op), which helps transferring data (per-vcpu
> params) between tool and hypervisor when the hypercall is invoked.
>
> 2) The new hypercall is handled by function sched_adjust_vcpu (in
> schedule.c), which would call a newly added hook (named as
> .adjust_vcpu, added to the scheduler interface (struct scheduler)).
>
> 3) In RTDS scheduler, .adjust_vcpu hooks a function defined in
> sched_rt.c, named as rt_vcpu_cntl. This function gets/sets the
> per-vcpu params.
>
>

>>
>>> --- a/xen/common/schedule.c
>>> +++ b/xen/common/schedule.c
>>> @@ -1104,6 +1104,30 @@ long sched_adjust(struct domain *d, struct xen_domctl_scheduler_op *op)
>>>      return ret;
>>>  }
>>>
>>> +/* Adjust scheduling parameter for the vcpus of a given domain. */
>>> +long sched_adjust_vcpu(
>>> +    struct domain *d,
>>> +    struct xen_domctl_scheduler_vcpu_op *op)
>>> +{
>>> +    long ret;
>>
>> I see no reason for this and the function return type to be "long".
>
> The reason is stated in the begining.

Sorry for missing some details in the last reply.

sched_adjust_vcpu is added here because I need a seperate function to
handle XEN_DOMCTL_scheduler_vcpu_op, which is a new hypercall
dedicated for per-vcpu get/set. This function is actually based on
sched_adjust (which is dedicated for per-dom get/set in rtds), so the
return type is also the same.

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



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

Re: [PATCH v2 for Xen 4.6 1/4] xen: enabling XL to set per-VCPU parameters of a domain for RTDS scheduler

[Chao Peng]

On Mon, May 25, 2015 at 07:05:52PM -0500, Chong Li wrote:
> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -841,6 +841,11 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>          copyback = 1;
>          break;
>  
> +    case XEN_DOMCTL_scheduler_vcpu_op:
> +        ret = sched_adjust_vcpu(d, &op->u.scheduler_vcpu_op);
> +        copyback = 1;

I didn't see any fields you need to copy back here ('vcpus' were copied back
in rt_vcpu_cntl() already).

> +{
> +    struct rt_private *prv = rt_priv(ops);
> +    struct rt_dom * const sdom = rt_dom(d);
> +    struct rt_vcpu *svc;
> +    struct list_head *iter;
> +    unsigned long flags;
> +    int rc = 0;
> +    xen_domctl_sched_rtds_params_t local_sched;
> +    unsigned int vcpuid;
> +    unsigned int i;

 'vcpuid' is only used in 'get' path once while 'i' is used in 'set' path
 only, perhaps merge the two variables?

> +
> +    switch ( op->cmd )
> +    {
> +    case XEN_DOMCTL_SCHEDOP_getvcpuinfo:
> +        spin_lock_irqsave(&prv->lock, flags);
> +        list_for_each( iter, &sdom->vcpu )
> +        {
> +            svc = list_entry(iter, struct rt_vcpu, sdom_elem);
> +            vcpuid = svc->vcpu->vcpu_id;
> +
> +            local_sched.budget = svc->budget / MICROSECS(1);
> +            local_sched.period = svc->period / MICROSECS(1);
> +            if ( copy_to_guest_offset(op->u.rtds.vcpus, vcpuid,
> +                    &local_sched, 1) )
> +            {
> +                spin_unlock_irqrestore(&prv->lock, flags);
> +                return  -EFAULT;
                          ^Double spaces.

> +            }
> +            hypercall_preempt_check();

The check itself does nothing for preemption, you need return –ERESTART
or call hypercall_create_continuation to make the preemption happen.

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

'nr_vcpus' is not actually used untile now but in xc side you do pass
that in.

Regards
Chao

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

Re: [PATCH v2 for Xen 4.6 1/4] xen: enabling XL to set per-VCPU parameters of a domain for RTDS scheduler

[Jan Beulich]

>>> On 26.05.15 at 19:18, <lichong659@gmail.com> wrote:
> On Tue, May 26, 2015 at 4:08 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 26.05.15 at 02:05, <lichong659@gmail.com> wrote:
>>> Add two hypercalls(XEN_DOMCTL_SCHEDOP_getvcpuinfo/putvcpuinfo) to get/set a
>>> domain's
>>> per-VCPU parameters. Hypercalls are handled by newly added hook
>>> (.adjust_vcpu) in the
>>> scheduler interface.
>>>
>>> Add a new data structure (struct xen_domctl_scheduler_vcpu_op) for
>>> transferring data
>>> between tool and hypervisor.
>>>
>>> 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>
>>> ---
>>
>> Missing brief description of changes in v2 here.
> 
> Based on Dario's suggestion in PATCH v1, we think it would be better
> to make the per-vcpu get/set functionalities more general, rather than
> just serving for rtds scheduler. So, the changes in v2 are:
> 
> 1) Add a new hypercall, XEN_DOMCTL_scheduler_vcpu_op. Any scheduler
> can use it for per-vcpu get/set. There is a new data structure (struct
> xen_domctl_scheduler_vcpu_op), which helps transferring data (per-vcpu
> params) between tool and hypervisor when the hypercall is invoked.
> 
> 2) The new hypercall is handled by function sched_adjust_vcpu (in
> schedule.c), which would call a newly added hook (named as
> .adjust_vcpu, added to the scheduler interface (struct scheduler)).
> 
> 3) In RTDS scheduler, .adjust_vcpu hooks a function defined in
> sched_rt.c, named as rt_vcpu_cntl. This function gets/sets the
> per-vcpu params.

No, that looks to be the description of the patch as a whole, not
one of the delta between v1 and v2 (which is what I was asking for).

>>> +
>>> +    switch ( op->cmd )
>>> +    {
>>> +    case XEN_DOMCTL_SCHEDOP_getvcpuinfo:
>>> +        spin_lock_irqsave(&prv->lock, flags);
>>> +        list_for_each( iter, &sdom->vcpu )
>>> +        {
>>> +            svc = list_entry(iter, struct rt_vcpu, sdom_elem);
>>> +            vcpuid = svc->vcpu->vcpu_id;
>>> +
>>> +            local_sched.budget = svc->budget / MICROSECS(1);
>>> +            local_sched.period = svc->period / MICROSECS(1);
>>> +            if ( copy_to_guest_offset(op->u.rtds.vcpus, vcpuid,
>>
>> What makes you think that the caller has provided enough slots?
> 
> This code works in a similar way as the one for XEN_SYSCTL_numainfo.
> So if there is no enough slot in guest space, en error code is
> returned.

I don't think I saw any place you returned an error here. The
only error being returned is -EFAULT when the copy-out fails.

>>> +                    &local_sched, 1) )
>>
>> You're leaking hypervisor stack contents here, but by reading
>> the structure from guest memory first to honor its vcpuid field
>> (this making "get" match "put") you'd avoid this anyway.
>>
> 
> The structure in guest memory has nothing, and it will hold per-vcpu
> params after this XEN_DOMCTL_SCHEDOP_getvcpuinfo hypercall is well
> served.
> Does "leaking hypervisor stack" mean that I need to call
> "local_sched.vcpuid = vcpuid" before copying this local_sched to the
> guest memory?

Not just that. You'd also need to make sure padding space is
cleared. But as said, I suppose you want to copy in what the
guest provided anyway, in which case the leak is implicitly
gone.

>>> +            }
>>> +            hypercall_preempt_check();
>>
>> ???
> 
> We've talked about this in patch v1
> (http://lists.xenproject.org/archives/html/xen-devel/2015-05/msg01152.html).
> When a domain has too many VCPUs, we need to make sure the spin lock
> does not last for too long.

Right, but the call above does nothing like that. Please look at other
uses of it to understand what needs to be done here.

>>> +        }
>>> +        spin_unlock_irqrestore(&prv->lock, flags);
>>> +        break;
>>> +    case XEN_DOMCTL_SCHEDOP_putvcpuinfo:
>>> +        spin_lock_irqsave(&prv->lock, flags);
>>> +        for( i = 0; i < op->u.rtds.nr_vcpus; i++ )
>>> +        {
>>> +            if ( copy_from_guest_offset(&local_sched,
>>> +                    op->u.rtds.vcpus, i, 1) )
>>> +            {
>>> +                spin_unlock_irqrestore(&prv->lock, flags);
>>> +                return -EFAULT;
>>> +            }
>>> +            if ( local_sched.period <= 0 || local_sched.budget <= 0 )
>>> +            {
>>> +                spin_unlock_irqrestore(&prv->lock, flags);
>>> +                return -EINVAL;
>>> +            }
>>> +            svc = rt_vcpu(d->vcpu[local_sched.vcpuid]);
>>
>> You mustn't assume local_sched.vcpuid to represent a valid array
>> index.
> 
> Do you mean that I locate the vcpu by comparing the local_sched.vcpuid
> with the IDs of each element in the vcpu array?

No, you have to bound check it (against array bounds) before using
it as an array index.

>>> --- a/xen/common/schedule.c
>>> +++ b/xen/common/schedule.c
>>> @@ -1104,6 +1104,30 @@ long sched_adjust(struct domain *d, struct 
> xen_domctl_scheduler_op *op)
>>>      return ret;
>>>  }
>>>
>>> +/* Adjust scheduling parameter for the vcpus of a given domain. */
>>> +long sched_adjust_vcpu(
>>> +    struct domain *d,
>>> +    struct xen_domctl_scheduler_vcpu_op *op)
>>> +{
>>> +    long ret;
>>
>> I see no reason for this and the function return type to be "long".
> 
> The reason is stated in the begining.

In the beginning of what? I don't see any such, and it would also be
odd for there to be an explanation of why a particular type was chosen.

Jan

Re: [PATCH v2 for Xen 4.6 1/4] xen: enabling XL to set per-VCPU parameters of a domain for RTDS scheduler

[Jan Beulich]

>>> On 27.05.15 at 07:33, <lichong659@gmail.com> wrote:
> On Tue, May 26, 2015 at 12:18 PM, Chong Li <lichong659@gmail.com> wrote:
>> On Tue, May 26, 2015 at 4:08 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>> On 26.05.15 at 02:05, <lichong659@gmail.com> wrote:
>>>> +/* Adjust scheduling parameter for the vcpus of a given domain. */
>>>> +long sched_adjust_vcpu(
>>>> +    struct domain *d,
>>>> +    struct xen_domctl_scheduler_vcpu_op *op)
>>>> +{
>>>> +    long ret;
>>>
>>> I see no reason for this and the function return type to be "long".
>>
>> The reason is stated in the begining.
> 
> Sorry for missing some details in the last reply.
> 
> sched_adjust_vcpu is added here because I need a seperate function to
> handle XEN_DOMCTL_scheduler_vcpu_op, which is a new hypercall
> dedicated for per-vcpu get/set. This function is actually based on
> sched_adjust (which is dedicated for per-dom get/set in rtds), so the
> return type is also the same.

You explain why you need the function, but I asked about why the
function return type and the quoted variable's types are "long".

Jan

Re: [PATCH v2 for Xen 4.6 1/4] xen: enabling XL to set per-VCPU parameters of a domain for RTDS scheduler

[Chong Li]

On Wed, May 27, 2015 at 5:02 AM, Chao Peng <chao.p.peng@linux.intel.com> wrote:
> On Mon, May 25, 2015 at 07:05:52PM -0500, Chong Li wrote:

>
> I didn't see any fields you need to copy back here ('vcpus' were copied back
> in rt_vcpu_cntl() already).
>
>> +{
>> +    struct rt_private *prv = rt_priv(ops);
>> +    struct rt_dom * const sdom = rt_dom(d);
>> +    struct rt_vcpu *svc;
>> +    struct list_head *iter;
>> +    unsigned long flags;
>> +    int rc = 0;
>> +    xen_domctl_sched_rtds_params_t local_sched;
>> +    unsigned int vcpuid;
>> +    unsigned int i;
>
>  'vcpuid' is only used in 'get' path once while 'i' is used in 'set' path
>  only, perhaps merge the two variables?
>
Yes, we can use vcpuid in both cases.

>
>> +        }
>> +        spin_unlock_irqrestore(&prv->lock, flags);
>> +        break;
>
> 'nr_vcpus' is not actually used untile now but in xc side you do pass
> that in.
>
Right. nr_vcpus is just useful when it is 'set' case. I'll remove this
from the 'get' case.

> Regards
> Chao

Thanks,
Chong

Re: [PATCH v2 for Xen 4.6 1/4] xen: enabling XL to set per-VCPU parameters of a domain for RTDS scheduler

[Dario Faggioli]

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

On Wed, 2015-05-27 at 12:41 +0100, Jan Beulich wrote:
> >>> On 26.05.15 at 19:18, <lichong659@gmail.com> wrote:
> > On Tue, May 26, 2015 at 4:08 AM, Jan Beulich <JBeulich@suse.com> wrote:
> >>>>> On 26.05.15 at 02:05, <lichong659@gmail.com> wrote:
> >>> Add two hypercalls(XEN_DOMCTL_SCHEDOP_getvcpuinfo/putvcpuinfo) to get/set a
> >>> domain's
> >>> per-VCPU parameters. Hypercalls are handled by newly added hook
> >>> (.adjust_vcpu) in the
> >>> scheduler interface.
> >>>
> >>> Add a new data structure (struct xen_domctl_scheduler_vcpu_op) for
> >>> transferring data
> >>> between tool and hypervisor.
> >>>
> >>> 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>
> >>> ---
> >>
> >> Missing brief description of changes in v2 here.
> > 
> > Based on Dario's suggestion in PATCH v1, we think it would be better
> > to make the per-vcpu get/set functionalities more general, rather than
> > just serving for rtds scheduler. So, the changes in v2 are:
> > 
> > 1) Add a new hypercall, XEN_DOMCTL_scheduler_vcpu_op. Any scheduler
> > can use it for per-vcpu get/set. There is a new data structure (struct
> > xen_domctl_scheduler_vcpu_op), which helps transferring data (per-vcpu
> > params) between tool and hypervisor when the hypercall is invoked.
> > 
> > 2) The new hypercall is handled by function sched_adjust_vcpu (in
> > schedule.c), which would call a newly added hook (named as
> > .adjust_vcpu, added to the scheduler interface (struct scheduler)).
> > 
> > 3) In RTDS scheduler, .adjust_vcpu hooks a function defined in
> > sched_rt.c, named as rt_vcpu_cntl. This function gets/sets the
> > per-vcpu params.
> 
> No, that looks to be the description of the patch as a whole, not
> one of the delta between v1 and v2 (which is what I was asking for).
> 
> >>> +
> >>> +    switch ( op->cmd )
> >>> +    {
> >>> +    case XEN_DOMCTL_SCHEDOP_getvcpuinfo:
> >>> +        spin_lock_irqsave(&prv->lock, flags);
> >>> +        list_for_each( iter, &sdom->vcpu )
> >>> +        {
> >>> +            svc = list_entry(iter, struct rt_vcpu, sdom_elem);
> >>> +            vcpuid = svc->vcpu->vcpu_id;
> >>> +
> >>> +            local_sched.budget = svc->budget / MICROSECS(1);
> >>> +            local_sched.period = svc->period / MICROSECS(1);
> >>> +            if ( copy_to_guest_offset(op->u.rtds.vcpus, vcpuid,
> >>
> >> What makes you think that the caller has provided enough slots?
> > 
> > This code works in a similar way as the one for XEN_SYSCTL_numainfo.
> > So if there is no enough slot in guest space, en error code is
> > returned.
> 
> I don't think I saw any place you returned an error here. The
> only error being returned is -EFAULT when the copy-out fails.
> 
Look again at XEN_SYSCTL_numainfo, in particular, at this part:

    if ( ni->num_nodes < num_nodes )
    {
        ret = -ENOBUFS;
        i = num_nodes;
    }
    else
        i = 0

Which, as you'll appreciate if you check from where ni->num_nodes and
num_nodes come from, if where the boundary checking we're asking for
happens.

Note how the 'i' variable is used in the rest of the block, and you'll
see what we mean, and can do something similar.

> >>> +                    &local_sched, 1) )
> >>
> >> You're leaking hypervisor stack contents here, but by reading
> >> the structure from guest memory first to honor its vcpuid field
> >> (this making "get" match "put") you'd avoid this anyway.
> >>
> > 
> > The structure in guest memory has nothing, and it will hold per-vcpu
> > params after this XEN_DOMCTL_SCHEDOP_getvcpuinfo hypercall is well
> > served.
> > Does "leaking hypervisor stack" mean that I need to call
> > "local_sched.vcpuid = vcpuid" before copying this local_sched to the
> > guest memory?
> 
> Not just that. You'd also need to make sure padding space is
> cleared. 
>
Indeed.

> But as said, I suppose you want to copy in what the
> guest provided anyway, in which case the leak is implicitly
> gone.
> 
Chong, you're saying above "The structure in guest memory has nothing".
AFAICT, what Jan means when he says "make get match put", is why don't
you, for XEN_DOMCTL_SCHEDOP_getvcpuinfo, allow the caller to provide an
array with fewer elements than the number of vcpus, and put in the
vcpuid field of those elements, the id-s of the vcpus she wants to
receive information upon.

I also was about to make a similar request, as it makes things more
consistent, between get and put. After all, if we think that it is
valuable to allow batching (i.e., having an hypercall that returns the
parameters of _a_bunch_ of vcpus, not just of one), and that it makes
sense to allow that bunch of vcpus to be incomplete and sparse, this is
true for both the directions, isn't it?

> >>> +            }
> >>> +            hypercall_preempt_check();
> >>
> >> ???
> > 
> > We've talked about this in patch v1
> > (http://lists.xenproject.org/archives/html/xen-devel/2015-05/msg01152.html).
> > When a domain has too many VCPUs, we need to make sure the spin lock
> > does not last for too long.
> 
> Right, but the call above does nothing like that. Please look at other
> uses of it to understand what needs to be done here.
> 
This being said, would it make sense to put down a threshold, below
which we don't need to check for preemption (nor to ask to reissue the
hypercall)?

Something like, if we're dealing with a request for the parameters of N
(== 16 ? 32 ?) vcpus, it's fine to do them all at once. Above that
limit, we just do bunches of N vcpus, and then do a preempt check. What
do you think Jan? And what do you think a suitable limit would be?

In fact, apart from the fact that it's used incorrectly, I don't think
that checking for preemption after _each_ step of the loop make much
sense...

> >>> --- a/xen/common/schedule.c
> >>> +++ b/xen/common/schedule.c
> >>> @@ -1104,6 +1104,30 @@ long sched_adjust(struct domain *d, struct 
> > xen_domctl_scheduler_op *op)
> >>>      return ret;
> >>>  }
> >>>
> >>> +/* Adjust scheduling parameter for the vcpus of a given domain. */
> >>> +long sched_adjust_vcpu(
> >>> +    struct domain *d,
> >>> +    struct xen_domctl_scheduler_vcpu_op *op)
> >>> +{
> >>> +    long ret;
> >>
> >> I see no reason for this and the function return type to be "long".
> > 
> > The reason is stated in the begining.
> 
> In the beginning of what? I don't see any such, and it would also be
> odd for there to be an explanation of why a particular type was chosen.
>
As Chong he's saying elsewhere, he's moslty following suit. Should we
consider a pre-patch making both sched_adjust() and
sched_adjust_global() retunr int?

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

Re: [PATCH v2 for Xen 4.6 1/4] xen: enabling XL to set per-VCPU parameters of a domain for RTDS scheduler

[Jan Beulich]

>>> On 29.05.15 at 15:01, <dario.faggioli@citrix.com> wrote:
> On Wed, 2015-05-27 at 12:41 +0100, Jan Beulich wrote:
>> >>> On 26.05.15 at 19:18, <lichong659@gmail.com> wrote:
>> > On Tue, May 26, 2015 at 4:08 AM, Jan Beulich <JBeulich@suse.com> wrote:
>> >>>>> On 26.05.15 at 02:05, <lichong659@gmail.com> wrote:
>> >>> +
>> >>> +    switch ( op->cmd )
>> >>> +    {
>> >>> +    case XEN_DOMCTL_SCHEDOP_getvcpuinfo:
>> >>> +        spin_lock_irqsave(&prv->lock, flags);
>> >>> +        list_for_each( iter, &sdom->vcpu )
>> >>> +        {
>> >>> +            svc = list_entry(iter, struct rt_vcpu, sdom_elem);
>> >>> +            vcpuid = svc->vcpu->vcpu_id;
>> >>> +
>> >>> +            local_sched.budget = svc->budget / MICROSECS(1);
>> >>> +            local_sched.period = svc->period / MICROSECS(1);
>> >>> +            if ( copy_to_guest_offset(op->u.rtds.vcpus, vcpuid,
>> >>
>> >> What makes you think that the caller has provided enough slots?
>> > 
>> > This code works in a similar way as the one for XEN_SYSCTL_numainfo.
>> > So if there is no enough slot in guest space, en error code is
>> > returned.
>> 
>> I don't think I saw any place you returned an error here. The
>> only error being returned is -EFAULT when the copy-out fails.
>> 
> Look again at XEN_SYSCTL_numainfo, in particular, at this part:
> 
>     if ( ni->num_nodes < num_nodes )
>     {
>         ret = -ENOBUFS;
>         i = num_nodes;
>     }
>     else
>         i = 0
> 
> Which, as you'll appreciate if you check from where ni->num_nodes and
> num_nodes come from, if where the boundary checking we're asking for
> happens.
> 
> Note how the 'i' variable is used in the rest of the block, and you'll
> see what we mean, and can do something similar.

I think this was targeted at Chong rather than me (while I was
listed as To, and Chong only on Cc)?

>> >>> +            }
>> >>> +            hypercall_preempt_check();
>> >>
>> >> ???
>> > 
>> > We've talked about this in patch v1
>> > 
> (http://lists.xenproject.org/archives/html/xen-devel/2015-05/msg01152.html).
>> > When a domain has too many VCPUs, we need to make sure the spin lock
>> > does not last for too long.
>> 
>> Right, but the call above does nothing like that. Please look at other
>> uses of it to understand what needs to be done here.
>> 
> This being said, would it make sense to put down a threshold, below
> which we don't need to check for preemption (nor to ask to reissue the
> hypercall)?
> 
> Something like, if we're dealing with a request for the parameters of N
> (== 16 ? 32 ?) vcpus, it's fine to do them all at once. Above that
> limit, we just do bunches of N vcpus, and then do a preempt check. What
> do you think Jan? And what do you think a suitable limit would be?

I have no problem with that, or with checking for preemption only
every so many iterations.

> In fact, apart from the fact that it's used incorrectly, I don't think
> that checking for preemption after _each_ step of the loop make much
> sense...

Whether checking at the beginning (but not for the first iteration)
or at the end (but not for the last one) doesn't really matter.

>> >>> --- a/xen/common/schedule.c
>> >>> +++ b/xen/common/schedule.c
>> >>> @@ -1104,6 +1104,30 @@ long sched_adjust(struct domain *d, struct 
>> > xen_domctl_scheduler_op *op)
>> >>>      return ret;
>> >>>  }
>> >>>
>> >>> +/* Adjust scheduling parameter for the vcpus of a given domain. */
>> >>> +long sched_adjust_vcpu(
>> >>> +    struct domain *d,
>> >>> +    struct xen_domctl_scheduler_vcpu_op *op)
>> >>> +{
>> >>> +    long ret;
>> >>
>> >> I see no reason for this and the function return type to be "long".
>> > 
>> > The reason is stated in the begining.
>> 
>> In the beginning of what? I don't see any such, and it would also be
>> odd for there to be an explanation of why a particular type was chosen.
>>
> As Chong he's saying elsewhere, he's moslty following suit. Should we
> consider a pre-patch making both sched_adjust() and
> sched_adjust_global() retunr int?

Perhaps. But the main point here is that people copying existing code
should sanity check what they copy (so to not spread oddities or even
bugs).

Jan

Re: [PATCH v2 for Xen 4.6 1/4] xen: enabling XL to set per-VCPU parameters of a domain for RTDS scheduler

[Dario Faggioli]

[-- Attachment #1.1.1: Type: text/plain, Size: 3894 bytes --]

On Mon, 2015-05-25 at 19:05 -0500, Chong Li wrote:

> diff --git a/xen/common/domctl.c b/xen/common/domctl.c
> index 28aea55..8143c44 100644
> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -841,6 +841,11 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>          copyback = 1;
>          break;
>  
> +    case XEN_DOMCTL_scheduler_vcpu_op:
> +        ret = sched_adjust_vcpu(d, &op->u.scheduler_vcpu_op);
> +        copyback = 1;
> +        break;
> +
>
So, thinking more about this, do we really want a full new DOMCTL, or do
we want to instrument XEN_DOMCTL_scheduler_op? That would mean adding
two operations there, and fiddling with struct xen_domctl_scheduler_op?

The problem I see is that, whatever I imagine putting this, there would
be some redundancy.

Quick-&-dirty, I put together the below... would it possibly make sense?

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 union xen_domctl_schedparam {                     
    xen_domctl_sched_credit_t credit;            
    xen_domctl_sched_credit2_t credit2;
    xen_domctl_sched_rtds_t rtds;
} xen_domctl_schedparam_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;
    } s;               
    uint16_t vcpuid; 
} xen_domctl_schedparam_vcpu_t;
DEFINE_XEN_GUEST_HANDLE(xen_domctl_schedparam_vcpu_t);
        
/* Set or get info? */
#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_* */ 
    union {
        xen_domctl_schedparam_t d;
        struct {
            XEN_GUEST_HANDLE_64(xen_domctl_schedparam_vcpu_t) vcpus;
            uint16_t nr_vcpus;
        } v;                         
    } u;    
}; 
typedef struct xen_domctl_scheduler_op xen_domctl_scheduler_op_t;
DEFINE_XEN_GUEST_HANDLE(xen_domctl_scheduler_op_t);

I'm also attaching a (build-tested only) patch to that effect (I'm
killing SEDF in there, so I don't have to care about it, as it's going
away anyway).

Thoughts?

>  /*
> - * set/get each vcpu info of each domain
> + * set/get per-domain params of a domain
>   */
>  static int
>  rt_dom_cntl(
> @@ -1115,6 +1115,74 @@ rt_dom_cntl(
>      return rc;
>  }
>  
> +/*
> + * set/get per-vcpu params of a domain
> + */
> +static int
> +rt_vcpu_cntl(
> +    const struct scheduler *ops,
> +    struct domain *d,
> +    struct xen_domctl_scheduler_vcpu_op *op)
> +{
>
I commented this function already, while replying to Jan.
 
> +/* Adjust scheduling parameter for the vcpus of a given domain. */
> +long sched_adjust_vcpu(
> +    struct domain *d,
> +    struct xen_domctl_scheduler_vcpu_op *op)
> +{
>
Actually, I don't think you even need this function.

Especially if we take the path of doing all this as subops of
DOMCTL_scheduler_op, you can just use sched_adjust, do some more
multiplexing in there, and the call the proper scheduler hook.

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.1.2: vcpuparams.patch --]
[-- Type: text/x-patch, Size: 6605 bytes --]

diff --git a/xen/common/Makefile b/xen/common/Makefile
index 1cddebc..3fdf931 100644
--- a/xen/common/Makefile
+++ b/xen/common/Makefile
@@ -31,7 +31,6 @@ obj-y += rbtree.o
 obj-y += rcupdate.o
 obj-y += sched_credit.o
 obj-y += sched_credit2.o
-obj-y += sched_sedf.o
 obj-y += sched_arinc653.o
 obj-y += sched_rt.o
 obj-y += schedule.o
diff --git a/xen/common/sched_credit.c b/xen/common/sched_credit.c
index 953ecb0..43b086b 100644
--- a/xen/common/sched_credit.c
+++ b/xen/common/sched_credit.c
@@ -1039,25 +1039,25 @@ csched_dom_cntl(
 
     if ( op->cmd == XEN_DOMCTL_SCHEDOP_getinfo )
     {
-        op->u.credit.weight = sdom->weight;
-        op->u.credit.cap = sdom->cap;
+        op->u.d.credit.weight = sdom->weight;
+        op->u.d.credit.cap = sdom->cap;
     }
     else
     {
         ASSERT(op->cmd == XEN_DOMCTL_SCHEDOP_putinfo);
 
-        if ( op->u.credit.weight != 0 )
+        if ( op->u.d.credit.weight != 0 )
         {
             if ( !list_empty(&sdom->active_sdom_elem) )
             {
                 prv->weight -= sdom->weight * sdom->active_vcpu_count;
-                prv->weight += op->u.credit.weight * sdom->active_vcpu_count;
+                prv->weight += op->u.d.credit.weight * sdom->active_vcpu_count;
             }
-            sdom->weight = op->u.credit.weight;
+            sdom->weight = op->u.d.credit.weight;
         }
 
-        if ( op->u.credit.cap != (uint16_t)~0U )
-            sdom->cap = op->u.credit.cap;
+        if ( op->u.d.credit.cap != (uint16_t)~0U )
+            sdom->cap = op->u.d.credit.cap;
 
     }
 
diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c
index 75e0321..8992423 100644
--- a/xen/common/sched_credit2.c
+++ b/xen/common/sched_credit2.c
@@ -1438,20 +1438,20 @@ csched2_dom_cntl(
 
     if ( op->cmd == XEN_DOMCTL_SCHEDOP_getinfo )
     {
-        op->u.credit2.weight = sdom->weight;
+        op->u.d.credit2.weight = sdom->weight;
     }
     else
     {
         ASSERT(op->cmd == XEN_DOMCTL_SCHEDOP_putinfo);
 
-        if ( op->u.credit2.weight != 0 )
+        if ( op->u.d.credit2.weight != 0 )
         {
             struct list_head *iter;
             int old_weight;
 
             old_weight = sdom->weight;
 
-            sdom->weight = op->u.credit2.weight;
+            sdom->weight = op->u.d.credit2.weight;
 
             /* Update weights for vcpus, and max_weight for runqueues on which they reside */
             list_for_each ( iter, &sdom->vcpu )
diff --git a/xen/common/sched_rt.c b/xen/common/sched_rt.c
index 7c39a9e..c7efda3 100644
--- a/xen/common/sched_rt.c
+++ b/xen/common/sched_rt.c
@@ -1091,12 +1091,12 @@ rt_dom_cntl(
     case XEN_DOMCTL_SCHEDOP_getinfo:
         spin_lock_irqsave(&prv->lock, flags);
         svc = list_entry(sdom->vcpu.next, struct rt_vcpu, sdom_elem);
-        op->u.rtds.period = svc->period / MICROSECS(1); /* transfer to us */
-        op->u.rtds.budget = svc->budget / MICROSECS(1);
+        op->u.d.rtds.period = svc->period / MICROSECS(1); /* transfer to us */
+        op->u.d.rtds.budget = svc->budget / MICROSECS(1);
         spin_unlock_irqrestore(&prv->lock, flags);
         break;
     case XEN_DOMCTL_SCHEDOP_putinfo:
-        if ( op->u.rtds.period == 0 || op->u.rtds.budget == 0 )
+        if ( op->u.d.rtds.period == 0 || op->u.d.rtds.budget == 0 )
         {
             rc = -EINVAL;
             break;
@@ -1105,8 +1105,8 @@ rt_dom_cntl(
         list_for_each( iter, &sdom->vcpu )
         {
             struct rt_vcpu * svc = list_entry(iter, struct rt_vcpu, sdom_elem);
-            svc->period = MICROSECS(op->u.rtds.period); /* transfer to nanosec */
-            svc->budget = MICROSECS(op->u.rtds.budget);
+            svc->period = MICROSECS(op->u.d.rtds.period); /* transfer to nanosec */
+            svc->budget = MICROSECS(op->u.d.rtds.budget);
         }
         spin_unlock_irqrestore(&prv->lock, flags);
         break;
diff --git a/xen/common/schedule.c b/xen/common/schedule.c
index f5a2e55..29f403c 100644
--- a/xen/common/schedule.c
+++ b/xen/common/schedule.c
@@ -65,7 +65,6 @@ DEFINE_PER_CPU(struct schedule_data, schedule_data);
 DEFINE_PER_CPU(struct scheduler *, scheduler);
 
 static const struct scheduler *schedulers[] = {
-    &sched_sedf_def,
     &sched_credit_def,
     &sched_credit2_def,
     &sched_arinc653_def,
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index 88f8002..c1e25f4 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -330,31 +330,50 @@ DEFINE_XEN_GUEST_HANDLE(xen_domctl_max_vcpus_t);
 #define XEN_SCHEDULER_ARINC653 7
 #define XEN_SCHEDULER_RTDS     8
 
+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 union xen_domctl_schedparam {
+    xen_domctl_sched_credit_t credit;
+    xen_domctl_sched_credit2_t credit2;
+    xen_domctl_sched_rtds_t rtds;
+} xen_domctl_schedparam_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;
+    } s;
+    uint16_t vcpuid;
+} xen_domctl_schedparam_vcpu_t;
+DEFINE_XEN_GUEST_HANDLE(xen_domctl_schedparam_vcpu_t);
+
 /* Set or get info? */
 #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_* */
     union {
-        struct xen_domctl_sched_sedf {
-            uint64_aligned_t period;
-            uint64_aligned_t slice;
-            uint64_aligned_t latency;
-            uint32_t extratime;
-            uint32_t weight;
-        } sedf;
-        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_schedparam_t d;
+        struct {
+            XEN_GUEST_HANDLE_64(xen_domctl_schedparam_vcpu_t) vcpus;
+            uint16_t nr_vcpus;
+        } v;
     } u;
 };
 typedef struct xen_domctl_scheduler_op xen_domctl_scheduler_op_t;

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

Re: [PATCH v2 for Xen 4.6 1/4] xen: enabling XL to set per-VCPU parameters of a domain for RTDS scheduler

[Chong Li]

On Fri, May 29, 2015 at 8:51 AM, Dario Faggioli
<dario.faggioli@citrix.com> wrote:
> On Mon, 2015-05-25 at 19:05 -0500, Chong Li wrote:
>
>> diff --git a/xen/common/domctl.c b/xen/common/domctl.c
>> index 28aea55..8143c44 100644
>> --- a/xen/common/domctl.c
>> +++ b/xen/common/domctl.c

>
> /* Set or get info? */
> #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_* */
>     union {
>         xen_domctl_schedparam_t d;
>         struct {
>             XEN_GUEST_HANDLE_64(xen_domctl_schedparam_vcpu_t) vcpus;
>             uint16_t nr_vcpus;
>         } v;
>     } u;
> };
> typedef struct xen_domctl_scheduler_op xen_domctl_scheduler_op_t;
> DEFINE_XEN_GUEST_HANDLE(xen_domctl_scheduler_op_t);
>
> I'm also attaching a (build-tested only) patch to that effect (I'm
> killing SEDF in there, so I don't have to care about it, as it's going
> away anyway).
>
> Thoughts?
>
I see. So now we put per-dom params and per-vcpu params into the same
structure (xen_domctl_scheduler_op). This structure will be handled in
sched_adjust and there should be a "switch" in that function to
distinguish per-dom get/set and per-vcpu get/set. Right?

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



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

Re: [PATCH v2 for Xen 4.6 1/4] xen: enabling XL to set per-VCPU parameters of a domain for RTDS scheduler

[George Dunlap]

On Fri, May 29, 2015 at 2:51 PM, Dario Faggioli
<dario.faggioli@citrix.com> wrote:
> On Mon, 2015-05-25 at 19:05 -0500, Chong Li wrote:
>
>> diff --git a/xen/common/domctl.c b/xen/common/domctl.c
>> index 28aea55..8143c44 100644
>> --- a/xen/common/domctl.c
>> +++ b/xen/common/domctl.c
>> @@ -841,6 +841,11 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>>          copyback = 1;
>>          break;
>>
>> +    case XEN_DOMCTL_scheduler_vcpu_op:
>> +        ret = sched_adjust_vcpu(d, &op->u.scheduler_vcpu_op);
>> +        copyback = 1;
>> +        break;
>> +
>>
> So, thinking more about this, do we really want a full new DOMCTL, or do
> we want to instrument XEN_DOMCTL_scheduler_op? That would mean adding
> two operations there, and fiddling with struct xen_domctl_scheduler_op?
>
> The problem I see is that, whatever I imagine putting this, there would
> be some redundancy.
>
> Quick-&-dirty, I put together the below... would it possibly make sense?
>
> 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 union xen_domctl_schedparam {
>     xen_domctl_sched_credit_t credit;
>     xen_domctl_sched_credit2_t credit2;
>     xen_domctl_sched_rtds_t rtds;
> } xen_domctl_schedparam_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;
>     } s;
>     uint16_t vcpuid;
> } xen_domctl_schedparam_vcpu_t;
> DEFINE_XEN_GUEST_HANDLE(xen_domctl_schedparam_vcpu_t);
>
> /* Set or get info? */
> #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_* */
>     union {
>         xen_domctl_schedparam_t d;
>         struct {
>             XEN_GUEST_HANDLE_64(xen_domctl_schedparam_vcpu_t) vcpus;
>             uint16_t nr_vcpus;
>         } v;
>     } u;
> };
> typedef struct xen_domctl_scheduler_op xen_domctl_scheduler_op_t;
> DEFINE_XEN_GUEST_HANDLE(xen_domctl_scheduler_op_t);
>
> I'm also attaching a (build-tested only) patch to that effect (I'm
> killing SEDF in there, so I don't have to care about it, as it's going
> away anyway).
>
> Thoughts?

After (finally) getting time to look at this series, I was going to
suggest something like this.

 -George

Re: [PATCH v2 for Xen 4.6 1/4] xen: enabling XL to set per-VCPU parameters of a domain for RTDS scheduler

[George Dunlap]

On Tue, May 26, 2015 at 1:05 AM, Chong Li <lichong659@gmail.com> wrote:
> Add two hypercalls(XEN_DOMCTL_SCHEDOP_getvcpuinfo/putvcpuinfo) to get/set a domain's
> per-VCPU parameters. Hypercalls are handled by newly added hook (.adjust_vcpu) in the
> scheduler interface.
>
> Add a new data structure (struct xen_domctl_scheduler_vcpu_op) for transferring data
> between tool and hypervisor.
>
> 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>

One comment that I didn't see addressed already...

> +static int
> +rt_vcpu_cntl(
> +    const struct scheduler *ops,
> +    struct domain *d,
> +    struct xen_domctl_scheduler_vcpu_op *op)
> +{
> +    struct rt_private *prv = rt_priv(ops);
> +    struct rt_dom * const sdom = rt_dom(d);
> +    struct rt_vcpu *svc;
> +    struct list_head *iter;
> +    unsigned long flags;
> +    int rc = 0;
> +    xen_domctl_sched_rtds_params_t local_sched;
> +    unsigned int vcpuid;
> +    unsigned int i;
> +
> +    switch ( op->cmd )
> +    {
> +    case XEN_DOMCTL_SCHEDOP_getvcpuinfo:
> +        spin_lock_irqsave(&prv->lock, flags);
> +        list_for_each( iter, &sdom->vcpu )
> +        {
> +            svc = list_entry(iter, struct rt_vcpu, sdom_elem);
> +            vcpuid = svc->vcpu->vcpu_id;
> +
> +            local_sched.budget = svc->budget / MICROSECS(1);
> +            local_sched.period = svc->period / MICROSECS(1);
> +            if ( copy_to_guest_offset(op->u.rtds.vcpus, vcpuid,
> +                    &local_sched, 1) )
> +            {
> +                spin_unlock_irqrestore(&prv->lock, flags);
> +                return  -EFAULT;
> +            }
> +            hypercall_preempt_check();
> +        }
> +        spin_unlock_irqrestore(&prv->lock, flags);
> +        break;
> +    case XEN_DOMCTL_SCHEDOP_putvcpuinfo:
> +        spin_lock_irqsave(&prv->lock, flags);
> +        for( i = 0; i < op->u.rtds.nr_vcpus; i++ )
> +        {
> +            if ( copy_from_guest_offset(&local_sched,
> +                    op->u.rtds.vcpus, i, 1) )
> +            {
> +                spin_unlock_irqrestore(&prv->lock, flags);
> +                return -EFAULT;
> +            }
> +            if ( local_sched.period <= 0 || local_sched.budget <= 0 )
> +            {
> +                spin_unlock_irqrestore(&prv->lock, flags);
> +                return -EINVAL;
> +            }
> +            svc = rt_vcpu(d->vcpu[local_sched.vcpuid]);
> +            svc->period = MICROSECS(local_sched.period);
> +            svc->budget = MICROSECS(local_sched.budget);
> +            hypercall_preempt_check();
> +        }

It looks like the interface here is assymetric.  That is, on
"putvcpuinfo", you assume there is an array of rtds.nr_vcpus length,
and you read vcpu_id from each one.  In "getvcpuinfo", you assume that
the array is the number of vcpus in the guest, and you just copy all
the vcpu data into it.

I think it would make more sense for it to work the same both ways --
so either always pass an array of all vcpus of the guest in and out;
or, have an array potentially specifying a subset of cpus in and out.

Thoughts?

 -George

Re: [PATCH v2 for Xen 4.6 1/4] xen: enabling XL to set per-VCPU parameters of a domain for RTDS scheduler

[Dario Faggioli]

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

On Fri, 2015-05-29 at 11:47 -0500, Chong Li wrote:
> On Fri, May 29, 2015 at 8:51 AM, Dario Faggioli
> <dario.faggioli@citrix.com> wrote:
> > On Mon, 2015-05-25 at 19:05 -0500, Chong Li wrote:
> >
> >> diff --git a/xen/common/domctl.c b/xen/common/domctl.c
> >> index 28aea55..8143c44 100644
> >> --- a/xen/common/domctl.c
> >> +++ b/xen/common/domctl.c
> 
> >
> > /* Set or get info? */
> > #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_* */
> >     union {
> >         xen_domctl_schedparam_t d;
> >         struct {
> >             XEN_GUEST_HANDLE_64(xen_domctl_schedparam_vcpu_t) vcpus;
> >             uint16_t nr_vcpus;
> >         } v;
> >     } u;
> > };
> > typedef struct xen_domctl_scheduler_op xen_domctl_scheduler_op_t;
> > DEFINE_XEN_GUEST_HANDLE(xen_domctl_scheduler_op_t);
> >
> > I'm also attaching a (build-tested only) patch to that effect (I'm
> > killing SEDF in there, so I don't have to care about it, as it's going
> > away anyway).
> >
> > Thoughts?
> >
> I see. So now we put per-dom params and per-vcpu params into the same
> structure (xen_domctl_scheduler_op). This structure will be handled in
> sched_adjust and there should be a "switch" in that function to
> distinguish per-dom get/set and per-vcpu get/set. Right?
> 
You're describing correctly what I had in mind and tried to do in the
draft patch, yes. Whether to do it or not, well, it's a proposal. George
says he also thinks the thing makes sense to him too, so I guess you can
go ahead and try doing so in v3.

Of course, others, feel free to chime in.

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

Re: [PATCH v2 for Xen 4.6 1/4] xen: enabling XL to set per-VCPU parameters of a domain for RTDS scheduler

[Dario Faggioli]

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

On Tue, 2015-06-02 at 17:28 +0100, George Dunlap wrote:
> On Tue, May 26, 2015 at 1:05 AM, Chong Li <lichong659@gmail.com> wrote:
> > Add two hypercalls(XEN_DOMCTL_SCHEDOP_getvcpuinfo/putvcpuinfo) to get/set a domain's
> > per-VCPU parameters. Hypercalls are handled by newly added hook (.adjust_vcpu) in the
> > scheduler interface.
> >
> > Add a new data structure (struct xen_domctl_scheduler_vcpu_op) for transferring data
> > between tool and hypervisor.
> >
> > 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>
> 
> One comment that I didn't see addressed already...
> 

> > +    switch ( op->cmd )
> > +    {
> > +    case XEN_DOMCTL_SCHEDOP_getvcpuinfo:
> > +        spin_lock_irqsave(&prv->lock, flags);
> > +        list_for_each( iter, &sdom->vcpu )
> > +        {
> > +            svc = list_entry(iter, struct rt_vcpu, sdom_elem);
> > +            vcpuid = svc->vcpu->vcpu_id;
> > +
> > +            local_sched.budget = svc->budget / MICROSECS(1);
> > +            local_sched.period = svc->period / MICROSECS(1);
> > +            if ( copy_to_guest_offset(op->u.rtds.vcpus, vcpuid,
> > +                    &local_sched, 1) )
> > +            {
> > +                spin_unlock_irqrestore(&prv->lock, flags);
> > +                return  -EFAULT;
> > +            }
> > +            hypercall_preempt_check();
> > +        }
> > +        spin_unlock_irqrestore(&prv->lock, flags);
> > +        break;
> > +    case XEN_DOMCTL_SCHEDOP_putvcpuinfo:
> > +        spin_lock_irqsave(&prv->lock, flags);
> > +        for( i = 0; i < op->u.rtds.nr_vcpus; i++ )
> > +        {
> > +            if ( copy_from_guest_offset(&local_sched,
> > +                    op->u.rtds.vcpus, i, 1) )
> > +            {
> > +                spin_unlock_irqrestore(&prv->lock, flags);
> > +                return -EFAULT;
> > +            }
> > +            if ( local_sched.period <= 0 || local_sched.budget <= 0 )
> > +            {
> > +                spin_unlock_irqrestore(&prv->lock, flags);
> > +                return -EINVAL;
> > +            }
> > +            svc = rt_vcpu(d->vcpu[local_sched.vcpuid]);
> > +            svc->period = MICROSECS(local_sched.period);
> > +            svc->budget = MICROSECS(local_sched.budget);
> > +            hypercall_preempt_check();
> > +        }
> 
> It looks like the interface here is assymetric.  That is, on
> "putvcpuinfo", you assume there is an array of rtds.nr_vcpus length,
> and you read vcpu_id from each one.  In "getvcpuinfo", you assume that
> the array is the number of vcpus in the guest, and you just copy all
> the vcpu data into it.
> 
> I think it would make more sense for it to work the same both ways --
> so either always pass an array of all vcpus of the guest in and out;
> or, have an array potentially specifying a subset of cpus in and out.
> 
> Thoughts?
> 
It would indeed. And in fact, just FTR, I did say pretty much the same
in <1432904488.5077.30.camel@citrix.com> :-D

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

Re: [PATCH v2 for Xen 4.6 1/4] xen: enabling XL to set per-VCPU parameters of a domain for RTDS scheduler

[Dario Faggioli]

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

On Fri, 2015-05-29 at 14:14 +0100, Jan Beulich wrote:
> >>> On 29.05.15 at 15:01, <dario.faggioli@citrix.com> wrote:
> > On Wed, 2015-05-27 at 12:41 +0100, Jan Beulich wrote:
> >> >>> On 26.05.15 at 19:18, <lichong659@gmail.com> wrote:
> >> > On Tue, May 26, 2015 at 4:08 AM, Jan Beulich <JBeulich@suse.com> wrote:
> >> >>>>> On 26.05.15 at 02:05, <lichong659@gmail.com> wrote:
> >> >>> +
> >> >>> +    switch ( op->cmd )
> >> >>> +    {
> >> >>> +    case XEN_DOMCTL_SCHEDOP_getvcpuinfo:
> >> >>> +        spin_lock_irqsave(&prv->lock, flags);
> >> >>> +        list_for_each( iter, &sdom->vcpu )
> >> >>> +        {
> >> >>> +            svc = list_entry(iter, struct rt_vcpu, sdom_elem);
> >> >>> +            vcpuid = svc->vcpu->vcpu_id;
> >> >>> +
> >> >>> +            local_sched.budget = svc->budget / MICROSECS(1);
> >> >>> +            local_sched.period = svc->period / MICROSECS(1);
> >> >>> +            if ( copy_to_guest_offset(op->u.rtds.vcpus, vcpuid,
> >> >>
> >> >> What makes you think that the caller has provided enough slots?
> >> > 
> >> > This code works in a similar way as the one for XEN_SYSCTL_numainfo.
> >> > So if there is no enough slot in guest space, en error code is
> >> > returned.
> >> 
> >> I don't think I saw any place you returned an error here. The
> >> only error being returned is -EFAULT when the copy-out fails.
> >> 
> > Look again at XEN_SYSCTL_numainfo, in particular, at this part:
> > 
> >     if ( ni->num_nodes < num_nodes )
> >     {
> >         ret = -ENOBUFS;
> >         i = num_nodes;
> >     }
> >     else
> >         i = 0
> > 
> > Which, as you'll appreciate if you check from where ni->num_nodes and
> > num_nodes come from, if where the boundary checking we're asking for
> > happens.
> > 
> > Note how the 'i' variable is used in the rest of the block, and you'll
> > see what we mean, and can do something similar.
> 
> I think this was targeted at Chong rather than me (while I was
> listed as To, and Chong only on Cc)?
> 
It was indeed targeted at him. :-)

I replied to your email to re-use and quote the points you made, but did
not think about changing what my MUA does by default when hitting
'Reply'... I never do that, actually, but I now see how it could be a
good idea to do so... I'll try to remember and fit that in my workflow.

> >> >>> +/* Adjust scheduling parameter for the vcpus of a given domain. */
> >> >>> +long sched_adjust_vcpu(
> >> >>> +    struct domain *d,
> >> >>> +    struct xen_domctl_scheduler_vcpu_op *op)
> >> >>> +{
> >> >>> +    long ret;
> >> >>
> >> >> I see no reason for this and the function return type to be "long".
> >> > 
> >> > The reason is stated in the begining.
> >> 
> >> In the beginning of what? I don't see any such, and it would also be
> >> odd for there to be an explanation of why a particular type was chosen.
> >>
> > As Chong he's saying elsewhere, he's moslty following suit. Should we
> > consider a pre-patch making both sched_adjust() and
> > sched_adjust_global() retunr int?
> 
> Perhaps. But the main point here is that people copying existing code
> should sanity check what they copy (so to not spread oddities or even
> bugs).
> 
Agreed, but in this case, he'd have ended up with:

long sched_adjust(...)
int sched_adjust_vcpu(...)
long sched_adjust_global(...)

So I think I see why Chong just went with 'long', that was my point.
Then of course, one could have spotted that it's the two existing ones
that are wrong/suboptimal, but that's more our responsibility than
Chong's fault, IMO (which does not mean that we should not point the
thing out and fix/ask to fix).

In any case, as far as this series is concerned, there should be no need
for a new function like this any longer. For "fixing" _adjust and
_adjust_global, I'll take care of 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