Message-ID: <vbw3nv635yu2w2surjjz4kzm3bzbrp4gsub4xrqnxa32r5dyzp@vyzmvf7xs4md>
Date: Thu, 25 Dec 2025 17:23:29 -0500
From: Aaron Tomlin <atomlin@...mlin.com>
To: Reinette Chatre <reinette.chatre@...el.com>
Cc: tony.luck@...el.com, Dave.Martin@....com, james.morse@....com, 
	babu.moger@....com, tglx@...utronix.de, mingo@...hat.com, bp@...en8.de, 
	dave.hansen@...ux.intel.com, sean@...e.io, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/3] fs/resctrl: Add helpers to check io_alloc support
 and enabled state

On Fri, Dec 19, 2025 at 09:05:10AM -0800, Reinette Chatre wrote:
> I have a different view on how this patch impacts maintainability. There are several
> techniques to help developers. For example, while the new functions introduced in this
> patch intend to be helpful to developer by including the comment "This function must
> be called under the cpu hotplug lock and and rdtgroup mutex" the right way to communicate
> this is to use lockdep_assert_cpus_held() and lockdep_assert_held(&rdtgroup_mutex). Existence
> of these tools demonstrate that even while knowing what the right thing to do is, mistakes
> can still appear.
> 
> Something else required by these new functions is that rdt_last_cmd_clear() needs to
> be called beforehand. This is not possible to automate like the examples above and
> thus relies on developer to "get right". As one that have seen many patches flow into
> resctrl I can say with confidence that this is one of the things where there are often
> mistakes.
> 
> With that in mind, note how every hunk includes rdt_last_cmd_clear() followed by the
> rdt_last_cmd_printf() being moved. How the buffer is used cannot be more clear, right?
> This patch adds a layer of indirection that makes this relationship more difficult to see.
> It thus does not simplify how to reason about this code.
> 
> Surely, rdt_last_cmd_clear() is not required to be a few lines away from a call that
> writes to the buffer but having these calls in the same function/scope makes it obvious
> where the buffer is cleared and where data is written to it. 
> 
> Also, as resctrl documentation states about the "last_cmd_status" file: "If the command
> failed, it will provide more information that can be conveyed in the error returns from
> file operations.". Now, while keeping this in mind, consider, for example how
> resctrl_io_alloc_write() appears to developer before and after this change. The current
> implementation is consistent: every time there is a failure it is accompanied by a
> write to last_cmd_status buffer to make sure the error details are conveyed to user space.
> After this change the function is inconsistent: some errors result in a print to
> last_cmd_status and some do not. It is not that the print to last_cmd_status is removed
> but it is behind another layer of indirection that makes resctrl_io_alloc_write()
> more difficult to read. A developer can no longer just look at resctrl_io_alloc_write()
> and learn how it interacts with user space.
> 
> Same for the error codes. It is important to know and be consistent which error codes
> are returned to user space. Adding these behind another layer of indirection where that
> is all that function does seems unnecessary to me.
> 
> In summary, no, I do not see how this change benefits maintainability.
> 
Hi Reinette,

Thank you for your exceedingly thorough analysis. You make a most
compelling case, and having reflected upon your points, I find myself in
agreement.

My original aim was to streamline the callsites, yet I concede that the
introduced indirection has come at the expense of transparency.

As you rightly point out, maintaining the visibility of
rdt_last_cmd_clear() alongside the subsequent write is vital for ensuring
that developers do not inadvertently skip the necessary reset of the status
buffer. Furthermore, the loss of visual consistency within functions such
as resctrl_io_alloc_write() - where the interaction with user space becomes
partially obscured - is a valid concern that outweighs the perceived
aesthetic benefit of the cleanup. I also take your point regarding the
preference for lockdep_assert_held() over purely comment - based
documentation; it is a far more robust mechanism for enforcing correctness.

In light of these observations, I shall drop this patch from the series
entirely. I wish you a pleasant and restful break over the festive period,
and I look forward to resuming our technical dialogue in the New Year.


Best regards,
-- 
Aaron Tomlin

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)

Powered by blists - more mailing lists