Received: by 2002:a05:6a10:c7c6:0:0:0:0 with SMTP id h6csp1301475pxy; Sun, 1 Aug 2021 20:18:24 -0700 (PDT) X-Google-Smtp-Source: ABdhPJwJw7ODFfAjdblq1zeJRZuXEFywAhtHZB0MAQGJ0iS5nKoQS2jSRzaW8NQIfTQojWhEtEZX X-Received: by 2002:a50:f61b:: with SMTP id c27mr16480261edn.96.1627874303973; Sun, 01 Aug 2021 20:18:23 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1627874303; cv=none; d=google.com; s=arc-20160816; b=A7QUVm4ZmC0kFezh2sBEhlm0MqfBS33KvrLi1Juu5t7ne5sa6aqPM8gDAyuAAionbc JMqV5kfGzTBR5XeF0NbGZvDFcfU/TAWWkJ1uSIjgtQEqqGYM+1JmA7BZBF+Arpdn+gqm pOerrhg7okf0qvaN54DaDr3C3XTj3s49RNUDwi7boStcE6zZg3VRYp6ZwdwEGrzzqJGP vOf/nbzvrUHvaXk+eD5ZyVv2qC/d3+eDkcj8Vu/+0WANT2GL5m0D4NqS3tbb1PmdSpVN ksSDzChsInFA8U4Tb+AsYBrn0Nyy/QenM7Siy4eeXzppqx5dMRjQT9DEuT6AlwaV3Qan cKTQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:content-language :in-reply-to:mime-version:user-agent:date:message-id:from:references :cc:to:subject; bh=JkDBOkAP8DVGKEmGxixaZDWqZ+DGsUGxEqA79q/YSdI=; b=QK+j8JCQ5yeDA9fn4q0xm3EodOUFvwLizMh0Wv8Y8MQkOKQhzvJ9MvWA+nrRRl1SLR fQKh7XgaVXYVat7qrdItmXWDzdWJP8DSe5sRl08NjOQ97Ceo6RSb8f/G/+RJ0doPcCvU hxQvL2aoV2k8Xa6zWwtu1Esbv8ZP7tGdEKbDNCWxQ9P3Z2MPefLWGdhvYVWo7pvMAyet AClFHN97virclvOisVAwXyvtmzpxs1Y9miWTqA7PHAiAadg3WHJjsgzrr//4nw/rM8gr ZJWhj57MkQXn2aYUQlk+pxPy1fjyTDmhRR/iII2tIHzBoNdHNAMG/GQzDT/+hLAtEeM1 H6tQ== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=intel.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id z5si7871942edb.235.2021.08.01.20.17.48; Sun, 01 Aug 2021 20:18:23 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=intel.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230341AbhHBDQA (ORCPT + 99 others); Sun, 1 Aug 2021 23:16:00 -0400 Received: from mga02.intel.com ([134.134.136.20]:13653 "EHLO mga02.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231361AbhHBDP4 (ORCPT ); Sun, 1 Aug 2021 23:15:56 -0400 X-IronPort-AV: E=McAfee;i="6200,9189,10063"; a="200548069" X-IronPort-AV: E=Sophos;i="5.84,287,1620716400"; d="scan'208";a="200548069" Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by orsmga101.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 01 Aug 2021 20:15:46 -0700 X-IronPort-AV: E=Sophos;i="5.84,287,1620716400"; d="scan'208";a="509810527" Received: from yjin15-mobl1.ccr.corp.intel.com (HELO [10.238.4.147]) ([10.238.4.147]) by fmsmga003-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 01 Aug 2021 20:15:43 -0700 Subject: Re: [PATCH v3 1/2] perf pmu: Add PMU alias support To: Riccardo Mancini Cc: acme@kernel.org, jolsa@kernel.org, peterz@infradead.org, mingo@redhat.com, alexander.shishkin@linux.intel.com, Linux-kernel@vger.kernel.org, linux-perf-users@vger.kernel.org, ak@linux.intel.com, kan.liang@intel.com References: <20210730070717.30997-1-yao.jin@linux.intel.com> <20210730070717.30997-2-yao.jin@linux.intel.com> <071252c1bcf72a3fddd5664ca82b05b6589957cc.camel@gmail.com> From: "Jin, Yao" Message-ID: Date: Mon, 2 Aug 2021 11:15:41 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.12.0 MIME-Version: 1.0 In-Reply-To: <071252c1bcf72a3fddd5664ca82b05b6589957cc.camel@gmail.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Riccardo, On 7/30/2021 4:28 PM, Riccardo Mancini wrote: > Hi, > > thanks for your quick revision. > > There is still one small memory issue with a variable not being freed, which I > noticed when running this patchset together with John's patchset. > > On Fri, 2021-07-30 at 15:07 +0800, Jin Yao wrote: >> From: Kan Liang >> >> A perf uncore PMU may have two PMU names, a real name and an alias. The >> alias is exported at /sys/bus/event_source/devices/uncore_*/alias. >> The perf tool should support the alias as well. >> >> Add alias_name in the struct perf_pmu to store the alias. For the PMU >> which doesn't have an alias. It's NULL. >> >> Introduce two X86 specific functions to retrieve the real name and the >> alias separately. >> >> Only go through the sysfs to retrieve the mapping between the real name >> and the alias once. The result is cached in a list, uncore_pmu_list. >> >> Nothing changed for the other ARCHs. >> >> With the patch, the perf tool can monitor the PMU with either the real >> name or the alias. >> >> Use the real name, >>  $ perf stat -e uncore_cha_2/event=1/ -x, >>    4044879584,,uncore_cha_2/event=1/,2528059205,100.00,, >> >> Use the alias, >>  $ perf stat -e uncore_type_0_2/event=1/ -x, >>    3659675336,,uncore_type_0_2/event=1/,2287306455,100.00,, >> >> Co-developed-by: Jin Yao >> Signed-off-by: Jin Yao >> Signed-off-by: Kan Liang >> --- >> v3: >>  - Use fgets to read alias string from sysfs. >>  - Resource cleanup. >> >> v2: >>  - No change. >> >>  tools/perf/arch/x86/util/pmu.c | 129 ++++++++++++++++++++++++++++++++- >>  tools/perf/util/parse-events.y |   3 +- >>  tools/perf/util/pmu.c          |  26 ++++++- >>  tools/perf/util/pmu.h          |   5 ++ >>  4 files changed, 158 insertions(+), 5 deletions(-) >> > >> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c >> index 44b90d638ad5..cc9af7942e7b 100644 >> --- a/tools/perf/util/pmu.c >> +++ b/tools/perf/util/pmu.c >> @@ -944,13 +944,28 @@ static int pmu_max_precise(const char *name) >>         return max_precise; >>  } >> >> -static struct perf_pmu *pmu_lookup(const char *name) >> +char * __weak >> +pmu_find_real_name(const char *name) >> +{ >> +       return strdup(name); >> +} >> + >> +char * __weak >> +pmu_find_alias_name(const char *name __maybe_unused) >> +{ >> +       return NULL; >> +} >> + >> +static struct perf_pmu *pmu_lookup(const char *lookup_name) >>  { >>         struct perf_pmu *pmu; >> +       char *name; >>         LIST_HEAD(format); >>         LIST_HEAD(aliases); >>         __u32 type; >> >> +       name = pmu_find_real_name(lookup_name); > > name is not freed if one of the following checks fails. > > /* > * The pmu data we store & need consists of the pmu > * type value and format definitions. Load both right > * now. > */ > if (pmu_format(name, &format)) > return NULL; > > /* > * Check the type first to avoid unnecessary work. > */ > if (pmu_type(name, &type)) > return NULL; > > if (pmu_aliases(name, &aliases)) > return NULL; > > Thanks, > Riccardo > Thanks so much for pointing this out. I also find I need to rebase the patch because it has some conflicts with John's patch. I will prepare v4 (which will be based on tmp.perf/urgent). Thanks Jin Yao > >> @@ -973,7 +988,8 @@ static struct perf_pmu *pmu_lookup(const char *name) >>                 return NULL; >> >>         pmu->cpus = pmu_cpumask(name); >> -       pmu->name = strdup(name); >> +       pmu->name = name; >> +       pmu->alias_name = pmu_find_alias_name(name); >>         pmu->type = type; >>         pmu->is_uncore = pmu_is_uncore(name); >>         if (pmu->is_uncore) >> @@ -1003,7 +1019,8 @@ static struct perf_pmu *pmu_find(const char *name) >>         struct perf_pmu *pmu; >> >>         list_for_each_entry(pmu, &pmus, list) >> -               if (!strcmp(pmu->name, name)) >> +               if (!strcmp(pmu->name, name) || >> +                   (pmu->alias_name && !strcmp(pmu->alias_name, name))) >>                         return pmu; >> >>         return NULL; >> @@ -1898,6 +1915,9 @@ bool perf_pmu__has_hybrid(void) >> >>  int perf_pmu__match(char *pattern, char *name, char *tok) >>  { >> +       if (!name) >> +               return -1; >> + >>         if (fnmatch(pattern, name, 0)) >>                 return -1; >> >> diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h >> index 926da483a141..f6ca9f6a06ef 100644 >> --- a/tools/perf/util/pmu.h >> +++ b/tools/perf/util/pmu.h >> @@ -21,6 +21,7 @@ enum { >>  #define PERF_PMU_FORMAT_BITS 64 >>  #define EVENT_SOURCE_DEVICE_PATH "/bus/event_source/devices/" >>  #define CPUS_TEMPLATE_CPU      "%s/bus/event_source/devices/%s/cpus" >> +#define MAX_PMU_NAME_LEN 128 >> >>  struct perf_event_attr; >> >> @@ -32,6 +33,7 @@ struct perf_pmu_caps { >> >>  struct perf_pmu { >>         char *name; >> +       char *alias_name;       /* PMU alias name */ >>         char *id; >>         __u32 type; >>         bool selectable; >> @@ -135,4 +137,7 @@ void perf_pmu__warn_invalid_config(struct perf_pmu *pmu, >> __u64 config, >>  bool perf_pmu__has_hybrid(void); >>  int perf_pmu__match(char *pattern, char *name, char *tok); >> >> +char *pmu_find_real_name(const char *name); >> +char *pmu_find_alias_name(const char *name); >> + >>  #endif /* __PMU_H */ > >