Remove unnecessary reference counts for structs with no gets. Add
reference count checking to comm_str and mem_info. Fix memory leaks
and errors detected on "perf mem report" by address sanitizer and leak
sanitizer.
Ian Rogers (8):
perf ui browser: Don't save pointer to stack memory
perf annotate: Fix memory leak in annotated_source
perf block-info: Remove unused refcount
perf cpumap: Remove refcnt from cpu_aggr_map
perf comm: Add reference count checking to comm_str
perf mem-info: Move mem-info out of mem-events and symbol
perf mem-info: Add reference count checking
perf hist: Avoid hist_entry_iter mem_info memory leak
tools/perf/builtin-c2c.c | 13 +-
tools/perf/builtin-report.c | 3 +-
tools/perf/builtin-script.c | 12 +-
tools/perf/builtin-stat.c | 16 +-
tools/perf/tests/mem.c | 11 +-
tools/perf/ui/browser.c | 4 +-
tools/perf/ui/browser.h | 2 +-
tools/perf/util/Build | 1 +
tools/perf/util/annotate.c | 6 +
tools/perf/util/block-info.c | 22 +-
tools/perf/util/block-info.h | 15 +-
tools/perf/util/comm.c | 196 +++++++++++-------
tools/perf/util/cpumap.c | 2 -
tools/perf/util/cpumap.h | 2 -
tools/perf/util/hist.c | 62 +++---
tools/perf/util/hist.h | 8 +-
tools/perf/util/machine.c | 7 +-
tools/perf/util/mem-events.c | 36 ++--
tools/perf/util/mem-events.h | 29 +--
tools/perf/util/mem-info.c | 35 ++++
tools/perf/util/mem-info.h | 54 +++++
.../scripting-engines/trace-event-python.c | 12 +-
tools/perf/util/sort.c | 69 +++---
tools/perf/util/symbol.c | 26 +--
tools/perf/util/symbol.h | 12 --
25 files changed, 370 insertions(+), 285 deletions(-)
create mode 100644 tools/perf/util/mem-info.c
create mode 100644 tools/perf/util/mem-info.h
--
2.45.0.rc1.225.g2a3ae87e7f-goog
ui_browser__show is capturing the input title that is stack allocated
memory in hist_browser__run. Avoid a use after return by strdup-ing
the string.
Fixes: 05e8b0804ec4 ("perf ui browser: Stop using 'self'")
Signed-off-by: Ian Rogers <[email protected]>
---
tools/perf/ui/browser.c | 4 +++-
tools/perf/ui/browser.h | 2 +-
2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/tools/perf/ui/browser.c b/tools/perf/ui/browser.c
index 603d11283cbd..c4cdf2ea69b7 100644
--- a/tools/perf/ui/browser.c
+++ b/tools/perf/ui/browser.c
@@ -287,7 +287,8 @@ int ui_browser__show(struct ui_browser *browser, const char *title,
mutex_lock(&ui__lock);
__ui_browser__show_title(browser, title);
- browser->title = title;
+ free(browser->title);
+ browser->title = strdup(title);
zfree(&browser->helpline);
va_start(ap, helpline);
@@ -304,6 +305,7 @@ void ui_browser__hide(struct ui_browser *browser)
mutex_lock(&ui__lock);
ui_helpline__pop();
zfree(&browser->helpline);
+ zfree(&browser->title);
mutex_unlock(&ui__lock);
}
diff --git a/tools/perf/ui/browser.h b/tools/perf/ui/browser.h
index 510ce4554050..6e98d5f8f71c 100644
--- a/tools/perf/ui/browser.h
+++ b/tools/perf/ui/browser.h
@@ -21,7 +21,7 @@ struct ui_browser {
u8 extra_title_lines;
int current_color;
void *priv;
- const char *title;
+ char *title;
char *helpline;
const char *no_samples_msg;
void (*refresh_dimensions)(struct ui_browser *browser);
--
2.45.0.rc1.225.g2a3ae87e7f-goog
Freeing hash map doesn't free the entries added to the hashmap, add
missing free.
Fixes: d3e7cad6f36d ("perf annotate: Add a hashmap for symbol histogram")
Signed-off-by: Ian Rogers <[email protected]>
---
tools/perf/util/annotate.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index d7d55263fc91..a83722f32d6b 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -107,9 +107,15 @@ static struct annotated_source *annotated_source__new(void)
static __maybe_unused void annotated_source__delete(struct annotated_source *src)
{
+ struct hashmap_entry *cur;
+ size_t bkt;
+
if (src == NULL)
return;
+ hashmap__for_each_entry(src->samples, cur, bkt)
+ zfree(&cur->pvalue);
+
hashmap__free(src->samples);
zfree(&src->histograms);
free(src);
--
2.45.0.rc1.225.g2a3ae87e7f-goog
block_info__get has no callers so the refcount is only ever one. As
such remove the reference counting logic and turn puts to deletes.
Signed-off-by: Ian Rogers <[email protected]>
---
tools/perf/util/block-info.c | 22 +++++-----------------
tools/perf/util/block-info.h | 15 +--------------
tools/perf/util/hist.c | 4 ++--
3 files changed, 8 insertions(+), 33 deletions(-)
diff --git a/tools/perf/util/block-info.c b/tools/perf/util/block-info.c
index 895ee8adf3b3..04068d48683f 100644
--- a/tools/perf/util/block-info.c
+++ b/tools/perf/util/block-info.c
@@ -43,26 +43,14 @@ static struct block_header_column {
}
};
-struct block_info *block_info__get(struct block_info *bi)
-{
- if (bi)
- refcount_inc(&bi->refcnt);
- return bi;
-}
-
-void block_info__put(struct block_info *bi)
+struct block_info *block_info__new(void)
{
- if (bi && refcount_dec_and_test(&bi->refcnt))
- free(bi);
+ return zalloc(sizeof(struct block_info));
}
-struct block_info *block_info__new(void)
+void block_info__delete(struct block_info *bi)
{
- struct block_info *bi = zalloc(sizeof(*bi));
-
- if (bi)
- refcount_set(&bi->refcnt, 1);
- return bi;
+ free(bi);
}
int64_t __block_info__cmp(struct hist_entry *left, struct hist_entry *right)
@@ -148,7 +136,7 @@ int block_info__process_sym(struct hist_entry *he, struct block_hist *bh,
he_block = hists__add_entry_block(&bh->block_hists,
&al, bi);
if (!he_block) {
- block_info__put(bi);
+ block_info__delete(bi);
return -1;
}
}
diff --git a/tools/perf/util/block-info.h b/tools/perf/util/block-info.h
index 96f53e89795e..0b9e1aad4c55 100644
--- a/tools/perf/util/block-info.h
+++ b/tools/perf/util/block-info.h
@@ -3,7 +3,6 @@
#define __PERF_BLOCK_H
#include <linux/types.h>
-#include <linux/refcount.h>
#include "hist.h"
#include "symbol.h"
#include "sort.h"
@@ -19,7 +18,6 @@ struct block_info {
u64 total_cycles;
int num;
int num_aggr;
- refcount_t refcnt;
};
struct block_fmt {
@@ -48,19 +46,8 @@ struct block_report {
int nr_fmts;
};
-struct block_hist;
-
struct block_info *block_info__new(void);
-struct block_info *block_info__get(struct block_info *bi);
-void block_info__put(struct block_info *bi);
-
-static inline void __block_info__zput(struct block_info **bi)
-{
- block_info__put(*bi);
- *bi = NULL;
-}
-
-#define block_info__zput(bi) __block_info__zput(&bi)
+void block_info__delete(struct block_info *bi);
int64_t __block_info__cmp(struct hist_entry *left, struct hist_entry *right);
diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index 55ea6afcc437..b8a508cd0b14 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -631,7 +631,7 @@ static struct hist_entry *hists__findnew_entry(struct hists *hists,
*/
mem_info__zput(entry->mem_info);
- block_info__zput(entry->block_info);
+ block_info__delete(entry->block_info);
kvm_info__zput(entry->kvm_info);
@@ -1341,7 +1341,7 @@ void hist_entry__delete(struct hist_entry *he)
}
if (he->block_info)
- block_info__zput(he->block_info);
+ block_info__delete(he->block_info);
if (he->kvm_info)
kvm_info__zput(he->kvm_info);
--
2.45.0.rc1.225.g2a3ae87e7f-goog
Reference count checking of an rbtree is troublesome as each pointer
should have a reference, switch to using a sorted array. Remove an
indirection by embedding the reference count with the string. Use
pthread_once to safely initialize the comm_strs and reader writer
mutex.
Signed-off-by: Ian Rogers <[email protected]>
---
tools/perf/util/comm.c | 196 ++++++++++++++++++++++++++---------------
1 file changed, 126 insertions(+), 70 deletions(-)
diff --git a/tools/perf/util/comm.c b/tools/perf/util/comm.c
index afb8d4fd2644..1aa9a08e5b03 100644
--- a/tools/perf/util/comm.c
+++ b/tools/perf/util/comm.c
@@ -1,108 +1,164 @@
// SPDX-License-Identifier: GPL-2.0
#include "comm.h"
#include <errno.h>
-#include <stdlib.h>
-#include <stdio.h>
#include <string.h>
+#include <internal/rc_check.h>
#include <linux/refcount.h>
-#include <linux/rbtree.h>
#include <linux/zalloc.h>
#include "rwsem.h"
-struct comm_str {
- char *str;
- struct rb_node rb_node;
+DECLARE_RC_STRUCT(comm_str) {
refcount_t refcnt;
+ char str[];
};
-/* Should perhaps be moved to struct machine */
-static struct rb_root comm_str_root;
-static struct rw_semaphore comm_str_lock = {.lock = PTHREAD_RWLOCK_INITIALIZER,};
+static struct comm_strs {
+ struct rw_semaphore lock;
+ struct comm_str **strs;
+ int num_strs;
+ int capacity;
+} _comm_strs;
+
+static void comm_strs__init(void)
+{
+ init_rwsem(&_comm_strs.lock);
+ _comm_strs.capacity = 16;
+ _comm_strs.num_strs = 0;
+ _comm_strs.strs = calloc(16, sizeof(*_comm_strs.strs));
+}
+
+static struct comm_strs *comm_strs__get(void)
+{
+ static pthread_once_t comm_strs_type_once = PTHREAD_ONCE_INIT;
+
+ pthread_once(&comm_strs_type_once, comm_strs__init);
+
+ return &_comm_strs;
+}
+
+static refcount_t *comm_str__refcnt(struct comm_str *cs)
+{
+ return &RC_CHK_ACCESS(cs)->refcnt;
+}
+
+static const char *comm_str__str(const struct comm_str *cs)
+{
+ return &RC_CHK_ACCESS(cs)->str[0];
+}
static struct comm_str *comm_str__get(struct comm_str *cs)
{
- if (cs && refcount_inc_not_zero(&cs->refcnt))
- return cs;
+ struct comm_str *result;
+
+ if (RC_CHK_GET(result, cs))
+ refcount_inc_not_zero(comm_str__refcnt(cs));
- return NULL;
+ return result;
}
static void comm_str__put(struct comm_str *cs)
{
- if (cs && refcount_dec_and_test(&cs->refcnt)) {
- down_write(&comm_str_lock);
- rb_erase(&cs->rb_node, &comm_str_root);
- up_write(&comm_str_lock);
- zfree(&cs->str);
- free(cs);
+ if (cs && refcount_dec_and_test(comm_str__refcnt(cs))) {
+ struct comm_strs *comm_strs = comm_strs__get();
+ int i;
+
+ down_write(&comm_strs->lock);
+ for (i = 0; i < comm_strs->num_strs; i++) {
+ if (comm_strs->strs[i] == cs)
+ break;
+ }
+ for (; i < comm_strs->num_strs - 1; i++)
+ comm_strs->strs[i] = comm_strs->strs[i + 1];
+
+ comm_strs->num_strs--;
+ up_write(&comm_strs->lock);
+ RC_CHK_FREE(cs);
+ } else {
+ RC_CHK_PUT(cs);
}
}
-static struct comm_str *comm_str__alloc(const char *str)
+static struct comm_str *comm_str__new(const char *str)
{
- struct comm_str *cs;
+ struct comm_str *result = NULL;
+ RC_STRUCT(comm_str) *cs;
- cs = zalloc(sizeof(*cs));
- if (!cs)
- return NULL;
-
- cs->str = strdup(str);
- if (!cs->str) {
- free(cs);
- return NULL;
+ cs = malloc(sizeof(*cs) + strlen(str) + 1);
+ if (ADD_RC_CHK(result, cs)) {
+ refcount_set(comm_str__refcnt(result), 1);
+ strcpy(&cs->str[0], str);
}
+ return result;
+}
- refcount_set(&cs->refcnt, 1);
+static int comm_str__cmp(const void *_lhs, const void *_rhs)
+{
+ const struct comm_str *lhs = *(const struct comm_str * const *)_lhs;
+ const struct comm_str *rhs = *(const struct comm_str * const *)_rhs;
- return cs;
+ return strcmp(comm_str__str(lhs), comm_str__str(rhs));
}
-static
-struct comm_str *__comm_str__findnew(const char *str, struct rb_root *root)
+static int comm_str__search(const void *_key, const void *_member)
{
- struct rb_node **p = &root->rb_node;
- struct rb_node *parent = NULL;
- struct comm_str *iter, *new;
- int cmp;
-
- while (*p != NULL) {
- parent = *p;
- iter = rb_entry(parent, struct comm_str, rb_node);
-
- /*
- * If we race with comm_str__put, iter->refcnt is 0
- * and it will be removed within comm_str__put call
- * shortly, ignore it in this search.
- */
- cmp = strcmp(str, iter->str);
- if (!cmp && comm_str__get(iter))
- return iter;
-
- if (cmp < 0)
- p = &(*p)->rb_left;
- else
- p = &(*p)->rb_right;
- }
+ const char *key = _key;
+ const struct comm_str *member = *(const struct comm_str * const *)_member;
- new = comm_str__alloc(str);
- if (!new)
- return NULL;
+ return strcmp(key, comm_str__str(member));
+}
+
+static struct comm_str *__comm_strs__find(struct comm_strs *comm_strs, const char *str)
+{
+ struct comm_str **result;
- rb_link_node(&new->rb_node, parent, p);
- rb_insert_color(&new->rb_node, root);
+ result = bsearch(str, comm_strs->strs, comm_strs->num_strs, sizeof(struct comm_str *),
+ comm_str__search);
- return new;
+ if (!result)
+ return NULL;
+
+ return comm_str__get(*result);
}
-static struct comm_str *comm_str__findnew(const char *str, struct rb_root *root)
+static struct comm_str *comm_strs__findnew(const char *str)
{
- struct comm_str *cs;
+ struct comm_strs *comm_strs = comm_strs__get();
+ struct comm_str *result;
- down_write(&comm_str_lock);
- cs = __comm_str__findnew(str, root);
- up_write(&comm_str_lock);
+ if (!comm_strs)
+ return NULL;
- return cs;
+ down_read(&comm_strs->lock);
+ result = __comm_strs__find(comm_strs, str);
+ up_read(&comm_strs->lock);
+ if (result)
+ return result;
+
+ down_write(&comm_strs->lock);
+ result = __comm_strs__find(comm_strs, str);
+ if (!result) {
+ if (comm_strs->num_strs == comm_strs->capacity) {
+ struct comm_str **tmp;
+
+ tmp = reallocarray(comm_strs->strs,
+ comm_strs->capacity + 16,
+ sizeof(*comm_strs->strs));
+ if (!tmp) {
+ up_write(&comm_strs->lock);
+ return NULL;
+ }
+ comm_strs->strs = tmp;
+ comm_strs->capacity += 16;
+ }
+ result = comm_str__new(str);
+ if (result) {
+ comm_strs->strs[comm_strs->num_strs++] = result;
+ qsort(comm_strs->strs, comm_strs->num_strs, sizeof(struct comm_str *),
+ comm_str__cmp);
+ }
+ }
+ up_write(&comm_strs->lock);
+ return result;
}
struct comm *comm__new(const char *str, u64 timestamp, bool exec)
@@ -115,7 +171,7 @@ struct comm *comm__new(const char *str, u64 timestamp, bool exec)
comm->start = timestamp;
comm->exec = exec;
- comm->comm_str = comm_str__findnew(str, &comm_str_root);
+ comm->comm_str = comm_strs__findnew(str);
if (!comm->comm_str) {
free(comm);
return NULL;
@@ -128,7 +184,7 @@ int comm__override(struct comm *comm, const char *str, u64 timestamp, bool exec)
{
struct comm_str *new, *old = comm->comm_str;
- new = comm_str__findnew(str, &comm_str_root);
+ new = comm_strs__findnew(str);
if (!new)
return -ENOMEM;
@@ -149,5 +205,5 @@ void comm__free(struct comm *comm)
const char *comm__str(const struct comm *comm)
{
- return comm->comm_str->str;
+ return comm_str__str(comm->comm_str);
}
--
2.45.0.rc1.225.g2a3ae87e7f-goog
It is assigned a value of 1 and never incremented. Remove and replace
puts with delete.
Signed-off-by: Ian Rogers <[email protected]>
---
tools/perf/builtin-stat.c | 16 +++-------------
tools/perf/util/cpumap.c | 2 --
tools/perf/util/cpumap.h | 2 --
3 files changed, 3 insertions(+), 17 deletions(-)
diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index 65a3dd7ffac3..35f79b48e8dc 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -1631,23 +1631,13 @@ static int perf_stat_init_aggr_mode(void)
static void cpu_aggr_map__delete(struct cpu_aggr_map *map)
{
- if (map) {
- WARN_ONCE(refcount_read(&map->refcnt) != 0,
- "cpu_aggr_map refcnt unbalanced\n");
- free(map);
- }
-}
-
-static void cpu_aggr_map__put(struct cpu_aggr_map *map)
-{
- if (map && refcount_dec_and_test(&map->refcnt))
- cpu_aggr_map__delete(map);
+ free(map);
}
static void perf_stat__exit_aggr_mode(void)
{
- cpu_aggr_map__put(stat_config.aggr_map);
- cpu_aggr_map__put(stat_config.cpus_aggr_map);
+ cpu_aggr_map__delete(stat_config.aggr_map);
+ cpu_aggr_map__delete(stat_config.cpus_aggr_map);
stat_config.aggr_map = NULL;
stat_config.cpus_aggr_map = NULL;
}
diff --git a/tools/perf/util/cpumap.c b/tools/perf/util/cpumap.c
index 6a270d640acb..27094211edd8 100644
--- a/tools/perf/util/cpumap.c
+++ b/tools/perf/util/cpumap.c
@@ -180,8 +180,6 @@ struct cpu_aggr_map *cpu_aggr_map__empty_new(int nr)
cpus->nr = nr;
for (i = 0; i < nr; i++)
cpus->map[i] = aggr_cpu_id__empty();
-
- refcount_set(&cpus->refcnt, 1);
}
return cpus;
diff --git a/tools/perf/util/cpumap.h b/tools/perf/util/cpumap.h
index 26cf76c693f5..ee0f6139b04a 100644
--- a/tools/perf/util/cpumap.h
+++ b/tools/perf/util/cpumap.h
@@ -5,7 +5,6 @@
#include <stdbool.h>
#include <stdio.h>
#include <perf/cpumap.h>
-#include <linux/refcount.h>
/** Identify where counts are aggregated, -1 implies not to aggregate. */
struct aggr_cpu_id {
@@ -37,7 +36,6 @@ struct aggr_cpu_id {
/** A collection of aggr_cpu_id values, the "built" version is sorted and uniqued. */
struct cpu_aggr_map {
- refcount_t refcnt;
/** Number of valid entries. */
int nr;
/** The entries. */
--
2.45.0.rc1.225.g2a3ae87e7f-goog
Move mem-info to its own header rather than having it split between
mem-events and symbol.
Signed-off-by: Ian Rogers <[email protected]>
---
tools/perf/builtin-c2c.c | 1 +
tools/perf/builtin-report.c | 1 +
tools/perf/builtin-script.c | 1 +
tools/perf/tests/mem.c | 1 +
tools/perf/util/Build | 1 +
tools/perf/util/hist.c | 1 +
tools/perf/util/machine.c | 1 +
tools/perf/util/mem-events.c | 16 +++++-----
tools/perf/util/mem-events.h | 29 +++++++------------
tools/perf/util/mem-info.c | 28 ++++++++++++++++++
tools/perf/util/mem-info.h | 28 ++++++++++++++++++
.../scripting-engines/trace-event-python.c | 1 +
tools/perf/util/sort.c | 1 +
tools/perf/util/symbol.c | 26 +----------------
tools/perf/util/symbol.h | 12 --------
15 files changed, 85 insertions(+), 63 deletions(-)
create mode 100644 tools/perf/util/mem-info.c
create mode 100644 tools/perf/util/mem-info.h
diff --git a/tools/perf/builtin-c2c.c b/tools/perf/builtin-c2c.c
index 1f1d17df9b9a..c624414b2285 100644
--- a/tools/perf/builtin-c2c.c
+++ b/tools/perf/builtin-c2c.c
@@ -38,6 +38,7 @@
#include "ui/browsers/hists.h"
#include "thread.h"
#include "mem2node.h"
+#include "mem-info.h"
#include "symbol.h"
#include "ui/ui.h"
#include "ui/progress.h"
diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index b5525f4f7090..6eb1d90b589f 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -31,6 +31,7 @@
#include "util/evsel.h"
#include "util/evswitch.h"
#include "util/header.h"
+#include "util/mem-info.h"
#include "util/session.h"
#include "util/srcline.h"
#include "util/tool.h"
diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
index f7c3c3868c3c..58af4f3aa592 100644
--- a/tools/perf/builtin-script.c
+++ b/tools/perf/builtin-script.c
@@ -32,6 +32,7 @@
#include "util/time-utils.h"
#include "util/path.h"
#include "util/event.h"
+#include "util/mem-info.h"
#include "ui/ui.h"
#include "print_binary.h"
#include "print_insn.h"
diff --git a/tools/perf/tests/mem.c b/tools/perf/tests/mem.c
index 56014ec7d49d..f694a60d8a73 100644
--- a/tools/perf/tests/mem.c
+++ b/tools/perf/tests/mem.c
@@ -1,6 +1,7 @@
// SPDX-License-Identifier: GPL-2.0
#include "util/map_symbol.h"
#include "util/mem-events.h"
+#include "util/mem-info.h"
#include "util/symbol.h"
#include "linux/perf_event.h"
#include "util/debug.h"
diff --git a/tools/perf/util/Build b/tools/perf/util/Build
index 292170a99ab6..da64efd8718f 100644
--- a/tools/perf/util/Build
+++ b/tools/perf/util/Build
@@ -141,6 +141,7 @@ perf-y += term.o
perf-y += help-unknown-cmd.o
perf-y += dlfilter.o
perf-y += mem-events.o
+perf-y += mem-info.o
perf-y += vsprintf.o
perf-y += units.o
perf-y += time-utils.o
diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index b8a508cd0b14..56453a02cdf4 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -9,6 +9,7 @@
#include "map_symbol.h"
#include "branch.h"
#include "mem-events.h"
+#include "mem-info.h"
#include "session.h"
#include "namespaces.h"
#include "cgroup.h"
diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index a3ff2ab154bd..d5a01ef92668 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -16,6 +16,7 @@
#include "map_symbol.h"
#include "branch.h"
#include "mem-events.h"
+#include "mem-info.h"
#include "path.h"
#include "srcline.h"
#include "symbol.h"
diff --git a/tools/perf/util/mem-events.c b/tools/perf/util/mem-events.c
index 637cbd4a7bfb..f618a5ccc7af 100644
--- a/tools/perf/util/mem-events.c
+++ b/tools/perf/util/mem-events.c
@@ -10,7 +10,9 @@
#include <linux/kernel.h>
#include "map_symbol.h"
#include "mem-events.h"
+#include "mem-info.h"
#include "debug.h"
+#include "evsel.h"
#include "symbol.h"
#include "pmu.h"
#include "pmus.h"
@@ -281,7 +283,7 @@ static const char * const tlb_access[] = {
"Fault",
};
-int perf_mem__tlb_scnprintf(char *out, size_t sz, struct mem_info *mem_info)
+int perf_mem__tlb_scnprintf(char *out, size_t sz, const struct mem_info *mem_info)
{
size_t l = 0, i;
u64 m = PERF_MEM_TLB_NA;
@@ -359,7 +361,7 @@ static const char * const mem_hops[] = {
"board",
};
-static int perf_mem__op_scnprintf(char *out, size_t sz, struct mem_info *mem_info)
+static int perf_mem__op_scnprintf(char *out, size_t sz, const struct mem_info *mem_info)
{
u64 op = PERF_MEM_LOCK_NA;
int l;
@@ -383,7 +385,7 @@ static int perf_mem__op_scnprintf(char *out, size_t sz, struct mem_info *mem_inf
return l;
}
-int perf_mem__lvl_scnprintf(char *out, size_t sz, struct mem_info *mem_info)
+int perf_mem__lvl_scnprintf(char *out, size_t sz, const struct mem_info *mem_info)
{
union perf_mem_data_src data_src;
int printed = 0;
@@ -465,7 +467,7 @@ static const char * const snoopx_access[] = {
"Peer",
};
-int perf_mem__snp_scnprintf(char *out, size_t sz, struct mem_info *mem_info)
+int perf_mem__snp_scnprintf(char *out, size_t sz, const struct mem_info *mem_info)
{
size_t i, l = 0;
u64 m = PERF_MEM_SNOOP_NA;
@@ -507,7 +509,7 @@ int perf_mem__snp_scnprintf(char *out, size_t sz, struct mem_info *mem_info)
return l;
}
-int perf_mem__lck_scnprintf(char *out, size_t sz, struct mem_info *mem_info)
+int perf_mem__lck_scnprintf(char *out, size_t sz, const struct mem_info *mem_info)
{
u64 mask = PERF_MEM_LOCK_NA;
int l;
@@ -525,7 +527,7 @@ int perf_mem__lck_scnprintf(char *out, size_t sz, struct mem_info *mem_info)
return l;
}
-int perf_mem__blk_scnprintf(char *out, size_t sz, struct mem_info *mem_info)
+int perf_mem__blk_scnprintf(char *out, size_t sz, const struct mem_info *mem_info)
{
size_t l = 0;
u64 mask = PERF_MEM_BLK_NA;
@@ -548,7 +550,7 @@ int perf_mem__blk_scnprintf(char *out, size_t sz, struct mem_info *mem_info)
return l;
}
-int perf_script__meminfo_scnprintf(char *out, size_t sz, struct mem_info *mem_info)
+int perf_script__meminfo_scnprintf(char *out, size_t sz, const struct mem_info *mem_info)
{
int i = 0;
diff --git a/tools/perf/util/mem-events.h b/tools/perf/util/mem-events.h
index 15d5f0320d27..ca31014d7934 100644
--- a/tools/perf/util/mem-events.h
+++ b/tools/perf/util/mem-events.h
@@ -3,13 +3,7 @@
#define __PERF_MEM_EVENTS_H
#include <stdbool.h>
-#include <stdint.h>
-#include <stdio.h>
#include <linux/types.h>
-#include <linux/refcount.h>
-#include <linux/perf_event.h>
-#include "stat.h"
-#include "evsel.h"
struct perf_mem_event {
bool record;
@@ -21,13 +15,6 @@ struct perf_mem_event {
const char *event_name;
};
-struct mem_info {
- struct addr_map_symbol iaddr;
- struct addr_map_symbol daddr;
- union perf_mem_data_src data_src;
- refcount_t refcnt;
-};
-
enum {
PERF_MEM_EVENTS__LOAD,
PERF_MEM_EVENTS__STORE,
@@ -35,6 +22,10 @@ enum {
PERF_MEM_EVENTS__MAX,
};
+struct evsel;
+struct mem_info;
+struct perf_pmu;
+
extern unsigned int perf_mem_events__loads_ldlat;
extern struct perf_mem_event perf_mem_events[PERF_MEM_EVENTS__MAX];
@@ -49,13 +40,13 @@ bool is_mem_loads_aux_event(struct evsel *leader);
void perf_pmu__mem_events_list(struct perf_pmu *pmu);
int perf_mem_events__record_args(const char **rec_argv, int *argv_nr);
-int perf_mem__tlb_scnprintf(char *out, size_t sz, struct mem_info *mem_info);
-int perf_mem__lvl_scnprintf(char *out, size_t sz, struct mem_info *mem_info);
-int perf_mem__snp_scnprintf(char *out, size_t sz, struct mem_info *mem_info);
-int perf_mem__lck_scnprintf(char *out, size_t sz, struct mem_info *mem_info);
-int perf_mem__blk_scnprintf(char *out, size_t sz, struct mem_info *mem_info);
+int perf_mem__tlb_scnprintf(char *out, size_t sz, const struct mem_info *mem_info);
+int perf_mem__lvl_scnprintf(char *out, size_t sz, const struct mem_info *mem_info);
+int perf_mem__snp_scnprintf(char *out, size_t sz, const struct mem_info *mem_info);
+int perf_mem__lck_scnprintf(char *out, size_t sz, const struct mem_info *mem_info);
+int perf_mem__blk_scnprintf(char *out, size_t sz, const struct mem_info *mem_info);
-int perf_script__meminfo_scnprintf(char *bf, size_t size, struct mem_info *mem_info);
+int perf_script__meminfo_scnprintf(char *bf, size_t size, const struct mem_info *mem_info);
struct c2c_stats {
u32 nr_entries;
diff --git a/tools/perf/util/mem-info.c b/tools/perf/util/mem-info.c
new file mode 100644
index 000000000000..ff0dfdb5369a
--- /dev/null
+++ b/tools/perf/util/mem-info.c
@@ -0,0 +1,28 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <linux/zalloc.h>
+#include "mem-info.h"
+
+struct mem_info *mem_info__get(struct mem_info *mi)
+{
+ if (mi)
+ refcount_inc(&mi->refcnt);
+ return mi;
+}
+
+void mem_info__put(struct mem_info *mi)
+{
+ if (mi && refcount_dec_and_test(&mi->refcnt)) {
+ addr_map_symbol__exit(&mi->iaddr);
+ addr_map_symbol__exit(&mi->daddr);
+ free(mi);
+ }
+}
+
+struct mem_info *mem_info__new(void)
+{
+ struct mem_info *mi = zalloc(sizeof(*mi));
+
+ if (mi)
+ refcount_set(&mi->refcnt, 1);
+ return mi;
+}
diff --git a/tools/perf/util/mem-info.h b/tools/perf/util/mem-info.h
new file mode 100644
index 000000000000..bb7d8375d73c
--- /dev/null
+++ b/tools/perf/util/mem-info.h
@@ -0,0 +1,28 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __PERF_MEM_INFO_H
+#define __PERF_MEM_INFO_H
+
+#include <linux/refcount.h>
+#include <linux/perf_event.h>
+#include "map_symbol.h"
+
+struct mem_info {
+ struct addr_map_symbol iaddr;
+ struct addr_map_symbol daddr;
+ union perf_mem_data_src data_src;
+ refcount_t refcnt;
+};
+
+struct mem_info *mem_info__new(void);
+struct mem_info *mem_info__get(struct mem_info *mi);
+void mem_info__put(struct mem_info *mi);
+
+static inline void __mem_info__zput(struct mem_info **mi)
+{
+ mem_info__put(*mi);
+ *mi = NULL;
+}
+
+#define mem_info__zput(mi) __mem_info__zput(&mi)
+
+#endif /* __PERF_MEM_INFO_H */
diff --git a/tools/perf/util/scripting-engines/trace-event-python.c b/tools/perf/util/scripting-engines/trace-event-python.c
index c2caa5720299..c77fe08a0545 100644
--- a/tools/perf/util/scripting-engines/trace-event-python.c
+++ b/tools/perf/util/scripting-engines/trace-event-python.c
@@ -45,6 +45,7 @@
#include "../thread.h"
#include "../comm.h"
#include "../machine.h"
+#include "../mem-info.h"
#include "../db-export.h"
#include "../thread-stack.h"
#include "../trace-event.h"
diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
index 704664e5b4ea..711ef69306f3 100644
--- a/tools/perf/util/sort.c
+++ b/tools/perf/util/sort.c
@@ -23,6 +23,7 @@
#include "strlist.h"
#include "strbuf.h"
#include "mem-events.h"
+#include "mem-info.h"
#include "annotate.h"
#include "annotate-data.h"
#include "event.h"
diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index 7772a4d3e66c..eb3319baa1b5 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -27,6 +27,7 @@
#include "symbol.h"
#include "map_symbol.h"
#include "mem-events.h"
+#include "mem-info.h"
#include "symsrc.h"
#include "strlist.h"
#include "intlist.h"
@@ -2570,31 +2571,6 @@ int symbol__config_symfs(const struct option *opt __maybe_unused,
return 0;
}
-struct mem_info *mem_info__get(struct mem_info *mi)
-{
- if (mi)
- refcount_inc(&mi->refcnt);
- return mi;
-}
-
-void mem_info__put(struct mem_info *mi)
-{
- if (mi && refcount_dec_and_test(&mi->refcnt)) {
- addr_map_symbol__exit(&mi->iaddr);
- addr_map_symbol__exit(&mi->daddr);
- free(mi);
- }
-}
-
-struct mem_info *mem_info__new(void)
-{
- struct mem_info *mi = zalloc(sizeof(*mi));
-
- if (mi)
- refcount_set(&mi->refcnt, 1);
- return mi;
-}
-
/*
* Checks that user supplied symbol kernel files are accessible because
* the default mechanism for accessing elf files fails silently. i.e. if
diff --git a/tools/perf/util/symbol.h b/tools/perf/util/symbol.h
index 071837ddce2a..3fb5d146d9b1 100644
--- a/tools/perf/util/symbol.h
+++ b/tools/perf/util/symbol.h
@@ -268,18 +268,6 @@ enum {
SDT_NOTE_IDX_REFCTR,
};
-struct mem_info *mem_info__new(void);
-struct mem_info *mem_info__get(struct mem_info *mi);
-void mem_info__put(struct mem_info *mi);
-
-static inline void __mem_info__zput(struct mem_info **mi)
-{
- mem_info__put(*mi);
- *mi = NULL;
-}
-
-#define mem_info__zput(mi) __mem_info__zput(&mi)
-
int symbol__validate_sym_arguments(void);
#endif /* __PERF_SYMBOL */
--
2.45.0.rc1.225.g2a3ae87e7f-goog
struct mem_info is reference counted while branch_info and he_cache
are not. Break apart the priv field in hist_entry_iter so that we can
know which values are owned by the iter and do the appropriate free or
put. Move hide_unresolved to marginally shrink the size of the now
grown struct.
Signed-off-by: Ian Rogers <[email protected]>
---
tools/perf/util/hist.c | 39 ++++++++++++++-------------------------
tools/perf/util/hist.h | 8 +++++---
2 files changed, 19 insertions(+), 28 deletions(-)
diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index 00814d42d5f1..2e9e193179dd 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -476,13 +476,6 @@ static int hist_entry__init(struct hist_entry *he,
he->branch_info->to.ms.map = map__get(he->branch_info->to.ms.map);
}
- if (he->mem_info) {
- mem_info__iaddr(he->mem_info)->ms.map =
- map__get(mem_info__iaddr(he->mem_info)->ms.map);
- mem_info__daddr(he->mem_info)->ms.map =
- map__get(mem_info__daddr(he->mem_info)->ms.map);
- }
-
if (hist_entry__has_callchains(he) && symbol_conf.use_callchain)
callchain_init(he->callchain);
@@ -574,7 +567,6 @@ static struct hist_entry *hist_entry__new(struct hist_entry *template,
he = NULL;
}
}
-
return he;
}
@@ -747,7 +739,7 @@ __hists__add_entry(struct hists *hists,
.filtered = symbol__parent_filter(sym_parent) | al->filtered,
.hists = hists,
.branch_info = bi,
- .mem_info = mi,
+ .mem_info = mem_info__get(mi),
.kvm_info = ki,
.block_info = block_info,
.transaction = sample->transaction,
@@ -836,7 +828,7 @@ iter_prepare_mem_entry(struct hist_entry_iter *iter, struct addr_location *al)
if (mi == NULL)
return -ENOMEM;
- iter->priv = mi;
+ iter->mi = mi;
return 0;
}
@@ -844,7 +836,7 @@ static int
iter_add_single_mem_entry(struct hist_entry_iter *iter, struct addr_location *al)
{
u64 cost;
- struct mem_info *mi = iter->priv;
+ struct mem_info *mi = iter->mi;
struct hists *hists = evsel__hists(iter->evsel);
struct perf_sample *sample = iter->sample;
struct hist_entry *he;
@@ -891,12 +883,7 @@ iter_finish_mem_entry(struct hist_entry_iter *iter,
err = hist_entry__append_callchain(he, iter->sample);
out:
- /*
- * We don't need to free iter->priv (mem_info) here since the mem info
- * was either already freed in hists__findnew_entry() or passed to a
- * new hist entry by hist_entry__new().
- */
- iter->priv = NULL;
+ mem_info__zput(iter->mi);
iter->he = NULL;
return err;
@@ -915,7 +902,7 @@ iter_prepare_branch_entry(struct hist_entry_iter *iter, struct addr_location *al
iter->curr = 0;
iter->total = sample->branch_stack->nr;
- iter->priv = bi;
+ iter->bi = bi;
return 0;
}
@@ -929,7 +916,7 @@ iter_add_single_branch_entry(struct hist_entry_iter *iter __maybe_unused,
static int
iter_next_branch_entry(struct hist_entry_iter *iter, struct addr_location *al)
{
- struct branch_info *bi = iter->priv;
+ struct branch_info *bi = iter->bi;
int i = iter->curr;
if (bi == NULL)
@@ -958,7 +945,7 @@ iter_add_next_branch_entry(struct hist_entry_iter *iter, struct addr_location *a
int i = iter->curr;
int err = 0;
- bi = iter->priv;
+ bi = iter->bi;
if (iter->hide_unresolved && !(bi[i].from.ms.sym && bi[i].to.ms.sym))
goto out;
@@ -987,7 +974,7 @@ static int
iter_finish_branch_entry(struct hist_entry_iter *iter,
struct addr_location *al __maybe_unused)
{
- zfree(&iter->priv);
+ zfree(&iter->bi);
iter->he = NULL;
return iter->curr >= iter->total ? 0 : -1;
@@ -1055,7 +1042,7 @@ iter_prepare_cumulative_entry(struct hist_entry_iter *iter,
if (he_cache == NULL)
return -ENOMEM;
- iter->priv = he_cache;
+ iter->he_cache = he_cache;
iter->curr = 0;
return 0;
@@ -1068,7 +1055,7 @@ iter_add_single_cumulative_entry(struct hist_entry_iter *iter,
struct evsel *evsel = iter->evsel;
struct hists *hists = evsel__hists(evsel);
struct perf_sample *sample = iter->sample;
- struct hist_entry **he_cache = iter->priv;
+ struct hist_entry **he_cache = iter->he_cache;
struct hist_entry *he;
int err = 0;
@@ -1126,7 +1113,7 @@ iter_add_next_cumulative_entry(struct hist_entry_iter *iter,
{
struct evsel *evsel = iter->evsel;
struct perf_sample *sample = iter->sample;
- struct hist_entry **he_cache = iter->priv;
+ struct hist_entry **he_cache = iter->he_cache;
struct hist_entry *he;
struct hist_entry he_tmp = {
.hists = evsel__hists(evsel),
@@ -1192,7 +1179,9 @@ static int
iter_finish_cumulative_entry(struct hist_entry_iter *iter,
struct addr_location *al __maybe_unused)
{
- zfree(&iter->priv);
+ mem_info__zput(iter->mi);
+ zfree(&iter->bi);
+ zfree(&iter->he_cache);
iter->he = NULL;
return 0;
diff --git a/tools/perf/util/hist.h b/tools/perf/util/hist.h
index 5260822b9773..8fb3bdd29188 100644
--- a/tools/perf/util/hist.h
+++ b/tools/perf/util/hist.h
@@ -132,18 +132,20 @@ struct hist_entry_iter {
int total;
int curr;
- bool hide_unresolved;
-
struct evsel *evsel;
struct perf_sample *sample;
struct hist_entry *he;
struct symbol *parent;
- void *priv;
+
+ struct mem_info *mi;
+ struct branch_info *bi;
+ struct hist_entry **he_cache;
const struct hist_iter_ops *ops;
/* user-defined callback function (optional) */
int (*add_entry_cb)(struct hist_entry_iter *iter,
struct addr_location *al, bool single, void *arg);
+ bool hide_unresolved;
};
extern const struct hist_iter_ops hist_iter_normal;
--
2.45.0.rc1.225.g2a3ae87e7f-goog
Add reference count checking and switch struct mem_info usage to use
accessor functions.
Signed-off-by: Ian Rogers <[email protected]>
---
tools/perf/builtin-c2c.c | 12 ++--
tools/perf/builtin-report.c | 2 +-
tools/perf/builtin-script.c | 11 ++-
tools/perf/tests/mem.c | 10 +--
tools/perf/util/hist.c | 26 +++----
tools/perf/util/machine.c | 6 +-
tools/perf/util/mem-events.c | 20 +++---
tools/perf/util/mem-info.c | 29 +++++---
tools/perf/util/mem-info.h | 28 +++++++-
.../scripting-engines/trace-event-python.c | 11 ++-
tools/perf/util/sort.c | 68 +++++++++----------
11 files changed, 135 insertions(+), 88 deletions(-)
diff --git a/tools/perf/builtin-c2c.c b/tools/perf/builtin-c2c.c
index c624414b2285..c157bd31f2e5 100644
--- a/tools/perf/builtin-c2c.c
+++ b/tools/perf/builtin-c2c.c
@@ -530,7 +530,7 @@ static int dcacheline_entry(struct perf_hpp_fmt *fmt, struct perf_hpp *hpp,
char buf[20];
if (he->mem_info)
- addr = cl_address(he->mem_info->daddr.addr, chk_double_cl);
+ addr = cl_address(mem_info__daddr(he->mem_info)->addr, chk_double_cl);
return scnprintf(hpp->buf, hpp->size, "%*s", width, HEX_STR(buf, addr));
}
@@ -568,7 +568,7 @@ static int offset_entry(struct perf_hpp_fmt *fmt, struct perf_hpp *hpp,
char buf[20];
if (he->mem_info)
- addr = cl_offset(he->mem_info->daddr.al_addr, chk_double_cl);
+ addr = cl_offset(mem_info__daddr(he->mem_info)->al_addr, chk_double_cl);
return scnprintf(hpp->buf, hpp->size, "%*s", width, HEX_STR(buf, addr));
}
@@ -580,10 +580,10 @@ offset_cmp(struct perf_hpp_fmt *fmt __maybe_unused,
uint64_t l = 0, r = 0;
if (left->mem_info)
- l = cl_offset(left->mem_info->daddr.addr, chk_double_cl);
+ l = cl_offset(mem_info__daddr(left->mem_info)->addr, chk_double_cl);
if (right->mem_info)
- r = cl_offset(right->mem_info->daddr.addr, chk_double_cl);
+ r = cl_offset(mem_info__daddr(right->mem_info)->addr, chk_double_cl);
return (int64_t)(r - l);
}
@@ -597,7 +597,7 @@ iaddr_entry(struct perf_hpp_fmt *fmt, struct perf_hpp *hpp,
char buf[20];
if (he->mem_info)
- addr = he->mem_info->iaddr.addr;
+ addr = mem_info__iaddr(he->mem_info)->addr;
return scnprintf(hpp->buf, hpp->size, "%*s", width, HEX_STR(buf, addr));
}
@@ -2593,7 +2593,7 @@ perf_c2c_cacheline_browser__title(struct hist_browser *browser,
he = cl_browser->he;
if (he->mem_info)
- addr = cl_address(he->mem_info->daddr.addr, chk_double_cl);
+ addr = cl_address(mem_info__daddr(he->mem_info)->addr, chk_double_cl);
scnprintf(bf, size, "Cacheline 0x%lx", addr);
return 0;
diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index 6eb1d90b589f..0892b6e3e5e7 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -186,7 +186,7 @@ static int hist_iter__report_callback(struct hist_entry_iter *iter,
} else if (rep->mem_mode) {
mi = he->mem_info;
- err = addr_map_symbol__inc_samples(&mi->daddr, sample, evsel);
+ err = addr_map_symbol__inc_samples(mem_info__daddr(mi), sample, evsel);
if (err)
goto out;
diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
index 58af4f3aa592..c16224b1fef3 100644
--- a/tools/perf/builtin-script.c
+++ b/tools/perf/builtin-script.c
@@ -2051,13 +2051,18 @@ static int evlist__max_name_len(struct evlist *evlist)
static int data_src__fprintf(u64 data_src, FILE *fp)
{
- struct mem_info mi = { .data_src.val = data_src };
+ struct mem_info *mi = mem_info__new();
char decode[100];
char out[100];
static int maxlen;
int len;
- perf_script__meminfo_scnprintf(decode, 100, &mi);
+ if (!mi)
+ return -ENOMEM;
+
+ mem_info__data_src(mi)->val = data_src;
+ perf_script__meminfo_scnprintf(decode, 100, mi);
+ mem_info__put(mi);
len = scnprintf(out, 100, "%16" PRIx64 " %s", data_src, decode);
if (maxlen < len)
@@ -2498,7 +2503,7 @@ static int process_attr(struct perf_tool *tool, union perf_event *event,
evsel = evlist__last(*pevlist);
if (!evsel->priv) {
- if (scr->per_event_dump) {
+ if (scr->per_event_dump) {
evsel->priv = evsel_script__new(evsel, scr->session->data);
if (!evsel->priv)
return -ENOMEM;
diff --git a/tools/perf/tests/mem.c b/tools/perf/tests/mem.c
index f694a60d8a73..cb3d749e157b 100644
--- a/tools/perf/tests/mem.c
+++ b/tools/perf/tests/mem.c
@@ -13,12 +13,14 @@ static int check(union perf_mem_data_src data_src,
{
char out[100];
char failure[100];
- struct mem_info mi = { .data_src = data_src };
-
+ struct mem_info *mi = mem_info__new();
int n;
- n = perf_mem__snp_scnprintf(out, sizeof out, &mi);
- n += perf_mem__lvl_scnprintf(out + n, sizeof out - n, &mi);
+ TEST_ASSERT_VAL("Memory allocation failed", mi);
+ *mem_info__data_src(mi) = data_src;
+ n = perf_mem__snp_scnprintf(out, sizeof out, mi);
+ n += perf_mem__lvl_scnprintf(out + n, sizeof out - n, mi);
+ mem_info__put(mi);
scnprintf(failure, sizeof failure, "unexpected %s", out);
TEST_ASSERT_VAL(failure, !strcmp(string, out));
return 0;
diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index 56453a02cdf4..00814d42d5f1 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -154,8 +154,8 @@ void hists__calc_col_len(struct hists *hists, struct hist_entry *h)
}
if (h->mem_info) {
- if (h->mem_info->daddr.ms.sym) {
- symlen = (int)h->mem_info->daddr.ms.sym->namelen + 4
+ if (mem_info__daddr(h->mem_info)->ms.sym) {
+ symlen = (int)mem_info__daddr(h->mem_info)->ms.sym->namelen + 4
+ unresolved_col_width + 2;
hists__new_col_len(hists, HISTC_MEM_DADDR_SYMBOL,
symlen);
@@ -169,8 +169,8 @@ void hists__calc_col_len(struct hists *hists, struct hist_entry *h)
symlen);
}
- if (h->mem_info->iaddr.ms.sym) {
- symlen = (int)h->mem_info->iaddr.ms.sym->namelen + 4
+ if (mem_info__iaddr(h->mem_info)->ms.sym) {
+ symlen = (int)mem_info__iaddr(h->mem_info)->ms.sym->namelen + 4
+ unresolved_col_width + 2;
hists__new_col_len(hists, HISTC_MEM_IADDR_SYMBOL,
symlen);
@@ -180,8 +180,8 @@ void hists__calc_col_len(struct hists *hists, struct hist_entry *h)
symlen);
}
- if (h->mem_info->daddr.ms.map) {
- symlen = dso__name_len(map__dso(h->mem_info->daddr.ms.map));
+ if (mem_info__daddr(h->mem_info)->ms.map) {
+ symlen = dso__name_len(map__dso(mem_info__daddr(h->mem_info)->ms.map));
hists__new_col_len(hists, HISTC_MEM_DADDR_DSO,
symlen);
} else {
@@ -477,8 +477,10 @@ static int hist_entry__init(struct hist_entry *he,
}
if (he->mem_info) {
- he->mem_info->iaddr.ms.map = map__get(he->mem_info->iaddr.ms.map);
- he->mem_info->daddr.ms.map = map__get(he->mem_info->daddr.ms.map);
+ mem_info__iaddr(he->mem_info)->ms.map =
+ map__get(mem_info__iaddr(he->mem_info)->ms.map);
+ mem_info__daddr(he->mem_info)->ms.map =
+ map__get(mem_info__daddr(he->mem_info)->ms.map);
}
if (hist_entry__has_callchains(he) && symbol_conf.use_callchain)
@@ -526,8 +528,8 @@ static int hist_entry__init(struct hist_entry *he,
zfree(&he->branch_info);
}
if (he->mem_info) {
- map_symbol__exit(&he->mem_info->iaddr.ms);
- map_symbol__exit(&he->mem_info->daddr.ms);
+ map_symbol__exit(&mem_info__iaddr(he->mem_info)->ms);
+ map_symbol__exit(&mem_info__daddr(he->mem_info)->ms);
}
err:
map_symbol__exit(&he->ms);
@@ -1336,8 +1338,8 @@ void hist_entry__delete(struct hist_entry *he)
}
if (he->mem_info) {
- map_symbol__exit(&he->mem_info->iaddr.ms);
- map_symbol__exit(&he->mem_info->daddr.ms);
+ map_symbol__exit(&mem_info__iaddr(he->mem_info)->ms);
+ map_symbol__exit(&mem_info__daddr(he->mem_info)->ms);
mem_info__zput(he->mem_info);
}
diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index d5a01ef92668..8477edefc299 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -2013,11 +2013,11 @@ struct mem_info *sample__resolve_mem(struct perf_sample *sample,
if (!mi)
return NULL;
- ip__resolve_ams(al->thread, &mi->iaddr, sample->ip);
- ip__resolve_data(al->thread, al->cpumode, &mi->daddr,
+ ip__resolve_ams(al->thread, mem_info__iaddr(mi), sample->ip);
+ ip__resolve_data(al->thread, al->cpumode, mem_info__daddr(mi),
sample->addr, sample->phys_addr,
sample->data_page_size);
- mi->data_src.val = sample->data_src;
+ mem_info__data_src(mi)->val = sample->data_src;
return mi;
}
diff --git a/tools/perf/util/mem-events.c b/tools/perf/util/mem-events.c
index f618a5ccc7af..6dda47bb774f 100644
--- a/tools/perf/util/mem-events.c
+++ b/tools/perf/util/mem-events.c
@@ -293,7 +293,7 @@ int perf_mem__tlb_scnprintf(char *out, size_t sz, const struct mem_info *mem_inf
out[0] = '\0';
if (mem_info)
- m = mem_info->data_src.mem_dtlb;
+ m = mem_info__const_data_src(mem_info)->mem_dtlb;
hit = m & PERF_MEM_TLB_HIT;
miss = m & PERF_MEM_TLB_MISS;
@@ -367,7 +367,7 @@ static int perf_mem__op_scnprintf(char *out, size_t sz, const struct mem_info *m
int l;
if (mem_info)
- op = mem_info->data_src.mem_op;
+ op = mem_info__const_data_src(mem_info)->mem_op;
if (op & PERF_MEM_OP_NA)
l = scnprintf(out, sz, "N/A");
@@ -400,7 +400,7 @@ int perf_mem__lvl_scnprintf(char *out, size_t sz, const struct mem_info *mem_inf
if (!mem_info)
goto na;
- data_src = mem_info->data_src;
+ data_src = *mem_info__const_data_src(mem_info);
if (data_src.mem_lvl & PERF_MEM_LVL_HIT)
memcpy(hit_miss, "hit", 3);
@@ -476,7 +476,7 @@ int perf_mem__snp_scnprintf(char *out, size_t sz, const struct mem_info *mem_inf
out[0] = '\0';
if (mem_info)
- m = mem_info->data_src.mem_snoop;
+ m = mem_info__const_data_src(mem_info)->mem_snoop;
for (i = 0; m && i < ARRAY_SIZE(snoop_access); i++, m >>= 1) {
if (!(m & 0x1))
@@ -490,7 +490,7 @@ int perf_mem__snp_scnprintf(char *out, size_t sz, const struct mem_info *mem_inf
m = 0;
if (mem_info)
- m = mem_info->data_src.mem_snoopx;
+ m = mem_info__const_data_src(mem_info)->mem_snoopx;
for (i = 0; m && i < ARRAY_SIZE(snoopx_access); i++, m >>= 1) {
if (!(m & 0x1))
@@ -515,7 +515,7 @@ int perf_mem__lck_scnprintf(char *out, size_t sz, const struct mem_info *mem_inf
int l;
if (mem_info)
- mask = mem_info->data_src.mem_lock;
+ mask = mem_info__const_data_src(mem_info)->mem_lock;
if (mask & PERF_MEM_LOCK_NA)
l = scnprintf(out, sz, "N/A");
@@ -536,7 +536,7 @@ int perf_mem__blk_scnprintf(char *out, size_t sz, const struct mem_info *mem_inf
out[0] = '\0';
if (mem_info)
- mask = mem_info->data_src.mem_blk;
+ mask = mem_info__const_data_src(mem_info)->mem_blk;
if (!mask || (mask & PERF_MEM_BLK_NA)) {
l += scnprintf(out + l, sz - l, " N/A");
@@ -572,8 +572,8 @@ int perf_script__meminfo_scnprintf(char *out, size_t sz, const struct mem_info *
int c2c_decode_stats(struct c2c_stats *stats, struct mem_info *mi)
{
- union perf_mem_data_src *data_src = &mi->data_src;
- u64 daddr = mi->daddr.addr;
+ union perf_mem_data_src *data_src = mem_info__data_src(mi);
+ u64 daddr = mem_info__daddr(mi)->addr;
u64 op = data_src->mem_op;
u64 lvl = data_src->mem_lvl;
u64 snoop = data_src->mem_snoop;
@@ -700,7 +700,7 @@ do { \
return -1;
}
- if (!mi->daddr.ms.map || !mi->iaddr.ms.map) {
+ if (!mem_info__daddr(mi)->ms.map || !mem_info__iaddr(mi)->ms.map) {
stats->nomap++;
return -1;
}
diff --git a/tools/perf/util/mem-info.c b/tools/perf/util/mem-info.c
index ff0dfdb5369a..27d67721a695 100644
--- a/tools/perf/util/mem-info.c
+++ b/tools/perf/util/mem-info.c
@@ -4,25 +4,32 @@
struct mem_info *mem_info__get(struct mem_info *mi)
{
- if (mi)
- refcount_inc(&mi->refcnt);
- return mi;
+ struct mem_info *result;
+
+ if (RC_CHK_GET(result, mi))
+ refcount_inc(mem_info__refcnt(mi));
+
+ return result;
}
void mem_info__put(struct mem_info *mi)
{
- if (mi && refcount_dec_and_test(&mi->refcnt)) {
- addr_map_symbol__exit(&mi->iaddr);
- addr_map_symbol__exit(&mi->daddr);
- free(mi);
+ if (mi && refcount_dec_and_test(mem_info__refcnt(mi))) {
+ addr_map_symbol__exit(mem_info__iaddr(mi));
+ addr_map_symbol__exit(mem_info__daddr(mi));
+ RC_CHK_FREE(mi);
+ } else {
+ RC_CHK_PUT(mi);
}
}
struct mem_info *mem_info__new(void)
{
- struct mem_info *mi = zalloc(sizeof(*mi));
+ struct mem_info *result = NULL;
+ RC_STRUCT(mem_info) *mi = zalloc(sizeof(*mi));
+
+ if (ADD_RC_CHK(result, mi))
+ refcount_set(mem_info__refcnt(result), 1);
- if (mi)
- refcount_set(&mi->refcnt, 1);
- return mi;
+ return result;
}
diff --git a/tools/perf/util/mem-info.h b/tools/perf/util/mem-info.h
index bb7d8375d73c..0f68e29f311b 100644
--- a/tools/perf/util/mem-info.h
+++ b/tools/perf/util/mem-info.h
@@ -4,9 +4,10 @@
#include <linux/refcount.h>
#include <linux/perf_event.h>
+#include <internal/rc_check.h>
#include "map_symbol.h"
-struct mem_info {
+DECLARE_RC_STRUCT(mem_info) {
struct addr_map_symbol iaddr;
struct addr_map_symbol daddr;
union perf_mem_data_src data_src;
@@ -25,4 +26,29 @@ static inline void __mem_info__zput(struct mem_info **mi)
#define mem_info__zput(mi) __mem_info__zput(&mi)
+static inline struct addr_map_symbol *mem_info__iaddr(struct mem_info *mi)
+{
+ return &RC_CHK_ACCESS(mi)->iaddr;
+}
+
+static inline struct addr_map_symbol *mem_info__daddr(struct mem_info *mi)
+{
+ return &RC_CHK_ACCESS(mi)->daddr;
+}
+
+static inline union perf_mem_data_src *mem_info__data_src(struct mem_info *mi)
+{
+ return &RC_CHK_ACCESS(mi)->data_src;
+}
+
+static inline const union perf_mem_data_src *mem_info__const_data_src(const struct mem_info *mi)
+{
+ return &RC_CHK_ACCESS(mi)->data_src;
+}
+
+static inline refcount_t *mem_info__refcnt(struct mem_info *mi)
+{
+ return &RC_CHK_ACCESS(mi)->refcnt;
+}
+
#endif /* __PERF_MEM_INFO_H */
diff --git a/tools/perf/util/scripting-engines/trace-event-python.c b/tools/perf/util/scripting-engines/trace-event-python.c
index c77fe08a0545..fb00f3ad6815 100644
--- a/tools/perf/util/scripting-engines/trace-event-python.c
+++ b/tools/perf/util/scripting-engines/trace-event-python.c
@@ -721,15 +721,20 @@ static void set_sample_read_in_dict(PyObject *dict_sample,
}
static void set_sample_datasrc_in_dict(PyObject *dict,
- struct perf_sample *sample)
+ struct perf_sample *sample)
{
- struct mem_info mi = { .data_src.val = sample->data_src };
+ struct mem_info *mi = mem_info__new();
char decode[100];
+ if (!mi)
+ Py_FatalError("couldn't create mem-info");
+
pydict_set_item_string_decref(dict, "datasrc",
PyLong_FromUnsignedLongLong(sample->data_src));
- perf_script__meminfo_scnprintf(decode, 100, &mi);
+ mem_info__data_src(mi)->val = sample->data_src;
+ perf_script__meminfo_scnprintf(decode, 100, mi);
+ mem_info__put(mi);
pydict_set_item_string_decref(dict, "datasrc_decode",
_PyUnicode_FromString(decode));
diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
index 711ef69306f3..cd39ea972193 100644
--- a/tools/perf/util/sort.c
+++ b/tools/perf/util/sort.c
@@ -1365,9 +1365,9 @@ sort__daddr_cmp(struct hist_entry *left, struct hist_entry *right)
uint64_t l = 0, r = 0;
if (left->mem_info)
- l = left->mem_info->daddr.addr;
+ l = mem_info__daddr(left->mem_info)->addr;
if (right->mem_info)
- r = right->mem_info->daddr.addr;
+ r = mem_info__daddr(right->mem_info)->addr;
return (int64_t)(r - l);
}
@@ -1379,8 +1379,8 @@ static int hist_entry__daddr_snprintf(struct hist_entry *he, char *bf,
struct map_symbol *ms = NULL;
if (he->mem_info) {
- addr = he->mem_info->daddr.addr;
- ms = &he->mem_info->daddr.ms;
+ addr = mem_info__daddr(he->mem_info)->addr;
+ ms = &mem_info__daddr(he->mem_info)->ms;
}
return _hist_entry__sym_snprintf(ms, addr, he->level, bf, size, width);
}
@@ -1391,9 +1391,9 @@ sort__iaddr_cmp(struct hist_entry *left, struct hist_entry *right)
uint64_t l = 0, r = 0;
if (left->mem_info)
- l = left->mem_info->iaddr.addr;
+ l = mem_info__iaddr(left->mem_info)->addr;
if (right->mem_info)
- r = right->mem_info->iaddr.addr;
+ r = mem_info__iaddr(right->mem_info)->addr;
return (int64_t)(r - l);
}
@@ -1405,8 +1405,8 @@ static int hist_entry__iaddr_snprintf(struct hist_entry *he, char *bf,
struct map_symbol *ms = NULL;
if (he->mem_info) {
- addr = he->mem_info->iaddr.addr;
- ms = &he->mem_info->iaddr.ms;
+ addr = mem_info__iaddr(he->mem_info)->addr;
+ ms = &mem_info__iaddr(he->mem_info)->ms;
}
return _hist_entry__sym_snprintf(ms, addr, he->level, bf, size, width);
}
@@ -1418,9 +1418,9 @@ sort__dso_daddr_cmp(struct hist_entry *left, struct hist_entry *right)
struct map *map_r = NULL;
if (left->mem_info)
- map_l = left->mem_info->daddr.ms.map;
+ map_l = mem_info__daddr(left->mem_info)->ms.map;
if (right->mem_info)
- map_r = right->mem_info->daddr.ms.map;
+ map_r = mem_info__daddr(right->mem_info)->ms.map;
return _sort__dso_cmp(map_l, map_r);
}
@@ -1431,7 +1431,7 @@ static int hist_entry__dso_daddr_snprintf(struct hist_entry *he, char *bf,
struct map *map = NULL;
if (he->mem_info)
- map = he->mem_info->daddr.ms.map;
+ map = mem_info__daddr(he->mem_info)->ms.map;
return _hist_entry__dso_snprintf(map, bf, size, width);
}
@@ -1443,12 +1443,12 @@ sort__locked_cmp(struct hist_entry *left, struct hist_entry *right)
union perf_mem_data_src data_src_r;
if (left->mem_info)
- data_src_l = left->mem_info->data_src;
+ data_src_l = *mem_info__data_src(left->mem_info);
else
data_src_l.mem_lock = PERF_MEM_LOCK_NA;
if (right->mem_info)
- data_src_r = right->mem_info->data_src;
+ data_src_r = *mem_info__data_src(right->mem_info);
else
data_src_r.mem_lock = PERF_MEM_LOCK_NA;
@@ -1471,12 +1471,12 @@ sort__tlb_cmp(struct hist_entry *left, struct hist_entry *right)
union perf_mem_data_src data_src_r;
if (left->mem_info)
- data_src_l = left->mem_info->data_src;
+ data_src_l = *mem_info__data_src(left->mem_info);
else
data_src_l.mem_dtlb = PERF_MEM_TLB_NA;
if (right->mem_info)
- data_src_r = right->mem_info->data_src;
+ data_src_r = *mem_info__data_src(right->mem_info);
else
data_src_r.mem_dtlb = PERF_MEM_TLB_NA;
@@ -1499,12 +1499,12 @@ sort__lvl_cmp(struct hist_entry *left, struct hist_entry *right)
union perf_mem_data_src data_src_r;
if (left->mem_info)
- data_src_l = left->mem_info->data_src;
+ data_src_l = *mem_info__data_src(left->mem_info);
else
data_src_l.mem_lvl = PERF_MEM_LVL_NA;
if (right->mem_info)
- data_src_r = right->mem_info->data_src;
+ data_src_r = *mem_info__data_src(right->mem_info);
else
data_src_r.mem_lvl = PERF_MEM_LVL_NA;
@@ -1527,12 +1527,12 @@ sort__snoop_cmp(struct hist_entry *left, struct hist_entry *right)
union perf_mem_data_src data_src_r;
if (left->mem_info)
- data_src_l = left->mem_info->data_src;
+ data_src_l = *mem_info__data_src(left->mem_info);
else
data_src_l.mem_snoop = PERF_MEM_SNOOP_NA;
if (right->mem_info)
- data_src_r = right->mem_info->data_src;
+ data_src_r = *mem_info__data_src(right->mem_info);
else
data_src_r.mem_snoop = PERF_MEM_SNOOP_NA;
@@ -1563,8 +1563,8 @@ sort__dcacheline_cmp(struct hist_entry *left, struct hist_entry *right)
if (left->cpumode > right->cpumode) return -1;
if (left->cpumode < right->cpumode) return 1;
- l_map = left->mem_info->daddr.ms.map;
- r_map = right->mem_info->daddr.ms.map;
+ l_map = mem_info__daddr(left->mem_info)->ms.map;
+ r_map = mem_info__daddr(right->mem_info)->ms.map;
/* if both are NULL, jump to sort on al_addr instead */
if (!l_map && !r_map)
@@ -1599,8 +1599,8 @@ sort__dcacheline_cmp(struct hist_entry *left, struct hist_entry *right)
addr:
/* al_addr does all the right addr - start + offset calculations */
- l = cl_address(left->mem_info->daddr.al_addr, chk_double_cl);
- r = cl_address(right->mem_info->daddr.al_addr, chk_double_cl);
+ l = cl_address(mem_info__daddr(left->mem_info)->al_addr, chk_double_cl);
+ r = cl_address(mem_info__daddr(right->mem_info)->al_addr, chk_double_cl);
if (l > r) return -1;
if (l < r) return 1;
@@ -1617,11 +1617,11 @@ static int hist_entry__dcacheline_snprintf(struct hist_entry *he, char *bf,
char level = he->level;
if (he->mem_info) {
- struct map *map = he->mem_info->daddr.ms.map;
+ struct map *map = mem_info__daddr(he->mem_info)->ms.map;
struct dso *dso = map ? map__dso(map) : NULL;
- addr = cl_address(he->mem_info->daddr.al_addr, chk_double_cl);
- ms = &he->mem_info->daddr.ms;
+ addr = cl_address(mem_info__daddr(he->mem_info)->al_addr, chk_double_cl);
+ ms = &mem_info__daddr(he->mem_info)->ms;
/* print [s] for shared data mmaps */
if ((he->cpumode != PERF_RECORD_MISC_KERNEL) &&
@@ -1806,12 +1806,12 @@ sort__blocked_cmp(struct hist_entry *left, struct hist_entry *right)
union perf_mem_data_src data_src_r;
if (left->mem_info)
- data_src_l = left->mem_info->data_src;
+ data_src_l = *mem_info__data_src(left->mem_info);
else
data_src_l.mem_blk = PERF_MEM_BLK_NA;
if (right->mem_info)
- data_src_r = right->mem_info->data_src;
+ data_src_r = *mem_info__data_src(right->mem_info);
else
data_src_r.mem_blk = PERF_MEM_BLK_NA;
@@ -1840,9 +1840,9 @@ sort__phys_daddr_cmp(struct hist_entry *left, struct hist_entry *right)
uint64_t l = 0, r = 0;
if (left->mem_info)
- l = left->mem_info->daddr.phys_addr;
+ l = mem_info__daddr(left->mem_info)->phys_addr;
if (right->mem_info)
- r = right->mem_info->daddr.phys_addr;
+ r = mem_info__daddr(right->mem_info)->phys_addr;
return (int64_t)(r - l);
}
@@ -1854,7 +1854,7 @@ static int hist_entry__phys_daddr_snprintf(struct hist_entry *he, char *bf,
size_t ret = 0;
size_t len = BITS_PER_LONG / 4;
- addr = he->mem_info->daddr.phys_addr;
+ addr = mem_info__daddr(he->mem_info)->phys_addr;
ret += repsep_snprintf(bf + ret, size - ret, "[%c] ", he->level);
@@ -1881,9 +1881,9 @@ sort__data_page_size_cmp(struct hist_entry *left, struct hist_entry *right)
uint64_t l = 0, r = 0;
if (left->mem_info)
- l = left->mem_info->daddr.data_page_size;
+ l = mem_info__daddr(left->mem_info)->data_page_size;
if (right->mem_info)
- r = right->mem_info->daddr.data_page_size;
+ r = mem_info__daddr(right->mem_info)->data_page_size;
return (int64_t)(r - l);
}
@@ -1894,7 +1894,7 @@ static int hist_entry__data_page_size_snprintf(struct hist_entry *he, char *bf,
char str[PAGE_SIZE_NAME_LEN];
return repsep_snprintf(bf, size, "%-*s", width,
- get_page_size_name(he->mem_info->daddr.data_page_size, str));
+ get_page_size_name(mem_info__daddr(he->mem_info)->data_page_size, str));
}
struct sort_entry sort_mem_data_page_size = {
--
2.45.0.rc1.225.g2a3ae87e7f-goog
On Tue, May 07, 2024 at 11:35:38AM -0700, Ian Rogers wrote:
> ui_browser__show is capturing the input title that is stack allocated
> memory in hist_browser__run. Avoid a use after return by strdup-ing
> the string.
But everything happens in that context, i.e. hist_brower__run() will
call ui_browser__ methods and then exit.
We end up having browser->title pointing to returned stack memory
(invalid) but there will be no references to it, no?
If we return to hist_browser__run() we then call ui_browser__show
passing a new title, for "live" stack memory, rinse repeat. Or have you
noticed an actual use-after-"free"?
- Arnaldo
> Fixes: 05e8b0804ec4 ("perf ui browser: Stop using 'self'")
> Signed-off-by: Ian Rogers <[email protected]>
> ---
> tools/perf/ui/browser.c | 4 +++-
> tools/perf/ui/browser.h | 2 +-
> 2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/tools/perf/ui/browser.c b/tools/perf/ui/browser.c
> index 603d11283cbd..c4cdf2ea69b7 100644
> --- a/tools/perf/ui/browser.c
> +++ b/tools/perf/ui/browser.c
> @@ -287,7 +287,8 @@ int ui_browser__show(struct ui_browser *browser, const char *title,
> mutex_lock(&ui__lock);
> __ui_browser__show_title(browser, title);
>
> - browser->title = title;
> + free(browser->title);
> + browser->title = strdup(title);
> zfree(&browser->helpline);
>
> va_start(ap, helpline);
> @@ -304,6 +305,7 @@ void ui_browser__hide(struct ui_browser *browser)
> mutex_lock(&ui__lock);
> ui_helpline__pop();
> zfree(&browser->helpline);
> + zfree(&browser->title);
> mutex_unlock(&ui__lock);
> }
>
> diff --git a/tools/perf/ui/browser.h b/tools/perf/ui/browser.h
> index 510ce4554050..6e98d5f8f71c 100644
> --- a/tools/perf/ui/browser.h
> +++ b/tools/perf/ui/browser.h
> @@ -21,7 +21,7 @@ struct ui_browser {
> u8 extra_title_lines;
> int current_color;
> void *priv;
> - const char *title;
> + char *title;
> char *helpline;
> const char *no_samples_msg;
> void (*refresh_dimensions)(struct ui_browser *browser);
> --
> 2.45.0.rc1.225.g2a3ae87e7f-goog
On Tue, May 07, 2024 at 05:20:59PM -0300, Arnaldo Carvalho de Melo wrote:
> On Tue, May 07, 2024 at 11:35:38AM -0700, Ian Rogers wrote:
> > ui_browser__show is capturing the input title that is stack allocated
> > memory in hist_browser__run. Avoid a use after return by strdup-ing
> > the string.
>
> But everything happens in that context, i.e. hist_brower__run() will
> call ui_browser__ methods and then exit.
>
> We end up having browser->title pointing to returned stack memory
> (invalid) but there will be no references to it, no?
>
> If we return to hist_browser__run() we then call ui_browser__show
> passing a new title, for "live" stack memory, rinse repeat. Or have you
> noticed an actual use-after-"free"?
And I'll take the patch, I'm just trying to figure it out if it fixed a
real bug or if it just makes the code more future proof, i.e. to avoid
us adding code that actually uses invalid stack memory.
- Arnaldo
> - Arnaldo
>
> > Fixes: 05e8b0804ec4 ("perf ui browser: Stop using 'self'")
> > Signed-off-by: Ian Rogers <[email protected]>
> > ---
> > tools/perf/ui/browser.c | 4 +++-
> > tools/perf/ui/browser.h | 2 +-
> > 2 files changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/perf/ui/browser.c b/tools/perf/ui/browser.c
> > index 603d11283cbd..c4cdf2ea69b7 100644
> > --- a/tools/perf/ui/browser.c
> > +++ b/tools/perf/ui/browser.c
> > @@ -287,7 +287,8 @@ int ui_browser__show(struct ui_browser *browser, const char *title,
> > mutex_lock(&ui__lock);
> > __ui_browser__show_title(browser, title);
> >
> > - browser->title = title;
> > + free(browser->title);
> > + browser->title = strdup(title);
> > zfree(&browser->helpline);
> >
> > va_start(ap, helpline);
> > @@ -304,6 +305,7 @@ void ui_browser__hide(struct ui_browser *browser)
> > mutex_lock(&ui__lock);
> > ui_helpline__pop();
> > zfree(&browser->helpline);
> > + zfree(&browser->title);
> > mutex_unlock(&ui__lock);
> > }
> >
> > diff --git a/tools/perf/ui/browser.h b/tools/perf/ui/browser.h
> > index 510ce4554050..6e98d5f8f71c 100644
> > --- a/tools/perf/ui/browser.h
> > +++ b/tools/perf/ui/browser.h
> > @@ -21,7 +21,7 @@ struct ui_browser {
> > u8 extra_title_lines;
> > int current_color;
> > void *priv;
> > - const char *title;
> > + char *title;
> > char *helpline;
> > const char *no_samples_msg;
> > void (*refresh_dimensions)(struct ui_browser *browser);
> > --
> > 2.45.0.rc1.225.g2a3ae87e7f-goog
On Tue, May 07, 2024 at 11:35:37AM -0700, Ian Rogers wrote:
> Remove unnecessary reference counts for structs with no gets. Add
> reference count checking to comm_str and mem_info. Fix memory leaks
> and errors detected on "perf mem report" by address sanitizer and leak
> sanitizer.
Applied locally, doing build tests. Will soon go to tmp.perf-tools-next.
- Arnaldo
> Ian Rogers (8):
> perf ui browser: Don't save pointer to stack memory
> perf annotate: Fix memory leak in annotated_source
> perf block-info: Remove unused refcount
> perf cpumap: Remove refcnt from cpu_aggr_map
> perf comm: Add reference count checking to comm_str
> perf mem-info: Move mem-info out of mem-events and symbol
> perf mem-info: Add reference count checking
> perf hist: Avoid hist_entry_iter mem_info memory leak
>
> tools/perf/builtin-c2c.c | 13 +-
> tools/perf/builtin-report.c | 3 +-
> tools/perf/builtin-script.c | 12 +-
> tools/perf/builtin-stat.c | 16 +-
> tools/perf/tests/mem.c | 11 +-
> tools/perf/ui/browser.c | 4 +-
> tools/perf/ui/browser.h | 2 +-
> tools/perf/util/Build | 1 +
> tools/perf/util/annotate.c | 6 +
> tools/perf/util/block-info.c | 22 +-
> tools/perf/util/block-info.h | 15 +-
> tools/perf/util/comm.c | 196 +++++++++++-------
> tools/perf/util/cpumap.c | 2 -
> tools/perf/util/cpumap.h | 2 -
> tools/perf/util/hist.c | 62 +++---
> tools/perf/util/hist.h | 8 +-
> tools/perf/util/machine.c | 7 +-
> tools/perf/util/mem-events.c | 36 ++--
> tools/perf/util/mem-events.h | 29 +--
> tools/perf/util/mem-info.c | 35 ++++
> tools/perf/util/mem-info.h | 54 +++++
> .../scripting-engines/trace-event-python.c | 12 +-
> tools/perf/util/sort.c | 69 +++---
> tools/perf/util/symbol.c | 26 +--
> tools/perf/util/symbol.h | 12 --
> 25 files changed, 370 insertions(+), 285 deletions(-)
> create mode 100644 tools/perf/util/mem-info.c
> create mode 100644 tools/perf/util/mem-info.h
>
> --
> 2.45.0.rc1.225.g2a3ae87e7f-goog
On Tue, May 07, 2024 at 01:48:28PM -0700, Ian Rogers wrote:
> On Tue, May 7, 2024 at 1:22 PM Arnaldo Carvalho de Melo <[email protected]> wrote:
> >
> > On Tue, May 07, 2024 at 05:20:59PM -0300, Arnaldo Carvalho de Melo wrote:
> > > On Tue, May 07, 2024 at 11:35:38AM -0700, Ian Rogers wrote:
> > > > ui_browser__show is capturing the input title that is stack allocated
> > > > memory in hist_browser__run. Avoid a use after return by strdup-ing
> > > > the string.
> > >
> > > But everything happens in that context, i.e. hist_brower__run() will
> > > call ui_browser__ methods and then exit.
> > >
> > > We end up having browser->title pointing to returned stack memory
> > > (invalid) but there will be no references to it, no?
> > >
> > > If we return to hist_browser__run() we then call ui_browser__show
> > > passing a new title, for "live" stack memory, rinse repeat. Or have you
> > > noticed an actual use-after-"free"?
> >
> > And I'll take the patch, I'm just trying to figure it out if it fixed a
> > real bug or if it just makes the code more future proof, i.e. to avoid
> > us adding code that actually uses invalid stack memory.
>
> My command line using tui is:
> $ sudo bash -c 'rm /tmp/asan.log*; export
> ASAN_OPTIONS="log_path=/tmp/asan.log"; /tmp/perf/perf mem record -a
> sleep 1; /tmp/perf/perf mem report'
> I then go to the perf annotate view and quit. This triggers the asan
> error (from the log file):
> ```
Thanks, it is indeed a bug, I'll keep that Fixes tag, people interested
in the full details can hopefully find this message going from the Link:
tag.
- Arnaldo
> ==1254591==ERROR: AddressSanitizer: stack-use-after-return on address
> 0x7f2813331920 at pc 0x7f28180
> 65991 bp 0x7fff0a21c750 sp 0x7fff0a21bf10
> READ of size 80 at 0x7f2813331920 thread T0
> #0 0x7f2818065990 in __interceptor_strlen
> ../../../../src/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:461
> #1 0x7f2817698251 in SLsmg_write_wrapped_string
> (/lib/x86_64-linux-gnu/libslang.so.2+0x98251)
> #2 0x7f28176984b9 in SLsmg_write_nstring
> (/lib/x86_64-linux-gnu/libslang.so.2+0x984b9)
> #3 0x55c94045b365 in ui_browser__write_nstring ui/browser.c:60
> #4 0x55c94045c558 in __ui_browser__show_title ui/browser.c:266
> #5 0x55c94045c776 in ui_browser__show ui/browser.c:288
> #6 0x55c94045c06d in ui_browser__handle_resize ui/browser.c:206
> #7 0x55c94047979b in do_annotate ui/browsers/hists.c:2458
> #8 0x55c94047fb17 in evsel__hists_browse ui/browsers/hists.c:3412
> #9 0x55c940480a0c in perf_evsel_menu__run ui/browsers/hists.c:3527
> #10 0x55c940481108 in __evlist__tui_browse_hists ui/browsers/hists.c:3613
> #11 0x55c9404813f7 in evlist__tui_browse_hists ui/browsers/hists.c:3661
> #12 0x55c93ffa253f in report__browse_hists tools/perf/builtin-report.c:671
> #13 0x55c93ffa58ca in __cmd_report tools/perf/builtin-report.c:1141
> #14 0x55c93ffaf159 in cmd_report tools/perf/builtin-report.c:1805
> #15 0x55c94000c05c in report_events tools/perf/builtin-mem.c:374
> #16 0x55c94000d96d in cmd_mem tools/perf/builtin-mem.c:516
> #17 0x55c9400e44ee in run_builtin tools/perf/perf.c:350
> #18 0x55c9400e4a5a in handle_internal_command tools/perf/perf.c:403
> #19 0x55c9400e4e22 in run_argv tools/perf/perf.c:447
> #20 0x55c9400e53ad in main tools/perf/perf.c:561
> #21 0x7f28170456c9 in __libc_start_call_main
> ../sysdeps/nptl/libc_start_call_main.h:58
> #22 0x7f2817045784 in __libc_start_main_impl ../csu/libc-start.c:360
> #23 0x55c93ff544c0 in _start (/tmp/perf/perf+0x19a4c0) (BuildId:
> 84899b0e8c7d3a3eaa67b2eb35e3d8b2f8cd4c93)
>
> Address 0x7f2813331920 is located in stack of thread T0 at offset 32 in frame
> #0 0x55c94046e85e in hist_browser__run ui/browsers/hists.c:746
>
> This frame has 1 object(s):
> [32, 192) 'title' (line 747) <== Memory access at offset 32 is
> inside this variable
> HINT: this may be a false positive if your program uses some custom
> stack unwind mechanism, swapcontext or vfork
> ```
> hist_browser__run isn't on the stack so the asan error looks legit.
> There's no clean init/exit on struct ui_browser so I may be trading a
> use-after-return for a memory leak, but that seems look a good trade
> anyway.
>
> Thanks,
> Ian
>
> > - Arnaldo
> >
> > > - Arnaldo
> > >
> > > > Fixes: 05e8b0804ec4 ("perf ui browser: Stop using 'self'")
> > > > Signed-off-by: Ian Rogers <[email protected]>
> > > > ---
> > > > tools/perf/ui/browser.c | 4 +++-
> > > > tools/perf/ui/browser.h | 2 +-
> > > > 2 files changed, 4 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/tools/perf/ui/browser.c b/tools/perf/ui/browser.c
> > > > index 603d11283cbd..c4cdf2ea69b7 100644
> > > > --- a/tools/perf/ui/browser.c
> > > > +++ b/tools/perf/ui/browser.c
> > > > @@ -287,7 +287,8 @@ int ui_browser__show(struct ui_browser *browser, const char *title,
> > > > mutex_lock(&ui__lock);
> > > > __ui_browser__show_title(browser, title);
> > > >
> > > > - browser->title = title;
> > > > + free(browser->title);
> > > > + browser->title = strdup(title);
> > > > zfree(&browser->helpline);
> > > >
> > > > va_start(ap, helpline);
> > > > @@ -304,6 +305,7 @@ void ui_browser__hide(struct ui_browser *browser)
> > > > mutex_lock(&ui__lock);
> > > > ui_helpline__pop();
> > > > zfree(&browser->helpline);
> > > > + zfree(&browser->title);
> > > > mutex_unlock(&ui__lock);
> > > > }
> > > >
> > > > diff --git a/tools/perf/ui/browser.h b/tools/perf/ui/browser.h
> > > > index 510ce4554050..6e98d5f8f71c 100644
> > > > --- a/tools/perf/ui/browser.h
> > > > +++ b/tools/perf/ui/browser.h
> > > > @@ -21,7 +21,7 @@ struct ui_browser {
> > > > u8 extra_title_lines;
> > > > int current_color;
> > > > void *priv;
> > > > - const char *title;
> > > > + char *title;
> > > > char *helpline;
> > > > const char *no_samples_msg;
> > > > void (*refresh_dimensions)(struct ui_browser *browser);
> > > > --
> > > > 2.45.0.rc1.225.g2a3ae87e7f-goog
On Tue, May 07, 2024 at 06:04:43PM -0300, Arnaldo Carvalho de Melo wrote:
> On Tue, May 07, 2024 at 01:48:28PM -0700, Ian Rogers wrote:
> > On Tue, May 7, 2024 at 1:22 PM Arnaldo Carvalho de Melo <[email protected]> wrote:
> > >
> > > On Tue, May 07, 2024 at 05:20:59PM -0300, Arnaldo Carvalho de Melo wrote:
> > > > On Tue, May 07, 2024 at 11:35:38AM -0700, Ian Rogers wrote:
> > > > > ui_browser__show is capturing the input title that is stack allocated
> > > > > memory in hist_browser__run. Avoid a use after return by strdup-ing
> > > > > the string.
> > > >
> > > > But everything happens in that context, i.e. hist_brower__run() will
> > > > call ui_browser__ methods and then exit.
> > > >
> > > > We end up having browser->title pointing to returned stack memory
> > > > (invalid) but there will be no references to it, no?
> > > >
> > > > If we return to hist_browser__run() we then call ui_browser__show
> > > > passing a new title, for "live" stack memory, rinse repeat. Or have you
> > > > noticed an actual use-after-"free"?
> > >
> > > And I'll take the patch, I'm just trying to figure it out if it fixed a
> > > real bug or if it just makes the code more future proof, i.e. to avoid
> > > us adding code that actually uses invalid stack memory.
> >
> > My command line using tui is:
> > $ sudo bash -c 'rm /tmp/asan.log*; export
> > ASAN_OPTIONS="log_path=/tmp/asan.log"; /tmp/perf/perf mem record -a
> > sleep 1; /tmp/perf/perf mem report'
> > I then go to the perf annotate view and quit. This triggers the asan
> > error (from the log file):
> > ```
>
> Thanks, it is indeed a bug, I'll keep that Fixes tag, people interested
> in the full details can hopefully find this message going from the Link:
> tag.
Nah, I added your explanation to the cset log message.
Thanks,
- Arnaldo
On Tue, May 7, 2024 at 2:07 PM Arnaldo Carvalho de Melo <[email protected]> wrote:
>
> On Tue, May 07, 2024 at 06:04:43PM -0300, Arnaldo Carvalho de Melo wrote:
> > On Tue, May 07, 2024 at 01:48:28PM -0700, Ian Rogers wrote:
> > > On Tue, May 7, 2024 at 1:22 PM Arnaldo Carvalho de Melo <[email protected]> wrote:
> > > >
> > > > On Tue, May 07, 2024 at 05:20:59PM -0300, Arnaldo Carvalho de Melo wrote:
> > > > > On Tue, May 07, 2024 at 11:35:38AM -0700, Ian Rogers wrote:
> > > > > > ui_browser__show is capturing the input title that is stack allocated
> > > > > > memory in hist_browser__run. Avoid a use after return by strdup-ing
> > > > > > the string.
> > > > >
> > > > > But everything happens in that context, i.e. hist_brower__run() will
> > > > > call ui_browser__ methods and then exit.
> > > > >
> > > > > We end up having browser->title pointing to returned stack memory
> > > > > (invalid) but there will be no references to it, no?
> > > > >
> > > > > If we return to hist_browser__run() we then call ui_browser__show
> > > > > passing a new title, for "live" stack memory, rinse repeat. Or have you
> > > > > noticed an actual use-after-"free"?
> > > >
> > > > And I'll take the patch, I'm just trying to figure it out if it fixed a
> > > > real bug or if it just makes the code more future proof, i.e. to avoid
> > > > us adding code that actually uses invalid stack memory.
> > >
> > > My command line using tui is:
> > > $ sudo bash -c 'rm /tmp/asan.log*; export
> > > ASAN_OPTIONS="log_path=/tmp/asan.log"; /tmp/perf/perf mem record -a
> > > sleep 1; /tmp/perf/perf mem report'
> > > I then go to the perf annotate view and quit. This triggers the asan
> > > error (from the log file):
> > > ```
> >
> > Thanks, it is indeed a bug, I'll keep that Fixes tag, people interested
> > in the full details can hopefully find this message going from the Link:
> > tag.
>
> Nah, I added your explanation to the cset log message.
Okay, I found I needed this to avoid a segv introduced by this patch:
```
diff --git a/tools/perf/ui/browser.c b/tools/perf/ui/browser.c
index c4cdf2ea69b7..19503e838738 100644
--- a/tools/perf/ui/browser.c
+++ b/tools/perf/ui/browser.c
@@ -203,7 +203,7 @@ void ui_browser__refresh_dimensions(struct
ui_browser *browser)
void ui_browser__handle_resize(struct ui_browser *browser)
{
ui__refresh_dimensions(false);
- ui_browser__show(browser, browser->title, ui_helpline__current);
+ ui_browser__show(browser, browser->title ?: "", ui_helpline__current);
ui_browser__refresh(browser);
}
```
I also found a use-after-free issue with patch 5. I'll send a v2.
Thanks,
Ian
> Thanks,
>
> - Arnaldo
On Tue, May 7, 2024 at 6:07 PM Arnaldo Carvalho de Melo
<[email protected]> wrote:
>
>
>
> On Tue, May 7, 2024, 9:51 PM Ian Rogers <[email protected]> wrote:
>>
>> On Tue, May 7, 2024 at 2:07 PM Arnaldo Carvalho de Melo <[email protected]> wrote:
>> >
>> > On Tue, May 07, 2024 at 06:04:43PM -0300, Arnaldo Carvalho de Melo wrote:
>> > > On Tue, May 07, 2024 at 01:48:28PM -0700, Ian Rogers wrote:
>> > > > On Tue, May 7, 2024 at 1:22 PM Arnaldo Carvalho de Melo <[email protected]> wrote:
>> > > > >
>> > > > > On Tue, May 07, 2024 at 05:20:59PM -0300, Arnaldo Carvalho de Melo wrote:
>> > > > > > On Tue, May 07, 2024 at 11:35:38AM -0700, Ian Rogers wrote:
>> > > > > > > ui_browser__show is capturing the input title that is stack allocated
>> > > > > > > memory in hist_browser__run. Avoid a use after return by strdup-ing
>> > > > > > > the string.
>> > > > > >
>> > > > > > But everything happens in that context, i.e. hist_brower__run() will
>> > > > > > call ui_browser__ methods and then exit.
>> > > > > >
>> > > > > > We end up having browser->title pointing to returned stack memory
>> > > > > > (invalid) but there will be no references to it, no?
>> > > > > >
>> > > > > > If we return to hist_browser__run() we then call ui_browser__show
>> > > > > > passing a new title, for "live" stack memory, rinse repeat. Or have you
>> > > > > > noticed an actual use-after-"free"?
>> > > > >
>> > > > > And I'll take the patch, I'm just trying to figure it out if it fixed a
>> > > > > real bug or if it just makes the code more future proof, i.e. to avoid
>> > > > > us adding code that actually uses invalid stack memory.
>> > > >
>> > > > My command line using tui is:
>> > > > $ sudo bash -c 'rm /tmp/asan.log*; export
>> > > > ASAN_OPTIONS="log_path=/tmp/asan.log"; /tmp/perf/perf mem record -a
>> > > > sleep 1; /tmp/perf/perf mem report'
>> > > > I then go to the perf annotate view and quit. This triggers the asan
>> > > > error (from the log file):
>> > > > ```
>> > >
>> > > Thanks, it is indeed a bug, I'll keep that Fixes tag, people interested
>> > > in the full details can hopefully find this message going from the Link:
>> > > tag.
>> >
>> > Nah, I added your explanation to the cset log message.
>>
>>
>> Okay, I found I needed this to avoid a segv introduced by this patch:
>> ```
>> diff --git a/tools/perf/ui/browser.c b/tools/perf/ui/browser.c
>> index c4cdf2ea69b7..19503e838738 100644
>> --- a/tools/perf/ui/browser.c
>> +++ b/tools/perf/ui/browser.c
>> @@ -203,7 +203,7 @@ void ui_browser__refresh_dimensions(struct
>> ui_browser *browser)
>> void ui_browser__handle_resize(struct ui_browser *browser)
>> {
>> ui__refresh_dimensions(false);
>> - ui_browser__show(browser, browser->title, ui_helpline__current);
>> + ui_browser__show(browser, browser->title ?: "", ui_helpline__current);
>> ui_browser__refresh(browser);
>> }
>> ```
>> I also found a use-after-free issue with patch 5. I'll send a v2.
>>
>> Please send a fix, it's already in perf-tools-next.
>
Okay. It looks like you accidentally pushed tmp.perf-tools.next, that
is a .next rather than a -next (dot not dash). I'll work from
perf-tools-next.
Thanks,
Ian
> Thanks,
>
> - Arnaldo
>
>> Thanks,
>> Ian
>>
>> > Thanks,
>> >
>> > - Arnaldo