linux-kernel - Re: [PATCH v10 02/28] x86/resctrl: Move L3 initialization into new helper function

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] [day] [month] [year] [list]

Message-ID: <20250923100956.GAaNJx9BYhXKkfNJ71@fat_crate.local>
Date: Tue, 23 Sep 2025 12:09:56 +0200
From: Borislav Petkov <bp@...en8.de>
To: "Luck, Tony" <tony.luck@...el.com>
Cc: Reinette Chatre <reinette.chatre@...el.com>,
	Fenghua Yu <fenghuay@...dia.com>,
	Maciej Wieczor-Retman <maciej.wieczor-retman@...el.com>,
	Peter Newman <peternewman@...gle.com>,
	James Morse <james.morse@....com>, Babu Moger <babu.moger@....com>,
	Drew Fustini <dfustini@...libre.com>,
	Dave Martin <Dave.Martin@....com>, Chen Yu <yu.c.chen@...el.com>,
	x86@...nel.org, linux-kernel@...r.kernel.org,
	patches@...ts.linux.dev
Subject: Re: [PATCH v10 02/28] x86/resctrl: Move L3 initialization into new
 helper function

On Fri, Sep 19, 2025 at 11:09:54AM -0700, Luck, Tony wrote:
> I understand that Boris doesn't want to see large amounts of text copied
> from the cover letter into each patch. But there is still a need to meet the
> "Describe your problem" requirement for a good commit message.
> 
> Several of the prepatory patches in this series make changes to expand the
> capabilities of fs/resctrl to handle monitor events from resources other
> than RDT_RESOURCE_L3.
> 
> Is a single short sentence repeated in several patches "too much"?

It probably isn't too much but having boilerplate code in commit messages
feels to the reader - at least to me - like: lemme jump over the boilerplate
code in order to go to the meat of the change. And then the meat of the change
is basically explaining the diff. And then I'm left scratching my head, why is
this even in my mbox.

I dunno, maybe it is just me but I end up doing that pretty often. :-\

For example, this particular commit I'd write somewhere along those lines:

"Carve out the resource monitoring domain init code into a separate helper in
order to be able to initialize new types of monitoring domains besides the
usual L3 ones."

Or so.

Short'n'sweet.

You don't explain what the code does as that should be visible from the diff
itself. You're explaining the why: "hey, I need this code into a separate
helper in order to do bla, so I'm carving it out."

That's it.

IOW, you don't necessarily have to go through the common steps of describing
the problem first, especially when you're doing preparatory, refactoring
patches.

Reading the next patch, the commit message is perfectly fine even without:

"All monitoring events are associated with the L3 resource."

Patch 4 doesn't have a boilerplate sentence and it is perfectly fine as it is.

Patch 5 you can start like this:

"Up until now, all monitoring events were associated with the L3 resource and
it made sense to use the L3 specific "struct rdt_mon_domain *" arguments to
functions manipulating domains...."

And even if you have the gist of the boilerplate, it reads better because it
is not simply slapped there mechanically.

This is not needed:

"Update kerneldoc for mon_data::sum to note that it is only used for
RDT_RESOURCE_L3."

because it is in the diff. Unless there's a reason for it to be called out in
the commit message...? And now I'm wondering why is this called out...

See what I mean?

Patch 6 I'd write like this:

"Use a generic struct rdt_domain_hdr representing a generic domain header in
struct rmid_read in order to support other telemetry events' domains besides
an L3 one. Adjust the code interacting with it to the new struct layout."

Something like that.

IOW, this is basically explaining the intention and *why* the patch is there.
No need for "for the calls from mon_event_read() via smp_call*() to
__mon_event_count() and resctrl_arch_rmid_read() and
resctrl_arch_cntr_read()." as that is functionality details. If one is really
interested, one can apply the patch and stare at it but the *why* is a lot
more important.

Dunno, this is ofc subjective but IMNSVHO, writing things this way makes it
a lot easier to review and understand the why. The what one can figure out.

And *if* one can't figure out the what because there's something non-trivial,
then by all means, call that out in the commit message.

And so on...

I hope I'm making some sense here.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette