[2/3] sched: don't call get_group() for covered cpus

Message ID 7a61e955abdcbb1dfa9fe493f11a5ec53a11ddd3.1370948150.git.viresh.kumar@linaro.org
State Accepted
Headers show

Commit Message

Viresh Kumar June 11, 2013, 11:02 a.m.
In build_sched_groups() we don't need to call get_group() for cpus which are
already covered in previous iterations. So, call get_group() after checking if
cpu is covered or not.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 kernel/sched/core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Peter Zijlstra June 18, 2013, 10:03 a.m. | #1
On Tue, Jun 11, 2013 at 04:32:44PM +0530, Viresh Kumar wrote:
> In build_sched_groups() we don't need to call get_group() for cpus which are
> already covered in previous iterations. So, call get_group() after checking if
> cpu is covered or not.
> 

Aside from not needing it; doing it would mark the group used and
eventually leak it since we wouldn't connect it and not find it again to
free it, right?

So effectively this fixes a memory leak?

> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  kernel/sched/core.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 4abc743..27842f5 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -5344,12 +5344,12 @@ build_sched_groups(struct sched_domain *sd, int cpu)
>  
>  	for_each_cpu(i, span) {
>  		struct sched_group *sg;
> -		int group = get_group(i, sdd, &sg);
> -		int j;
> +		int group, j;
>  
>  		if (cpumask_test_cpu(i, covered))
>  			continue;
>  
> +		group = get_group(i, sdd, &sg);
>  		cpumask_clear(sched_group_cpus(sg));
>  		sg->sgp->power = 0;
>  		cpumask_setall(sched_group_mask(sg));
> -- 
> 1.7.12.rc2.18.g61b472e
>
Viresh Kumar June 18, 2013, 10:15 a.m. | #2
On 18 June 2013 15:33, Peter Zijlstra <peterz@infradead.org> wrote:
> On Tue, Jun 11, 2013 at 04:32:44PM +0530, Viresh Kumar wrote:
>> In build_sched_groups() we don't need to call get_group() for cpus which are
>> already covered in previous iterations. So, call get_group() after checking if
>> cpu is covered or not.
>>
>
> Aside from not needing it; doing it would mark the group used and
> eventually leak it since we wouldn't connect it and not find it again to
> free it, right?
>
> So effectively this fixes a memory leak?

Exactly. To be precise: In cases where sg->cpumask contained more
than one cpu (For any topology level), this patch helps freeing sg's
memory for all cpus leaving the group leader.

May I update the logs again?
Peter Zijlstra June 18, 2013, 3:19 p.m. | #3
On Tue, Jun 18, 2013 at 03:45:15PM +0530, Viresh Kumar wrote:
> On 18 June 2013 15:33, Peter Zijlstra <peterz@infradead.org> wrote:
> > On Tue, Jun 11, 2013 at 04:32:44PM +0530, Viresh Kumar wrote:
> >> In build_sched_groups() we don't need to call get_group() for cpus which are
> >> already covered in previous iterations. So, call get_group() after checking if
> >> cpu is covered or not.
> >>
> >
> > Aside from not needing it; doing it would mark the group used and
> > eventually leak it since we wouldn't connect it and not find it again to
> > free it, right?
> >
> > So effectively this fixes a memory leak?
> 
> Exactly. To be precise: In cases where sg->cpumask contained more
> than one cpu (For any topology level), this patch helps freeing sg's
> memory for all cpus leaving the group leader.
> 
> May I update the logs again?

Sure. Just send a new patch and I'll substitute.
Viresh Kumar June 18, 2013, 3:29 p.m. | #4
On 18 June 2013 20:49, Peter Zijlstra <peterz@infradead.org> wrote:
> Sure. Just send a new patch and I'll substitute.

This is the new log (patch is attached):

sched: Fix memory leakage in build_sched_groups()

In build_sched_groups() we don't need to call get_group() for cpus which are
already covered in previous iterations. Calling get_group() would mark the group
used and eventually leak it since we wouldn't connect it and not find it again
to free it.

This will happen only in cases where sg->cpumask contained more than one cpu
(For any topology level). This patch would free sg's memory for all cpus leaving
the group leader as the group isn't marked used now.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 kernel/sched/core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Peter Zijlstra June 18, 2013, 5:30 p.m. | #5
On Tue, Jun 18, 2013 at 08:59:39PM +0530, Viresh Kumar wrote:
> On 18 June 2013 20:49, Peter Zijlstra <peterz@infradead.org> wrote:
> > Sure. Just send a new patch and I'll substitute.
> 
> This is the new log (patch is attached):
> 
> sched: Fix memory leakage in build_sched_groups()
> 
> In build_sched_groups() we don't need to call get_group() for cpus which are
> already covered in previous iterations. Calling get_group() would mark the group
> used and eventually leak it since we wouldn't connect it and not find it again
> to free it.
> 
> This will happen only in cases where sg->cpumask contained more than one cpu
> (For any topology level). This patch would free sg's memory for all cpus leaving
> the group leader as the group isn't marked used now.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

Thanks!

Patch

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 4abc743..27842f5 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -5344,12 +5344,12 @@  build_sched_groups(struct sched_domain *sd, int cpu)
 
 	for_each_cpu(i, span) {
 		struct sched_group *sg;
-		int group = get_group(i, sdd, &sg);
-		int j;
+		int group, j;
 
 		if (cpumask_test_cpu(i, covered))
 			continue;
 
+		group = get_group(i, sdd, &sg);
 		cpumask_clear(sched_group_cpus(sg));
 		sg->sgp->power = 0;
 		cpumask_setall(sched_group_mask(sg));