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

[Chong Li <lichong659@xxxxxxxxx>]

On Mon, Mar 7, 2016 at 6:59 AM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>> On 06.03.16 at 18:55, <lichong659@xxxxxxxxx> wrote:

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

Based on previous discussion, XEN_DOMCTL_SCHEDOP_getinfo only returns
the default parameters,
no matter whether vcpu is created yet or not. But I can absolutely
explain this in the description.
>
>> @@ -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)?

Dario suggested this in the last post, because vcpus is a handle and
needs to be validated.

>> +            {
>> +                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?

In libxc, we re-issue hypercall if the current one is preempted.

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

The right side is to check whether the vcpus array is fully processed.
When it is true and no error occurs (rc == 0), we
update op->u.v.nr_vcpus, which is returned to libxc, and helps xc
function figuring out how many un-processed vcpus should
be taken care of in the next hypercall.


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

I'll change it.

Thanks for your comments.
Chong



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