Re: [PATCH v9 07/12] x86/resctrl: Add a global ERDT monitor scale

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

On Fri, Oct 23, 2026 at 05:02:05PM -0700, Reinette Chatre wrote:
> Hi Chenyu,
> 
> On 10/13/26 7:59 PM, Chen Yu wrote:
> > The limbo handler polls the LLC occupancy of stale RMIDs and frees an RMID
> 
> "stale RMIDs"? This is a new concept unfamiliar to resctrl.
>

OK, changed to "limbo RMIDs".

> > once its occupancy drops below max_threshold_occupancy. User space writes
> 
> Please do not interchangeably use the name of the resctrl file and the variable
> that backs it because the variable that backs the file may not have the value
> that user space wrote to the file.
> 

OK, will use resctrl_rmid_realloc_threshold for the value the limbo handler
compares against, and only mentions "max_threshold_occupancy" as the file
userspace writes to.

> > that threshold in bytes, but the hardware counts occupancy in units of the
> > occupancy scale, so the threshold is rounded down to a multiple of that
> > scale. The scale is the number of bytes one counter unit represents, i.e.
> 
> This does not sound right, the threshold is explicitly noted as:
>  	"This is the threshold cache occupancy in *bytes* ..."
> 

Yes, the threshold is always in bytes, let me rephrase it:
The occupancy counter only has the granularity of its scale, the number of
bytes one counter unit represents, so resctrl_arch_round_mon_val() rounds the
written byte value down to a multiple of that scale before it is stored in
resctrl_rmid_realloc_threshold.

> > the granularity of the occupancy counter.
> > 
> > ERDT may provide per-domain monitoring scale factors, but resctrl rounds
> > max_threshold_occupancy through a single global
> > resctrl_rmid_realloc_threshold value that is visible to user space.
> > 
> > Prepare for that by adding erdt_scale/erdt_get_scale() and teaching
> 
> "Prepare for that" - Prepare for what?
>

Will remove this sentence after rearranging the patch sequence.

> > resctrl_arch_round_mon_val() to consult the ERDT scale when firmware has
> > populated one.
> > 
> > When CMRC parsing provides per-domain up_scale values, erdt_scale will be
> > updated with max(). The reason to use max is because 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.
> > 
> > For now erdt_scale is intentionally left at its default value of 0.
> 
> This is a clear signal that the patch ordering needs to change.
>
> > +/*
> > + * Used only to round the global resctrl_rmid_realloc_threshold for the limbo
> > + * handler. resctrl_rmid_realloc_threshold is exposed to user space as a single
> > + * global value, so ERDT uses one scale derived from the per-domain
> > + * cmrc->up_scale values. max() keeps this single threshold conservative across
> > + * domains because the 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.
> 
> This documents changes that do not exist at this point in series. Please reorder
> this patch to ensure its dependencies land first.
>

OK, will adjust this patch to include above max() logic and adjust the
patch sequence:
("x86/resctrl: Add the ERDT CMT scale") will be moved after
("x86/resctrl: Introduce erdt_cpu_has() and erdt_support()")
and before
("x86/resctrl: Add MMIO-based LLC occupancy monitoring support")

> > + */
> > +static unsigned int erdt_scale;
> > +
> > +unsigned int erdt_get_scale(void)
> > +{
> > +	return erdt_scale;
> 
> The name and usage implies that this is a global scale. This will become more confusing
> when trying to land the memory bandwidth monitoring changes that have separate scale values.
> 
> Having this change pretend it is a global change and then add a snippet to final
> patch to constrain it with a "if (!erdt_cpu_has(X86_FEATURE_CQM_OCCUP_LLC))" just makes
> this work difficult to consume. 
> 
> If this is specific to the CMT scale only then let the global variable and its accessor
> make that obvious. Let this change land after the CMRC parsing and erdt_cpu_has()
> introduction so that it can be introduced as one coherent change.
>

OK, adjusted the patch sequence as suggested.

> >  
> >  unsigned int resctrl_arch_round_mon_val(unsigned int val)
> >  {
> > -	unsigned int scale = boot_cpu_data.x86_cache_occ_scale;
> > +	unsigned int scale = erdt_get_scale();
> > +
> > +	if (!scale)
> > +		scale = boot_cpu_data.x86_cache_occ_scale;
> >  
> >  	/* h/w works in units of "boot_cpu_data.x86_cache_occ_scale" */
> 
> The fixup of this comment found in patch #12 needs to be squashed here.
>

OK, will do.

thanks,
Chenyu