Re: [Xen-devel] [PATCH v8]xen: sched: convert RTDS from time to event driven model

["Chen, Tianyang" <tiche@xxxxxxxxxxxxxx>]

On 03/11/2016 11:54 PM, Meng Xu wrote:
> I'm focusing on the style and the logic in the replenish handler:
>
> >   /*
> > @@ -160,6 +180,7 @@ struct rt_private {
> >    */
> >   struct rt_vcpu {
> >       struct list_head q_elem;    /* on the runq/depletedq list */
> > +    struct list_head replq_elem;/* on the repl event list */
> missing space before /*
I wasn't sure if all the comments should be lined up or not. Maybe there 
should be one more space for all the fields so they nicely line up?
> > +static int
> > +deadline_queue_insert(struct rt_vcpu * (*_get_q_elem)(struct list_head *elem),
> > +    struct rt_vcpu *svc, struct list_head *elem, struct list_head *queue)
> > +{
> > +    struct list_head *iter;
> > +    int pos = 0;
> > +
> > +    list_for_each(iter, queue)
> space after ( and before )
> If not sure about the space, we can refer to the sched_credit.c for
> the style as well, beside the CODING_STYLE file. :-)
Ok. But in __runq_pick() there is no space. Also there is no space in 
the definition of this macro:
#define list_for_each(pos, head) \
Which one should I follow?


> > @@ -707,7 +888,14 @@ burn_budget(const struct scheduler *ops, struct rt_vcpu 
> > *svc, s_time_t now)
> >       svc->cur_budget -= delta;
> >
> >       if ( svc->cur_budget < 0 )
> Although this line is not changed in the patch. I'm thinking maybe it should be
> if ( svc->cur_budget <= 0 )
> because budget == 0 also means depleted budget.
Right.

> > +            /*
> > +             * If the vcpu is running, let the head
> > +             * of the run queue tickle if it has a
> > +             * higher priority.
> > +             */
> > +            struct rt_vcpu *next_on_runq = __q_elem(runq->next);
> > +            if ( svc->cur_deadline >= next_on_runq->cur_deadline )
> It's better to be if ( svc->cur_deadline > next_on_runq->cur_deadline
> ), to avoid the unnecessary tickle when they have same priority.
>
> We assume priority tie is broken arbitrarily.
OK.

> Great and expedite work, Tianyang!
> This version looks good.
> Can you set up a repo. with the previous version of the patch and this
> version of the patch so that I can diff. these two versions to make
> sure I didn't miss anything you modified from the last version.
Sure.

> One more thing we should think about is:
> How can we "prove/test" the correctness of the scheduler?
> Can we use xentrace to record the scheduling trace and then write a
> userspace program to check the scheduling trace is obeying the
> priority rules of the scheduler.
Thanks for the review Meng, I am still exploring xentrace and it can 
output scheduling events such as which vcpu is running on a pcpu. I 
think it's possible for the userspace program to check RTDS, based on 
cur_budget and cur_deadline. We need to have a very clear outline of 
rules, for the things we are concerned about. When you say correctness, 
what does it include? I'm thinking about rules for when a vcpu should 
preempt, tickle and actually be picked.

Thanks,
Tianyang

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