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

[Meng Xu <xumengpanda@xxxxxxxxx>]

2015-07-08 1:33 GMT-07:00 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?

Right. we can use 100us as a threadhold for budget too. Maybe printing
warning is a better idea?
If users know what they are doing and expecting, it is ok. But too
small value (<1us) will potentially cause system freeze.


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

right.

Thanks,

Meng
-- 


-----------
Meng Xu
PhD Student in Computer and Information Science
University of Pennsylvania
http://www.cis.upenn.edu/~mengxu/

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