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;
+	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;
+
 
 	closid = rgrp->closid;
 	rmid = rgrp->mon.rmid;
-	idx = resctrl_arch_rmid_idx_encode(closid, rmid);
-	pmbm_data = &dom_mbm->mbm_local[idx];
+	pmbm_data = get_mbm_state(dom_mbm, closid, rmid, evt_id);
+	if (WARN_ON_ONCE(!pmbm_data))
+		return;
 
 	dom_mba = get_ctrl_domain_from_cpu(smp_processor_id(), r_mba);
 	if (!dom_mba) {
@@ -784,7 +795,9 @@ static void update_mba_bw(struct rdtgroup *rgrp, struct rdt_mon_domain *dom_mbm)
 	 */
 	head = &rgrp->mon.crdtgrp_list;
 	list_for_each_entry(entry, head, mon.crdtgrp_list) {
-		cmbm_data = &dom_mbm->mbm_local[entry->mon.rmid];
+		cmbm_data = get_mbm_state(dom_mbm, entry->closid, entry->mon.rmid, evt_id);
+		if (WARN_ON_ONCE(!cmbm_data))
+			return;
 		cur_bw += cmbm_data->prev_bw;
 	}
 
-- 
2.47.0

Hi Tony,

On 11/13/24 4: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

"feedback look" -> "feedback loop"

> configured for each ctrl_mon group.

From "Changelog" in Documentation/process/maintainer-tip.rst:
	"It's also useful to structure the changelog into several paragraphs and not     
	 lump everything together into a single one. A good structure is to explain      
	 the context, the problem and the solution in separate paragraphs and this       
	 order."

I do not find there to be a context nor problem description in the
changelog. Do you believe this changelog is appropriate for tip? Am I missing
something?

> 
> 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())

This change in the check is unexpected because at this point the event is still enforced to be
local MBM. This change is also undocumented so difficult to reason about.

>  		return;
>  
>  	r_mba = &rdt_resources_all[RDT_RESOURCE_MBA].r_resctrl;
> +	evt_id = rgrp->mba_mbps_event;

(To also answer the question in https://lore.kernel.org/all/Zzvtj8n1_ukhnRWT@agluck-desk3/ )

One key point from previous patch is that there is a new "contract" that rgrp->mba_mbps_event
is valid if mba_sc is enabled. If that contract is respected with appropriate initialization
and change of rgrp->mba_mbps_event then I do not believe that the three checks below
nor the "is_mbm_enabled()" above in reader of rgrp->mba_mbps_event are necessary since the
access of rgrp->mba_mbps_event is within "contract".
Note that caller does the checking if mba_sc is enabled:
	if (is_mba_sc(NULL))
		update_mba_bw(prgrp, d);

Thus doing same check within update_mba_bw() should not be necessary. 

It does take a lot of digging to understand so it would really be helpful to document these types
of design decisions and reinforce them through the series.

> +
> +	if (WARN_ON_ONCE(!is_mbm_event(evt_id)))
> +		return;
> +	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;
> +
>  
>  	closid = rgrp->closid;
>  	rmid = rgrp->mon.rmid;
> -	idx = resctrl_arch_rmid_idx_encode(closid, rmid);
> -	pmbm_data = &dom_mbm->mbm_local[idx];
> +	pmbm_data = get_mbm_state(dom_mbm, closid, rmid, evt_id);
> +	if (WARN_ON_ONCE(!pmbm_data))
> +		return;
>  
>  	dom_mba = get_ctrl_domain_from_cpu(smp_processor_id(), r_mba);
>  	if (!dom_mba) {
> @@ -784,7 +795,9 @@ static void update_mba_bw(struct rdtgroup *rgrp, struct rdt_mon_domain *dom_mbm)
>  	 */
>  	head = &rgrp->mon.crdtgrp_list;
>  	list_for_each_entry(entry, head, mon.crdtgrp_list) {
> -		cmbm_data = &dom_mbm->mbm_local[entry->mon.rmid];
> +		cmbm_data = get_mbm_state(dom_mbm, entry->closid, entry->mon.rmid, evt_id);
> +		if (WARN_ON_ONCE(!cmbm_data))
> +			return;
>  		cur_bw += cmbm_data->prev_bw;
>  	}
>  

Reinette

Hi Tony,

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).

> +
>   
>   	closid = rgrp->closid;
>   	rmid = rgrp->mon.rmid;
> -	idx = resctrl_arch_rmid_idx_encode(closid, rmid);
> -	pmbm_data = &dom_mbm->mbm_local[idx];
> +	pmbm_data = get_mbm_state(dom_mbm, closid, rmid, evt_id);
> +	if (WARN_ON_ONCE(!pmbm_data))
> +		return;
>   
>   	dom_mba = get_ctrl_domain_from_cpu(smp_processor_id(), r_mba);
>   	if (!dom_mba) {
> @@ -784,7 +795,9 @@ static void update_mba_bw(struct rdtgroup *rgrp, struct rdt_mon_domain *dom_mbm)
>   	 */
>   	head = &rgrp->mon.crdtgrp_list;
>   	list_for_each_entry(entry, head, mon.crdtgrp_list) {
> -		cmbm_data = &dom_mbm->mbm_local[entry->mon.rmid];
> +		cmbm_data = get_mbm_state(dom_mbm, entry->closid, entry->mon.rmid, evt_id);
> +		if (WARN_ON_ONCE(!cmbm_data))
> +			return;
>   		cur_bw += cmbm_data->prev_bw;
>   	}
>   

-- 
- Babu Moger

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().

See: https://lore.kernel.org/r/bb30835f-5be9-44b4-8544-2f528e7fc573@intel.com

In theory all of these tests could be dropped. As you point out the
sanity checks are done higher in the call sequence. But some folks
like the "belt and braces" approach to such sanity checks.

> > +
> >   	closid = rgrp->closid;
> >   	rmid = rgrp->mon.rmid;
> > -	idx = resctrl_arch_rmid_idx_encode(closid, rmid);
> > -	pmbm_data = &dom_mbm->mbm_local[idx];
> > +	pmbm_data = get_mbm_state(dom_mbm, closid, rmid, evt_id);
> > +	if (WARN_ON_ONCE(!pmbm_data))
> > +		return;
> >   	dom_mba = get_ctrl_domain_from_cpu(smp_processor_id(), r_mba);
> >   	if (!dom_mba) {
> > @@ -784,7 +795,9 @@ static void update_mba_bw(struct rdtgroup *rgrp, struct rdt_mon_domain *dom_mbm)
> >   	 */
> >   	head = &rgrp->mon.crdtgrp_list;
> >   	list_for_each_entry(entry, head, mon.crdtgrp_list) {
> > -		cmbm_data = &dom_mbm->mbm_local[entry->mon.rmid];
> > +		cmbm_data = get_mbm_state(dom_mbm, entry->closid, entry->mon.rmid, evt_id);
> > +		if (WARN_ON_ONCE(!cmbm_data))
> > +			return;
> >   		cur_bw += cmbm_data->prev_bw;
> >   	}
> 
> -- 
> - Babu Moger

-Tony

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.

> 
> See: https://lore.kernel.org/r/bb30835f-5be9-44b4-8544-2f528e7fc573@intel.com
> 
> In theory all of these tests could be dropped. As you point out the
> sanity checks are done higher in the call sequence. But some folks
> like the "belt and braces" approach to such sanity checks.

If that "higher in the call sequence" can be trusted, yes. That was not the
case when I made those statements. Sprinkling WARN() that continues execution
in a known bad state does not seem safe to me either.

Reinette

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.

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

  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?

> 
> > 
> > See: https://lore.kernel.org/r/bb30835f-5be9-44b4-8544-2f528e7fc573@intel.com
> > 
> > In theory all of these tests could be dropped. As you point out the
> > sanity checks are done higher in the call sequence. But some folks
> > like the "belt and braces" approach to such sanity checks.
> 
> If that "higher in the call sequence" can be trusted, yes. That was not the
> case when I made those statements. Sprinkling WARN() that continues execution
> in a known bad state does not seem safe to me either.
> 
> Reinette

-Tony

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