Re: [PATCH v8 5/9] x86/resctrl: Parse ACPI CMRC table

[Chen Yu <yu.c.chen@intel.com>]

Hi Reinette,

On Mon, Sep 28, 2026 at 02:46:32PM -0700, Reinette Chatre wrote:
> Hi Chenyu,
> 
> On 9/17/26 9:50 PM, Chen Yu wrote:
> > diff --git a/arch/x86/include/asm/resctrl.h b/arch/x86/include/asm/resctrl.h
> > index 8f6edcdcfd87..9a32ed418c33 100644
> > --- a/arch/x86/include/asm/resctrl.h
> > +++ b/arch/x86/include/asm/resctrl.h
> > @@ -49,6 +49,8 @@ DECLARE_STATIC_KEY_FALSE(rdt_enable_key);
> >  DECLARE_STATIC_KEY_FALSE(rdt_alloc_enable_key);
> >  DECLARE_STATIC_KEY_FALSE(rdt_mon_enable_key);
> >  
> > +unsigned int erdt_get_scale(void);
> > +
> 
> Adding this prototype to asm header file seems out of place. One needs to look
> at later patches to learn this is because of upcoming use in
> resctrl_arch_round_mon_val(). Beyond that, resctrl_arch_round_mon_val() also
> later needs erdt_cpu_has() that even more looks like the wrong thing to do
> when it comes to the asm header file.
> 
> resctrl_arch_round_mon_val() is used in two places, during system initialization
> and when user space updates resctrl_rmid_realloc_threshold via a write to
> "max_threshold_occupancy". Neither is a hot path requiring this to be inline
> code. 
> 
> Aiming to keep resctrl_arch_round_mon_val() as an inline function is causing this
> ERDT support to be unnecessarily fragmented. Could you please add a preparatory
> patch that moves resctrl_arch_round_mon_val() to a c file and add its prototype
> to include/linux/resctrl.h? This means that a change to MPAM driver is also needed
> that I do not expect objection against. To make this easier it would help to
> place the stub among the more stable resctrl_arch_* calls in MPAM driver.
>

Got it, thanks for providing this detailed information. Let me make this change.

> >  static inline bool resctrl_arch_alloc_capable(void)
> >  {
> >  	return rdt_alloc_capable;
> > diff --git a/arch/x86/kernel/cpu/resctrl/erdt.c b/arch/x86/kernel/cpu/resctrl/erdt.c
> > index 249ba547d7c8..a8a7417c2f82 100644
> > --- a/arch/x86/kernel/cpu/resctrl/erdt.c
> > +++ b/arch/x86/kernel/cpu/resctrl/erdt.c
> > @@ -23,6 +23,7 @@ static LIST_HEAD(domain_info_list);
> >  static bool erdt_enabled;
> >  
> >  #define ERDT_VALID_VERSION		1
> > +#define CMRC_SUPPORTED_INDEX_FN		1
> >  #define RMDD_FLAG_CPU_L3_DOMAIN		BIT(0)
> >  
> >  /* Bitmask of valid sub-tables found in the first RMDD, used to ensure all RMDDs match. */
> > @@ -37,11 +38,26 @@ static u16 first_rmdd_domain_id;
> >   */
> >  static unsigned int erdt_max_rmid;
> >  
> > +/*
> > + * Used only by the limbo handler to round resctrl_rmid_realloc_threshold.
> 
> This implies erdt_scale is used by limbo handler but limbo handler only uses
> resctrl_rmid_realloc_threshold directly, no?
> 

Right, limbo handler only uses resctrl_rmid_realloc_threshold, which is
rounded by legacy/erdt_get_scale(). Let me revise this comment.

> > + * resctrl_rmid_realloc_threshold is a single global value, and
> > + * resctrl_arch_round_mon_val() takes no domain argument, so a single scale has
> > + * to be derived from the per-domain cmrc->up_scale. max() is chosen because the
> 
> This does not sound right. Using the fact that a function does not take an argument as
> a motivation just makes one wonder why the function cannot just be changed?
> 
> "resctrl_rmid_realloc_threshold is a single global value" is accurate and the reason
> why it needs to stay that way is because it is exposed to user space as such. That
> was done before RDT introduced per domain scaling. If keeping it a global is ok for
> ERDT then please highlight this, otherwise resctrl needs an enhancement.
> 

It is OK for ERDT to use the global scale factor, let me revise the
commit log as well as the comment.

> Apart from above it looks like introduction of erdt_scale and erdt_get_scale() would
> benefit from a separate commit. The comment above clearly notes its complexity but
> there is no mention of it in changelog. 
>

OK, let me split this change into a new patch and add a corresponding description
in the commit log as well.

> > + * rounding is a floor: a larger scale yields a slightly lower threshold, i.e. an
> > + * RMID has to drop to a slightly lower occupancy before it is reused.
> > + */

[ ... ]

> > +static __init int cmrc_init(struct acpi_subtbl_hdr_16 *subtbl,
> > +			    struct erdt_domain_info *domain_info)
> 
> Same comment as for cacd_init().
>

OK, will convert the return value to bool.

> >  static inline struct acpi_subtbl_hdr_16 *rmdd_subtbl(struct acpi_erdt_rmdd *rmdd)
> >  {
> >  	return (void *)rmdd + sizeof(*rmdd);
> > @@ -170,6 +230,19 @@ static __init bool parse_rmdd_table(struct acpi_subtbl_hdr_16 *rmdd_hdr)
> >  
> >  			subtbl_mask |= BIT(ACPI_ERDT_TYPE_CACD);
> >  			break;
> > +		case ACPI_ERDT_TYPE_CMRC:
> > +			/*
> > +			 * Only one CMRC is supported per domain as there is no
> > +			 * method to distinguish different CMRCs within a domain.
> > +			 */
> 
> Please note how this comment style is different from comment used to describe parsing
> of other RMDD sub-tables (before or after "case"). Please stick one style and use it
> consistently.
>

OK, it is one single-line comment above the case, the other a multi-line block inside the
case body. I'll standardize them for consistency.

thanks,
Chenyu