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.1Hi 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
© 2016 - 2024 Red Hat, Inc.