[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <AANLkTi=qE-tFox+qGyASmqCwZuvbq0wPH0v0Ai4NJf+O@mail.gmail.com>
Date: Thu, 24 Feb 2011 19:25:01 -0800
From: Paul Turner <pjt@...gle.com>
To: Peter Zijlstra <a.p.zijlstra@...llo.nl>
Cc: linux-kernel@...r.kernel.org,
Bharata B Rao <bharata@...ux.vnet.ibm.com>,
Dhaval Giani <dhaval@...ux.vnet.ibm.com>,
Balbir Singh <balbir@...ux.vnet.ibm.com>,
Vaidyanathan Srinivasan <svaidy@...ux.vnet.ibm.com>,
Gautham R Shenoy <ego@...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>
Subject: Re: [CFS Bandwidth Control v4 6/7] sched: hierarchical task
accounting for SCHED_OTHER
On Wed, Feb 23, 2011 at 5:32 AM, Peter Zijlstra <a.p.zijlstra@...llo.nl> wrote:
> On Tue, 2011-02-15 at 19:18 -0800, Paul Turner wrote:
>
>> @@ -1846,6 +1846,11 @@ static const struct sched_class rt_sched
>>
>> #include "sched_stats.h"
>>
>> +static void mod_nr_running(struct rq *rq, long delta)
>> +{
>> + rq->nr_running += delta;
>> +}
>
> I personally don't see much use in such trivial wrappers.. if you're
> going to rework all the nr_running stuff you might as well remove all of
> that.
I'm ok with that, I was trying to preserve some of the existing
encapsulation but there isn't a need for it.
Will do.
>
>> Index: tip/kernel/sched_fair.c
>> ===================================================================
>> --- tip.orig/kernel/sched_fair.c
>> +++ tip/kernel/sched_fair.c
>> @@ -81,6 +81,8 @@ unsigned int normalized_sysctl_sched_wak
>>
>> const_debug unsigned int sysctl_sched_migration_cost = 500000UL;
>>
>> +static void account_hier_tasks(struct sched_entity *se, int delta);
>> +
>> /*
>> * The exponential sliding window over which load is averaged for shares
>> * distribution.
>> @@ -933,6 +935,40 @@ static inline void update_entity_shares_
>> }
>> #endif /* CONFIG_FAIR_GROUP_SCHED */
>>
>> +#ifdef CONFIG_CFS_BANDWIDTH
>> +/* maintain hierarchal task counts on group entities */
>> +static void account_hier_tasks(struct sched_entity *se, int delta)
>> +{
>> + struct rq *rq = rq_of(cfs_rq_of(se));
>> + struct cfs_rq *cfs_rq;
>> +
>> + for_each_sched_entity(se) {
>> + /* a throttled entity cannot affect its parent hierarchy */
>> + if (group_cfs_rq(se) && cfs_rq_throttled(group_cfs_rq(se)))
>> + break;
>> +
>> + /* we affect our queuing entity */
>> + cfs_rq = cfs_rq_of(se);
>> + cfs_rq->h_nr_tasks += delta;
>> + }
>> +
>> + /* account for global nr_running delta to hierarchy change */
>> + if (!se)
>> + mod_nr_running(rq, delta);
>> +}
>> +#else
>> +/*
>> + * In the absence of group throttling, all operations are guaranteed to be
>> + * globally visible at the root rq level.
>> + */
>> +static void account_hier_tasks(struct sched_entity *se, int delta)
>> +{
>> + struct rq *rq = rq_of(cfs_rq_of(se));
>> +
>> + mod_nr_running(rq, delta);
>> +}
>> +#endif
>
> While Balbir suggested expanding the _hier_ thing, I'd suggest to simply
> drop it altogether, way too much typing ;-), but that is if you cannot
> get rid of the extra hierarchy iteration, see below.
>
>> +
>> static void enqueue_sleeper(struct cfs_rq *cfs_rq, struct sched_entity *se)
>> {
>> #ifdef CONFIG_SCHEDSTATS
>> @@ -1428,6 +1464,7 @@ enqueue_task_fair(struct rq *rq, struct
>> update_cfs_shares(cfs_rq);
>> }
>>
>> + account_hier_tasks(&p->se, 1);
>> hrtick_update(rq);
>> }
>>
>> @@ -1461,6 +1498,7 @@ static void dequeue_task_fair(struct rq
>> update_cfs_shares(cfs_rq);
>> }
>>
>> + account_hier_tasks(&p->se, -1);
>> hrtick_update(rq);
>> }
>>
>> @@ -1488,6 +1526,8 @@ static u64 tg_request_cfs_quota(struct t
>> return delta;
>> }
>>
>> +static void account_hier_tasks(struct sched_entity *se, int delta);
>> +
>> static void throttle_cfs_rq(struct cfs_rq *cfs_rq)
>> {
>> struct sched_entity *se;
>> @@ -1507,6 +1547,7 @@ static void throttle_cfs_rq(struct cfs_r
>> if (!se->on_rq)
>> goto out_throttled;
>>
>> + account_hier_tasks(se, -cfs_rq->h_nr_tasks);
>> for_each_sched_entity(se) {
>> struct cfs_rq *cfs_rq = cfs_rq_of(se);
>>
>> @@ -1541,6 +1582,7 @@ static void unthrottle_cfs_rq(struct cfs
>> cfs_rq->load_stamp = cfs_rq->load_last = rq->clock_task;
>>
>> cfs_rq->throttled = 0;
>> + account_hier_tasks(se, cfs_rq->h_nr_tasks);
>> for_each_sched_entity(se) {
>> if (se->on_rq)
>> break;
>
> All call-sites are right next to a for_each_sched_entity() iteration, is
> there really no way to fold those loops?
>
I was trying to avoid some duplication since many of those for_each
iterations are partial and I didn't want to dump the same loop for the
remnants everywhere.
However, thinking about it I think this can be cleaned up and refined
to by having account_hier iterate from wherever they finish and
avoiding that nastiness.
>> Index: tip/kernel/sched_rt.c
>> ===================================================================
>> --- tip.orig/kernel/sched_rt.c
>> +++ tip/kernel/sched_rt.c
>> @@ -906,6 +906,8 @@ enqueue_task_rt(struct rq *rq, struct ta
>>
>> if (!task_current(rq, p) && p->rt.nr_cpus_allowed > 1)
>> enqueue_pushable_task(rq, p);
>> +
>> + inc_nr_running(rq);
>> }
>>
>> static void dequeue_task_rt(struct rq *rq, struct task_struct *p, int flags)
>> @@ -916,6 +918,8 @@ static void dequeue_task_rt(struct rq *r
>> dequeue_rt_entity(rt_se);
>>
>> dequeue_pushable_task(rq, p);
>> +
>> + dec_nr_running(rq);
>> }
>>
>> /*
>> @@ -1783,4 +1787,3 @@ static void print_rt_stats(struct seq_fi
>> rcu_read_unlock();
>> }
>> #endif /* CONFIG_SCHED_DEBUG */
>
>
> You mentioned something about -rt having the same problem, yet you don't
> fix it.. tskkk :-)
>
Yeah I agree :) -- I was planning on fixing this in a follow-up
[honest!] as it's not as important since it doesn't affect idle
balancing to the same magnitude. I wanted to first air the notion of
hierarchal task accounting in one of the schedulers then mirror the
support across the other schedulers with consensus reached.
I can make that part of this series if you'd prefer.
>
--
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