linux-kernel - Re: [PATCH v2 00/13] Tool and hwmon PMUs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ZvkAA8NW_5ssmsuf@google.com>
Date: Sun, 29 Sep 2024 00:21:39 -0700
From: Namhyung Kim <namhyung@...nel.org>
To: Ian Rogers <irogers@...gle.com>
Cc: Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...hat.com>,
	Arnaldo Carvalho de Melo <acme@...nel.org>,
	Mark Rutland <mark.rutland@....com>,
	Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
	Jiri Olsa <jolsa@...nel.org>,
	Adrian Hunter <adrian.hunter@...el.com>,
	Kan Liang <kan.liang@...ux.intel.com>,
	John Garry <john.g.garry@...cle.com>, Will Deacon <will@...nel.org>,
	James Clark <james.clark@...aro.org>,
	Mike Leach <mike.leach@...aro.org>, Leo Yan <leo.yan@...ux.dev>,
	Ravi Bangoria <ravi.bangoria@....com>,
	Weilin Wang <weilin.wang@...el.com>,
	Jing Zhang <renyu.zj@...ux.alibaba.com>,
	Xu Yang <xu.yang_2@....com>, Sandipan Das <sandipan.das@....com>,
	Benjamin Gray <bgray@...ux.ibm.com>,
	Athira Jajeev <atrajeev@...ux.vnet.ibm.com>,
	Howard Chu <howardchu95@...il.com>,
	Dominique Martinet <asmadeus@...ewreck.org>,
	Yang Jihong <yangjihong@...edance.com>,
	Colin Ian King <colin.i.king@...il.com>,
	Veronika Molnarova <vmolnaro@...hat.com>,
	"Dr. David Alan Gilbert" <linux@...blig.org>,
	Oliver Upton <oliver.upton@...ux.dev>,
	Changbin Du <changbin.du@...wei.com>, Ze Gao <zegao2021@...il.com>,
	Andi Kleen <ak@...ux.intel.com>,
	Clément Le Goffic <clement.legoffic@...s.st.com>,
	Sun Haiyong <sunhaiyong@...ngson.cn>,
	Junhao He <hejunhao3@...wei.com>,
	Tiezhu Yang <yangtiezhu@...ngson.cn>,
	Yicong Yang <yangyicong@...ilicon.com>,
	linux-kernel@...r.kernel.org, linux-perf-users@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v2 00/13] Tool and hwmon PMUs

On Fri, Sep 27, 2024 at 11:09:49AM -0700, Ian Rogers wrote:
> On Fri, Sep 27, 2024 at 10:22 AM Namhyung Kim <namhyung@...nel.org> wrote:
> >
> > On Thu, Sep 26, 2024 at 12:47:26PM -0700, Ian Rogers wrote:
> > > On Fri, Sep 13, 2024 at 7:34 AM Ian Rogers <irogers@...gle.com> wrote:
> > > >
> > > > On Thu, Sep 12, 2024 at 3:50 PM Namhyung Kim <namhyung@...nel.org> wrote:
> > > > >
> > > > > On Thu, Sep 12, 2024 at 12:03:27PM -0700, Ian Rogers wrote:
> > > > > > Rather than have fake and tool PMUs being special flags in an evsel,
> > > > > > create special PMUs. This allows, for example, duration_time to also
> > > > > > be tool/duration_time/. Once adding events to the tools PMU is just
> > > > > > adding to an array, add events for nearly all the expr literals like
> > > > > > num_cpus_online. Rather than create custom logic for finding and
> > > > > > describing the tool events use json and add a notion of common json
> > > > > > for the tool events.
> > > > > >
> > > > > > Following the convention of the tool PMU, create a hwmon PMU that
> > > > > > exposes hwmon data for reading. For example, the following shows
> > > > > > reading the CPU temperature and 2 fan speeds alongside the uncore
> > > > > > frequency:
> > > > > > ```
> > > > > > $ perf stat -e temp_cpu,fan1,hwmon_thinkpad/fan2/,tool/num_cpus_online/ -M UNCORE_FREQ -I 1000
> > > > > >      1.001153138              52.00 'C   temp_cpu
> > > > > >      1.001153138              2,588 rpm  fan1
> > > > > >      1.001153138              2,482 rpm  hwmon_thinkpad/fan2/
> > > > > >      1.001153138                  8      tool/num_cpus_online/
> > > > > >      1.001153138      1,077,101,397      UNC_CLOCK.SOCKET                 #     1.08 UNCORE_FREQ
> > > > > >      1.001153138      1,012,773,595      duration_time
> > > > > > ...
> > > > > > ```
> > > > > >
> > > > > > Additional data on the hwmon events is in perf list:
> > > > > > ```
> > > > > > $ perf list
> > > > > > ...
> > > > > > hwmon:
> > > > > > ...
> > > > > >   temp_core_0 OR temp2
> > > > > >        [Temperature in unit coretemp named Core 0. crit=100'C,max=100'C crit_alarm=0'C. Unit:
> > > > > >         hwmon_coretemp]
> > > > > > ...
> > > > > > ```
> > > > > >
> > > > > > v2: Address Namhyung's review feedback. Rebase dropping 4 patches
> > > > > >     applied by Arnaldo, fix build breakage reported by Arnaldo.
> > > > > >
> > > > > > Ian Rogers (13):
> > > > > >   perf pmu: Simplify an asprintf error message
> > > > > >   perf pmu: Allow hardcoded terms to be applied to attributes
> > > > > >   perf parse-events: Expose/rename config_term_name
> > > > > >   perf tool_pmu: Factor tool events into their own PMU
> > > > > >   perf tool_pmu: Rename enum perf_tool_event to tool_pmu_event
> > > > > >   perf tool_pmu: Rename perf_tool_event__* to tool_pmu__*
> > > > > >   perf tool_pmu: Move expr literals to tool_pmu
> > > > > >   perf jevents: Add tool event json under a common architecture
> > > > > >   perf tool_pmu: Switch to standard pmu functions and json descriptions
> > > > > >   perf tests: Add tool PMU test
> > > > > >   perf hwmon_pmu: Add a tool PMU exposing events from hwmon in sysfs
> > > > > >   perf test: Add hwmon "PMU" test
> > > > > >   perf docs: Document tool and hwmon events
> > > > >
> > > > > For patch 1-10,
> > > > >
> > > > > Acked-by: Namhyung Kim <namhyung@...nel.org>
> > >
> > > I thought the plan was for 1 to 10 to be in v6.12 and the remaining 3
> > > to be in perf-tools-next/v6.13. I'm not seeing any of the series in
> > > perf-tools so should everything be going in perf-tools-next?
> >
> > Ok, I'll pick up the tools_pmu changes first.

It doesn't apply cleanly anymore.  Please rebase.

> >
> > And I think it'd be much easier for me if you break the hwmon change
> > like with basic PMU enabling and unit/alias support.
> 
> I'd kept the hwmon PMU as a single addition on purpose - testing and
> documentation are follow up patches. Typically a new driver would be a
> single commit, and so I think this is the LKML norm. For example:
> https://lore.kernel.org/lkml/20190326151753.19384-3-shameerali.kolothum.thodi@huawei.com/
> 
> Having multiple commits where things are only partially working means
> bisects will be broken. It also means changes I have on top of this
> can end up conflicting with what you're doing. I agree this means we
> have a big patch when the new thing is added, I think this is normal
> in the case of a driver - which to some extent this is.

I think it depends, and of course I want bisectable patches.  A
standalone driver with well-known pattern of implementation could come
in a single commit.  But this is new and I'm not familiar with the hwmon
interfaces and behaviors.  So I'm asking you to split the minimal code
that can run with perf stat from the full-fledged version.  That would
help maintainers understand and maintain the code better.

Thanks,
Namhyung