From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751654AbcGRRNL (ORCPT ); Mon, 18 Jul 2016 13:13:11 -0400 Received: from foss.arm.com ([217.140.101.70]:59557 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751232AbcGRRNJ (ORCPT ); Mon, 18 Jul 2016 13:13:09 -0400 Date: Mon, 18 Jul 2016 18:13:03 +0100 From: Mark Rutland To: Suzuki K Poulose Cc: linux-kernel@vger.kernel.org, acme@kernel.org, adrian.hunter@intel.com, alexander.shishkin@linux.intel.com, hekuang@huawei.com, jolsa@kernel.org, kan.liang@intel.com, mingo@redhat.com, peterz@infradead.org, wangnan0@huawei.com Subject: Re: [RFCv2 4/4] perf: util: support sysfs supported_cpumask file Message-ID: <20160718171303.GI10069@leverpostej> References: <1468577293-19667-1-git-send-email-mark.rutland@arm.com> <1468577293-19667-5-git-send-email-mark.rutland@arm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Jul 18, 2016 at 05:38:16PM +0100, Suzuki K Poulose wrote: > On 15/07/16 11:08, Mark Rutland wrote: > >For system PMUs, the perf tools have long expected a cpumask file under > >sysfs, describing the single CPU which they support events being > >opened/handled on. Prior patches in this series have reworked this > >support to support multiple CPUs in a mask, as is required to handle > >heterogeneous CPU PMUs. > > > >Unfortunately, adding a cpumask file to CPU PMUs would break existing > >userspace. Prior to this series, perf record will refuse to open events, > >and perf stat may unexpectedly block at exit time. In the absence of a > >cpumask, perf stat is functional. > > > >To address this, this patch adds support for a new file, > >supported_cpumask, which can be used to describe heterogeneous CPUs, > >without the risk of breaking existing userspace binaries. > > > >Signed-off-by: Mark Rutland > >--- > > tools/perf/util/pmu.c | 15 ++++++++++++--- > > 1 file changed, 12 insertions(+), 3 deletions(-) > > > >diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c > >index ddb0261..06c985c 100644 > >--- a/tools/perf/util/pmu.c > >+++ b/tools/perf/util/pmu.c > >@@ -445,14 +445,23 @@ static struct cpu_map *pmu_cpumask(const char *name) > > FILE *file; > > struct cpu_map *cpus; > > const char *sysfs = sysfs__mountpoint(); > >+ const char *path_template[] = { > >+ "%s/bus/event_source/devices/%s/cpumask", > >+ "%s/bus/event_source/devices/%s/supported_cpumask", > >+ NULL > >+ }; > >+ unsigned int i; > > > > if (!sysfs) > > return NULL; > > > >- snprintf(path, PATH_MAX, > >- "%s/bus/event_source/devices/%s/cpumask", sysfs, name); > >+ for (i = 0; i < ARRAY_SIZE(path_template); i++) { > > The check could be "path_template[i]" to avoid an iteration with NULL > template. True. I'd reworked this loop a few times to try to avoid duplicated checks, but evidently I'd messed that up. I'll clean this up. > > >+ snprintf(path, PATH_MAX, *path_template, sysfs, name); > > Btw, did you mean to use path_template[i] here instead of *path_template ? > > >+ if (stat(path, &st) == 0) > >+ break; > >+ } > > > >- if (stat(path, &st) < 0) > >+ if (!*path_template) > > Same here ? Yes. On both counts I meant to use the current iteration's entry. Thanks for spotting that. I'll fix that up. Thanks, Mark.