From: Arnaldo Carvalho de Melo <acme@kernel.org>
To: Ian Rogers <irogers@google.com>
Cc: Namhyung Kim <namhyung@kernel.org>,
Peter Zijlstra <peterz@infradead.org>,
Ingo Molnar <mingo@redhat.com>,
Mark Rutland <mark.rutland@arm.com>,
Alexander Shishkin <alexander.shishkin@linux.intel.com>,
Jiri Olsa <jolsa@kernel.org>,
Adrian Hunter <adrian.hunter@intel.com>,
Kan Liang <kan.liang@linux.intel.com>,
Ravi Bangoria <ravi.bangoria@amd.com>,
Weilin Wang <weilin.wang@intel.com>,
Yoshihiro Furudera <fj5100bi@fujitsu.com>,
James Clark <james.clark@linaro.org>,
Athira Jajeev <atrajeev@linux.vnet.ibm.com>,
Howard Chu <howardchu95@gmail.com>,
Oliver Upton <oliver.upton@linux.dev>,
Changbin Du <changbin.du@huawei.com>,
Ze Gao <zegao2021@gmail.com>, Junhao He <hejunhao3@huawei.com>,
linux-kernel@vger.kernel.org, linux-perf-users@vger.kernel.org
Subject: Re: [PATCH v6 0/5] Hwmon PMUs
Date: Fri, 25 Oct 2024 18:01:17 -0300 [thread overview]
Message-ID: <ZxwHHWbjIbGh1RU8@x1> (raw)
In-Reply-To: <CAP-5=fVTU8nUfadXgpUd4my9emsY4c_7znMa9_RWD6VZbGYhZA@mail.gmail.com>
On Fri, Oct 25, 2024 at 11:26:26AM -0700, Ian Rogers wrote:
> On Fri, Oct 25, 2024 at 10:30 AM Namhyung Kim <namhyung@kernel.org> wrote:
> > On Thu, Oct 24, 2024 at 06:33:27PM -0700, Ian Rogers wrote:
> > > 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
That is super important, having patches being super small and doing just
one thing helps in bisecting problems.
If two things are done in one patch, and one of them causes a problem,
then bisection is a very effective way of finding out what exactly
caused a problem.
But bisection is not the only benefit from breaking down larger patches
into smaller ones.
We want to have more people joining our ranks, doing low level tooling
and kernel work.
Writing new functionality in a series of patches, growing in complexity
is a way to reduce the cognitive load on understantind how something
works.
As much as trying to emulate how the kernel community works is a good
model as that community has been producing a lot of good code in a
frantic, athletic pace, and as much as I can agree with you that adding
a new piece of code will not affect bisectability as its new code, I
think having it broken down in multiple patches benefits revieweing.
Reviewing is something we should do more, but its very taxing.
One would rather try to write as much code as possible, leaving to
others the reviewing part.
But its a balancing act.
Whatever we can do to help reviewers, like taking into account what they
say they would prefer as a way to submit our work, even if it isn't
exactly of our liking, is one such thing.
So if Namhyung says that it would be best for you to try to break down
your patches into smaller ones, like I did say to you in the past, even
taking the trouble to do it myself, in the process introducing problems,
later fixed, I think you should try to do what he says.
He is the maintainer, try to address his comments.
- Arnaldo
next prev parent reply other threads:[~2024-10-25 21:01 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-10-22 18:06 [PATCH v6 0/5] Hwmon PMUs Ian Rogers
2024-10-22 18:06 ` [PATCH v6 1/5] tools api io: Ensure line_len_out is always initialized Ian Rogers
2024-10-22 18:06 ` [PATCH v6 2/5] perf hwmon_pmu: Add a tool PMU exposing events from hwmon in sysfs Ian Rogers
2024-10-22 18:06 ` [PATCH v6 3/5] perf pmu: Add calls enabling the hwmon_pmu Ian Rogers
2024-10-22 18:06 ` [PATCH v6 4/5] perf test: Add hwmon "PMU" test Ian Rogers
2024-10-22 18:06 ` [PATCH v6 5/5] perf docs: Document tool and hwmon events Ian Rogers
2024-10-24 3:06 ` [PATCH v6 0/5] Hwmon PMUs Namhyung Kim
2024-10-24 7:07 ` Ian Rogers
2024-10-24 16:40 ` Namhyung Kim
2024-10-25 1:33 ` Ian Rogers
2024-10-25 17:30 ` Namhyung Kim
2024-10-25 18:26 ` Ian Rogers
2024-10-25 21:01 ` Arnaldo Carvalho de Melo [this message]
2024-10-25 23:07 ` Ian Rogers
2024-10-26 17:16 ` Namhyung Kim
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZxwHHWbjIbGh1RU8@x1 \
--to=acme@kernel.org \
--cc=adrian.hunter@intel.com \
--cc=alexander.shishkin@linux.intel.com \
--cc=atrajeev@linux.vnet.ibm.com \
--cc=changbin.du@huawei.com \
--cc=fj5100bi@fujitsu.com \
--cc=hejunhao3@huawei.com \
--cc=howardchu95@gmail.com \
--cc=irogers@google.com \
--cc=james.clark@linaro.org \
--cc=jolsa@kernel.org \
--cc=kan.liang@linux.intel.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-perf-users@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=mingo@redhat.com \
--cc=namhyung@kernel.org \
--cc=oliver.upton@linux.dev \
--cc=peterz@infradead.org \
--cc=ravi.bangoria@amd.com \
--cc=weilin.wang@intel.com \
--cc=zegao2021@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).