Re: [PATCH v3 1/7] sched: introduce primitives to account for CFSbandwidth tracking

[Balbir Singh]

* Peter Zijlstra <peterz@xxxxxxxxxxxxx> [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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/