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

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

On Tue, 2015-07-07 at 23:06 -0700, Meng Xu wrote:
> 2015-07-07 7:39 GMT-07:00 Dario Faggioli <dario.faggioli@xxxxxxxxxx>:
> > On Tue, 2015-07-07 at 09:59 +0100, Jan Beulich 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.
> >>
> > This indeed does not belong in here. And of course, things should
> > build... So, Chong, either deal with SEDF as well, if basing your
> > patches on a tree where it is still there, or base on top of my patches,
> > ignore it, but state the dependency, as Jan is asking.
> >
> >> > @@ -1157,8 +1158,75 @@ 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]);
> >> > +            svc->period = MICROSECS(local_sched.s.rtds.period);
> >> > +            svc->budget = MICROSECS(local_sched.s.rtds.budget);
> >>
> >> Are all input values valid here?
> >>
> > That's a good point, actually. Right now, SEDF does some range
> > enforcement, by means of these values:
> >
> > #define PERIOD_MAX MILLISECS(10000) /* 10s  */
> > #define PERIOD_MIN (MICROSECS(10))  /* 10us */
> > #define SLICE_MIN (MICROSECS(5))    /*  5us */
> >
> > Chong, it probably makes sense to (in a separate patch), introduce
> > something like this in RTDS too (with SLICE_MIN-->BUDGET_MIN), and then
> > use them, in this patch, for sanity checking the input.
> >
> > It also makes sense to check and enforce budget<=period, IMO.
> >
> > About the specific values, I'm open to proposals. I think something like
> > the SEDF's one is fine. Meng?
> 
> We are trying to make some range enforcement for RTDS scheduler. Is my
> understanding correct? (It should be, but just in case. :-) )
> 
We are wondering whether that could be necessary/useful, and IMO, it
would.

> As to the range of period, I think the max value can be as large as
> the type of period (ie. s_time_t) can represent. When we want a
> dedicated CPU for a guest, we will set budget=period and  can set the
> period to a very very large value to avoid the unnecessarily
> invocation of the scheduler.
>
Makes sense. We do have STIME_MAX and, given that period is something
that is added to current time during scheduling, STIME_DELTA_MAX.

Maybe, put something together basing on those? 

> As to the min value of period, I think it should be >=100us. The
> scheduler overhead of running a large box could be 1us if the runq is
> long and competetion of the runq lock is heavy. If the scheduler is
> potentially invoked every 10us, the scheduler overhead will be 10% of
> total computation time, which seems a lot to me.
> 
Ok.

> As to the range of budget, the min value can be 5us, the same with
> SEDF; 
>
Well, wouldn't the above reasoning about overhead apply here too?
Budgets of 5us mean the scheduler can be invoked every 5us for budget
enforcement. If 10us was unreasonable, 5 is even more so.

Therefore, 100us here too? Or maybe let's allow for lower values (like
50us or 10us), but print a warning?

> the max value is the value of period of the same VCPU.
> 
Yep.

And, whatever the values, it would be useful to have comments somewhere
(either when the values are defined or enforced), stating what you said
above.

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)

Attachment: signature.asc
Description: This is a digitally signed message part

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