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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
On Sun, Mar 19, 2023 at 11:16 PM Leo Yan <[email protected]> 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 <[email protected]>
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
>
Em Mon, Mar 20, 2023 at 10:43:11AM -0700, Ian Rogers escreveu:
> On Sun, Mar 19, 2023 at 11:16 PM Leo Yan <[email protected]> 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 <[email protected]>
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
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 <[email protected]>
> ---
> 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 <[email protected]>
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 <[email protected]>
Cc: Alexander Shishkin <[email protected]>
Cc: Alexei Starovoitov <[email protected]>
Cc: Alexey Budankov <[email protected]>
Cc: Andi Kleen <[email protected]>
Cc: Andrii Nakryiko <[email protected]>
Cc: Daniel Borkmann <[email protected]>
Cc: Florian Fainelli <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Ian Rogers <[email protected]>
Cc: Igor Lubashev <[email protected]>
Cc: Jin Yao <[email protected]>
Cc: Jiri Olsa <[email protected]>
Cc: Jiwei Sun <[email protected]>
Cc: John Garry <[email protected]>
Cc: Kan Liang <[email protected]>
Cc: Leo Yan <[email protected]>
Cc: Mark Rutland <[email protected]>
Cc: Martin KaFai Lau <[email protected]>
Cc: Namhyung Kim <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Stephane Eranian <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Yonghong Song <[email protected]>
Cc: yuzhoujian <[email protected]>
Signed-off-by: Arnaldo Carvalho de Melo <[email protected]>
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