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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 29.06.15 at 04:44, <lichong659@xxxxxxxxx> wrote:
> --- 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

Stray change. Or perhaps the file doesn't build anymore, in which case
you should instead have stated that the patch is dependent upon the
series removing SEDF.

> @@ -1157,8 +1158,75 @@ 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;
> +    case XEN_DOMCTL_SCHEDOP_getvcpuinfo:
> +        spin_lock_irqsave(&prv->lock, flags);
> +        for( index = 0; index < op->u.v.nr_vcpus; index++ )

Coding style (more further down).

> +        {
> +            if ( copy_from_guest_offset(&local_sched,
> +                    op->u.v.vcpus, index, 1) )

Indentation.

> +            {
> +                rc = -EFAULT;
> +                break;
> +            }
> +            if ( local_sched.vcpuid >= d->max_vcpus
> +                    || d->vcpu[local_sched.vcpuid] == NULL )

|| belongs at the end of the first line. Indentation.

> +            {
> +                rc = -EINVAL;
> +                break;
> +            }
> +            svc = rt_vcpu(d->vcpu[local_sched.vcpuid]);
> +
> +            local_sched.vcpuid = svc->vcpu->vcpu_id;

Why? If at all, this should be an ASSERT().

> +            local_sched.s.rtds.budget = svc->budget / MICROSECS(1);
> +            local_sched.s.rtds.period = svc->period / MICROSECS(1);
> +            if( index >= op->u.v.nr_vcpus ) /* not enough guest buffer*/

Impossible due to the containing loop's condition.

> +            {
> +                rc = -ENOBUFS;
> +                break;
> +            }
> +            if ( copy_to_guest_offset(op->u.v.vcpus, index,

__copy_to_guest_offset()

> +    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]);
> +            svc->period = MICROSECS(local_sched.s.rtds.period);
> +            svc->budget = MICROSECS(local_sched.s.rtds.budget);

Are all input values valid here?

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

See above.

> @@ -1054,7 +1053,9 @@ long sched_adjust(struct domain *d, struct 
> xen_domctl_scheduler_op *op)
>  
>      if ( (op->sched_id != DOM2OP(d)->sched_id) ||
>           ((op->cmd != XEN_DOMCTL_SCHEDOP_putinfo) &&
> -          (op->cmd != XEN_DOMCTL_SCHEDOP_getinfo)) )
> +          (op->cmd != XEN_DOMCTL_SCHEDOP_getinfo) &&
> +          (op->cmd != XEN_DOMCTL_SCHEDOP_putvcpuinfo) &&
> +          (op->cmd != XEN_DOMCTL_SCHEDOP_getvcpuinfo)) )
>          return -EINVAL;

Convert to switch() please.

> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -330,31 +330,59 @@ DEFINE_XEN_GUEST_HANDLE(xen_domctl_max_vcpus_t);
>  #define XEN_SCHEDULER_ARINC653 7
>  #define XEN_SCHEDULER_RTDS     8
>  
> +typedef struct xen_domctl_sched_sedf {
> +            uint64_aligned_t period;
> +            uint64_aligned_t slice;
> +            uint64_aligned_t latency;
> +            uint32_t extratime;
> +            uint32_t weight;
> +} xen_domctl_sched_sedf_t;

Indentation.

> +typedef union xen_domctl_schedparam {
> +    xen_domctl_sched_sedf_t sedf;
> +    xen_domctl_sched_credit_t credit;
> +    xen_domctl_sched_credit2_t credit2;
> +    xen_domctl_sched_rtds_t rtds;
> +} xen_domctl_schedparam_t;

I don't see the need for this extra wrapper type. Nor do I see the
need for the typedef here and above - they're generally only
created if you want to also define a matching guest handle type.

> +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;

Explicit padding please.

> +} 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;

With this type gone I'm not even sure we need to wrap this in
another union; not doing so would eliminate some of the other
changes in this patch.

Jan

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