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

[Chong Li <lichong659@xxxxxxxxx>]

On Tue, Jul 7, 2015 at 3:59 AM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>> 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(
>
>> +            {
>> +                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().

My bad. This should not be there.
>
>> +            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.

Yes, you're right.
>
>> +            {
>> +                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?

Vcpuid, Period and budget have been validated in libxl. But we can
still repeat that validation here, if it's needed.
>
>> --- 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.

I see your point. Because of xen_domctl_schedparam_vcpu_t, we still
need to define struct xen_domctl_sched_sedf/credit/credit2/rtds
outside of struct xen_domctl_scheduler_op. Then the struct would be
like:

 struct xen_domctl_scheduler_op {
     uint32_t sched_id;  /* XEN_SCHEDULER_* */
     uint32_t cmd;       /* XEN_DOMCTL_SCHEDOP_* */
     union {
        struct xen_domctl_sched_sedf sedf;
        struct xen_domctl_sched_credit credit;
        struct xen_domctl_sched_credit2 credit2;
        struct xen_domctl_sched_rtds 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;

This design is good for compatibility. Dario, what do you think?

Chong
>
> Jan



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