Re: [PATCH v3 19/21] x86/resctrl: Rename and change the units of resctrl_cqm_threshold

[Reinette Chatre <reinette.chatre@intel.com>]

Hi James,

On 2/17/2022 10:21 AM, James Morse wrote:
> resctrl_cqm_threshold is stored in a hardware specific chunk size,
> but exposed to user-space as bytes.
> 
> This means the filesystem parts of resctrl need to know how the hardware
> counts, to convert the user provided byte value to chunks. The interface
> between the architecture's resctrl code and the filesystem ought to
> treat everything as bytes.
> 
> Change the unit of resctrl_cqm_threshold to bytes. resctrl_arch_rmid_read()
> still returns its value in chunks, so this needs converting to bytes.
> As all the callers have been touched, rename the variable to
> resctrl_rmid_realloc_threshold, which describes what the value is for.
> 
> Signed-off-by: James Morse <james.morse@arm.com>

...

  
> @@ -762,10 +763,7 @@ int rdt_get_mon_l3_config(struct rdt_resource *r)
>  	 *
>  	 * For a 35MB LLC and 56 RMIDs, this is ~1.8% of the LLC.
>  	 */
> -	resctrl_cqm_threshold = cl_size * 1024 / r->num_rmid;
> -
> -	/* h/w works in units of "boot_cpu_data.x86_cache_occ_scale" */
> -	resctrl_cqm_threshold /= hw_res->mon_scale;
> +	resctrl_rmid_realloc_threshold = cl_size * 1024 / r->num_rmid;
>  
>  	ret = dom_data_init(r);
>  	if (ret)
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index 7ec089d72ab7..93b3697027df 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -1030,10 +1030,7 @@ static int rdt_delay_linear_show(struct kernfs_open_file *of,
>  static int max_threshold_occ_show(struct kernfs_open_file *of,
>  				  struct seq_file *seq, void *v)
>  {
> -	struct rdt_resource *r = of->kn->parent->priv;
> -	struct rdt_hw_resource *hw_res = resctrl_to_arch_res(r);
> -
> -	seq_printf(seq, "%u\n", resctrl_cqm_threshold * hw_res->mon_scale);
> +	seq_printf(seq, "%u\n", resctrl_rmid_realloc_threshold);
>  
>  	return 0;
>  }


This change has some user visible impact that I am still digesting but thought
that I would share for your consideration.

As seen in the above two snippets, the original code did:

resctrl_cqm_threshold /= hw_res->mon_scale; /* resctrl_cqm_threshold used internally */

resctrl_cqm_threshold * hw_res->mon_scale; /* this is displayed to user */

The original loss due to truncation during the division is not recovered
when the value is displayed to the user the user may see significant differences
before and after this patch.

I tried this out on a system with a large cache and the before and after 
information is significant:
Before this patch:
info/L3_MON/max_threshold_occupancy:147456

After this patch:
info/L3_MON/max_threshold_occupancy:196608

As I understand this change indeed represents the information more accurately but
I found it noteworthy that this is not just a simple "change the units" and
may thus have broader impact and may indeed result in different behavior that
should be considered.

Reinette