| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20101014123834.GB13048@balbir.in.ibm.com>
Date: Thu, 14 Oct 2010 18:08:34 +0530
From: Balbir Singh <balbir@...ux.vnet.ibm.com>
To: Peter Zijlstra <peterz@...radead.org>
Cc: Bharata B Rao <bharata@...ux.vnet.ibm.com>,
linux-kernel@...r.kernel.org,
Dhaval Giani <dhaval.giani@...il.com>,
Vaidyanathan Srinivasan <svaidy@...ux.vnet.ibm.com>,
Srivatsa Vaddagiri <vatsa@...ibm.com>,
Kamalesh Babulal <kamalesh@...ux.vnet.ibm.com>,
Ingo Molnar <mingo@...e.hu>,
Pavel Emelyanov <xemul@...nvz.org>,
Herbert Poetzl <herbert@...hfloor.at>,
Avi Kivity <avi@...hat.com>,
Chris Friesen <cfriesen@...tel.com>,
Paul Menage <menage@...gle.com>,
Mike Waychison <mikew@...gle.com>,
Paul Turner <pjt@...gle.com>, Nikhil Rao <ncrao@...gle.com>
Subject: Re: [PATCH v3 1/7] sched: introduce primitives to account for CFS
bandwidth tracking
* Peter Zijlstra <peterz@...radead.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists