| Message ID | 20231012062359.1616786-4-irogers@google.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | Improvements to memory use | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
Hi Ian, On Wed, Oct 11, 2023 at 11:24 PM Ian Rogers <irogers@google.com> wrote: > > Caught using reference count checking on perf top with > "--call-graph=lbr". After this no memory leaks were detected. > > Fixes: 57849998e2cd ("perf report: Add processing for cycle histograms") > Signed-off-by: Ian Rogers <irogers@google.com> > --- > tools/perf/util/hist.c | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c > index 3dc8a4968beb..ac8c0ef48a7f 100644 > --- a/tools/perf/util/hist.c > +++ b/tools/perf/util/hist.c > @@ -2676,8 +2676,6 @@ void hist__account_cycles(struct branch_stack *bs, struct addr_location *al, > > /* If we have branch cycles always annotate them. */ > if (bs && bs->nr && entries[0].flags.cycles) { > - int i; > - Seems not necessary. > bi = sample__resolve_bstack(sample, al); It looks like this increases the refcount for each bi entry and it didn't put the refcounts. > if (bi) { > struct addr_map_symbol *prev = NULL; > @@ -2692,7 +2690,7 @@ void hist__account_cycles(struct branch_stack *bs, struct addr_location *al, > * Note that perf stores branches reversed from > * program order! > */ > - for (i = bs->nr - 1; i >= 0; i--) { > + for (int i = bs->nr - 1; i >= 0; i--) { > addr_map_symbol__account_cycles(&bi[i].from, > nonany_branch_mode ? NULL : prev, > bi[i].flags.cycles); > @@ -2701,6 +2699,12 @@ void hist__account_cycles(struct branch_stack *bs, struct addr_location *al, > if (total_cycles) > *total_cycles += bi[i].flags.cycles; > } > + for (unsigned int i = 0; i < bs->nr; i++) { Can we just reuse the int i above? Thanks, Namhyung > + map__put(bi[i].to.ms.map); > + maps__put(bi[i].to.ms.maps); > + map__put(bi[i].from.ms.map); > + maps__put(bi[i].from.ms.maps); > + } > free(bi); > } > } > -- > 2.42.0.609.gbb76f46606-goog >
On Wed, Oct 18, 2023 at 4:16 PM Namhyung Kim <namhyung@kernel.org> wrote: > > Hi Ian, > > On Wed, Oct 11, 2023 at 11:24 PM Ian Rogers <irogers@google.com> wrote: > > > > Caught using reference count checking on perf top with > > "--call-graph=lbr". After this no memory leaks were detected. > > > > Fixes: 57849998e2cd ("perf report: Add processing for cycle histograms") > > Signed-off-by: Ian Rogers <irogers@google.com> > > --- > > tools/perf/util/hist.c | 10 +++++++--- > > 1 file changed, 7 insertions(+), 3 deletions(-) > > > > diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c > > index 3dc8a4968beb..ac8c0ef48a7f 100644 > > --- a/tools/perf/util/hist.c > > +++ b/tools/perf/util/hist.c > > @@ -2676,8 +2676,6 @@ void hist__account_cycles(struct branch_stack *bs, struct addr_location *al, > > > > /* If we have branch cycles always annotate them. */ > > if (bs && bs->nr && entries[0].flags.cycles) { > > - int i; > > - > > Seems not necessary. > > > bi = sample__resolve_bstack(sample, al); > > It looks like this increases the refcount for each bi entry and > it didn't put the refcounts. Right, this is why the loop doing the puts is added. > > > if (bi) { > > struct addr_map_symbol *prev = NULL; > > @@ -2692,7 +2690,7 @@ void hist__account_cycles(struct branch_stack *bs, struct addr_location *al, > > * Note that perf stores branches reversed from > > * program order! > > */ > > - for (i = bs->nr - 1; i >= 0; i--) { > > + for (int i = bs->nr - 1; i >= 0; i--) { > > addr_map_symbol__account_cycles(&bi[i].from, > > nonany_branch_mode ? NULL : prev, > > bi[i].flags.cycles); > > @@ -2701,6 +2699,12 @@ void hist__account_cycles(struct branch_stack *bs, struct addr_location *al, > > if (total_cycles) > > *total_cycles += bi[i].flags.cycles; > > } > > + for (unsigned int i = 0; i < bs->nr; i++) { > > Can we just reuse the int i above? I wanted to move to unsigned for consistency with the rest of the branch_stack code, nr is a u64, but when iterating down the sign matters - so this fixes up where possible. Thanks, Ian > Thanks, > Namhyung > > > > + map__put(bi[i].to.ms.map); > > + maps__put(bi[i].to.ms.maps); > > + map__put(bi[i].from.ms.map); > > + maps__put(bi[i].from.ms.maps); > > + } > > free(bi); > > } > > } > > -- > > 2.42.0.609.gbb76f46606-goog > >
diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c index 3dc8a4968beb..ac8c0ef48a7f 100644 --- a/tools/perf/util/hist.c +++ b/tools/perf/util/hist.c @@ -2676,8 +2676,6 @@ void hist__account_cycles(struct branch_stack *bs, struct addr_location *al, /* If we have branch cycles always annotate them. */ if (bs && bs->nr && entries[0].flags.cycles) { - int i; - bi = sample__resolve_bstack(sample, al); if (bi) { struct addr_map_symbol *prev = NULL; @@ -2692,7 +2690,7 @@ void hist__account_cycles(struct branch_stack *bs, struct addr_location *al, * Note that perf stores branches reversed from * program order! */ - for (i = bs->nr - 1; i >= 0; i--) { + for (int i = bs->nr - 1; i >= 0; i--) { addr_map_symbol__account_cycles(&bi[i].from, nonany_branch_mode ? NULL : prev, bi[i].flags.cycles); @@ -2701,6 +2699,12 @@ void hist__account_cycles(struct branch_stack *bs, struct addr_location *al, if (total_cycles) *total_cycles += bi[i].flags.cycles; } + for (unsigned int i = 0; i < bs->nr; i++) { + map__put(bi[i].to.ms.map); + maps__put(bi[i].to.ms.maps); + map__put(bi[i].from.ms.map); + maps__put(bi[i].from.ms.maps); + } free(bi); } }
Caught using reference count checking on perf top with "--call-graph=lbr". After this no memory leaks were detected. Fixes: 57849998e2cd ("perf report: Add processing for cycle histograms") Signed-off-by: Ian Rogers <irogers@google.com> --- tools/perf/util/hist.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)