[PATCH 0/2] perf kvm: Fix memory leak

[PATCH 0/2] perf kvm: Fix memory leak

[Leo Yan]

This patch seris is to address memory leak issues in perf kvm.

The first patch introduces refcnt in structure kvm_info, so we can avoid
memory leak for it.

The second patch explicitly delete histograms entries before program
exiting rather than relying on kernel releasing memory space.


Leo Yan (2):
  perf kvm: Support refcnt in structure kvm_info
  perf kvm: Delete histograms entries before exiting

 tools/perf/builtin-kvm.c   |  6 ++++--
 tools/perf/util/hist.c     |  5 +++++
 tools/perf/util/kvm-stat.h | 37 +++++++++++++++++++++++++++++++++++++
 3 files changed, 46 insertions(+), 2 deletions(-)

-- 
2.39.2

[PATCH 1/2] perf kvm: Support refcnt in structure kvm_info

[Leo Yan]

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.2

Re: [PATCH 1/2] perf kvm: Support refcnt in structure kvm_info

[Arnaldo Carvalho de Melo]

Em 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);

Re: [PATCH 1/2] perf kvm: Support refcnt in structure kvm_info

[Leo Yan]

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

Re: [PATCH 1/2] perf kvm: Support refcnt in structure kvm_info

[Arnaldo Carvalho de Melo]

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

[PATCH 2/2] perf kvm: Delete histograms entries before exiting

[Leo Yan]

It's good not to release resources for a program when kernel cleans up
memory space, this patch explicitly releases histograms entries with
hists__delete_entries().

Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 tools/perf/builtin-kvm.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/perf/builtin-kvm.c b/tools/perf/builtin-kvm.c
index 1e1cb5a9d0a2..fb9dc0dc46f9 100644
--- a/tools/perf/builtin-kvm.c
+++ b/tools/perf/builtin-kvm.c
@@ -1528,6 +1528,8 @@ static int kvm_events_live_report(struct perf_kvm_stat *kvm)
 	}
 
 out:
+	hists__delete_entries(&kvm_hists.hists);
+
 	if (kvm->timerfd >= 0)
 		close(kvm->timerfd);
 
@@ -1690,6 +1692,7 @@ static int kvm_events_report_vcpu(struct perf_kvm_stat *kvm)
 	kvm_display(kvm);
 
 exit:
+	hists__delete_entries(&kvm_hists.hists);
 	return ret;
 }
 
-- 
2.39.2

Re: [PATCH 0/2] perf kvm: Fix memory leak

[Ian Rogers]

On Sun, Mar 19, 2023 at 11:16 PM Leo Yan <leo.yan@linaro.org> wrote:
>
> This patch seris is to address memory leak issues in perf kvm.
>
> The first patch introduces refcnt in structure kvm_info, so we can avoid
> memory leak for it.
>
> The second patch explicitly delete histograms entries before program
> exiting rather than relying on kernel releasing memory space.
>

Series:
Acked-by: Ian Rogers <irogers@google.com>

Thanks,
Ian

> Leo Yan (2):
>   perf kvm: Support refcnt in structure kvm_info
>   perf kvm: Delete histograms entries before exiting
>
>  tools/perf/builtin-kvm.c   |  6 ++++--
>  tools/perf/util/hist.c     |  5 +++++
>  tools/perf/util/kvm-stat.h | 37 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 46 insertions(+), 2 deletions(-)
>
> --
> 2.39.2
>

Re: [PATCH 0/2] perf kvm: Fix memory leak

[Arnaldo Carvalho de Melo]

Em Mon, Mar 20, 2023 at 10:43:11AM -0700, Ian Rogers escreveu:
> On Sun, Mar 19, 2023 at 11:16 PM Leo Yan <leo.yan@linaro.org> wrote:
> >
> > This patch seris is to address memory leak issues in perf kvm.
> >
> > The first patch introduces refcnt in structure kvm_info, so we can avoid
> > memory leak for it.
> >
> > The second patch explicitly delete histograms entries before program
> > exiting rather than relying on kernel releasing memory space.
> >
> 
> Series:
> Acked-by: Ian Rogers <irogers@google.com>

I applied this, but the second patch may end up delaying tool exit by
traversing data structures holding lots of objects to needless free
them.

There are places in perf where we do it conditionally for that reason.

At some point I want to try signalling we're exiting using some global
variable and then making all memory free operations become nops.

- Arnaldo


 
> Thanks,
> Ian
> 
> > Leo Yan (2):
> >   perf kvm: Support refcnt in structure kvm_info
> >   perf kvm: Delete histograms entries before exiting
> >
> >  tools/perf/builtin-kvm.c   |  6 ++++--
> >  tools/perf/util/hist.c     |  5 +++++
> >  tools/perf/util/kvm-stat.h | 37 +++++++++++++++++++++++++++++++++++++
> >  3 files changed, 46 insertions(+), 2 deletions(-)
> >
> > --
> > 2.39.2
> >

-- 

- Arnaldo