Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1715566 > unrolled thread

[rfc patch] sched/topology: fix domain reconstruction memory leakage

Back to article view | Back to linux.kernel

Contents

  [rfc patch] sched/topology: fix domain reconstruction memory leakage Mike Galbraith <efault@gmx.de> - 2017-08-19 08:20 +0200
    Re: [rfc patch] sched/topology: fix domain reconstruction memory  leakage Mike Galbraith <efault@gmx.de> - 2017-08-19 08:30 +0200
    Re: [rfc patch] sched/topology: fix domain reconstruction memory  leakage Peter Zijlstra <peterz@infradead.org> - 2017-08-21 10:20 +0200
      Re: [rfc patch] sched/topology: fix domain reconstruction memory  leakage Mike Galbraith <efault@gmx.de> - 2017-08-21 13:50 +0200
      Re: [rfc patch] sched/topology: fix domain reconstruction memory  leakage Mike Galbraith <efault@gmx.de> - 2017-08-21 15:40 +0200
        Re: [rfc patch] sched/topology: fix domain reconstruction memory  leakage Peter Zijlstra <peterz@infradead.org> - 2017-08-21 15:50 +0200

#1715566 — [rfc patch] sched/topology: fix domain reconstruction memory leakage

Greetings,

While beating on cpu hotplug with the shiny new topology fixes
backported, my memory poor 8 socket box fairly quickly leaked itself to
death, 0c0e776a9b0f being the culprit.  With the below applied, box
took a severe beating overnight without a whimper.

I'm wondering (ergo rfc) if free_sched_groups() shouldn't be renamed to
put_sched_groups() instead, with overlapping domains taking a group
reference reference as well so they can put both sg/sgc rather than put
one free the other.  Those places that want an explicit free can pass
free to only explicitly free sg (or use two functions).  Minimalist
approach works (minus signs, yay), but could perhaps use some "pretty".

sched/topology: fix domain reconstruction memory leakage

Since 0c0e776a9b0f, build_sched_groups() takes a reference on each
sg and sgc during domain generation, where previously it only took
a reference on the first group.  Iterate groups and drop all added
references during domain destruction, otherwise CPU hotplug leaks.

Signed-off-by: Mike Galbraith <mgalbraith@suse.de>
Fixes: 0c0e776a9b0f ("sched/topology: Rewrite get_group()")
---
 kernel/sched/topology.c |   19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

--- a/kernel/sched/topology.c
+++ b/kernel/sched/topology.c
@@ -323,7 +323,7 @@ static struct root_domain *alloc_rootdom
 	return rd;
 }
 
-static void free_sched_groups(struct sched_group *sg, int free_sgc)
+static void free_sched_groups(struct sched_group *sg, int put)
 {
 	struct sched_group *tmp, *first;
 
@@ -334,10 +334,11 @@ static void free_sched_groups(struct sch
 	do {
 		tmp = sg->next;
 
-		if (free_sgc && atomic_dec_and_test(&sg->sgc->ref))
+		if (put && atomic_dec_and_test(&sg->sgc->ref))
 			kfree(sg->sgc);
+		if (put < 2 || atomic_dec_and_test(&sg->ref))
+			kfree(sg);
 
-		kfree(sg);
 		sg = tmp;
 	} while (sg != first);
 }
@@ -345,15 +346,11 @@ static void free_sched_groups(struct sch
 static void destroy_sched_domain(struct sched_domain *sd)
 {
 	/*
-	 * If its an overlapping domain it has private groups, iterate and
-	 * nuke them all.
+	 * If it's an overlapping domain it has private groups, iterate,
+	 * freeing groups, otherwise dropping group references.  In both
+	 * cases, we must drop group capacity references.
 	 */
-	if (sd->flags & SD_OVERLAP) {
-		free_sched_groups(sd->groups, 1);
-	} else if (atomic_dec_and_test(&sd->groups->ref)) {
-		kfree(sd->groups->sgc);
-		kfree(sd->groups);
-	}
+	free_sched_groups(sd->groups, !(sd->flags & SD_OVERLAP)+1);
 	if (sd->shared && atomic_dec_and_test(&sd->shared->ref))
 		kfree(sd->shared);
 	kfree(sd);

[toc] | [next] | [standalone]

#1715569 — Re: [rfc patch] sched/topology: fix domain reconstruction memory leakage

On Sat, 2017-08-19 at 08:10 +0200, Mike Galbraith wrote:
> 
> Signed-off-by: Mike Galbraith <efault@gmx.de>
(grr, wrong /me autographed the damn thing)

[toc] | [prev] | [next] | [standalone]

#1716168 — Re: [rfc patch] sched/topology: fix domain reconstruction memory leakage

On Sat, Aug 19, 2017 at 08:10:49AM +0200, Mike Galbraith wrote:
> Greetings,
> 
> While beating on cpu hotplug with the shiny new topology fixes
> backported, my memory poor 8 socket box fairly quickly leaked itself to
> death, 0c0e776a9b0f being the culprit.  With the below applied, box
> took a severe beating overnight without a whimper.
> 
> I'm wondering (ergo rfc) if free_sched_groups() shouldn't be renamed to
> put_sched_groups() instead, with overlapping domains taking a group
> reference reference as well so they can put both sg/sgc rather than put
> one free the other.  Those places that want an explicit free can pass
> free to only explicitly free sg (or use two functions).  Minimalist
> approach works (minus signs, yay), but could perhaps use some "pretty".
> 
> sched/topology: fix domain reconstruction memory leakage

I was sitting on this one:

  https://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git/commit/?h=sched/core&id=c63d18dd6ea59eec5cba857835f788943ff9f0d5

is that the same?

[toc] | [prev] | [next] | [standalone]

#1716358 — Re: [rfc patch] sched/topology: fix domain reconstruction memory leakage

On Mon, 2017-08-21 at 10:16 +0200, Peter Zijlstra wrote:
> On Sat, Aug 19, 2017 at 08:10:49AM +0200, Mike Galbraith wrote:
> > Greetings,
> > 
> > While beating on cpu hotplug with the shiny new topology fixes
> > backported, my memory poor 8 socket box fairly quickly leaked itself to
> > death, 0c0e776a9b0f being the culprit.  With the below applied, box
> > took a severe beating overnight without a whimper.
> > 
> > I'm wondering (ergo rfc) if free_sched_groups() shouldn't be renamed to
> > put_sched_groups() instead, with overlapping domains taking a group
> > reference reference as well so they can put both sg/sgc rather than put
> > one free the other.  Those places that want an explicit free can pass
> > free to only explicitly free sg (or use two functions).  Minimalist
> > approach works (minus signs, yay), but could perhaps use some "pretty".
> > 
> > sched/topology: fix domain reconstruction memory leakage
> 
> I was sitting on this one:
> 
>   https://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git/commit/?h=sched/core&id=c63d18dd6ea59eec5cba857835f788943ff9f0d5
> 
> is that the same?

If that doesn't muck up the other places that were doing explicit sd
free without putting sgc, cool (I still like rename.. but then me and
pretty don't seem to get along all that well, so whatever works;).

	-Mike

[toc] | [prev] | [next] | [standalone]

#1716456 — Re: [rfc patch] sched/topology: fix domain reconstruction memory leakage

On Mon, 2017-08-21 at 10:16 +0200, Peter Zijlstra wrote:
> On Sat, Aug 19, 2017 at 08:10:49AM +0200, Mike Galbraith wrote:
> > Greetings,
> > 
> > While beating on cpu hotplug with the shiny new topology fixes
> > backported, my memory poor 8 socket box fairly quickly leaked itself to
> > death, 0c0e776a9b0f being the culprit.  With the below applied, box
> > took a severe beating overnight without a whimper.
> > 
> > I'm wondering (ergo rfc) if free_sched_groups() shouldn't be renamed to
> > put_sched_groups() instead, with overlapping domains taking a group
> > reference reference as well so they can put both sg/sgc rather than put
> > one free the other.  Those places that want an explicit free can pass
> > free to only explicitly free sg (or use two functions).  Minimalist
> > approach works (minus signs, yay), but could perhaps use some "pretty".
> > 
> > sched/topology: fix domain reconstruction memory leakage
> 
> I was sitting on this one:
> 
>   https://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git/commit/?h=sched/core&id=c63d18dd6ea59eec5cba857835f788943ff9f0d5

The comment in the patch reads better to me like so:

@@ -345,15 +346,12 @@ static void free_sched_groups(struct sch
 static void destroy_sched_domain(struct sched_domain *sd)
 {
 	/*
-	 * If its an overlapping domain it has private groups, iterate and
-	 * nuke them all.
+	 * A normal sched domain may have multiple group references, an
+	 * overlapping domain, having private groups, only one.  Iterate,
+	 * dropping group/capacity references, freeing where none remain.
 	 */
-	if (sd->flags & SD_OVERLAP) {
-		free_sched_groups(sd->groups, 1);
-	} else if (atomic_dec_and_test(&sd->groups->ref)) {
-		kfree(sd->groups->sgc);
-		kfree(sd->groups);
-	}
+	free_sched_groups(sd->groups, 1);
+
 	if (sd->shared && atomic_dec_and_test(&sd->shared->ref))
 		kfree(sd->shared);
 	kfree(sd);

[toc] | [prev] | [next] | [standalone]

#1716494 — Re: [rfc patch] sched/topology: fix domain reconstruction memory leakage

On Mon, Aug 21, 2017 at 03:30:29PM +0200, Mike Galbraith wrote:
> On Mon, 2017-08-21 at 10:16 +0200, Peter Zijlstra wrote:
> > On Sat, Aug 19, 2017 at 08:10:49AM +0200, Mike Galbraith wrote:
> > > Greetings,
> > > 
> > > While beating on cpu hotplug with the shiny new topology fixes
> > > backported, my memory poor 8 socket box fairly quickly leaked itself to
> > > death, 0c0e776a9b0f being the culprit.  With the below applied, box
> > > took a severe beating overnight without a whimper.
> > > 
> > > I'm wondering (ergo rfc) if free_sched_groups() shouldn't be renamed to
> > > put_sched_groups() instead, with overlapping domains taking a group
> > > reference reference as well so they can put both sg/sgc rather than put
> > > one free the other.  Those places that want an explicit free can pass
> > > free to only explicitly free sg (or use two functions).  Minimalist
> > > approach works (minus signs, yay), but could perhaps use some "pretty".
> > > 
> > > sched/topology: fix domain reconstruction memory leakage
> > 
> > I was sitting on this one:
> > 
> >   https://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git/commit/?h=sched/core&id=c63d18dd6ea59eec5cba857835f788943ff9f0d5
> 
> The comment in the patch reads better to me like so:
> 
> @@ -345,15 +346,12 @@ static void free_sched_groups(struct sch
>  static void destroy_sched_domain(struct sched_domain *sd)
>  {
>  	/*
> +	 * A normal sched domain may have multiple group references, an
> +	 * overlapping domain, having private groups, only one.  Iterate,
> +	 * dropping group/capacity references, freeing where none remain.
>  	 */
> 	free_sched_groups(sd->groups, 1);

Made it so, thanks!

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web