From: Dietmar Eggemann <[email protected]>
The struct sched_avg of struct rq is only used in case group
scheduling is enabled inside __update_tg_runnable_avg() to update
per-cpu representation of a task group. I.e. that there is no need to
maintain the runnable avg of a rq in the !CONFIG_FAIR_GROUP_SCHED case.
This patch guards struct sched_avg of struct rq and
update_rq_runnable_avg() with CONFIG_FAIR_GROUP_SCHED.
There is an extra empty definition for update_rq_runnable_avg()
necessary for the !CONFIG_FAIR_GROUP_SCHED && CONFIG_SMP case.
The function print_cfs_group_stats() which prints out struct sched_avg
of struct rq is already guarded with CONFIG_FAIR_GROUP_SCHED.
Signed-off-by: Dietmar Eggemann <[email protected]>
---
Hi,
I was just wondering what the overall policy is when it comes to guard
specific functionality in the scheduler code? Do we want to to guard
something like the fair group scheduling support completely?
The patch is against tip/master .
kernel/sched/fair.c | 13 +++++++------
kernel/sched/sched.h | 2 ++
2 files changed, 9 insertions(+), 6 deletions(-)
diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 5f6ddbef80af..76c6513b6889 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -2376,12 +2376,19 @@ static inline void __update_group_entity_contrib(struct sched_entity *se)
se->avg.load_avg_contrib >>= NICE_0_SHIFT;
}
}
+
+static inline void update_rq_runnable_avg(struct rq *rq, int runnable)
+{
+ __update_entity_runnable_avg(rq_clock_task(rq), &rq->avg, runnable);
+ __update_tg_runnable_avg(&rq->avg, &rq->cfs);
+}
#else /* CONFIG_FAIR_GROUP_SCHED */
static inline void __update_cfs_rq_tg_load_contrib(struct cfs_rq *cfs_rq,
int force_update) {}
static inline void __update_tg_runnable_avg(struct sched_avg *sa,
struct cfs_rq *cfs_rq) {}
static inline void __update_group_entity_contrib(struct sched_entity *se) {}
+static inline void update_rq_runnable_avg(struct rq *rq, int runnable) {}
#endif /* CONFIG_FAIR_GROUP_SCHED */
static inline void __update_task_entity_contrib(struct sched_entity *se)
@@ -2480,12 +2487,6 @@ static void update_cfs_rq_blocked_load(struct cfs_rq *cfs_rq, int force_update)
__update_cfs_rq_tg_load_contrib(cfs_rq, force_update);
}
-static inline void update_rq_runnable_avg(struct rq *rq, int runnable)
-{
- __update_entity_runnable_avg(rq_clock_task(rq), &rq->avg, runnable);
- __update_tg_runnable_avg(&rq->avg, &rq->cfs);
-}
-
/* Add the load generated by se into cfs_rq's child load-average */
static inline void enqueue_entity_load_avg(struct cfs_rq *cfs_rq,
struct sched_entity *se,
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index 4be68da1fe00..63beab7512e7 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -630,7 +630,9 @@ struct rq {
struct llist_head wake_list;
#endif
+#ifdef CONFIG_FAIR_GROUP_SCHED
struct sched_avg avg;
+#endif
};
static inline int cpu_of(struct rq *rq)
--
1.7.9.5
> The struct sched_avg of struct rq is only used in case group
> scheduling is enabled inside __update_tg_runnable_avg() to update
> per-cpu representation of a task group. I.e. that there is no need to
> maintain the runnable avg of a rq in the !CONFIG_FAIR_GROUP_SCHED case.
>
> This patch guards struct sched_avg of struct rq and
> update_rq_runnable_avg() with CONFIG_FAIR_GROUP_SCHED.
>
While this patch looks good, I see fields in sched_avg viz decay_count,
last_runnable_update, load_avg_contrib only relevant to sched_entity.
i.e they don't seem to be updated or used for rq->avg. Should we look at
splitting sched_avg so that rq->avg doesn't have unwanted fields?
--
Thanks and Regards
Srikar Dronamraju
On 25/02/14 13:16, Srikar Dronamraju wrote:
>> The struct sched_avg of struct rq is only used in case group
>> scheduling is enabled inside __update_tg_runnable_avg() to update
>> per-cpu representation of a task group. I.e. that there is no need to
>> maintain the runnable avg of a rq in the !CONFIG_FAIR_GROUP_SCHED case.
>>
>> This patch guards struct sched_avg of struct rq and
>> update_rq_runnable_avg() with CONFIG_FAIR_GROUP_SCHED.
>>
>
> While this patch looks good, I see fields in sched_avg viz decay_count,
> last_runnable_update, load_avg_contrib only relevant to sched_entity.
> i.e they don't seem to be updated or used for rq->avg. Should we look at
> splitting sched_avg so that rq->avg doesn't have unwanted fields?
Yes, AFAICS at least load_avg_contrib and decay_count are only relevant
for struct sched_entity whereas last_runnable_update is used in
__update_entity_runnable_avg() itself.
By having struct sched_avg embedded into struct sched_entity and struct
rq, __update_entity_runnable_avg() and __update_tg_runnable_avg() can be
used on both and moreover, all current members of struct sched_avg
belong logically together.
With this patch I was more interested in the fact that we do not have to
maintain the load avg for struct rq in a !CONFIG_FAIR_GROUP_SCHED system.
-- Dietmar
>
> --
> Thanks and Regards
> Srikar Dronamraju
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
Dietmar Eggemann <[email protected]> writes:
> On 25/02/14 13:16, Srikar Dronamraju wrote:
>>> The struct sched_avg of struct rq is only used in case group
>>> scheduling is enabled inside __update_tg_runnable_avg() to update
>>> per-cpu representation of a task group. I.e. that there is no need to
>>> maintain the runnable avg of a rq in the !CONFIG_FAIR_GROUP_SCHED case.
>>>
>>> This patch guards struct sched_avg of struct rq and
>>> update_rq_runnable_avg() with CONFIG_FAIR_GROUP_SCHED.
>>>
Reviewed-by: Ben Segall <[email protected]>
If we ever decide to use runnable_avg_sum/period in balance or
something, it's trivial enough to move it back, and there is no point in
calculating unusued stats until then.
>>
>> While this patch looks good, I see fields in sched_avg viz decay_count,
>> last_runnable_update, load_avg_contrib only relevant to sched_entity.
>> i.e they don't seem to be updated or used for rq->avg. Should we look at
>> splitting sched_avg so that rq->avg doesn't have unwanted fields?
>
> Yes, AFAICS at least load_avg_contrib and decay_count are only relevant
> for struct sched_entity whereas last_runnable_update is used in
> __update_entity_runnable_avg() itself.
>
> By having struct sched_avg embedded into struct sched_entity and struct
> rq, __update_entity_runnable_avg() and __update_tg_runnable_avg() can be
> used on both and moreover, all current members of struct sched_avg
> belong logically together.
>
> With this patch I was more interested in the fact that we do not have to
> maintain the load avg for struct rq in a !CONFIG_FAIR_GROUP_SCHED system.
>
Yeah, last_runnable_update and runnable_avg_sum/period are used,
decay_count and load_avg_contrib could be moved to the se just fine, and
won't cause any problems.
On Tue, Feb 25, 2014 at 11:47:42AM +0000, Dietmar Eggemann wrote:
> +++ b/kernel/sched/sched.h
> @@ -630,7 +630,9 @@ struct rq {
> struct llist_head wake_list;
> #endif
>
> +#ifdef CONFIG_FAIR_GROUP_SCHED
> struct sched_avg avg;
> +#endif
> };
There is already a CONFIG_FAIR_GROUP_SCHED #ifdef in that structure;
does it make sense to move this variable in there instead of adding yet
another #ifdef?
On 25/02/14 20:52, Peter Zijlstra wrote:
> On Tue, Feb 25, 2014 at 11:47:42AM +0000, Dietmar Eggemann wrote:
>> +++ b/kernel/sched/sched.h
>> @@ -630,7 +630,9 @@ struct rq {
>> struct llist_head wake_list;
>> #endif
>>
>> +#ifdef CONFIG_FAIR_GROUP_SCHED
>> struct sched_avg avg;
>> +#endif
>> };
>
> There is already a CONFIG_FAIR_GROUP_SCHED #ifdef in that structure;
> does it make sense to move this variable in there instead of adding yet
> another #ifdef?
>
I changed the patch accordingly.
-- >8 --
Subject: [PATCH] sched: put rq's sched_avg under CONFIG_FAIR_GROUP_SCHED
The struct sched_avg of struct rq is only used in case group
scheduling is enabled inside __update_tg_runnable_avg() to update
per-cpu representation of a task group. I.e. that there is no need to
maintain the runnable avg of a rq in the !CONFIG_FAIR_GROUP_SCHED case.
This patch guards struct sched_avg of struct rq and
update_rq_runnable_avg() with CONFIG_FAIR_GROUP_SCHED.
There is an extra empty definition for update_rq_runnable_avg()
necessary for the !CONFIG_FAIR_GROUP_SCHED && CONFIG_SMP case.
The function print_cfs_group_stats() which prints out struct sched_avg
of struct rq is already guarded with CONFIG_FAIR_GROUP_SCHED.
Signed-off-by: Dietmar Eggemann <[email protected]>
---
kernel/sched/fair.c | 13 +++++++------
kernel/sched/sched.h | 4 ++--
2 files changed, 9 insertions(+), 8 deletions(-)
diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 5f6ddbef80af..76c6513b6889 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -2376,12 +2376,19 @@ static inline void __update_group_entity_contrib(struct sched_entity *se)
se->avg.load_avg_contrib >>= NICE_0_SHIFT;
}
}
+
+static inline void update_rq_runnable_avg(struct rq *rq, int runnable)
+{
+ __update_entity_runnable_avg(rq_clock_task(rq), &rq->avg, runnable);
+ __update_tg_runnable_avg(&rq->avg, &rq->cfs);
+}
#else /* CONFIG_FAIR_GROUP_SCHED */
static inline void __update_cfs_rq_tg_load_contrib(struct cfs_rq *cfs_rq,
int force_update) {}
static inline void __update_tg_runnable_avg(struct sched_avg *sa,
struct cfs_rq *cfs_rq) {}
static inline void __update_group_entity_contrib(struct sched_entity *se) {}
+static inline void update_rq_runnable_avg(struct rq *rq, int runnable) {}
#endif /* CONFIG_FAIR_GROUP_SCHED */
static inline void __update_task_entity_contrib(struct sched_entity *se)
@@ -2480,12 +2487,6 @@ static void update_cfs_rq_blocked_load(struct cfs_rq *cfs_rq, int force_update)
__update_cfs_rq_tg_load_contrib(cfs_rq, force_update);
}
-static inline void update_rq_runnable_avg(struct rq *rq, int runnable)
-{
- __update_entity_runnable_avg(rq_clock_task(rq), &rq->avg, runnable);
- __update_tg_runnable_avg(&rq->avg, &rq->cfs);
-}
-
/* Add the load generated by se into cfs_rq's child load-average */
static inline void enqueue_entity_load_avg(struct cfs_rq *cfs_rq,
struct sched_entity *se,
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index 4be68da1fe00..b1fb1a62c52d 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -540,6 +540,8 @@ struct rq {
#ifdef CONFIG_FAIR_GROUP_SCHED
/* list of leaf cfs_rq on this cpu: */
struct list_head leaf_cfs_rq_list;
+
+ struct sched_avg avg;
#endif /* CONFIG_FAIR_GROUP_SCHED */
/*
@@ -629,8 +631,6 @@ struct rq {
#ifdef CONFIG_SMP
struct llist_head wake_list;
#endif
-
- struct sched_avg avg;
};
static inline int cpu_of(struct rq *rq)
--
1.7.9.5
On Wed, Feb 26, 2014 at 11:19:33AM +0000, Dietmar Eggemann wrote:
> I changed the patch accordingly.
Thanks!
Dietmar Eggemann <[email protected]> writes:
> On 25/02/14 20:52, Peter Zijlstra wrote:
>> On Tue, Feb 25, 2014 at 11:47:42AM +0000, Dietmar Eggemann wrote:
>>> +++ b/kernel/sched/sched.h
>>> @@ -630,7 +630,9 @@ struct rq {
>>> struct llist_head wake_list;
>>> #endif
>>>
>>> +#ifdef CONFIG_FAIR_GROUP_SCHED
>>> struct sched_avg avg;
>>> +#endif
>>> };
>>
>> There is already a CONFIG_FAIR_GROUP_SCHED #ifdef in that structure;
>> does it make sense to move this variable in there instead of adding yet
>> another #ifdef?
>>
>
> I changed the patch accordingly.
>
> -- >8 --
> Subject: [PATCH] sched: put rq's sched_avg under CONFIG_FAIR_GROUP_SCHED
>
> The struct sched_avg of struct rq is only used in case group
> scheduling is enabled inside __update_tg_runnable_avg() to update
> per-cpu representation of a task group. I.e. that there is no need to
> maintain the runnable avg of a rq in the !CONFIG_FAIR_GROUP_SCHED case.
>
> This patch guards struct sched_avg of struct rq and
> update_rq_runnable_avg() with CONFIG_FAIR_GROUP_SCHED.
>
> There is an extra empty definition for update_rq_runnable_avg()
> necessary for the !CONFIG_FAIR_GROUP_SCHED && CONFIG_SMP case.
>
> The function print_cfs_group_stats() which prints out struct sched_avg
> of struct rq is already guarded with CONFIG_FAIR_GROUP_SCHED.
>
> Signed-off-by: Dietmar Eggemann <[email protected]>
Reviewed-by: Ben Segall <[email protected]>
Commit-ID: f5f9739d7a0ccbdcf913a0b3604b134129d14f7e
Gitweb: http://git.kernel.org/tip/f5f9739d7a0ccbdcf913a0b3604b134129d14f7e
Author: Dietmar Eggemann <[email protected]>
AuthorDate: Wed, 26 Feb 2014 11:19:33 +0000
Committer: Ingo Molnar <[email protected]>
CommitDate: Thu, 27 Feb 2014 12:41:00 +0100
sched: Put rq's sched_avg under CONFIG_FAIR_GROUP_SCHED
The struct sched_avg of struct rq is only used in case group
scheduling is enabled inside __update_tg_runnable_avg() to update
per-cpu representation of a task group. I.e. that there is no need to
maintain the runnable avg of a rq in the !CONFIG_FAIR_GROUP_SCHED case.
This patch guards struct sched_avg of struct rq and
update_rq_runnable_avg() with CONFIG_FAIR_GROUP_SCHED.
There is an extra empty definition for update_rq_runnable_avg()
necessary for the !CONFIG_FAIR_GROUP_SCHED && CONFIG_SMP case.
The function print_cfs_group_stats() which prints out struct sched_avg
of struct rq is already guarded with CONFIG_FAIR_GROUP_SCHED.
Reviewed-by: Ben Segall <[email protected]>
Signed-off-by: Dietmar Eggemann <[email protected]>
Signed-off-by: Peter Zijlstra <[email protected]>
Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: Ingo Molnar <[email protected]>
---
kernel/sched/fair.c | 13 +++++++------
kernel/sched/sched.h | 4 ++--
2 files changed, 9 insertions(+), 8 deletions(-)
diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index a3a41c61..be4f7d9 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -2374,12 +2374,19 @@ static inline void __update_group_entity_contrib(struct sched_entity *se)
se->avg.load_avg_contrib >>= NICE_0_SHIFT;
}
}
+
+static inline void update_rq_runnable_avg(struct rq *rq, int runnable)
+{
+ __update_entity_runnable_avg(rq_clock_task(rq), &rq->avg, runnable);
+ __update_tg_runnable_avg(&rq->avg, &rq->cfs);
+}
#else /* CONFIG_FAIR_GROUP_SCHED */
static inline void __update_cfs_rq_tg_load_contrib(struct cfs_rq *cfs_rq,
int force_update) {}
static inline void __update_tg_runnable_avg(struct sched_avg *sa,
struct cfs_rq *cfs_rq) {}
static inline void __update_group_entity_contrib(struct sched_entity *se) {}
+static inline void update_rq_runnable_avg(struct rq *rq, int runnable) {}
#endif /* CONFIG_FAIR_GROUP_SCHED */
static inline void __update_task_entity_contrib(struct sched_entity *se)
@@ -2478,12 +2485,6 @@ static void update_cfs_rq_blocked_load(struct cfs_rq *cfs_rq, int force_update)
__update_cfs_rq_tg_load_contrib(cfs_rq, force_update);
}
-static inline void update_rq_runnable_avg(struct rq *rq, int runnable)
-{
- __update_entity_runnable_avg(rq_clock_task(rq), &rq->avg, runnable);
- __update_tg_runnable_avg(&rq->avg, &rq->cfs);
-}
-
/* Add the load generated by se into cfs_rq's child load-average */
static inline void enqueue_entity_load_avg(struct cfs_rq *cfs_rq,
struct sched_entity *se,
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index d608125..046084e 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -541,6 +541,8 @@ struct rq {
#ifdef CONFIG_FAIR_GROUP_SCHED
/* list of leaf cfs_rq on this cpu: */
struct list_head leaf_cfs_rq_list;
+
+ struct sched_avg avg;
#endif /* CONFIG_FAIR_GROUP_SCHED */
/*
@@ -630,8 +632,6 @@ struct rq {
#ifdef CONFIG_SMP
struct llist_head wake_list;
#endif
-
- struct sched_avg avg;
};
static inline int cpu_of(struct rq *rq)