Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230331202949.810326-1-namhyung@kernel.org>
In-Reply-To: <20230331202949.810326-1-namhyung@kernel.org>
From:   Ian Rogers <irogers@google.com>
Date:   Mon, 3 Apr 2023 10:28:28 -0700
Message-ID: <CAP-5=fWC1v+vsi-qmYXy8VMKXOsscYobucwS3GZw=gycdnAc3w@mail.gmail.com>
Subject: Re: [PATCHSET 0/9] perf tools: Update pmu scan using openat() (v1)
To:     Namhyung Kim <namhyung@kernel.org>
Cc:     Arnaldo Carvalho de Melo <acme@kernel.org>,
        Jiri Olsa <jolsa@kernel.org>,
        Adrian Hunter <adrian.hunter@intel.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@kernel.org>,
        LKML <linux-kernel@vger.kernel.org>,
        linux-perf-users@vger.kernel.org,
        Kan Liang <kan.liang@linux.intel.com>,
        Leo Yan <leo.yan@linaro.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Fri, Mar 31, 2023 at 1:29=E2=80=AFPM Namhyung Kim <namhyung@kernel.org> =
wrote:
>
> Hello,
>
> This patchset changes PMU info scanning on sysfs using openat()
> basically.  I got reports of occasional contention on the
> opening files in sysfs.  While the root cause was a separate
> issue, I discovered some inefficiencies in the perf code.
>
> To scan PMUs, it roughly does something like below:
>
>   dir =3D opendir("/sys/bus/event_source/devices");
>   while (dentry =3D readdir(dir)) {
>     char buf[PATH_MAX];
>
>     snprintf(buf, sizeof(buf), "%s/%s",
>              "/sys/bus/event_source/devices", dentry->d_name);
>     fd =3D open(buf, O_RDONLY);
>     ...
>   }
>
> But this is not good since it needs to copy the string to build the
> absolute pathname, and it makes redundant pathname walk (from the /sys)
> in the kernel unnecessarily.  We can use openat(2) to open the file in
> the given directory.
>
> Add a couple of new helper to return the file descriptor of PMU
> directory so that it can use it with relative paths.
>
>  * perf_pmu__event_source_devices_fd()
>    - returns a fd for the PMU root ("/sys/bus/event_source/devices")
>
>  * perf_pmu__pathname_fd()
>    - returns a fd for "<pmu>/<file>" under the PMU root
>
> Now the above code can be converted something like below:
>
>   dirfd =3D perf_pmu__event_source_devices_fd();
>   dir =3D fdopendir(dirfd);
>   while (dentry =3D readdir(dir)) {
>     fd =3D openat(dirfd, dentry->d_name, O_RDONLY);
>     ...
>   }
>
> I added a benchmark for pmu-scan and it showed a slight speedup
> in the normal case too.
>
>   $ ./perf.old bench internals pmu-scan
>   # Running 'internals/pmu-scan' benchmark:
>   Computing performance of sysfs PMU event scan for 100 times
>     Average PMU scanning took: 6670.970 usec (+- 13.022 usec)
>
>   $ ./perf.new bench internals pmu-scan
>   # Running 'internals/pmu-scan' benchmark:
>   Computing performance of sysfs PMU event scan for 100 times
>     Average PMU scanning took: 6296.980 usec (+- 14.891 usec)
>
> The 5~6% of improvement might be small but it may have bigger impact
> when the system is contended.
>
> You can get the code from 'perf/pmu-scan-v1' branch in
>
>   git://git.kernel.org/pub/scm/linux/kernel/git/namhyung/linux-perf.git
>
> Thanks,
> Namhyung

Some nice stack size savings too. The reentrant fix was pre-existing. Serie=
s:
Acked-by: Ian Rogers <irogers@google.com>

Thanks,
Ian

> Namhyung Kim (9):
>   perf list: Use relative path for tracepoint scan
>   perf tools: Fix a asan issue in parse_events_multi_pmu_add()
>   perf pmu: Add perf_pmu__destroy() function
>   perf bench: Add pmu-scan benchmark
>   perf pmu: Use relative path for sysfs scan
>   perf pmu: Use relative path in perf_pmu__caps_parse()
>   perf pmu: Use relative path in setup_pmu_alias_list()
>   perf pmu: Add perf_pmu__{open,scan}_file_at()
>   perf intel-pt: Use perf_pmu__scan_file_at() if possible
>
>  tools/perf/arch/x86/util/intel-pt.c |  52 ++++--
>  tools/perf/arch/x86/util/pmu.c      |  13 +-
>  tools/perf/bench/Build              |   1 +
>  tools/perf/bench/bench.h            |   1 +
>  tools/perf/bench/pmu-scan.c         | 184 ++++++++++++++++++
>  tools/perf/builtin-bench.c          |   1 +
>  tools/perf/tests/pmu.c              |   9 +-
>  tools/perf/util/parse-events.c      |   2 +-
>  tools/perf/util/pmu.c               | 278 ++++++++++++++++++++--------
>  tools/perf/util/pmu.h               |  12 +-
>  tools/perf/util/print-events.c      |  26 ++-
>  11 files changed, 466 insertions(+), 113 deletions(-)
>  create mode 100644 tools/perf/bench/pmu-scan.c
>
>
> base-commit: 417c6adfb155f906f0441cc1034827f6e2b3c372
> --
> 2.40.0.348.gf938b09366-goog
>