Message-ID: <2hvcvpqrjrbnv2bxsp3fetvxwxnatav7b5x2fkwwoismi2oaex@uquqtap42qra>
Date: Thu, 18 Dec 2025 19:21:30 -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 3/3] x86/resctrl: Add "*" shorthand to set minimum
 io_alloc CBM for all domains

On Tue, Dec 16, 2025 at 09:53:20PM -0800, Reinette Chatre wrote:
Hi Reinette,

> Kernel code and patches to kernel have a couple of style requirements that
> are documented. It takes some getting used to but it is required that submissions
> follow the kernel style in order to be considered for inclusion.
> Documentation relevant to above are:
> Documentation/process/submitting-patches.rst: Consider whole document but specifically
> search for "This patch" for information related to above.
> Documentation/process/maintainer-tip.rst: Consider whole document but specifically
> consider "Changelog" relevant to this part.

Thank you for the guidance regarding the kernel style and submission requirements.

While I do have experience in kernel development, I appreciate the reminder
that strict adherence to the documented process is paramount. I will
carefully re-read the specified sections to ensure full compliance with the
expected standards.

> Is there a reason to limit this implementation to only support the minimum CBM?
> This feature can easily enable a user to set identical CBM across all domains
> without the CBM being required to be the minimum CBM, no?

You are quite right to raise this point.

Indeed, supporting arbitrary identical CBM values across domains was the
original intent of this work. I appreciate the suggestion and fully agree
that the implementation should be expanded to allow for this additional
flexibility, rather than being restricted to the minimum CBM value.

> No. resctrl_arch_update_domains() is a helper to just program
> configurations. Please avoid bleeding feature specific handling into
> dedicated helpers. 

Acknowledged.

> Why is this new parser needed? 

It can be dropped. Kindly ignore. 

> This does not look right. If I understand correctly out_val contains the
> user provided value/CBM, that is if user provided "*=3" then out_val
> contains "3". This value is in turn compared against the *number of bits*
> required. First, if indeed just forcing user to set the minimum CBM
> (which I already mentioned seems restrictive) then if out_val is "3" and
> min_cbm_bits is "2" then this check will fail while the user indeed did
> set the minimum CBM, no? Additionally, user providing value of 6 or C or
> any other value with two consecutive bits set would also be a valid value
> as a "minimum", no?

You are quite right to point this out; I appreciate the thorough review of
the validation logic. My original intention with this implementation was to
provide a mechanism to "reset" the configuration to the minimum supported
CBM only. However, the restriction itself is unnecessary.

> This additional parsing and checks adds unnecessary complexity. Just keep
> original parsing that relies on parse_cbm() that calls cbm_validate()
> that ensures the provided CBM is valid for this resource while taking
> min_cbm_bits into account.

Acknowledged.

> I agree with Babu [1]. This is a lot of complexity just to support
> handling of an additional character. resctrl does a lot of parsing of
> per-domain user space input and the current implementation follows the
> same pattern used throughout resctrl. Introducing a new pattern and
> parser unique to IO alloc feature adds unnecessary complexity. Simple and
> consistent is better. I considered your response [2] but to me this code
> is not more readable than a simple implementation built on top of
> existing, familiar, and consistent patterns. Another motivation from [2]
> is that this is easier to maintain but I fail to see that. About the
> motivation from [2] for "possible enhancements down the line" ... when/if
> need for an enhancement is required then code can be refactored to
> support it.

Acknowledged.

> On a workflow note: please allow for discussions about your submission.
> Taking a long time to respond and then responding that you disagree with
> feedback immediately followed by a new version of the series that does
> not address feedback is not productive.

Understood.


Kind regards,
-- 
Aaron Tomlin

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

Powered by blists - more mailing lists