Users can modify the configuration of assignable events. Whenever the
event configuration is updated, MBM assignments must be revised across
all monitor groups within the impacted domains.

Signed-off-by: Babu Moger <babu.moger@amd.com>
---
v9: Again patch changed completely based on the comment.
    https://lore.kernel.org/lkml/03b278b5-6c15-4d09-9ab7-3317e84a409e@intel.com/
    Introduced resctrl_mon_event_config_set to handle IPI.
    But sending another IPI inside IPI causes problem. Kernel reports SMP
    warning. So, introduced resctrl_arch_update_cntr() to send the command directly.

v8: Patch changed completely.
    Updated the assignment on same IPI as the event is updated.
    Could not do the way we discussed in the thread.
    https://lore.kernel.org/lkml/f77737ac-d3f6-3e4b-3565-564f79c86ca8@amd.com/
    Needed to figure out event type to update the configuration.

v7: New patch to update the assignments. Missed it earlier.
---
 arch/x86/kernel/cpu/resctrl/rdtgroup.c | 86 +++++++++++++++++++++++---
 include/linux/resctrl.h                |  3 +-
 2 files changed, 79 insertions(+), 10 deletions(-)

diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index 5b8bb8bd913c..7646d67ea10e 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -1710,6 +1710,7 @@ static int rdtgroup_size_show(struct kernfs_open_file *of,
 }
 
 struct mon_config_info {
+	struct rdt_resource *r;
 	struct rdt_mon_domain *d;
 	u32 evtid;
 	u32 mon_config;
@@ -1735,26 +1736,28 @@ u32 resctrl_arch_mon_event_config_get(struct rdt_mon_domain *d,
 	return INVALID_CONFIG_VALUE;
 }
 
-void resctrl_arch_mon_event_config_set(void *info)
+void resctrl_arch_mon_event_config_set(struct rdt_mon_domain *d,
+				       enum resctrl_event_id eventid, u32 val)
 {
-	struct mon_config_info *mon_info = info;
 	struct rdt_hw_mon_domain *hw_dom;
 	unsigned int index;
 
-	index = mon_event_config_index_get(mon_info->evtid);
+	index = mon_event_config_index_get(eventid);
 	if (index == INVALID_CONFIG_INDEX)
 		return;
 
-	wrmsr(MSR_IA32_EVT_CFG_BASE + index, mon_info->mon_config, 0);
+	wrmsr(MSR_IA32_EVT_CFG_BASE + index, val, 0);
 
-	hw_dom = resctrl_to_arch_mon_dom(mon_info->d);
+	hw_dom = resctrl_to_arch_mon_dom(d);
 
-	switch (mon_info->evtid) {
+	switch (eventid) {
 	case QOS_L3_MBM_TOTAL_EVENT_ID:
-		hw_dom->mbm_total_cfg = mon_info->mon_config;
+		hw_dom->mbm_total_cfg = val;
 		break;
 	case QOS_L3_MBM_LOCAL_EVENT_ID:
-		hw_dom->mbm_local_cfg = mon_info->mon_config;
+		hw_dom->mbm_local_cfg = val;
+		break;
+	default:
 		break;
 	}
 }
@@ -1826,6 +1829,70 @@ static int mbm_local_bytes_config_show(struct kernfs_open_file *of,
 	return 0;
 }
 
+static struct rdtgroup *rdtgroup_find_grp_by_cntr_id_index(int cntr_id, unsigned int index)
+{
+	struct rdtgroup *prgrp, *crgrp;
+
+	/* Check if the cntr_id is associated to the event type updated */
+	list_for_each_entry(prgrp, &rdt_all_groups, rdtgroup_list) {
+		if (prgrp->mon.cntr_id[index] == cntr_id)
+			return prgrp;
+
+		list_for_each_entry(crgrp, &prgrp->mon.crdtgrp_list, mon.crdtgrp_list) {
+			if (crgrp->mon.cntr_id[index] == cntr_id)
+				return crgrp;
+		}
+	}
+
+	return NULL;
+}
+
+static void resctrl_arch_update_cntr(struct rdt_resource *r, struct rdt_mon_domain *d,
+				     enum resctrl_event_id evtid, u32 rmid,
+				     u32 closid, u32 cntr_id, u32 val)
+{
+	union l3_qos_abmc_cfg abmc_cfg = { 0 };
+
+	abmc_cfg.split.cfg_en = 1;
+	abmc_cfg.split.cntr_en = 1;
+	abmc_cfg.split.cntr_id = cntr_id;
+	abmc_cfg.split.bw_src = rmid;
+	abmc_cfg.split.bw_type = val;
+
+	wrmsrl(MSR_IA32_L3_QOS_ABMC_CFG, abmc_cfg.full);
+}
+
+static void resctrl_mon_event_config_set(void *info)
+{
+	struct mon_config_info *mon_info = info;
+	struct rdt_mon_domain *d = mon_info->d;
+	struct rdt_resource *r = mon_info->r;
+	struct rdtgroup *rdtgrp;
+	unsigned int index;
+	u32 cntr_id;
+
+	resctrl_arch_mon_event_config_set(d, mon_info->evtid, mon_info->mon_config);
+
+	if (!resctrl_arch_mbm_cntr_assign_enabled(r))
+		return;
+
+	index = mon_event_config_index_get(mon_info->evtid);
+	if (index == INVALID_CONFIG_INDEX)
+		return;
+
+	for (cntr_id = 0; cntr_id < r->mon.num_mbm_cntrs; cntr_id++) {
+		if (test_bit(cntr_id, d->mbm_cntr_map)) {
+			rdtgrp = rdtgroup_find_grp_by_cntr_id_index(cntr_id, index);
+			if (rdtgrp)
+				resctrl_arch_update_cntr(mon_info->r, d,
+							 mon_info->evtid,
+							 rdtgrp->mon.rmid,
+							 rdtgrp->closid,
+							 cntr_id,
+							 mon_info->mon_config);
+		}
+	}
+}
 
 static void mbm_config_write_domain(struct rdt_resource *r,
 				    struct rdt_mon_domain *d, u32 evtid, u32 val)
@@ -1841,6 +1908,7 @@ static void mbm_config_write_domain(struct rdt_resource *r,
 	if (config_val == INVALID_CONFIG_VALUE || config_val == val)
 		return;
 
+	mon_info.r = r;
 	mon_info.d = d;
 	mon_info.evtid = evtid;
 	mon_info.mon_config = val;
@@ -1852,7 +1920,7 @@ static void mbm_config_write_domain(struct rdt_resource *r,
 	 * on one CPU is observed by all the CPUs in the domain.
 	 */
 	smp_call_function_any(&d->hdr.cpu_mask,
-			      resctrl_arch_mon_event_config_set,
+			      resctrl_mon_event_config_set,
 			      &mon_info, 1);
 
 	/*
diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
index 0b8eeb8afc68..4dc858d7aa10 100644
--- a/include/linux/resctrl.h
+++ b/include/linux/resctrl.h
@@ -356,7 +356,8 @@ void resctrl_arch_reset_rmid(struct rdt_resource *r, struct rdt_mon_domain *d,
  */
 void resctrl_arch_reset_rmid_all(struct rdt_resource *r, struct rdt_mon_domain *d);
 
-void resctrl_arch_mon_event_config_set(void *info);
+void resctrl_arch_mon_event_config_set(struct rdt_mon_domain *d,
+				       enum resctrl_event_id eventid, u32 val);
 u32 resctrl_arch_mon_event_config_get(struct rdt_mon_domain *d,
 				      enum resctrl_event_id eventid);
 
-- 
2.34.1

Hi Babu,

On 10/29/24 4:21 PM, Babu Moger wrote:
> Users can modify the configuration of assignable events. Whenever the
> event configuration is updated, MBM assignments must be revised across
> all monitor groups within the impacted domains.

Please revisit the "Changelog" section in 
Documentation/process/maintainer-tip.rst

> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
> v9: Again patch changed completely based on the comment.
>     https://lore.kernel.org/lkml/03b278b5-6c15-4d09-9ab7-3317e84a409e@intel.com/
>     Introduced resctrl_mon_event_config_set to handle IPI.
>     But sending another IPI inside IPI causes problem. Kernel reports SMP
>     warning. So, introduced resctrl_arch_update_cntr() to send the command directly.

I see ... the WARN is because there is a check whether IRQs are disabled before
the check whether the function can be run locally.

> 
> v8: Patch changed completely.
>     Updated the assignment on same IPI as the event is updated.
>     Could not do the way we discussed in the thread.
>     https://lore.kernel.org/lkml/f77737ac-d3f6-3e4b-3565-564f79c86ca8@amd.com/
>     Needed to figure out event type to update the configuration.
> 
> v7: New patch to update the assignments. Missed it earlier.
> ---
>  arch/x86/kernel/cpu/resctrl/rdtgroup.c | 86 +++++++++++++++++++++++---
>  include/linux/resctrl.h                |  3 +-
>  2 files changed, 79 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index 5b8bb8bd913c..7646d67ea10e 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -1710,6 +1710,7 @@ static int rdtgroup_size_show(struct kernfs_open_file *of,
>  }
>  
>  struct mon_config_info {
> +	struct rdt_resource *r;
>  	struct rdt_mon_domain *d;
>  	u32 evtid;
>  	u32 mon_config;
> @@ -1735,26 +1736,28 @@ u32 resctrl_arch_mon_event_config_get(struct rdt_mon_domain *d,
>  	return INVALID_CONFIG_VALUE;
>  }
>  
> -void resctrl_arch_mon_event_config_set(void *info)
> +void resctrl_arch_mon_event_config_set(struct rdt_mon_domain *d,
> +				       enum resctrl_event_id eventid, u32 val)
>  {
> -	struct mon_config_info *mon_info = info;
>  	struct rdt_hw_mon_domain *hw_dom;
>  	unsigned int index;
>  
> -	index = mon_event_config_index_get(mon_info->evtid);
> +	index = mon_event_config_index_get(eventid);
>  	if (index == INVALID_CONFIG_INDEX)
>  		return;
>  
> -	wrmsr(MSR_IA32_EVT_CFG_BASE + index, mon_info->mon_config, 0);
> +	wrmsr(MSR_IA32_EVT_CFG_BASE + index, val, 0);
>  
> -	hw_dom = resctrl_to_arch_mon_dom(mon_info->d);
> +	hw_dom = resctrl_to_arch_mon_dom(d);
>  
> -	switch (mon_info->evtid) {
> +	switch (eventid) {
>  	case QOS_L3_MBM_TOTAL_EVENT_ID:
> -		hw_dom->mbm_total_cfg = mon_info->mon_config;
> +		hw_dom->mbm_total_cfg = val;
>  		break;
>  	case QOS_L3_MBM_LOCAL_EVENT_ID:
> -		hw_dom->mbm_local_cfg = mon_info->mon_config;
> +		hw_dom->mbm_local_cfg = val;
> +		break;
> +	default:
>  		break;
>  	}
>  }
> @@ -1826,6 +1829,70 @@ static int mbm_local_bytes_config_show(struct kernfs_open_file *of,
>  	return 0;
>  }
>  
> +static struct rdtgroup *rdtgroup_find_grp_by_cntr_id_index(int cntr_id, unsigned int index)
> +{
> +	struct rdtgroup *prgrp, *crgrp;
> +
> +	/* Check if the cntr_id is associated to the event type updated */
> +	list_for_each_entry(prgrp, &rdt_all_groups, rdtgroup_list) {
> +		if (prgrp->mon.cntr_id[index] == cntr_id)
> +			return prgrp;
> +
> +		list_for_each_entry(crgrp, &prgrp->mon.crdtgrp_list, mon.crdtgrp_list) {
> +			if (crgrp->mon.cntr_id[index] == cntr_id)
> +				return crgrp;
> +		}
> +	}
> +
> +	return NULL;
> +}
> +
> +static void resctrl_arch_update_cntr(struct rdt_resource *r, struct rdt_mon_domain *d,
> +				     enum resctrl_event_id evtid, u32 rmid,
> +				     u32 closid, u32 cntr_id, u32 val)
> +{
> +	union l3_qos_abmc_cfg abmc_cfg = { 0 };
> +
> +	abmc_cfg.split.cfg_en = 1;
> +	abmc_cfg.split.cntr_en = 1;
> +	abmc_cfg.split.cntr_id = cntr_id;
> +	abmc_cfg.split.bw_src = rmid;
> +	abmc_cfg.split.bw_type = val;
> +
> +	wrmsrl(MSR_IA32_L3_QOS_ABMC_CFG, abmc_cfg.full);

Is it needed to create an almost duplicate function? What if instead 
only resctrl_arch_config_cntr() exists and it uses parameter to decide
whether to call resctrl_abmc_config_one_amd() directly or via 
smp_call_function_any()? I think that should help to make clear how
the code flows. 
Also note that this is an almost identical arch callback with no
error return. I expect that building on existing resctrl_arch_config_cntr()
will make things easier to understand.

> +}
> +
> +static void resctrl_mon_event_config_set(void *info)
> +{
> +	struct mon_config_info *mon_info = info;
> +	struct rdt_mon_domain *d = mon_info->d;
> +	struct rdt_resource *r = mon_info->r;

Note that local variable r is created here while the function is inconsistent by
switching between using r and mon_info->r.

> +	struct rdtgroup *rdtgrp;
> +	unsigned int index;
> +	u32 cntr_id;
> +
> +	resctrl_arch_mon_event_config_set(d, mon_info->evtid, mon_info->mon_config);
> +
> +	if (!resctrl_arch_mbm_cntr_assign_enabled(r))
> +		return;
> +
> +	index = mon_event_config_index_get(mon_info->evtid);

This is an AMD arch specific helper to know which offset of an MSR to use. It should
not be used directly in resctrl fs code, this is what MBM_EVENT_ARRAY_INDEX was created for.

Since MBM_EVENT_ARRAY_INDEX is a macro it can be called closer to where it is used,
within  rdtgroup_find_grp_by_cntr_id_index(), which prompts a reconsider of that function name.

> +	if (index == INVALID_CONFIG_INDEX)
> +		return;
> +
> +	for (cntr_id = 0; cntr_id < r->mon.num_mbm_cntrs; cntr_id++) {
> +		if (test_bit(cntr_id, d->mbm_cntr_map)) {
> +			rdtgrp = rdtgroup_find_grp_by_cntr_id_index(cntr_id, index);
> +			if (rdtgrp)
> +				resctrl_arch_update_cntr(mon_info->r, d,
> +							 mon_info->evtid,
> +							 rdtgrp->mon.rmid,
> +							 rdtgrp->closid,
> +							 cntr_id,
> +							 mon_info->mon_config);
> +		}
> +	}
> +}

Could you please add some function comments to explain the flow here? For example,
what should reader consider if there is to rdtgroup found?

Reinette

Hi Reinette,

On 11/18/2024 1:43 PM, Reinette Chatre wrote:
> Hi Babu,
> 
> On 10/29/24 4:21 PM, Babu Moger wrote:
>> Users can modify the configuration of assignable events. Whenever the
>> event configuration is updated, MBM assignments must be revised across
>> all monitor groups within the impacted domains.
> 
> Please revisit the "Changelog" section in
> Documentation/process/maintainer-tip.rst
> 

ok.

Imperative mood, context, problem and solution.

>>
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> ---
>> v9: Again patch changed completely based on the comment.
>>      https://lore.kernel.org/lkml/03b278b5-6c15-4d09-9ab7-3317e84a409e@intel.com/
>>      Introduced resctrl_mon_event_config_set to handle IPI.
>>      But sending another IPI inside IPI causes problem. Kernel reports SMP
>>      warning. So, introduced resctrl_arch_update_cntr() to send the command directly.
> 
> I see ... the WARN is because there is a check whether IRQs are disabled before
> the check whether the function can be run locally.

ok

> 
>>
>> v8: Patch changed completely.
>>      Updated the assignment on same IPI as the event is updated.
>>      Could not do the way we discussed in the thread.
>>      https://lore.kernel.org/lkml/f77737ac-d3f6-3e4b-3565-564f79c86ca8@amd.com/
>>      Needed to figure out event type to update the configuration.
>>
>> v7: New patch to update the assignments. Missed it earlier.
>> ---
>>   arch/x86/kernel/cpu/resctrl/rdtgroup.c | 86 +++++++++++++++++++++++---
>>   include/linux/resctrl.h                |  3 +-
>>   2 files changed, 79 insertions(+), 10 deletions(-)
>>
>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> index 5b8bb8bd913c..7646d67ea10e 100644
>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> @@ -1710,6 +1710,7 @@ static int rdtgroup_size_show(struct kernfs_open_file *of,
>>   }
>>   
>>   struct mon_config_info {
>> +	struct rdt_resource *r;
>>   	struct rdt_mon_domain *d;
>>   	u32 evtid;
>>   	u32 mon_config;
>> @@ -1735,26 +1736,28 @@ u32 resctrl_arch_mon_event_config_get(struct rdt_mon_domain *d,
>>   	return INVALID_CONFIG_VALUE;
>>   }
>>   
>> -void resctrl_arch_mon_event_config_set(void *info)
>> +void resctrl_arch_mon_event_config_set(struct rdt_mon_domain *d,
>> +				       enum resctrl_event_id eventid, u32 val)
>>   {
>> -	struct mon_config_info *mon_info = info;
>>   	struct rdt_hw_mon_domain *hw_dom;
>>   	unsigned int index;
>>   
>> -	index = mon_event_config_index_get(mon_info->evtid);
>> +	index = mon_event_config_index_get(eventid);
>>   	if (index == INVALID_CONFIG_INDEX)
>>   		return;
>>   
>> -	wrmsr(MSR_IA32_EVT_CFG_BASE + index, mon_info->mon_config, 0);
>> +	wrmsr(MSR_IA32_EVT_CFG_BASE + index, val, 0);
>>   
>> -	hw_dom = resctrl_to_arch_mon_dom(mon_info->d);
>> +	hw_dom = resctrl_to_arch_mon_dom(d);
>>   
>> -	switch (mon_info->evtid) {
>> +	switch (eventid) {
>>   	case QOS_L3_MBM_TOTAL_EVENT_ID:
>> -		hw_dom->mbm_total_cfg = mon_info->mon_config;
>> +		hw_dom->mbm_total_cfg = val;
>>   		break;
>>   	case QOS_L3_MBM_LOCAL_EVENT_ID:
>> -		hw_dom->mbm_local_cfg = mon_info->mon_config;
>> +		hw_dom->mbm_local_cfg = val;
>> +		break;
>> +	default:
>>   		break;
>>   	}
>>   }
>> @@ -1826,6 +1829,70 @@ static int mbm_local_bytes_config_show(struct kernfs_open_file *of,
>>   	return 0;
>>   }
>>   
>> +static struct rdtgroup *rdtgroup_find_grp_by_cntr_id_index(int cntr_id, unsigned int index)
>> +{
>> +	struct rdtgroup *prgrp, *crgrp;
>> +
>> +	/* Check if the cntr_id is associated to the event type updated */
>> +	list_for_each_entry(prgrp, &rdt_all_groups, rdtgroup_list) {
>> +		if (prgrp->mon.cntr_id[index] == cntr_id)
>> +			return prgrp;
>> +
>> +		list_for_each_entry(crgrp, &prgrp->mon.crdtgrp_list, mon.crdtgrp_list) {
>> +			if (crgrp->mon.cntr_id[index] == cntr_id)
>> +				return crgrp;
>> +		}
>> +	}
>> +
>> +	return NULL;
>> +}
>> +
>> +static void resctrl_arch_update_cntr(struct rdt_resource *r, struct rdt_mon_domain *d,
>> +				     enum resctrl_event_id evtid, u32 rmid,
>> +				     u32 closid, u32 cntr_id, u32 val)
>> +{
>> +	union l3_qos_abmc_cfg abmc_cfg = { 0 };
>> +
>> +	abmc_cfg.split.cfg_en = 1;
>> +	abmc_cfg.split.cntr_en = 1;
>> +	abmc_cfg.split.cntr_id = cntr_id;
>> +	abmc_cfg.split.bw_src = rmid;
>> +	abmc_cfg.split.bw_type = val;
>> +
>> +	wrmsrl(MSR_IA32_L3_QOS_ABMC_CFG, abmc_cfg.full);
> 
> Is it needed to create an almost duplicate function? What if instead
> only resctrl_arch_config_cntr() exists and it uses parameter to decide
> whether to call resctrl_abmc_config_one_amd() directly or via
> smp_call_function_any()? I think that should help to make clear how
> the code flows.
> Also note that this is an almost identical arch callback with no
> error return. I expect that building on existing resctrl_arch_config_cntr()
> will make things easier to understand.

It can be done. But it takes another parameter to the function.
It has 7 parameters already. This will be 8th.
Will change it if that is ok.

> 
>> +}
>> +
>> +static void resctrl_mon_event_config_set(void *info)
>> +{
>> +	struct mon_config_info *mon_info = info;
>> +	struct rdt_mon_domain *d = mon_info->d;
>> +	struct rdt_resource *r = mon_info->r;
> 
> Note that local variable r is created here while the function is inconsistent by
> switching between using r and mon_info->r.

Sure. Got it.

> 
>> +	struct rdtgroup *rdtgrp;
>> +	unsigned int index;
>> +	u32 cntr_id;
>> +
>> +	resctrl_arch_mon_event_config_set(d, mon_info->evtid, mon_info->mon_config);
>> +
>> +	if (!resctrl_arch_mbm_cntr_assign_enabled(r))
>> +		return;
>> +
>> +	index = mon_event_config_index_get(mon_info->evtid);
> 
> This is an AMD arch specific helper to know which offset of an MSR to use. It should
> not be used directly in resctrl fs code, this is what MBM_EVENT_ARRAY_INDEX was created for.

Sure.

> 
> Since MBM_EVENT_ARRAY_INDEX is a macro it can be called closer to where it is used,
> within  rdtgroup_find_grp_by_cntr_id_index(), which prompts a reconsider of that function name.


How about ?

static struct rdtgroup *rdtgroup_find_grp_by_cntr_id_event(int cntr_id, 
enum resctrl_event_id evtid)

Will move the macro MBM_EVENT_ARRAY_INDEX inside the function.


> 
>> +	if (index == INVALID_CONFIG_INDEX)
>> +		return;
>> +
>> +	for (cntr_id = 0; cntr_id < r->mon.num_mbm_cntrs; cntr_id++) {
>> +		if (test_bit(cntr_id, d->mbm_cntr_map)) {
>> +			rdtgrp = rdtgroup_find_grp_by_cntr_id_index(cntr_id, index);
>> +			if (rdtgrp)
>> +				resctrl_arch_update_cntr(mon_info->r, d,
>> +							 mon_info->evtid,
>> +							 rdtgrp->mon.rmid,
>> +							 rdtgrp->closid,
>> +							 cntr_id,
>> +							 mon_info->mon_config);
>> +		}
>> +	}
>> +}
> 
> Could you please add some function comments to explain the flow here? For example,
> what should reader consider if there is to rdtgroup found?

Sure.

-- 
- Babu Moger

Hi Babu,

On 11/20/24 6:14 PM, Moger, Babu wrote:
> On 11/18/2024 1:43 PM, Reinette Chatre wrote:
>> On 10/29/24 4:21 PM, Babu Moger wrote:

>>> +static void resctrl_arch_update_cntr(struct rdt_resource *r, struct rdt_mon_domain *d,
>>> +                     enum resctrl_event_id evtid, u32 rmid,
>>> +                     u32 closid, u32 cntr_id, u32 val)
>>> +{
>>> +    union l3_qos_abmc_cfg abmc_cfg = { 0 };
>>> +
>>> +    abmc_cfg.split.cfg_en = 1;
>>> +    abmc_cfg.split.cntr_en = 1;
>>> +    abmc_cfg.split.cntr_id = cntr_id;
>>> +    abmc_cfg.split.bw_src = rmid;
>>> +    abmc_cfg.split.bw_type = val;
>>> +
>>> +    wrmsrl(MSR_IA32_L3_QOS_ABMC_CFG, abmc_cfg.full);
>>
>> Is it needed to create an almost duplicate function? What if instead
>> only resctrl_arch_config_cntr() exists and it uses parameter to decide
>> whether to call resctrl_abmc_config_one_amd() directly or via
>> smp_call_function_any()? I think that should help to make clear how
>> the code flows.
>> Also note that this is an almost identical arch callback with no
>> error return. I expect that building on existing resctrl_arch_config_cntr()
>> will make things easier to understand.
> 
> It can be done. But it takes another parameter to the function.
> It has 7 parameters already. This will be 8th.
> Will change it if that is ok.

Please correct me if I am wrong but I am not familiar with a restriction on number
of parameters. It seems unnecessary to me to create two almost duplicate 7 parameter
functions to avoid one 8 parameter function.

>> Since MBM_EVENT_ARRAY_INDEX is a macro it can be called closer to where it is used,
>> within  rdtgroup_find_grp_by_cntr_id_index(), which prompts a reconsider of that function name.
> 
> 
> How about ?
> 
> static struct rdtgroup *rdtgroup_find_grp_by_cntr_id_event(int cntr_id, enum resctrl_event_id evtid)

... or for something shorter just get_rdtgroup_from_cntr_event(), but no hard requirement.

Reinette

Hi Reinette,

On 11/21/2024 2:58 PM, Reinette Chatre wrote:
> Hi Babu,
> 
> On 11/20/24 6:14 PM, Moger, Babu wrote:
>> On 11/18/2024 1:43 PM, Reinette Chatre wrote:
>>> On 10/29/24 4:21 PM, Babu Moger wrote:
> 
>>>> +static void resctrl_arch_update_cntr(struct rdt_resource *r, struct rdt_mon_domain *d,
>>>> +                     enum resctrl_event_id evtid, u32 rmid,
>>>> +                     u32 closid, u32 cntr_id, u32 val)
>>>> +{
>>>> +    union l3_qos_abmc_cfg abmc_cfg = { 0 };
>>>> +
>>>> +    abmc_cfg.split.cfg_en = 1;
>>>> +    abmc_cfg.split.cntr_en = 1;
>>>> +    abmc_cfg.split.cntr_id = cntr_id;
>>>> +    abmc_cfg.split.bw_src = rmid;
>>>> +    abmc_cfg.split.bw_type = val;
>>>> +
>>>> +    wrmsrl(MSR_IA32_L3_QOS_ABMC_CFG, abmc_cfg.full);
>>>
>>> Is it needed to create an almost duplicate function? What if instead
>>> only resctrl_arch_config_cntr() exists and it uses parameter to decide
>>> whether to call resctrl_abmc_config_one_amd() directly or via
>>> smp_call_function_any()? I think that should help to make clear how
>>> the code flows.
>>> Also note that this is an almost identical arch callback with no
>>> error return. I expect that building on existing resctrl_arch_config_cntr()
>>> will make things easier to understand.
>>
>> It can be done. But it takes another parameter to the function.
>> It has 7 parameters already. This will be 8th.
>> Will change it if that is ok.
> 
> Please correct me if I am wrong but I am not familiar with a restriction on number
> of parameters. It seems unnecessary to me to create two almost duplicate 7 parameter
> functions to avoid one 8 parameter function.

I dont see any hard requirement. Will add one parameter for smp call or 
direct call.
> 
>>> Since MBM_EVENT_ARRAY_INDEX is a macro it can be called closer to where it is used,
>>> within  rdtgroup_find_grp_by_cntr_id_index(), which prompts a reconsider of that function name.
>>
>>
>> How about ?
>>
>> static struct rdtgroup *rdtgroup_find_grp_by_cntr_id_event(int cntr_id, enum resctrl_event_id evtid)
> 
> ... or for something shorter just get_rdtgroup_from_cntr_event(), but no hard requirement.
> 
Sure
Thanks
-- 
- Babu Moger