[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH V2 1/1] Improved RTDS scheduler
On Thu, 2015-12-31 at 05:20 -0500, Tianyang Chen wrote: > > @@ -147,6 +148,16 @@ static unsigned int nr_rt_ops; >  * Global lock is referenced by schedule_data.schedule_lock from all >  * physical cpus. It can be grabbed via vcpu_schedule_lock_irq() >  */ > + > +/* dedicated timer for replenishment */ > +static struct timer repl_timer; > + So, there's always only one timer... Even if we have multiple cpupool with RTDS as their scheduler, they share the replenishment timer? I think it makes more sense to make this per-scheduler. > +/* controls when to first start the timer*/ > +static int timer_started; > + I don't like this, and I don't think we need it. In fact, you removed it yourself from v3, AFAICT. > @@ -635,6 +652,13 @@ rt_vcpu_insert(const struct scheduler *ops, > struct vcpu *vc) >  > ÂÂÂÂÂ/* add rt_vcpu svc to scheduler-specific vcpu list of the dom */ > ÂÂÂÂÂlist_add_tail(&svc->sdom_elem, &svc->sdom->vcpu); > + > +ÂÂÂÂif(!timer_started) > +ÂÂÂÂ{ > +ÂÂÂÂÂÂÂÂ/* the first vcpu starts the timer for the first time*/ > +ÂÂÂÂÂÂÂÂtimer_started = 1; > +ÂÂÂÂÂÂÂÂset_timer(&repl_timer,svc->cur_deadline); > +ÂÂÂÂ} > Â} >  This also seems to be gone in v3, which is good. In fact, it uses timer_started, which I already said I didn't like. About the actual startup of the timer (no matter whether for first time or not). Here, you were doing it in _vcpu_insert() and not in _vcpu_wake(); in v3 you're doing it in _vcpu_wake() and not in _runq_insert()... Which one is the proper way? > @@ -792,44 +816,6 @@ __runq_pick(const struct scheduler *ops, const > cpumask_t *mask) > Â} >  > Â/* > - * Update vcpu's budget and > - * sort runq by insert the modifed vcpu back to runq > - * lock is grabbed before calling this function > - */ > -static void > -__repl_update(const struct scheduler *ops, s_time_t now) > -{ > Please, allow me to say that seeing this function going away, fills my heart with pure joy!! :-D > @@ -889,7 +874,7 @@ rt_schedule(const struct scheduler *ops, s_time_t > now, bool_t tasklet_work_sched > ÂÂÂÂÂÂÂÂÂ} > ÂÂÂÂÂ} >  > -ÂÂÂÂret.time = MIN(snext->budget, MAX_SCHEDULE); /* sched quantum */ > +ÂÂÂÂret.time = snext->budget; /* invoke the scheduler next time */ > ÂÂÂÂÂret.task = snext->vcpu; >  This is ok as it is done in v3 (i.e., snext->budget if !idle, -1 if idle). > @@ -1074,14 +1055,7 @@ rt_vcpu_wake(const struct scheduler *ops, > struct vcpu *vc) > ÂÂÂÂÂ/* insert svc to runq/depletedq because svc is not in queue now > */ > ÂÂÂÂÂ__runq_insert(ops, svc); >  > -ÂÂÂÂ__repl_update(ops, now); > - > -ÂÂÂÂASSERT(!list_empty(&prv->sdom)); > -ÂÂÂÂsdom = list_entry(prv->sdom.next, struct rt_dom, sdom_elem); > -ÂÂÂÂonline = cpupool_scheduler_cpumask(sdom->dom->cpupool); > -ÂÂÂÂsnext = __runq_pick(ops, online); /* pick snext from ALL valid > cpus */ > - > -ÂÂÂÂrunq_tickle(ops, snext); > +ÂÂÂÂrunq_tickle(ops, svc); > And this is another thing I especially like of this patch: it makes the wakeup path a lot simpler and a lot more similar to how it looks like in the other schedulers. Good job with this. :-) > @@ -1108,15 +1078,8 @@ rt_context_saved(const struct scheduler *ops, > struct vcpu *vc) > ÂÂÂÂÂif ( test_and_clear_bit(__RTDS_delayed_runq_add, &svc->flags) && > ÂÂÂÂÂÂÂÂÂÂlikely(vcpu_runnable(vc)) ) > ÂÂÂÂÂ{ > +ÂÂÂÂÂÂÂÂ/* only insert the pre-empted vcpu back*/ > ÂÂÂÂÂÂÂÂÂ__runq_insert(ops, svc); > -ÂÂÂÂÂÂÂÂ__repl_update(ops, NOW()); > - > -ÂÂÂÂÂÂÂÂASSERT(!list_empty(&prv->sdom)); > -ÂÂÂÂÂÂÂÂsdom = list_entry(prv->sdom.next, struct rt_dom, sdom_elem); > -ÂÂÂÂÂÂÂÂonline = cpupool_scheduler_cpumask(sdom->dom->cpupool); > -ÂÂÂÂÂÂÂÂsnext = __runq_pick(ops, online); /* pick snext from ALL > cpus */ > - > -ÂÂÂÂÂÂÂÂrunq_tickle(ops, snext); > ÂÂÂÂÂ} Mmm... I'll think about this more and let you know... But out of the top of my head, I think the tickling has to stay? You preempted a vcpu from the pcpu where it was running, maybe some other pcpu is either idle or running a vcpu with a later deadline, and should come and pick this one up? > @@ -1167,6 +1130,74 @@ rt_dom_cntl( > ÂÂÂÂÂreturn rc; > Â} >  > +static void repl_handler(void *data){ > +ÂÂÂÂunsigned long flags; > +ÂÂÂÂs_time_t now = NOW(); > +ÂÂÂÂs_time_t min_repl = LONG_MAX; /* max time used in comparison*/ > +ÂÂÂÂstruct scheduler *ops = data; > +ÂÂÂÂstruct rt_private *prv = rt_priv(ops); > +ÂÂÂÂstruct list_head *runq = rt_runq(ops); > +ÂÂÂÂstruct list_head *depletedq = rt_depletedq(ops); > +ÂÂÂÂstruct list_head *iter; > +ÂÂÂÂstruct list_head *tmp; > +ÂÂÂÂstruct rt_vcpu *svc = NULL; > + > +ÂÂÂÂspin_lock_irqsave(&prv->lock,flags); > + > +ÂÂÂÂstop_timer(&repl_timer); > + > +ÂÂÂÂlist_for_each_safe(iter, tmp, runq) > +ÂÂÂÂ{ > So, I'm a bit lost here. Why does the _replenishment_ timer's handler scans the runqueue (and, in v3, the running-queue as well)? I'd expect the replenishment timer to do, ehm, replenishments... And I wouldn't expect a vcpu that is either in the runqueue or running to need a replenishment (and, in case it would, I don't think we should take care of that here). > +ÂÂÂÂÂÂÂÂsvc = __q_elem(iter); > +ÂÂÂÂÂÂÂÂif ( now < svc->cur_deadline ) > +ÂÂÂÂÂÂÂÂÂÂÂÂbreak; > +ÂÂÂÂÂÂÂÂrt_update_deadline(now, svc); > +ÂÂÂÂÂÂÂÂ/* scan the runq to find the min release time > +ÂÂÂÂÂÂÂÂÂ* this happens when vcpus on runq miss deadline > +ÂÂÂÂÂÂÂÂÂ*/ > This is exactly my point. It looks to me that you're trying to catch ready or running vcpus missing their deadline in here, in the replenishment timer. I don't think this is appropriate... It makes the logic of the timer handler a lot more complicated than it should be. Oh, and one thing: the use of the term "release time" is IMO a bit misleading. Release of what? Typically, the release time of an RT task (or job) is when the task (or job) is declared ready to run... But I don't think it's used like this in here. I propose to just get rid of it. > +ÂÂÂÂÂÂÂÂif( min_repl> svc->cur_deadline ) > +ÂÂÂÂÂÂÂÂ{ > +ÂÂÂÂÂÂÂÂÂÂÂÂmin_repl = svc->cur_deadline; > +ÂÂÂÂÂÂÂÂ} > +ÂÂÂÂÂÂÂÂ/* reinsert the vcpu if its deadline is updated */ > +ÂÂÂÂÂÂÂÂ__q_remove(svc); > +ÂÂÂÂÂÂÂÂ__runq_insert(ops, svc); > One more proof of what I was trying to say. Is it really this handler's job to --basically-- re-sort the runqueue? I don't think so. What is the specific situation that you are trying to handle like this? In fact, this is also why I'm not convinced of the fact that we need the additional queue for running vcpus. Later in the thread, Meng says: Â"the current running VCPUs on cores also need to replenish their  budgets at the beginning of their next periods." And he makes the following example: Â"[a backlogged] VCPU has its period equal to its budget. Suppose this  VCPU is the only VCPU on a 4 core machine. This VCPU should keep  running on one core and never be put back to runq. In the current  code, this VCPU won't have its budget replenished." But I don't think I understand. When a vcpu runs out of budget, either: Âa. it needs an immediate replenishment Âb. it needs to go to depletedq, and a replenishment event for it   programmed (which may or may not require re-arming the   replenishment timer) Meng's example falls in a., AFAICT, and we can just deal with that when we handle the 'budget exhausted' event (in rt_schedule(), in this case, I think). The case you refer to in the comment above ("when vcpus on runq miss deadline") can either fall in a. or in b., but in both cases it seems to me that you can handle it when it happens, instead than inside this timer handling routine. > + > +ÂÂÂÂ/* if timer was triggered but none of the vcpus > +ÂÂÂÂÂ* need to be refilled, set the timer to be the > +ÂÂÂÂÂ* default period + now > +ÂÂÂÂÂ*/ > +ÂÂÂÂif(min_repl == LONG_MAX) > +ÂÂÂÂ{ > +ÂÂÂÂÂÂÂÂset_timer(&repl_timer, now + RTDS_DEFAULT_PERIOD); > I agree with Meng's point in this thread: this should not be necessary. If it is, it's most likely because of a bug or to something else. Let's figure out what it is, and fix it properly. (I see that in v3 this logic is gone, so hopefully you found and fixed the issue already.) Thanks and 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 _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |