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

[Chong Li <lichong659@xxxxxxxxx>]

On Mon, Jul 27, 2015 at 10:51 AM, Dario Faggioli
<dario.faggioli@xxxxxxxxxx> wrote:
> 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

>
>> @@ -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?

Do I just use printk here? Is there any other more appropriate print
function in xen?

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

I see. Will do.

Chong

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



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

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