Re: [Xen-devel] [PATCH v4 for Xen 4.6 1/4] xen: enable per-VCPU parameter settings for RTDS scheduler

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

On Fri, 2015-07-10 at 23:52 -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
> 
I'd add a note about the fact that, for now, this is only supported and
being implemented for RTDs.

> Signed-off-by: Chong Li <chong.li@xxxxxxxxx>
> Signed-off-by: Meng Xu <mengxu@xxxxxxxxxxxxx>
> Signed-off-by: Sisu Xi <xisisu@xxxxxxxxx>
> 
> ---
> 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
> 
Don't drop the 'Changes on PATCH v2' part, please. That helps having an
idea of the full history, reminding about previous comments being done,
etc.

> diff --git a/xen/common/domctl.c b/xen/common/domctl.c
> index 2a2d203..349f68b 100644
> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -839,6 +839,9 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) 
> u_domctl)
>  
>      case XEN_DOMCTL_scheduler_op:
>          ret = sched_adjust(d, &op->u.scheduler_op);
> +        if ( ret == -ERESTART )
> +            ret = hypercall_create_continuation(
> +                __HYPERVISOR_domctl, "h", u_domctl);
>          copyback = 1;
>          break;
>  
> diff --git a/xen/common/sched_rt.c b/xen/common/sched_rt.c
> index 4372486..fed9e09 100644
> --- a/xen/common/sched_rt.c
> +++ b/xen/common/sched_rt.c
> @@ -78,7 +78,6 @@
>   *    vcpu_insert, vcpu_remove, context_saved, __runq_insert
>   */
>  
> -
>  /*
>   * Default parameters:
>   * Period and budget in default is 10 and 4 ms, respectively
>
This hunk is spurious, just don't do these things. If you think the
white line should indeed be dropped, send a separate patch, i.e., let's
avoid mixing cosmetic and coding style fixes, with functional changes.

Of course, it's not really worthwhile to send a patch for a blank line.
If you, however, identify a bunch of coding style issues in the file,
you can well bundle them together and send a patch to fix them.

> @@ -86,6 +85,18 @@
>  #define RTDS_DEFAULT_PERIOD     (MICROSECS(10000))
>  #define RTDS_DEFAULT_BUDGET     (MICROSECS(4000))
>  
> +/*
> + * Max period: max delta of time type
> + * Min period: 100 us, considering the scheduling overhead
> + */
> +#define RTDS_MAX_PERIOD     (STIME_DELTA_MAX)
> +#define RTDS_MIN_PERIOD     (MICROSECS(100))
> +
So, minimum acceptable value is 100, or so one understands, when seeing
this.

Then, below, you do this:

            if ( period < MICROSECS(10) || period > RTDS_MAX_PERIOD ||
                          budget < MICROSECS(10) || budget > period )
            {
                rc = -EINVAL;
                break;
            }
            if ( period < RTDS_MIN_PERIOD || budget < RTDS_MIN_BUDGET )
                warn = 1;

I.e., we warn if the value is below the minimum acceptable, but we
accept it, while we error if the value is below some magic number (10,
in this case).

I'd do the opposite: make 10 the minimum, and #define it so. For gating
the warning, either use another #define, or open code 100, but put down
a comment about it.

> @@ -1137,14 +1148,17 @@ rt_dom_cntl(
>      struct list_head *iter;
>      unsigned long flags;
>      int rc = 0;
> +    int warn = 0;
> +    xen_domctl_schedparam_vcpu_t local_sched;
> +    s_time_t period, budget;
> +    unsigned int index;
>  
>      switch ( op->cmd )
>      {
> -    case XEN_DOMCTL_SCHEDOP_getinfo:
> +    case XEN_DOMCTL_SCHEDOP_getinfo: /* return the default parameters */
>          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.rtds.period = RTDS_DEFAULT_PERIOD / MICROSECS(1); /* transfer 
> to us */
> +        op->u.rtds.budget = RTDS_DEFAULT_BUDGET / MICROSECS(1);
>          spin_unlock_irqrestore(&prv->lock, flags);
>          break;
>      case XEN_DOMCTL_SCHEDOP_putinfo:
>
So, the interface is asymmetric:
 - getinfo: returns defaults
 - putinfo: sets params for all vcpus

This is fine, as there is no much else meaningful we can do with
getinfo, and I don't see putinfo changing the defaults that much useful.

However, I'd put down a brief comment about this here, and a less brief
one in the public header.

> @@ -1162,8 +1176,82 @@ rt_dom_cntl(

> +    case XEN_DOMCTL_SCHEDOP_putvcpuinfo:
> +        spin_lock_irqsave(&prv->lock, flags);
> +        for( index = 0; index < op->u.v.nr_vcpus; index++ )
> +        {
> +            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;
> +            }
> +            svc = rt_vcpu(d->vcpu[local_sched.vcpuid]);
> +            period = MICROSECS(local_sched.s.rtds.period);
> +            budget = MICROSECS(local_sched.s.rtds.budget);
> +            if ( period < MICROSECS(10) || period > RTDS_MAX_PERIOD ||
> +                          budget < MICROSECS(10) || budget > period )
> +            {
> +                rc = -EINVAL;
> +                break;
> +            }
> +            if ( period < RTDS_MIN_PERIOD || budget < RTDS_MIN_BUDGET )
> +                warn = 1;
> +
> +            svc->period = period;
> +            svc->budget = budget;
> +            if( hypercall_preempt_check() )
> +            {
> +                rc = -ERESTART;
> +                break;
> +            }
> +        }
> +        spin_unlock_irqrestore(&prv->lock, flags);
> +        break;
>      }
> -
> +    if ( rc == 0 && warn == 1 ) /* print warning in libxl */
> +        rc = 1;
>      return rc;
>  }
>  
Mmm.. And where is the documentation about the fact that return value of
1 means that the call succeeded, but some upper layer should print a
warning?

Remember that, when working in Xen, libxl is not the only toolstack one
should bear in mind.

In any case, even if documented, I don't like it that much... Why can't
we print a warning here, and just return success?

> diff --git a/xen/common/schedule.c b/xen/common/schedule.c
> index ecf1545..886e1b5 100644
> --- a/xen/common/schedule.c
> +++ b/xen/common/schedule.c
> @@ -1052,10 +1052,22 @@ 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;
> +    else
> +        switch ( op->cmd )
> +        {
> +        case XEN_DOMCTL_SCHEDOP_putinfo:
> +            break;
> +        case XEN_DOMCTL_SCHEDOP_getinfo:
> +            break;
> +        case XEN_DOMCTL_SCHEDOP_putvcpuinfo:
> +            break;
> +        case XEN_DOMCTL_SCHEDOP_getvcpuinfo:
> +            break;
> +        default:
> +            return -EINVAL;
> +        }
>  
>      /* NB: the pluggable scheduler code needs to take care
>       * of locking by itself. */
> 
So, what happens if someone calls XEN_DOMCTL_SCHEDOP_putvcpuinfo or
XEN_DOMCTL_SCHEDOP_getvcpuinfo on Credit or Credit2? The call reports
being successful, but no date is returned (by quickly inspecting
csched_dom_cntl() and csched2_dom_cntl())?

I think you should go there and add something like:

    if ( op->cmd == XEN_DOMCTL_SCHEDOP_putvcpuinfo ||
         op->cmd == XEN_DOMCTL_SCHEDOP_getvcpuinfo )
        return -ENOTSUP;

or something like that.

Again, I know this can't happen in libxl, but that does not really
matter much down here. :-)

> diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
> index bc45ea5..bfb432f 100644
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -330,31 +330,56 @@ DEFINE_XEN_GUEST_HANDLE(xen_domctl_max_vcpus_t);

>  /* Set or get info? */
>
As I said, I'll expand the comment here a bit, mentioning what happens
in the various cases (schedulers supporting or not per-vcpu
information), as well as how, for supporting ones, _getvcpuinfo and
_putvcpuinfo behave.

>  #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_sched_sedf_t sedf;
> +        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;
> +            uint16_t nr_vcpus;
> +        } v;
>      } u;
>  };
>  typedef struct xen_domctl_scheduler_op xen_domctl_scheduler_op_t;

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: signature.asc
Description: This is a digitally signed message part

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