Re: [PATCH v9 3/9] x86/resctrl: Modify update_mba_bw() to use per ctrl_mon group event

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

Hi Tony,

On 11/18/24 5:44 PM, Luck, Tony wrote:
> On Mon, Nov 18, 2024 at 04:51:38PM -0800, Reinette Chatre wrote:
>> Hi Tony,
>>
>> On 11/18/24 4:01 PM, Luck, Tony wrote:
>>> On Fri, Nov 15, 2024 at 10:21:01AM -0600, Moger, Babu wrote:
>>>> Hi Tony,
>>>
>>> Thanks for looking at this patch.
>>>
>>>>
>>>> On 11/13/2024 6:17 PM, Tony Luck wrote:
>>>>> Instead of hard-coding the memory bandwidth local event as the
>>>>> input to the mba_sc feedback look, use the event that the user
>>>>> configured for each ctrl_mon group.
>>>>>
>>>>> Signed-off-by: Tony Luck <tony.luck@intel.com>
>>>>> ---
>>>>>   arch/x86/kernel/cpu/resctrl/monitor.c | 23 ++++++++++++++++++-----
>>>>>   1 file changed, 18 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
>>>>> index 7ef1a293cc13..2176e355e864 100644
>>>>> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
>>>>> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
>>>>> @@ -752,20 +752,31 @@ static void update_mba_bw(struct rdtgroup *rgrp, struct rdt_mon_domain *dom_mbm)
>>>>>   	u32 closid, rmid, cur_msr_val, new_msr_val;
>>>>>   	struct mbm_state *pmbm_data, *cmbm_data;
>>>>>   	struct rdt_ctrl_domain *dom_mba;
>>>>> +	enum resctrl_event_id evt_id;
>>>>>   	struct rdt_resource *r_mba;
>>>>> -	u32 cur_bw, user_bw, idx;
>>>>>   	struct list_head *head;
>>>>>   	struct rdtgroup *entry;
>>>>> +	u32 cur_bw, user_bw;
>>>>> -	if (!is_mbm_local_enabled())
>>>>> +	if (!is_mbm_enabled())
>>>>>   		return;
>>>>>   	r_mba = &rdt_resources_all[RDT_RESOURCE_MBA].r_resctrl;
>>>>> +	evt_id = rgrp->mba_mbps_event;
>>>>> +
>>>>> +	if (WARN_ON_ONCE(!is_mbm_event(evt_id)))
>>>>> +		return;
>>>>
>>>> I feel this check is enough.
>>>>
>>>>> +	if (WARN_ON_ONCE(evt_id == QOS_L3_MBM_LOCAL_EVENT_ID && !is_mbm_local_enabled()))
>>>>> +		return;
>>>>> +	if (WARN_ON_ONCE(evt_id == QOS_L3_MBM_TOTAL_EVENT_ID && !is_mbm_total_enabled()))
>>>>> +		return;
>>>>
>>>> These two checks are not necessary.  You are already validating it while
>>>> initializing(in patch 7).
>>>
>>> I added this in response to a comment on v7 from Reinette that evt_id
>>> wasn't properly validated here - in conjuction with the change a few
>>> lines earlier that relaxed the check for is_mbm_local_enabled() to
>>> just is_mbm_enabled().
>>
>> right that patch had an issue ... the "initialize" code hardcoded support to be 
>> 	r->membw.mba_mbps_event = QOS_L3_MBM_LOCAL_EVENT_ID;
>> without any checking and then the handler used a relaxed check of
>> 	is_mbm_enabled()
>>
>> On a system that only supports total MBM the is_mbm_enabled() check will
>> pass while the event used will be local MBM.
> 
> In the v9 series I believe all the necessary checks are made outside
> of the update_mba_bw() function itself.
> 
>   update_mba_bw() is only called when is_mba_sc() returns true. Which
>   is the value of:
>   	rdt_resources_all[RDT_RESOURCE_MBA].r_resctrl.membw.mba_sc
>   which can only be set if mbm is enabled.

At this point in series mba_sc can still only be enabled if local MBM
is enabled.

> 
>   So instead of changing the check from is_mbm_local_enabled() to
>   is_mbm_enabled() it could be deleted.

Perhaps ... at this point in series without guidance from changelogs I am forced
to dig through layers to deciper what patches aim to do, how they go about it, and
how complete solution is built using these individual cryptic pieces.

>   rgrp->mba_mbps_event can only be set to QOS_L3_MBM_LOCAL_EVENT_ID
>   until patch 7 when the user can select QOS_L3_MBM_TOTAL_EVENT_ID
>   or patch 8 when the initiialization code can pick TOTAL on systems
>   that don't support LOCAL.
> 
>   So all three of the WARN_ON_ONCE() calls are unnecessary.
> 
> Should I drop all these checks in v10?

I am still trying to decipher this code and will aim to address this.

Reinette