Message ID | 1458146871-2813-2-git-send-email-lichong659@gmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On Wed, 2016-03-16 at 11:47 -0500, Chong Li wrote: > --- a/xen/common/sched_credit.c > +++ b/xen/common/sched_credit.c > @@ -1054,15 +1054,16 @@ csched_dom_cntl( > * lock. Runq lock not needed anywhere in here. */ > spin_lock_irqsave(&prv->lock, flags); > > - if ( op->cmd == XEN_DOMCTL_SCHEDOP_getinfo ) > + switch ( op->cmd ) > { > + case XEN_DOMCTL_SCHEDOP_putvcpuinfo: > + case XEN_DOMCTL_SCHEDOP_getvcpuinfo: > + return -EINVAL; > + case XEN_DOMCTL_SCHEDOP_getinfo: > op->u.credit.weight = sdom->weight; > op->u.credit.cap = sdom->cap; > Not feeling to strong about it, but I think switch ( op->cmd ) { case XEN_DOMCTL_SCHEDOP_getinfo: op->u.credit.weight = sdom->weight; op->u.credit.cap = sdom->cap; break; case XEN_DOMCTL_SCHEDOP_putinfo: if ( op->u.credit.weight != 0 ) { if ( !list_empty(&sdom->active_sdom_elem) ) { prv->weight -= sdom->weight * sdom->active_vcpu_count; prv->weight += op->u.credit.weight * sdom->active_vcpu_count; } sdom->weight = op->u.credit.weight; } if ( op->u.credit.cap != (uint16_t)~0U ) sdom->cap = op->u.credit.cap; break; case XEN_DOMCTL_SCHEDOP_putvcpuinfo: case XEN_DOMCTL_SCHEDOP_getvcpuinfo: default: return -EINVAL; } (i.e., grouping the cases that needs only returning -EINVAL) is better, the final result, more than the patch itself. And the same for Credit2, of course. > --- a/xen/common/sched_rt.c > +++ b/xen/common/sched_rt.c > @@ -1129,24 +1145,22 @@ rt_dom_cntl( > struct vcpu *v; > unsigned long flags; > int rc = 0; > + xen_domctl_schedparam_vcpu_t local_sched; > + s_time_t period, budget; > + uint32_t index = 0; > > 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; > - } > + /* Return the default parameters. > + * A previous bug fixed here: > + * The default PERIOD or BUDGET should be divided by > MICROSECS(1), > + * before returned to upper caller. > + */ Comment style (missing opening '/*'). Also, putting this kind of things in comments is not at all ideal. If doing this in this patch, you should mention it in the changelog. Or you do it in a separate patch (that you put before this one in the series). I'd say that, in this specific case, is not a big deal which one of the two approaches you take (mentioning or separate patch), but the having a separate one is indeed almost always preferable (e.g., the fix can be backported). > + 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); > I don't think we need to take the lock here... RTDS_DEFAULT_PERIOD is not going to change under our feet. :-) > @@ -1163,6 +1177,78 @@ rt_dom_cntl( > } > spin_unlock_irqrestore(&prv->lock, flags); > break; > + case XEN_DOMCTL_SCHEDOP_getvcpuinfo: > + 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; > + } > + > + spin_lock_irqsave(&prv->lock, flags); > + svc = rt_vcpu(d->vcpu[local_sched.vcpuid]); > + local_sched.u.rtds.budget = svc->budget / MICROSECS(1); > + local_sched.u.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, I know it's 0x3f in XEN_SYSCTL_pcitopoinfo, but unless I'm missing something, I don't see why this can't be "63". I'd also put a short comment right above, something like: /* Process a most 64 vCPUs without checking for preemptions. */ > + } > + if ( !rc ) /* notify upper caller how many vcpus have been > processed */ > Move the comment to the line above (and terminate it with a full stop). And by the way, there's a lot of code repetition here. What about handling get and set together, sort of like this: case XEN_DOMCTL_SCHEDOP_getvcpuinfo: | case XEN_DOMCTL_SCHEDOP_putvcpuinfo: |#ifdef CONFIG_HAS_PCI while ( index < op->u.v.nr_vcpus ) | case XEN_SYSCTL_pcitopoinfo: { | { if ( copy_from_guest_offset(&local_sched, | xen_sysctl_pcitopoinfo_t *ti = &op->u.pcitopoinfo; op->u.v.vcpus, index, 1) ) | unsigned int i = 0; { | rc = -EFAULT; | if ( guest_handle_is_null(ti->devs) || break; | guest_handle_is_null(ti->nodes) ) } | { | ret = -EINVAL; if ( local_sched.vcpuid >= d->max_vcpus || | break; d->vcpu[local_sched.vcpuid] == NULL ) | } { | rc = -EINVAL; | while ( i < ti->num_devs ) break; | { } | physdev_pci_device_t dev; | uint32_t node; if ( op->cmd == XEN_DOMCTL_SCHEDOP_getvcpuinfo ) | const struct pci_dev *pdev; { | spin_lock_irqsave(&prv->lock, flags); | if ( copy_from_guest_offset(&dev, ti->devs, i, 1) ) svc = rt_vcpu(d->vcpu[local_sched.vcpuid]); | { local_sched.u.rtds.budget = svc->budget / MICROSECS(1); | ret = -EFAULT; local_sched.u.rtds.period = svc->period / MICROSECS(1); | break; spin_unlock_irqrestore(&prv->lock, flags); | } | if ( copy_to_guest_offset(op->u.v.vcpus, index, | pcidevs_lock(); &local_sched, 1) ) | pdev = pci_get_pdev(dev.seg, dev.bus, dev.devfn); { | if ( !pdev ) rc = -EFAULT; | node = XEN_INVALID_DEV; break; | else if ( pdev->node == NUMA_NO_NODE ) } | node = XEN_INVALID_NODE_ID; } | else else | node = pdev->node; { | pcidevs_unlock(); period = MICROSECS(local_sched.u.rtds.period); | budget = MICROSECS(local_sched.u.rtds.budget); | if ( copy_to_guest_offset(ti->nodes, i, &node, 1) ) if ( period > RTDS_MAX_PERIOD || budget < RTDS_MIN_BUDGET || | { budget > period || period < RTDS_MIN_PERIOD ) | ret = -EFAULT; { | break; rc = -EINVAL; | } break; | } | /* Process a most 64 vCPUs without checking for preemptions. */ | if ( (++i > 0x3f) && hypercall_preempt_check() ) spin_lock_irqsave(&prv->lock, flags); | break; svc = rt_vcpu(d->vcpu[local_sched.vcpuid]); | } svc->period = period; | svc->budget = budget; | if ( !ret && (ti->num_devs != i) ) spin_unlock_irqrestore(&prv->lock, flags); | { | ti->num_devs = i; } | if ( __copy_field_to_guest(u_sysctl, op, u.pcitopoinfo.num_devs) ) /* Process a most 64 vCPUs without checking for preemptions. */ | ret = -EFAULT; if ( (++index > 63) && hypercall_preempt_check() ) | } break; | break; } | } |#endif /* Notify upper caller how many vcpus have been processed. */ | if ( !rc ) | case XEN_SYSCTL_tmem_op: op->u.v.nr_vcpus = index; | ret = tmem_control(&op->u.tmem_op); break; I have only compile tested this, but it looks to me that it can fly... > --- a/xen/include/public/domctl.h > +++ b/xen/include/public/domctl.h > + * Set or get info? > + * For schedulers supporting per-vcpu settings (e.g., RTDS): > + * XEN_DOMCTL_SCHEDOP_putinfo sets params for all vcpus; > + * XEN_DOMCTL_SCHEDOP_getinfo gets default params; > + * XEN_DOMCTL_SCHEDOP_put(get)vcpuinfo sets (gets) params of vcpus; > + * > + * For schedulers not supporting per-vcpu settings: > + * XEN_DOMCTL_SCHEDOP_putinfo sets params for all vcpus; > + * XEN_DOMCTL_SCHEDOP_getinfo gets domain-wise params; > + * XEN_DOMCTL_SCHEDOP_put(get)vcpuinfo returns error; > + */ > #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_* */ /* IN/OUT */ > union { > - 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_sched_credit_t credit; > + xen_domctl_sched_credit2_t credit2; > + xen_domctl_sched_rtds_t rtds; > + struct { > + XEN_GUEST_HANDLE_64(xen_domctl_schedparam_vcpu_t) vcpus; > + /* > + * IN: Number of elements in vcpus array. > + * OUT: Number of processed elements of vcpus array. > + */ > + uint32_t nr_vcpus; > + uint32_t padding; > + } v; > } u; > }; > That is: make it even more clear that the whole union is used as IN/OUT. Then, indeed, inside v, what is the meaning of the nr_vcpus field in each direction, as you're doing already. Regards, Dario
On Thu, Mar 17, 2016 at 5:03 AM, Dario Faggioli <dario.faggioli@citrix.com> wrote: > On Wed, 2016-03-16 at 11:47 -0500, Chong Li wrote: > >> --- a/xen/common/sched_rt.c >> +++ b/xen/common/sched_rt.c >> @@ -1129,24 +1145,22 @@ rt_dom_cntl( >> struct vcpu *v; >> unsigned long flags; >> int rc = 0; >> + xen_domctl_schedparam_vcpu_t local_sched; >> + s_time_t period, budget; >> + uint32_t index = 0; >> >> 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; >> - } >> + /* Return the default parameters. >> + * A previous bug fixed here: >> + * The default PERIOD or BUDGET should be divided by >> MICROSECS(1), >> + * before returned to upper caller. >> + */ > Comment style (missing opening '/*'). > > Also, putting this kind of things in comments is not at all ideal. If > doing this in this patch, you should mention it in the changelog. Or > you do it in a separate patch (that you put before this one in the > series). > > I'd say that, in this specific case, is not a big deal which one of the > two approaches you take (mentioning or separate patch), but the having > a separate one is indeed almost always preferable (e.g., the fix can be > backported). If I choose mentioning, do I move the comment to the changelog? Or do I keep it here and say it again in the changelog? Chong
>>> On 17.03.16 at 21:42, <lichong659@gmail.com> wrote: > On Thu, Mar 17, 2016 at 5:03 AM, Dario Faggioli > <dario.faggioli@citrix.com> wrote: >> On Wed, 2016-03-16 at 11:47 -0500, Chong Li wrote: > >> >>> --- a/xen/common/sched_rt.c >>> +++ b/xen/common/sched_rt.c >>> @@ -1129,24 +1145,22 @@ rt_dom_cntl( >>> struct vcpu *v; >>> unsigned long flags; >>> int rc = 0; >>> + xen_domctl_schedparam_vcpu_t local_sched; >>> + s_time_t period, budget; >>> + uint32_t index = 0; >>> >>> 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; >>> - } >>> + /* Return the default parameters. >>> + * A previous bug fixed here: >>> + * The default PERIOD or BUDGET should be divided by >>> MICROSECS(1), >>> + * before returned to upper caller. >>> + */ >> Comment style (missing opening '/*'). >> >> Also, putting this kind of things in comments is not at all ideal. If >> doing this in this patch, you should mention it in the changelog. Or >> you do it in a separate patch (that you put before this one in the >> series). >> >> I'd say that, in this specific case, is not a big deal which one of the >> two approaches you take (mentioning or separate patch), but the having >> a separate one is indeed almost always preferable (e.g., the fix can be >> backported). > > If I choose mentioning, do I move the comment to the changelog? Or do I keep > it here and say it again in the changelog? Just consider what would happen if everyone mentioned in comments the bugs they fixed. I think the answer to you question is obvious then... Jan
>>> On 17.03.16 at 11:03, <dario.faggioli@citrix.com> wrote: > On Wed, 2016-03-16 at 11:47 -0500, Chong Li wrote: >> --- a/xen/common/sched_credit.c >> +++ b/xen/common/sched_credit.c >> @@ -1054,15 +1054,16 @@ csched_dom_cntl( >> * lock. Runq lock not needed anywhere in here. */ >> spin_lock_irqsave(&prv->lock, flags); >> >> - if ( op->cmd == XEN_DOMCTL_SCHEDOP_getinfo ) >> + switch ( op->cmd ) >> { >> + case XEN_DOMCTL_SCHEDOP_putvcpuinfo: >> + case XEN_DOMCTL_SCHEDOP_getvcpuinfo: >> + return -EINVAL; >> + case XEN_DOMCTL_SCHEDOP_getinfo: >> op->u.credit.weight = sdom->weight; >> op->u.credit.cap = sdom->cap; >> > Not feeling to strong about it, but I think > > switch ( op->cmd ) > { > case XEN_DOMCTL_SCHEDOP_getinfo: > op->u.credit.weight = sdom->weight; > op->u.credit.cap = sdom->cap; > break; > case XEN_DOMCTL_SCHEDOP_putinfo: > if ( op->u.credit.weight != 0 ) > { > if ( !list_empty(&sdom->active_sdom_elem) ) > { > prv->weight -= sdom->weight * sdom->active_vcpu_count; > prv->weight += op->u.credit.weight * sdom->active_vcpu_count; > } > sdom->weight = op->u.credit.weight; > } > > if ( op->u.credit.cap != (uint16_t)~0U ) > sdom->cap = op->u.credit.cap; > break; > case XEN_DOMCTL_SCHEDOP_putvcpuinfo: > case XEN_DOMCTL_SCHEDOP_getvcpuinfo: > default: > return -EINVAL; > } > > (i.e., grouping the cases that needs only returning -EINVAL) is better, > the final result, more than the patch itself. In fact there's no point in explicitly naming the unhandled values as long as they're matching what "default:" does. Jan
On Fri, 2016-03-18 at 01:39 -0600, Jan Beulich wrote: > > > > > > On 17.03.16 at 21:42, <lichong659@gmail.com> wrote: > > On Thu, Mar 17, 2016 at 5:03 AM, Dario Faggioli > > <dario.faggioli@citrix.com> wrote: > > > > > > I'd say that, in this specific case, is not a big deal which one > > > of the > > > two approaches you take (mentioning or separate patch), but the > > > having > > > a separate one is indeed almost always preferable (e.g., the fix > > > can be > > > backported). > > If I choose mentioning, do I move the comment to the changelog? Or > > do I keep > > it here and say it again in the changelog? > Just consider what would happen if everyone mentioned in > comments the bugs they fixed. I think the answer to you question > is obvious then... > Exactly! :-) And, please (Chong), let me restate this: "having a separate one is indeed almost always preferable (e.g., the fix can be backported)" Regards, Dario
On Fri, Mar 18, 2016 at 5:47 AM, Dario Faggioli <dario.faggioli@citrix.com> wrote: > On Fri, 2016-03-18 at 01:39 -0600, Jan Beulich wrote: >> > >> > > > On 17.03.16 at 21:42, <lichong659@gmail.com> wrote: >> > On Thu, Mar 17, 2016 at 5:03 AM, Dario Faggioli >> > <dario.faggioli@citrix.com> wrote: >> > > >> > > I'd say that, in this specific case, is not a big deal which one >> > > of the >> > > two approaches you take (mentioning or separate patch), but the >> > > having >> > > a separate one is indeed almost always preferable (e.g., the fix >> > > can be >> > > backported). >> > If I choose mentioning, do I move the comment to the changelog? Or >> > do I keep >> > it here and say it again in the changelog? >> Just consider what would happen if everyone mentioned in >> comments the bugs they fixed. I think the answer to you question >> is obvious then... >> > Exactly! :-) > > And, please (Chong), let me restate this: "having a separate one is > indeed almost always preferable (e.g., the fix can be backported)" > Considering the complexity of making a new patch and today's the last day for posting, I'll mention this bug fix in the changlog. Chong
diff --git a/xen/common/sched_credit.c b/xen/common/sched_credit.c index 0dce790..82b0d14 100644 --- a/xen/common/sched_credit.c +++ b/xen/common/sched_credit.c @@ -1054,15 +1054,16 @@ csched_dom_cntl( * lock. Runq lock not needed anywhere in here. */ spin_lock_irqsave(&prv->lock, flags); - if ( op->cmd == XEN_DOMCTL_SCHEDOP_getinfo ) + switch ( op->cmd ) { + case XEN_DOMCTL_SCHEDOP_putvcpuinfo: + case XEN_DOMCTL_SCHEDOP_getvcpuinfo: + return -EINVAL; + case XEN_DOMCTL_SCHEDOP_getinfo: op->u.credit.weight = sdom->weight; op->u.credit.cap = sdom->cap; - } - else - { - ASSERT(op->cmd == XEN_DOMCTL_SCHEDOP_putinfo); - + break; + case XEN_DOMCTL_SCHEDOP_putinfo: if ( op->u.credit.weight != 0 ) { if ( !list_empty(&sdom->active_sdom_elem) ) @@ -1075,7 +1076,9 @@ csched_dom_cntl( if ( op->u.credit.cap != (uint16_t)~0U ) sdom->cap = op->u.credit.cap; - + break; + default: + return -EINVAL; } spin_unlock_irqrestore(&prv->lock, flags); diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c index 3c49ffa..46d54bc 100644 --- a/xen/common/sched_credit2.c +++ b/xen/common/sched_credit2.c @@ -1421,14 +1421,15 @@ csched2_dom_cntl( * runq lock to update csvcs. */ spin_lock_irqsave(&prv->lock, flags); - if ( op->cmd == XEN_DOMCTL_SCHEDOP_getinfo ) + switch ( op->cmd ) { + case XEN_DOMCTL_SCHEDOP_putvcpuinfo: + case XEN_DOMCTL_SCHEDOP_getvcpuinfo: + return -EINVAL; + case XEN_DOMCTL_SCHEDOP_getinfo: op->u.credit2.weight = sdom->weight; - } - else - { - ASSERT(op->cmd == XEN_DOMCTL_SCHEDOP_putinfo); - + break; + case XEN_DOMCTL_SCHEDOP_putinfo: if ( op->u.credit2.weight != 0 ) { struct vcpu *v; @@ -1457,6 +1458,9 @@ csched2_dom_cntl( vcpu_schedule_unlock(lock, svc->vcpu); } } + break; + default: + return -EINVAL; } spin_unlock_irqrestore(&prv->lock, flags); diff --git a/xen/common/sched_rt.c b/xen/common/sched_rt.c index 3f1d047..359c2db 100644 --- a/xen/common/sched_rt.c +++ b/xen/common/sched_rt.c @@ -86,6 +86,22 @@ #define RTDS_DEFAULT_PERIOD (MICROSECS(10000)) #define RTDS_DEFAULT_BUDGET (MICROSECS(4000)) +/* + * Max period: max delta of time type, because period is added to the time + * a vcpu activates, so this must not overflow. + * Min period: 10 us, considering the scheduling overhead (when period is + * too low, scheduling is invoked too frequently, causing high overhead). + */ +#define RTDS_MAX_PERIOD (STIME_DELTA_MAX) +#define RTDS_MIN_PERIOD (MICROSECS(10)) + +/* + * Min budget: 10 us, considering the scheduling overhead (when budget is + * consumed too fast, scheduling is invoked too frequently, causing + * high overhead). + */ +#define RTDS_MIN_BUDGET (MICROSECS(10)) + #define UPDATE_LIMIT_SHIFT 10 #define MAX_SCHEDULE (MILLISECS(1)) /* @@ -1129,24 +1145,22 @@ rt_dom_cntl( struct vcpu *v; unsigned long flags; int rc = 0; + xen_domctl_schedparam_vcpu_t local_sched; + s_time_t period, budget; + uint32_t index = 0; 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; - } + /* Return the default parameters. + * A previous bug fixed here: + * The default PERIOD or BUDGET should be divided by MICROSECS(1), + * before returned to upper caller. + */ + 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; case XEN_DOMCTL_SCHEDOP_putinfo: if ( op->u.rtds.period == 0 || op->u.rtds.budget == 0 ) @@ -1163,6 +1177,78 @@ rt_dom_cntl( } spin_unlock_irqrestore(&prv->lock, flags); break; + case XEN_DOMCTL_SCHEDOP_getvcpuinfo: + 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; + } + + spin_lock_irqsave(&prv->lock, flags); + svc = rt_vcpu(d->vcpu[local_sched.vcpuid]); + local_sched.u.rtds.budget = svc->budget / MICROSECS(1); + local_sched.u.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; + } + if ( !rc ) /* notify upper caller how many vcpus have been processed */ + op->u.v.nr_vcpus = index; + break; + + case XEN_DOMCTL_SCHEDOP_putvcpuinfo: + 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.u.rtds.period); + budget = MICROSECS(local_sched.u.rtds.budget); + if ( period > RTDS_MAX_PERIOD || budget < RTDS_MIN_BUDGET || + budget > period || period < RTDS_MIN_PERIOD ) + { + rc = -EINVAL; + break; + } + + spin_lock_irqsave(&prv->lock, flags); + svc = rt_vcpu(d->vcpu[local_sched.vcpuid]); + svc->period = period; + svc->budget = budget; + spin_unlock_irqrestore(&prv->lock, flags); + + if ( (++index > 0x3f) && hypercall_preempt_check() ) + break; + } + if ( !rc ) /* notify upper caller how many vcpus have been processed */ + op->u.v.nr_vcpus = index; + break; } return rc; diff --git a/xen/common/schedule.c b/xen/common/schedule.c index c195129..cd25e08 100644 --- a/xen/common/schedule.c +++ b/xen/common/schedule.c @@ -1148,11 +1148,20 @@ 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; + switch ( op->cmd ) + { + case XEN_DOMCTL_SCHEDOP_putinfo: + case XEN_DOMCTL_SCHEDOP_getinfo: + case XEN_DOMCTL_SCHEDOP_putvcpuinfo: + case XEN_DOMCTL_SCHEDOP_getvcpuinfo: + break; + default: + return -EINVAL; + } + /* NB: the pluggable scheduler code needs to take care * of locking by itself. */ if ( (ret = SCHED_OP(DOM2OP(d), adjust, d, op)) == 0 ) diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h index 7a56b3f..9297c01 100644 --- a/xen/include/public/domctl.h +++ b/xen/include/public/domctl.h @@ -338,24 +338,63 @@ DEFINE_XEN_GUEST_HANDLE(xen_domctl_max_vcpus_t); #define XEN_SCHEDULER_ARINC653 7 #define XEN_SCHEDULER_RTDS 8 -/* Set or get info? */ +typedef struct xen_domctl_sched_credit { + uint16_t weight; + uint16_t cap; +} xen_domctl_sched_credit_t; + +typedef struct xen_domctl_sched_credit2 { + uint16_t weight; +} xen_domctl_sched_credit2_t; + +typedef struct xen_domctl_sched_rtds { + uint32_t period; + uint32_t budget; +} xen_domctl_sched_rtds_t; + +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; + } u; + uint32_t vcpuid; + uint16_t padding[2]; +} xen_domctl_schedparam_vcpu_t; +DEFINE_XEN_GUEST_HANDLE(xen_domctl_schedparam_vcpu_t); + +/* + * Set or get info? + * For schedulers supporting per-vcpu settings (e.g., RTDS): + * XEN_DOMCTL_SCHEDOP_putinfo sets params for all vcpus; + * XEN_DOMCTL_SCHEDOP_getinfo gets default params; + * XEN_DOMCTL_SCHEDOP_put(get)vcpuinfo sets (gets) params of vcpus; + * + * For schedulers not supporting per-vcpu settings: + * XEN_DOMCTL_SCHEDOP_putinfo sets params for all vcpus; + * XEN_DOMCTL_SCHEDOP_getinfo gets domain-wise params; + * XEN_DOMCTL_SCHEDOP_put(get)vcpuinfo returns error; + */ #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_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_sched_credit_t credit; + xen_domctl_sched_credit2_t credit2; + xen_domctl_sched_rtds_t rtds; + struct { + XEN_GUEST_HANDLE_64(xen_domctl_schedparam_vcpu_t) vcpus; + /* + * IN: Number of elements in vcpus array. + * OUT: Number of processed elements of vcpus array. + */ + uint32_t nr_vcpus; + uint32_t padding; + } v; } u; }; typedef struct xen_domctl_scheduler_op xen_domctl_scheduler_op_t;