Message-ID: <CAP-5=fVTU8nUfadXgpUd4my9emsY4c_7znMa9_RWD6VZbGYhZA@mail.gmail.com>
Date: Fri, 25 Oct 2024 11:26:26 -0700
From: Ian Rogers <irogers@...gle.com>
To: Namhyung Kim <namhyung@...nel.org>
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>, 
	Ravi Bangoria <ravi.bangoria@....com>, Weilin Wang <weilin.wang@...el.com>, 
	Yoshihiro Furudera <fj5100bi@...itsu.com>, James Clark <james.clark@...aro.org>, 
	Athira Jajeev <atrajeev@...ux.vnet.ibm.com>, Howard Chu <howardchu95@...il.com>, 
	Oliver Upton <oliver.upton@...ux.dev>, Changbin Du <changbin.du@...wei.com>, 
	Ze Gao <zegao2021@...il.com>, Junhao He <hejunhao3@...wei.com>, linux-kernel@...r.kernel.org, 
	linux-perf-users@...r.kernel.org
Subject: Re: [PATCH v6 0/5] Hwmon PMUs

On Fri, Oct 25, 2024 at 10:30 AM Namhyung Kim <namhyung@...nel.org> wrote:
>
> On Thu, Oct 24, 2024 at 06:33:27PM -0700, Ian Rogers wrote:
> > On Thu, Oct 24, 2024 at 9:41 AM Namhyung Kim <namhyung@...nel.org> wrote:
> > >
> > > On Thu, Oct 24, 2024 at 12:07:46AM -0700, Ian Rogers wrote:
> > > > On Wed, Oct 23, 2024 at 8:06 PM Namhyung Kim <namhyung@...nel.org> wrote:
> > > > >
> > > > > Hi Ian,
> > > > >
> > > > > On Tue, Oct 22, 2024 at 11:06:18AM -0700, Ian Rogers wrote:
> > > > > > 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]
> > > > > > ...
> > > > > > ```
> > > > > >
> > > > > > v6: Add string.h #include for issue reported by kernel test robot.
> > > > > > v5: Fix asan issue in parse_hwmon_filename caught by a TMA metric.
> > > > > > v4: Drop merged patches 1 to 10. Separate adding the hwmon_pmu from
> > > > > >     the update to perf_pmu to use it. Try to make source of literal
> > > > > >     strings clearer via named #defines. Fix a number of GCC warnings.
> > > > > > v3: Rebase, add Namhyung's acked-by to patches 1 to 10.
> > > > > > v2: Address Namhyung's review feedback. Rebase dropping 4 patches
> > > > > >     applied by Arnaldo, fix build breakage reported by Arnaldo.
> > > > > >
> > > > > > Ian Rogers (5):
> > > > > >   tools api io: Ensure line_len_out is always initialized
> > > > > >   perf hwmon_pmu: Add a tool PMU exposing events from hwmon in sysfs
> > > > > >   perf pmu: Add calls enabling the hwmon_pmu
> > > > > >   perf test: Add hwmon "PMU" test
> > > > > >   perf docs: Document tool and hwmon events
> > > > >
> > > > > I think the patch 2 can be easily splitted into core and other parts
> > > > > like dealing with aliases and units.  I believe it'd be helpful for
> > > > > others (like me) to understand how it works.
> > > > >
> > > > > Please take a look at 'perf/hwmon-pmu' branch in:
> > > > >
> > > > >   https://git.kernel.org/pub/scm/linux/kernel/git/namhyung/linux-perf.git
> > > >
> > > > Thanks Namhyung but I'm not really seeing this making anything simpler
> > > > and I can see significant new bugs. Your new patch:
> > > > https://git.kernel.org/pub/scm/linux/kernel/git/namhyung/linux-perf.git/commit/?h=perf/hwmon-pmu&id=85c78b5bf71fb3e67ae815f7b2d044648fa08391
> > > > Has taken about 40% out of patch 2, but done so by splitting function
> > > > declarations from their definitions, enum declarations from any use,
> > >
> > > Yeah, it's just because I was lazy and you can split header files too
> > > (and please do so).
> > >
> > > > etc. It also adds in code like:
> > > >
> > > > snprintf(buf, sizeof(buf), "%s_input", evsel->name);
> > > >
> > > > but this would be a strange thing to do. The evsel->name is rewritten
> > > > by fallback logic, so cycles may become cycles:u if kernel profiling
> > >
> > > I know it doesn't work but just want to highlight how it's supposed to
> > > work.  Eventually what we need is a correct file name.  In fact, I think
> > > it'd work if we can pass a correct event name probably like:
> > >
> > >   perf stat -e hwmon5/name=fan1/ true
> >
> > But this isn't what the term name and evsel's name are for. They are
> > to allow you to do:
> > ```
> > $ perf stat -e cycles/name=foobar/ true
> >
> > Performance counter stats for 'true':
> >
> >         1,126,942      foobar
> >
> >       0.001681805 seconds time elapsed
> >
> >       0.001757000 seconds user
> >       0.000000000 seconds sys
> > ```
> > Why would you do this in code, change a fundamental of evsel behavior,
> > then just to delete it in the next patch?
>
> Well, I didn't change the actual behavior and it doesn't work yet.
> The deletion is just one line, and I think it reveals the intention of
> the next patch very well.
>
> >
> > > > is restricted. This is why we have metric-id in the evsel as we cannot
> > > > rely on the evsel->name not mutating when looking up events for the
> > > > sake of metrics. Using the name as part of a sysfs filename lookup
> > > > doesn't make sense to me as now the evsel fallback logic can break a
> > > > hwmon event. In the original patch the code was:
> > >
> > > The fallback logic is used only if the kernel returns an error.  Thus
> > > it'd be fine as long as it correctly finds the sysfs filename.  But it's
> > > not used in the final code and the change is a simple one-liner.
> >
> > But it's not. It's changing what evsel->name means to be an event
> > encoding. How does reverse config to name lookup work in this model?
> > How does the normal use of the name term work?
>
> It's intermediate code that is not activated yet.  So I think it's about
> to say how the code works.  If you really don't like to use evsel->name,
> maybe you can put a dummy name with a comment saying it'll be updated in
> next patch.
>
> >
> > > >
> > > > snprintf(buf, sizeof(buf), "%s%d_input", hwmon_type_strs[key.type], key.num);
> > > >
> > > > where those two values are constants and key.type and key.num both
> > > > values embedded in the config value the evsel fallback logic won't
> > > > change. But bringing in the code that does that basically brings in
> > > > all of the rest of patch 2.
> > >
> > > Right, that's why I did that way.
> > >
> > > >
> > > > So the patch is adding a PMU that looks broken, so rather than
> > > > simplifying things it just creates a broken intermediate state and
> > > > should that be fixed for the benefit of bisects?
> > >
> > > Actually it's not broken since it's not enabled yet. :)
> > >
> > >
> > > > It also complicates understanding as the declarations of functions and
> > > > enums have kernel-doc, but now the definitions of enums and functions
> > > > are split apart. For me, to understand the code I'd want to squash the
> > > > patches back together again so I could see a declaration with its
> > > > definition.
> > >
> > > Yep, please move the declarations to the patch 3.
> >
> > So I think moving the enum declarations into one patch is okay. But as
> > the enum values have no bearing on hardware constants, or something
> > outside of the code that uses them it smells strange to me. Ultimately
> > this is going to do little to the lines of code count but damage
> > readability. I'm not sure why we're doing this given the kernel model
> > for adding a driver is to add it as a large chunk. For example, here
> > is adding the intel PT driver:
> > https://lore.kernel.org/all/1422614392-114498-1-git-send-email-alexander.shishkin@linux.intel.com/T/#u
>
> Maybe others can understand a big patch easily, but I'm not.

My understanding is that we make small patches so that the codebase is
more bisectable. When there is something new, like a driver or here a
hwmon PMU, the first patch is large and then we switch to the small
patch model. I have seen patches adding constants ahead of them being
used, but not normally as enums. I've already reduced the size of the
patch by moving everything that isn't hwmon PMU out of the patch and
most of that has already landed. Moving enums out of a header file
okay, shouldn't break the build (a compiler may complain about unused
enums) but then I end up copying comments into commit messages and
doing something alien to what is done in the rest of the kernel. Not
defining a function when you declare it, that is in many cases a
compiler error and for good reason. Adding in changes that are what
are or could be compiler errors goes against making things bisectable.

So breaking up this patch is bad as:
1) it doesn't match existing kernel style,
2) it makes the patch harder to understand (declarations split from
definitions, etc.),
3) with new compiler errors/warnings the code will be less bisectable
as we're deliberately doing things we think wrong for the sake of a
lines-of-code size,
4) we increase the number of patches and commit messages, with commit
messages duplicating comments for things like functions or enums being
added,
5) with your patches we create an intermediate PMU with different
conventions than the rest of the code base and with bugs, impacting
bisectability and the ability to understand the code base.

So I'm arguing against doing this as it is contrary to both our normal
objectives and existing style. I have no real way of knowing when I've
cut something up small enough and if we're not building the code then
how do I build/test the intermediate states, I'm just out on a wild
goose chase.

Thanks,
Ian

Powered by blists - more mailing lists