hists__add_entry_ops() doesn't allocate a new histograms entry if it has
an existed entry for a KVM event, in this case, find_create_kvm_event()
allocates structure kvm_info but it's not used by any histograms and
never freed.
To fix the memory leak, this patch firstly introduces refcnt and a set
of functions for refcnt operations in the structure kvm_info. When the
data structure is not used anymore, it invokes kvm_info__zput() to
decrease reference count and release the structure.
Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
tools/perf/builtin-kvm.c | 3 +--
tools/perf/util/hist.c | 5 +++++
tools/perf/util/kvm-stat.h | 37 +++++++++++++++++++++++++++++++++++++
3 files changed, 43 insertions(+), 2 deletions(-)
diff --git a/tools/perf/builtin-kvm.c b/tools/perf/builtin-kvm.c
index 4c205df5106f..1e1cb5a9d0a2 100644
--- a/tools/perf/builtin-kvm.c
+++ b/tools/perf/builtin-kvm.c
@@ -768,7 +768,6 @@ static void kvm_he_free(void *he)
{
struct kvm_event *kvm_ev;
- free(((struct hist_entry *)he)->kvm_info);
kvm_ev = container_of(he, struct kvm_event, he);
free(kvm_ev);
}
@@ -788,7 +787,7 @@ static struct kvm_event *find_create_kvm_event(struct perf_kvm_stat *kvm,
BUG_ON(key->key == INVALID_KEY);
- ki = zalloc(sizeof(*ki));
+ ki = kvm_info__new();
if (!ki) {
pr_err("Failed to allocate kvm info\n");
return NULL;
diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index 3670136a0074..b296f572f881 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -628,6 +628,8 @@ static struct hist_entry *hists__findnew_entry(struct hists *hists,
block_info__zput(entry->block_info);
+ kvm_info__zput(entry->kvm_info);
+
/* If the map of an existing hist_entry has
* become out-of-date due to an exec() or
* similar, update it. Otherwise we will
@@ -1323,6 +1325,9 @@ void hist_entry__delete(struct hist_entry *he)
if (he->block_info)
block_info__zput(he->block_info);
+ if (he->kvm_info)
+ kvm_info__zput(he->kvm_info);
+
zfree(&he->res_samples);
zfree(&he->stat_acc);
free_srcline(he->srcline);
diff --git a/tools/perf/util/kvm-stat.h b/tools/perf/util/kvm-stat.h
index bc6c8e38ef50..9bf34c0e0056 100644
--- a/tools/perf/util/kvm-stat.h
+++ b/tools/perf/util/kvm-stat.h
@@ -10,6 +10,9 @@
#include "symbol.h"
#include "record.h"
+#include <stdlib.h>
+#include <linux/zalloc.h>
+
#define KVM_EVENT_NAME_LEN 40
struct evsel;
@@ -25,6 +28,7 @@ struct event_key {
struct kvm_info {
char name[KVM_EVENT_NAME_LEN];
+ refcount_t refcnt;
};
struct kvm_event_stats {
@@ -145,6 +149,39 @@ extern const char *vcpu_id_str;
extern const char *kvm_exit_reason;
extern const char *kvm_entry_trace;
extern const char *kvm_exit_trace;
+
+static inline struct kvm_info *kvm_info__get(struct kvm_info *ki)
+{
+ if (ki)
+ refcount_inc(&ki->refcnt);
+ return ki;
+}
+
+static inline void kvm_info__put(struct kvm_info *ki)
+{
+ if (ki && refcount_dec_and_test(&ki->refcnt))
+ free(ki);
+}
+
+static inline void __kvm_info__zput(struct kvm_info **ki)
+{
+ kvm_info__put(*ki);
+ *ki = NULL;
+}
+
+#define kvm_info__zput(ki) __kvm_info__zput(&ki)
+
+static inline struct kvm_info *kvm_info__new(void)
+{
+ struct kvm_info *ki;
+
+ ki = zalloc(sizeof(*ki));
+ if (ki)
+ refcount_set(&ki->refcnt, 1);
+
+ return ki;
+}
+
#endif /* HAVE_KVM_STAT_SUPPORT */
extern int kvm_add_default_arch_event(int *argc, const char **argv);
--
2.39.2Em Mon, Mar 20, 2023 at 02:16:18PM +0800, Leo Yan escreveu:
> hists__add_entry_ops() doesn't allocate a new histograms entry if it has
> an existed entry for a KVM event, in this case, find_create_kvm_event()
> allocates structure kvm_info but it's not used by any histograms and
> never freed.
>
> To fix the memory leak, this patch firstly introduces refcnt and a set
> of functions for refcnt operations in the structure kvm_info. When the
> data structure is not used anymore, it invokes kvm_info__zput() to
> decrease reference count and release the structure.
>
> Signed-off-by: Leo Yan <leo.yan@linaro.org>
> ---
> tools/perf/builtin-kvm.c | 3 +--
> tools/perf/util/hist.c | 5 +++++
> tools/perf/util/kvm-stat.h | 37 +++++++++++++++++++++++++++++++++++++
> 3 files changed, 43 insertions(+), 2 deletions(-)
>
> diff --git a/tools/perf/builtin-kvm.c b/tools/perf/builtin-kvm.c
> index 4c205df5106f..1e1cb5a9d0a2 100644
> --- a/tools/perf/builtin-kvm.c
> +++ b/tools/perf/builtin-kvm.c
> @@ -768,7 +768,6 @@ static void kvm_he_free(void *he)
> {
> struct kvm_event *kvm_ev;
>
> - free(((struct hist_entry *)he)->kvm_info);
> kvm_ev = container_of(he, struct kvm_event, he);
> free(kvm_ev);
> }
> @@ -788,7 +787,7 @@ static struct kvm_event *find_create_kvm_event(struct perf_kvm_stat *kvm,
>
> BUG_ON(key->key == INVALID_KEY);
>
> - ki = zalloc(sizeof(*ki));
> + ki = kvm_info__new();
> if (!ki) {
> pr_err("Failed to allocate kvm info\n");
> return NULL;
> diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
> index 3670136a0074..b296f572f881 100644
> --- a/tools/perf/util/hist.c
> +++ b/tools/perf/util/hist.c
> @@ -628,6 +628,8 @@ static struct hist_entry *hists__findnew_entry(struct hists *hists,
>
> block_info__zput(entry->block_info);
>
> + kvm_info__zput(entry->kvm_info);
> +
> /* If the map of an existing hist_entry has
> * become out-of-date due to an exec() or
> * similar, update it. Otherwise we will
> @@ -1323,6 +1325,9 @@ void hist_entry__delete(struct hist_entry *he)
> if (he->block_info)
> block_info__zput(he->block_info);
>
> + if (he->kvm_info)
> + kvm_info__zput(he->kvm_info);
> +
> zfree(&he->res_samples);
> zfree(&he->stat_acc);
> free_srcline(he->srcline);
> diff --git a/tools/perf/util/kvm-stat.h b/tools/perf/util/kvm-stat.h
> index bc6c8e38ef50..9bf34c0e0056 100644
> --- a/tools/perf/util/kvm-stat.h
> +++ b/tools/perf/util/kvm-stat.h
> @@ -10,6 +10,9 @@
> #include "symbol.h"
> #include "record.h"
>
> +#include <stdlib.h>
> +#include <linux/zalloc.h>
> +
> #define KVM_EVENT_NAME_LEN 40
>
> struct evsel;
> @@ -25,6 +28,7 @@ struct event_key {
>
> struct kvm_info {
> char name[KVM_EVENT_NAME_LEN];
> + refcount_t refcnt;
> };
>
> struct kvm_event_stats {
> @@ -145,6 +149,39 @@ extern const char *vcpu_id_str;
> extern const char *kvm_exit_reason;
> extern const char *kvm_entry_trace;
> extern const char *kvm_exit_trace;
> +
> +static inline struct kvm_info *kvm_info__get(struct kvm_info *ki)
> +{
> + if (ki)
> + refcount_inc(&ki->refcnt);
> + return ki;
> +}
> +
> +static inline void kvm_info__put(struct kvm_info *ki)
> +{
> + if (ki && refcount_dec_and_test(&ki->refcnt))
> + free(ki);
> +}
> +
> +static inline void __kvm_info__zput(struct kvm_info **ki)
> +{
> + kvm_info__put(*ki);
> + *ki = NULL;
> +}
> +
> +#define kvm_info__zput(ki) __kvm_info__zput(&ki)
> +
> +static inline struct kvm_info *kvm_info__new(void)
> +{
> + struct kvm_info *ki;
> +
> + ki = zalloc(sizeof(*ki));
> + if (ki)
> + refcount_set(&ki->refcnt, 1);
> +
> + return ki;
> +}
> +
> #endif /* HAVE_KVM_STAT_SUPPORT */
>
> extern int kvm_add_default_arch_event(int *argc, const char **argv);
I had to add this:
Provide a nop version of kvm_info__zput() to be used when
HAVE_KVM_STAT_SUPPORT isn't defined as it is used unconditionally in
hists__findnew_entry() and hist_entry__delete().
- Arnaldo
diff --git a/tools/perf/util/kvm-stat.h b/tools/perf/util/kvm-stat.h
index 9bf34c0e0056e390..90b854390c89708d 100644
--- a/tools/perf/util/kvm-stat.h
+++ b/tools/perf/util/kvm-stat.h
@@ -182,6 +182,9 @@ static inline struct kvm_info *kvm_info__new(void)
return ki;
}
+#else /* HAVE_KVM_STAT_SUPPORT */
+// We use this unconditionally in hists__findnew_entry() and hist_entry__delete()
+#define kvm_info__zput(ki)
#endif /* HAVE_KVM_STAT_SUPPORT */
extern int kvm_add_default_arch_event(int *argc, const char **argv);
On Tue, Mar 21, 2023 at 10:00:56AM -0300, Arnaldo Carvalho de Melo wrote:
[...]
> > +static inline void __kvm_info__zput(struct kvm_info **ki)
> > +{
> > + kvm_info__put(*ki);
> > + *ki = NULL;
> > +}
> > +
> > +#define kvm_info__zput(ki) __kvm_info__zput(&ki)
> > +
> > +static inline struct kvm_info *kvm_info__new(void)
> > +{
> > + struct kvm_info *ki;
> > +
> > + ki = zalloc(sizeof(*ki));
> > + if (ki)
> > + refcount_set(&ki->refcnt, 1);
> > +
> > + return ki;
> > +}
> > +
> > #endif /* HAVE_KVM_STAT_SUPPORT */
> >
> > extern int kvm_add_default_arch_event(int *argc, const char **argv);
>
> I had to add this:
>
> Provide a nop version of kvm_info__zput() to be used when
> HAVE_KVM_STAT_SUPPORT isn't defined as it is used unconditionally in
> hists__findnew_entry() and hist_entry__delete().
Thanks a lot, Arnaldo.
Just want to check, before I sent out this series I have run building
test with the command `make -C tools/perf build-test` and I didn't see
the building failure. Do I need to run other testing?
Thanks,
Leo
Em Tue, Mar 21, 2023 at 10:22:35PM +0800, Leo Yan escreveu:
> On Tue, Mar 21, 2023 at 10:00:56AM -0300, Arnaldo Carvalho de Melo wrote:
>
> [...]
>
> > > +static inline void __kvm_info__zput(struct kvm_info **ki)
> > > +{
> > > + kvm_info__put(*ki);
> > > + *ki = NULL;
> > > +}
> > > +
> > > +#define kvm_info__zput(ki) __kvm_info__zput(&ki)
> > > +
> > > +static inline struct kvm_info *kvm_info__new(void)
> > > +{
> > > + struct kvm_info *ki;
> > > +
> > > + ki = zalloc(sizeof(*ki));
> > > + if (ki)
> > > + refcount_set(&ki->refcnt, 1);
> > > +
> > > + return ki;
> > > +}
> > > +
> > > #endif /* HAVE_KVM_STAT_SUPPORT */
> > >
> > > extern int kvm_add_default_arch_event(int *argc, const char **argv);
> >
> > I had to add this:
> >
> > Provide a nop version of kvm_info__zput() to be used when
> > HAVE_KVM_STAT_SUPPORT isn't defined as it is used unconditionally in
> > hists__findnew_entry() and hist_entry__delete().
>
> Thanks a lot, Arnaldo.
>
> Just want to check, before I sent out this series I have run building
> test with the command `make -C tools/perf build-test` and I didn't see
> the building failure. Do I need to run other testing?
Yes, but I didn't manage yet to make them public in a way that you could
use it easily :-\
I have a set of perf build containers and in some cases
HAVE_KVM_STAT_SUPPORT isn't defined, thus I noticed the problem.
Since you're working on kvm stat, maybe you could add a way to disable
it from the make command line and then add it to tools/perf/tests/make?
Here is an example of this for something that was opt-in:
⬢[acme@toolbox perf-tools-next]$ git show 9300acc6fed8e957c8d60f6f8e4451b508feea2c
commit 9300acc6fed8e957c8d60f6f8e4451b508feea2c
Author: Arnaldo Carvalho de Melo <acme@redhat.com>
Date: Fri May 29 16:25:34 2020 -0300
perf build: Add a LIBPFM4=1 build test entry
So that when one runs:
$ make -C tools/perf build-test
We make sure that recent changes don't break that opt-in build.
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Alexey Budankov <alexey.budankov@linux.intel.com>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: Andrii Nakryiko <andriin@fb.com>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Ian Rogers <irogers@google.com>
Cc: Igor Lubashev <ilubashe@akamai.com>
Cc: Jin Yao <yao.jin@linux.intel.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Jiwei Sun <jiwei.sun@windriver.com>
Cc: John Garry <john.garry@huawei.com>
Cc: Kan Liang <kan.liang@linux.intel.com>
Cc: Leo Yan <leo.yan@linaro.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Martin KaFai Lau <kafai@fb.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Stephane Eranian <eranian@google.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Yonghong Song <yhs@fb.com>
Cc: yuzhoujian <yuzhoujian@didichuxing.com>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
diff --git a/tools/perf/tests/make b/tools/perf/tests/make
index 8fe6c7911f46e7ef..9b651dfe0a6b8a91 100644
--- a/tools/perf/tests/make
+++ b/tools/perf/tests/make
@@ -90,6 +90,7 @@ make_with_babeltrace:= LIBBABELTRACE=1
make_no_sdt := NO_SDT=1
make_no_syscall_tbl := NO_SYSCALL_TABLE=1
make_with_clangllvm := LIBCLANGLLVM=1
+make_with_libpfm4 := LIBPFM4=1
make_tags := tags
make_cscope := cscope
make_help := help
@@ -152,6 +153,7 @@ run += make_no_sdt
run += make_no_syscall_tbl
run += make_with_babeltrace
run += make_with_clangllvm
+run += make_with_libpfm4
run += make_help
run += make_doc
run += make_perf_o
⬢[acme@toolbox perf-tools-next]$
----------------------
Look at tools/perf/Makefile.perf and try to add a NO_KVM_STAT=1 perhaps,
and then add it to tools/perf/tests/make so that we catch problems like
the one I found and fixed in this thread.
Thanks,
- Arnaldo
© 2016 - 2026 Red Hat, Inc.