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 3/30/2022 9:45 AM, James Morse wrote:
> Hi Reinette,
> 
> On 17/03/2022 17:00, Reinette Chatre wrote:
>> 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.
> 
>>> @@ -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
> 
> Hmm. I hadn't considered that information would be lost by the current way of doing this.
> It looks like this happens because num_rmid isn't necessarily a power of 2.
> 
> 
>> 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.
> 
> I agree it more accurately reflects resctrl's calculation of "the number
> of lines tagged per RMID if all RMIDs have the same number of lines", but if that
> produces a number the hardware will never actually measure, then the rounding is still
> happening, but somewhere else.
> 
> I think the right thing to do is round resctrl_rmid_realloc_threshold down to the nearest
> multiple of hw_res->mon_scale in rdt_get_mon_l3_config(). This way the filesystem parts
> still handle things in bytes, and the architecture code provides the quantised value that
> will actually get measured. Its this value that should be reported to user-space.
> 
> It doesn't look like the 'Upscaling Factor' is guaranteed to be a power of 2, so I can't
> use the round_down() helpers.
> 
> I've added this to the commit message:
> | Neither r->num_rmid nor hw_res->mon_scale are guaranteed to be a power
> | of 2, so the existing code introduces a rounding error from resctrl's
> | theoretical fraction of the cache usage. This behaviour is kept as it
> | ensures the user visible value matches the value read from hardware
> | when the rmid will be reallocated.
> 
> and the hunk below, which fixes it for me.
> 
> 
> 
> Thanks,
> 
> James
> 
> ---------------%<---------------
> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
> index b18e227d585c..fb81d650c457 100644
> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
> @@ -753,6 +753,7 @@ int rdt_get_mon_l3_config(struct rdt_resource *r)
>         unsigned int mbm_offset = boot_cpu_data.x86_cache_mbm_width_offset;
>         struct rdt_hw_resource *hw_res = resctrl_to_arch_res(r);
>         unsigned int cl_size = boot_cpu_data.x86_cache_size;
> +       u64 threshold;
>         int ret;
> 
>         hw_res->mon_scale = boot_cpu_data.x86_cache_occ_scale;
> @@ -771,7 +772,15 @@ 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_rmid_realloc_threshold = cl_size * 1024 / r->num_rmid;
> +       threshold = cl_size * 1024 / r->num_rmid;
> +
> +       /*
> +        * Because num_rmid may not be a power of two, round the value
> +        * to the nearest multiple of hw_res->mon_scale so it matches a
> +        * value the hardware will measure. mon_scale may not be a power of 2.
> +        */
> +       threshold /= hw_res->mon_scale;
> +       resctrl_rmid_realloc_threshold = threshold * hw_res->mon_scale;
> 
>         ret = dom_data_init(r);
>         if (ret)
> ---------------%<---------------

Thank you for the added explanation. From what I can tell this also restores current
behavior.

Reinette