sched/topology: fix domain reconstruction memory leakage
RFC PATCH 1/1 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.
--- kernel/sched/topology.c | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-)
kernel/sched/topology.c +8 -11
@@ -323,7 +323,7 @@ static struct root_domain *alloc_rootdomreturn 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 schdo { 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 schstatic 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);
Quoted 2 lines · Mike Galbraith
Signed-off-by Mike Galbraith <[email protected]>
(grr, wrong /me autographed the damn thing)
Peter Zijlstra
maintainerQuoted 7 lines · Mike Galbraith
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,
0c0e776a9b0fbeing the culprit. With the below applied, box took a severe beating overnight without a whimper.
Quoted 8 lines
I'm wondering (ergo rfc) if
free_sched_groups()shouldn't be renamed toput_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:
is that the same?
Quoted 15 lines · Peter Zijlstra
Quoted 14 lines · Mike Galbraith
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,
0c0e776a9b0fbeing 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 toput_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".
Quoted 7 lines
Quoted 1 line
sched/topology: fix domain reconstruction memory leakage
I was sitting on this one:
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
Quoted 15 lines · Peter Zijlstra
Quoted 14 lines · Mike Galbraith
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,
0c0e776a9b0fbeing 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 toput_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".
Quoted 5 lines
Quoted 1 line
sched/topology: fix domain reconstruction memory leakage
I was sitting on this one:
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);
Peter Zijlstra
maintainerQuoted 25 lines · Mike Galbraith
Quoted 15 lines · Peter Zijlstra
Quoted 14 lines · Mike Galbraith
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,
0c0e776a9b0fbeing 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 toput_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".Quoted 5 lines
Quoted 1 line
sched/topology: fix domain reconstruction memory leakage
I was sitting on this one:
The comment in the patch reads better to me like so:
kernel/sched/topology.c static void free_sched_groups(struct sch · line 353
Quoted 8 lines
kernel/sched/topology.c +3
-0@@ -345,15 +346,12 @@ static void free_sched_groups(struct schstatic 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!