Date:   Tue, 21 Mar 2023 10:00:56 -0300
From:   Arnaldo Carvalho de Melo <acme@kernel.org>
To:     Leo Yan <leo.yan@linaro.org>
Cc:     Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@redhat.com>,
        Mark Rutland <mark.rutland@arm.com>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        Jiri Olsa <jolsa@kernel.org>,
        Namhyung Kim <namhyung@kernel.org>,
        Ian Rogers <irogers@google.com>,
        Adrian Hunter <adrian.hunter@intel.com>,
        James Clark <james.clark@arm.com>,
        linux-perf-users@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/2] perf kvm: Support refcnt in structure kvm_info
Message-ID: <ZBmqiKC1FSGI0/iE@kernel.org>
References: <20230320061619.29520-1-leo.yan@linaro.org>
 <20230320061619.29520-2-leo.yan@linaro.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230320061619.29520-2-leo.yan@linaro.org>
Precedence: bulk

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