Re: [PATCH v15 21/34] x86/resctrl: Refactor resctrl_arch_rmid_read()

[Moger, Babu]

Hi Reinette,

On 7/22/25 09:56, Reinette Chatre wrote:
> Hi Babu,
> 
> On 7/22/25 7:23 AM, Moger, Babu wrote:
>> Hi Reinette,
>>
>>
>> On 7/17/25 22:51, Reinette Chatre wrote:
>>> Hi Babu,
>>>
>>> On 7/8/25 3:17 PM, Babu Moger wrote:
>>>> resctrl_arch_rmid_read() modifies the value obtained from MSR_IA32_QM_CTR
>>>> to account for overflow. This adjustment is common to both
>>>
>>> The portion factored out does not just handle MBM overflow counts but also
>>> handles counter scaling for *all* events, including cache occupancy.
>>
>> Yes. Got it. thanks
>>
>>>
>>>> resctrl_arch_rmid_read() and resctrl_arch_cntr_read().
>>>>
>>>> To prepare for the implementation of resctrl_arch_cntr_read(), refactor
>>>> this logic into a new function called mbm_corrected_val().
>>>
>>> This thus cannot be made specific to MBM. More accurate may be
>>> get_corrected_val().
>>
>> Sure.
>>
>> Rephrased the changelog.
>>
>> x86/resctrl: Refactor resctrl_arch_rmid_read()
>>
>> resctrl_arch_rmid_read() modifies the value obtained from MSR_IA32_QM_CTR
> 
> "modifies" -> "adjusts"?

Sure.

> 
>> to account for the overflow for MBM events and apply counter scaling for
>> all the events. This logic is common to both resctrl_arch_rmid_read() and
>> resctrl_arch_cntr_read().
> 
> This may not be obvious since resctrl_arch_cntr_read() does not exist at this
> point in the series. Perhaps just make it a bit higher level, for example:
> "This logic is common to both reading an RMID and reading a hardware counter
> directly."
> 
Sure.

>>
>> To prepare for the implementation of resctrl_arch_cntr_read(), refactor
>> this logic into a new function called get_corrected_val().
> 
> No need for "function" when using (). Could be :
> "Refactor the hardware value adjustment logic into get_corrected_val() to
> prepare for support of reading a hardware counter."?
> 

Sure

-- 
Thanks
Babu Moger