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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 06.03.16 at 18:55, <lichong659@xxxxxxxxx> wrote:
> --- a/xen/common/sched_credit.c
> +++ b/xen/common/sched_credit.c
> @@ -1054,6 +1054,10 @@ csched_dom_cntl(
>       * lock. Runq lock not needed anywhere in here. */
>      spin_lock_irqsave(&prv->lock, flags);
>  
> +    if ( op->cmd == XEN_DOMCTL_SCHEDOP_putvcpuinfo ||
> +         op->cmd == XEN_DOMCTL_SCHEDOP_getvcpuinfo )
> +        return -EINVAL;
> +
>      if ( op->cmd == XEN_DOMCTL_SCHEDOP_getinfo )
>      {
>          op->u.credit.weight = sdom->weight;

Considering the rest of the code following where, I would - albeit
I'm not maintainer of this code - strongly suggest moving to
switch() in such cases, with the default case returning -EINVAL (or
maybe better -EOPNOTSUPP).

> @@ -1130,23 +1146,17 @@ rt_dom_cntl(
>      unsigned long flags;
>      int rc = 0;
>  
> +    xen_domctl_schedparam_vcpu_t local_sched;
> +    s_time_t period, budget;
> +    uint32_t index = 0;
> +

There's a stray blank line left ahead of this addition.

>      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;
> -        }
> +    case XEN_DOMCTL_SCHEDOP_getinfo: /* return the default parameters */
> +        spin_lock_irqsave(&prv->lock, flags);
> +        op->u.rtds.period = RTDS_DEFAULT_PERIOD / MICROSECS(1);
> +        op->u.rtds.budget = RTDS_DEFAULT_BUDGET / MICROSECS(1);
> +        spin_unlock_irqrestore(&prv->lock, flags);
>          break;

This alters the values returned when d->max_vcpus == 0 - while
this looks to be intentional, I think calling out such a bug fix in the
description is a must.

> @@ -1163,6 +1173,96 @@ rt_dom_cntl(
>          }
>          spin_unlock_irqrestore(&prv->lock, flags);
>          break;
> +    case XEN_DOMCTL_SCHEDOP_getvcpuinfo:
> +        if ( guest_handle_is_null(op->u.v.vcpus) )
> +        {
> +            rc = -EINVAL;

Perhaps rather -EFAULT? But then again - what is this check good for
(considering that it doesn't cover other obviously bad handle values)?

> +            break;
> +        }
> +        while ( index < op->u.v.nr_vcpus )
> +        {
> +            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 )

Again. And more below.

> +            {
> +                rc = -EINVAL;
> +                break;
> +            }
> +
> +            spin_lock_irqsave(&prv->lock, flags);
> +            svc = rt_vcpu(d->vcpu[local_sched.vcpuid]);
> +            local_sched.s.rtds.budget = svc->budget / MICROSECS(1);
> +            local_sched.s.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;
> +            }
> +            if ( (++index > 0x3f) && hypercall_preempt_check() )
> +                break;

So how is the caller going to be able to reliably read all vCPU-s'
information for a guest with more than 64 vCPU-s?

> +        }
> +
> +        if ( !rc && (op->u.v.nr_vcpus != index) )
> +            op->u.v.nr_vcpus = index;

I don't think the right side of the && is really necessary / useful.

> +        break;
> +    case XEN_DOMCTL_SCHEDOP_putvcpuinfo:

When switch statements get large, please put blank lines between
individual case blocks.

> +        if ( guest_handle_is_null(op->u.v.vcpus) )
> +        {
> +            rc = -EINVAL;
> +            break;
> +        }
> +        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;
> +            }
> +
> +            period = MICROSECS(local_sched.s.rtds.period);
> +            budget = MICROSECS(local_sched.s.rtds.budget);
> +            if ( period > RTDS_MAX_PERIOD || budget < RTDS_MIN_BUDGET ||
> +                          budget > period || period < RTDS_MIN_PERIOD )
> +            {
> +                rc = -EINVAL;
> +                break;
> +            }
> +
> +            /*
> +             * We accept period/budget less than 100 us, but will warn users 
> about
> +             * the large scheduling overhead due to it
> +             */
> +            if ( period < MICROSECS(100) || budget < MICROSECS(100) )
> +                printk("Warning: period or budget set to less than 100us.\n"
> +                       "This may result in high scheduling overhead.\n");

This should use a log level which is rate limited by default. Quite
likely that would be one of the guest log levels.

> --- a/xen/common/schedule.c
> +++ b/xen/common/schedule.c
> @@ -1148,10 +1148,19 @@ 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 )

Pointless else, pointlessly increasing the necessary indentation
for the entire switch().

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

Please call such unions "u", as done everywhere else.

> +    uint16_t vcpuid;

Any particular reason to limit this to 16 bits, when elsewhere
we commonly use 32 bits for vCPU IDs?

Jan

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