Messages in this thread |  | | | Date | Thu, 14 Oct 2010 18:08:34 +0530 | | From | Balbir Singh <> | | Subject | Re: [PATCH v3 1/7] sched: introduce primitives to account for CFS bandwidth tracking |
| |
* Peter Zijlstra <peterz@infradead.org> [2010-10-14 09:52:17]:
> On Wed, 2010-10-13 at 18:30 +0530, Balbir Singh wrote: > > > > -static void start_rt_bandwidth(struct rt_bandwidth *rt_b) > > > +static void start_bandwidth_timer(struct hrtimer *period_timer, ktime_t period) > > > { > > > - ktime_t now; > > > + unsigned long delta; > > > + ktime_t soft, hard, now; > > > + > > > + for (;;) { > > > + if (hrtimer_active(period_timer)) > > > + break; > > > > > > + now = hrtimer_cb_get_time(period_timer); > > > + hrtimer_forward(period_timer, now, period); > > > + > > > + soft = hrtimer_get_softexpires(period_timer); > > > + hard = hrtimer_get_expires(period_timer); > > > + delta = ktime_to_ns(ktime_sub(hard, soft)); > > > + __hrtimer_start_range_ns(period_timer, soft, delta, > > > + HRTIMER_MODE_ABS_PINNED, 0); > > > > This code can be replaced with > > > > hrtimer_start_expires(period_timer, HRTIMER_MODE_ABS_PINNED) if we > > don't care about wakeup_softirq, is there a reason we prefer to keep > > wakeup as 0? > > You cannot do wakeups while holding the rq->lock, can you? :-) >
:-) Yes, given that we wakeup softirq only for the current CPU. There is scope for some code reuse anyway, I'll see if I can send a patch.
> > > + } > > > +} > > > > > +static enum hrtimer_restart sched_cfs_period_timer(struct hrtimer *timer) > > > +{ > > > + struct cfs_bandwidth *cfs_b = > > > + container_of(timer, struct cfs_bandwidth, period_timer); > > > + ktime_t now; > > > + int overrun; > > > + int idle = 0; > > > + > > > + for (;;) { > > > + now = hrtimer_cb_get_time(timer); > > > + overrun = hrtimer_forward(timer, now, cfs_b->period); > > > + > > > + if (!overrun) > > > + break; > > > > What is the significance of overrun? overrun is set when delta > > > interval. The logic seems to be that hrtimer is forwarded in steps of > > cfs_b->period till we reach the desired time. > > > > Overrun is the number of periods missed. The goal is to increment the > quota for each period, if the timer is late 3 periods, we still need to > increment it 3 times, that's what overrun does. > > > > + > > > + idle = do_sched_cfs_period_timer(cfs_b, overrun); > > > + } > > > + > > > + return idle ? HRTIMER_NORESTART : HRTIMER_RESTART; > > > +} > > > > +static void start_cfs_bandwidth(struct cfs_bandwidth *cfs_b) > > > +{ > > > + if (cfs_b->quota == RUNTIME_INF) > > > + return; > > > + > > > + if (hrtimer_active(&cfs_b->period_timer)) > > > + return; > > > > Why the double check, start_bandwidth_timer also checks this. Is it to > > avoid doing the check under cfs_b->lock? > > Basically.. > > > > + > > > + raw_spin_lock(&cfs_b->lock); > > > + start_bandwidth_timer(&cfs_b->period_timer, cfs_b->period); > > > + raw_spin_unlock(&cfs_b->lock); > > > +} > > > + > > > > +#ifdef CONFIG_CFS_BANDWIDTH > > > +static int tg_set_cfs_bandwidth(struct task_group *tg, u64 period, u64 quota) > > > +{ > > > + int i; > > > + static DEFINE_MUTEX(mutex); > > > + > > > + if (tg == &init_task_group) > > > + return -EINVAL; > > > + > > > + if (!period) > > > + return -EINVAL; > > > + > > > + /* > > > + * Ensure we have at least one tick of bandwidth every period. This is > > > + * to prevent reaching a state of large arrears when throttled via > > > + * entity_tick() resulting in prolonged exit starvation. > > > + */ > > > + if (NS_TO_JIFFIES(quota) < 1) > > > + return -EINVAL; > > > > I hope we document this in the Documentation :) > > /me went and looked up arrears in a dictionary and wonders why 'debt' > wasn't good enough. > > > > + > > > + mutex_lock(&mutex); > > > + raw_spin_lock_irq(&tg->cfs_bandwidth.lock); > > > + tg->cfs_bandwidth.period = ns_to_ktime(period); > > > + tg->cfs_bandwidth.runtime = tg->cfs_bandwidth.quota = quota; > > > + raw_spin_unlock_irq(&tg->cfs_bandwidth.lock); > > > + > > > + for_each_possible_cpu(i) { > > > > Why not for_each_online_cpu()? > > Probably could be cured with a hotplug handler, but then you need to > track more state iirc. >
What more state? If a CPU is offline, we never get to it, do we? I think we need to do just an init and destroy - no?
> > > + struct cfs_rq *cfs_rq = tg->cfs_rq[i]; > > > + struct rq *rq = rq_of(cfs_rq); > > > + > > > + raw_spin_lock_irq(&rq->lock); > > > + init_cfs_rq_quota(cfs_rq); > > > + raw_spin_unlock_irq(&rq->lock); > > > + } > > > + mutex_unlock(&mutex); > > > + > > > + return 0; > > > +} >
-- Three Cheers, Balbir
|  |