The ABMC feature provides an option to the user to assign a hardware
counter to an RMID, event pair and monitor the bandwidth as long as it is
assigned. The assigned RMID will be tracked by the hardware until the user
unassigns it manually.
Configure the counters by writing to the L3_QOS_ABMC_CFG MSR and specifying
the counter ID, bandwidth source (RMID), and bandwidth event configuration.
Provide the interface to assign the counter ids to RMID.
The feature details are documented in the APM listed below [1].
[1] AMD64 Architecture Programmer's Manual Volume 2: System Programming
Publication # 24593 Revision 3.41 section 19.3.3.3 Assignable Bandwidth
Monitoring (ABMC).
Link: https://bugzilla.kernel.org/show_bug.cgi?id=206537
Signed-off-by: Babu Moger <babu.moger@amd.com>
---
v10: Added call resctrl_arch_reset_rmid() to reset the RMID in the domain
inside IPI call.
SMP and non-SMP call support is not required in resctrl_arch_config_cntr
with new domain specific assign approach/data structure.
Commit message update.
v9: Removed the code to reset the architectural state. It will done
in another patch.
v8: Rename resctrl_arch_assign_cntr to resctrl_arch_config_cntr.
v7: Separated arch and fs functions. This patch only has arch implementation.
Added struct rdt_resource to the interface resctrl_arch_assign_cntr.
Rename rdtgroup_abmc_cfg() to resctrl_abmc_config_one_amd().
v6: Removed mbm_cntr_alloc() from this patch to keep fs and arch code
separate.
Added code to update the counter assignment at domain level.
v5: Few name changes to match cntr_id.
Changed the function names to
rdtgroup_assign_cntr
resctr_arch_assign_cntr
More comments on commit log.
Added function summary.
v4: Commit message update.
User bitmap APIs where applicable.
Changed the interfaces considering MPAM(arm).
Added domain specific assignment.
v3: Removed the static from the prototype of rdtgroup_assign_abmc.
The function is not called directly from user anymore. These
changes are related to global assignment interface.
v2: Minor text changes in commit message.
---
arch/x86/kernel/cpu/resctrl/internal.h | 3 ++
arch/x86/kernel/cpu/resctrl/rdtgroup.c | 58 ++++++++++++++++++++++++++
2 files changed, 61 insertions(+)
diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
index 35bcf0e5ba7e..849bcfe4ea5b 100644
--- a/arch/x86/kernel/cpu/resctrl/internal.h
+++ b/arch/x86/kernel/cpu/resctrl/internal.h
@@ -701,5 +701,8 @@ bool closid_allocated(unsigned int closid);
int resctrl_find_cleanest_closid(void);
void arch_mbm_evt_config_init(struct rdt_hw_mon_domain *hw_dom);
unsigned int mon_event_config_index_get(u32 evtid);
+int resctrl_arch_config_cntr(struct rdt_resource *r, struct rdt_mon_domain *d,
+ enum resctrl_event_id evtid, u32 rmid, u32 closid,
+ u32 cntr_id, bool assign);
#endif /* _ASM_X86_RESCTRL_INTERNAL_H */
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index 72518e0ec2ec..e895d2415f22 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -1686,6 +1686,34 @@ unsigned int mon_event_config_index_get(u32 evtid)
}
}
+struct cntr_config {
+ struct rdt_resource *r;
+ struct rdt_mon_domain *d;
+ enum resctrl_event_id evtid;
+ u32 rmid;
+ u32 closid;
+ u32 cntr_id;
+ u32 val;
+ bool assign;
+};
+
+static void resctrl_abmc_config_one_amd(void *info)
+{
+ struct cntr_config *config = info;
+ union l3_qos_abmc_cfg abmc_cfg = { 0 };
+
+ abmc_cfg.split.cfg_en = 1;
+ abmc_cfg.split.cntr_en = config->assign ? 1 : 0;
+ abmc_cfg.split.cntr_id = config->cntr_id;
+ abmc_cfg.split.bw_src = config->rmid;
+ abmc_cfg.split.bw_type = config->val;
+
+ wrmsrl(MSR_IA32_L3_QOS_ABMC_CFG, abmc_cfg.full);
+
+ resctrl_arch_reset_rmid(config->r, config->d, config->closid,
+ config->rmid, config->evtid);
+}
+
static int mbm_config_show(struct seq_file *s, struct rdt_resource *r, u32 evtid)
{
struct rdt_mon_domain *dom;
@@ -1869,6 +1897,36 @@ static ssize_t mbm_local_bytes_config_write(struct kernfs_open_file *of,
return ret ?: nbytes;
}
+/*
+ * Send an IPI to the domain to assign the counter to RMID, event pair.
+ */
+int resctrl_arch_config_cntr(struct rdt_resource *r, struct rdt_mon_domain *d,
+ enum resctrl_event_id evtid, u32 rmid, u32 closid,
+ u32 cntr_id, bool assign)
+{
+ struct rdt_hw_mon_domain *hw_dom = resctrl_to_arch_mon_dom(d);
+ struct cntr_config config = { 0 };
+
+ config.r = r;
+ config.d = d;
+ config.evtid = evtid;
+ config.rmid = rmid;
+ config.closid = closid;
+ config.cntr_id = cntr_id;
+
+ /* Update the event configuration from the domain */
+ if (evtid == QOS_L3_MBM_TOTAL_EVENT_ID)
+ config.val = hw_dom->mbm_total_cfg;
+ else
+ config.val = hw_dom->mbm_local_cfg;
+
+ config.assign = assign;
+
+ smp_call_function_any(&d->hdr.cpu_mask, resctrl_abmc_config_one_amd, &config, 1);
+
+ return 0;
+}
+
/* rdtgroup information files for one cache resource. */
static struct rftype res_common_files[] = {
{
--
2.34.1(andipan.das@amd.com -> sandipan.das@amd.com to stop sending undeliverable emails)
Hi Babu,
On 12/12/24 12:15 PM, Babu Moger wrote:
> The ABMC feature provides an option to the user to assign a hardware
> counter to an RMID, event pair and monitor the bandwidth as long as it is
> assigned. The assigned RMID will be tracked by the hardware until the user
> unassigns it manually.
>
> Configure the counters by writing to the L3_QOS_ABMC_CFG MSR and specifying
> the counter ID, bandwidth source (RMID), and bandwidth event configuration.
>
> Provide the interface to assign the counter ids to RMID.
Until now in this series many patches "introduced interface X" and every
time it was some new resctrl file that user space interacts with. This
changelog starts with a context about "user to assign a hardware counter"
and ends with "Provide the interface", but there is no new user interface
in this patch. Can this be more specific about what this patch does?
>
> The feature details are documented in the APM listed below [1].
> [1] AMD64 Architecture Programmer's Manual Volume 2: System Programming
> Publication # 24593 Revision 3.41 section 19.3.3.3 Assignable Bandwidth
> Monitoring (ABMC).
>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=206537
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
....
> ---
> arch/x86/kernel/cpu/resctrl/internal.h | 3 ++
> arch/x86/kernel/cpu/resctrl/rdtgroup.c | 58 ++++++++++++++++++++++++++
> 2 files changed, 61 insertions(+)
>
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> index 35bcf0e5ba7e..849bcfe4ea5b 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -701,5 +701,8 @@ bool closid_allocated(unsigned int closid);
> int resctrl_find_cleanest_closid(void);
> void arch_mbm_evt_config_init(struct rdt_hw_mon_domain *hw_dom);
> unsigned int mon_event_config_index_get(u32 evtid);
> +int resctrl_arch_config_cntr(struct rdt_resource *r, struct rdt_mon_domain *d,
> + enum resctrl_event_id evtid, u32 rmid, u32 closid,
> + u32 cntr_id, bool assign);
>
> #endif /* _ASM_X86_RESCTRL_INTERNAL_H */
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index 72518e0ec2ec..e895d2415f22 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -1686,6 +1686,34 @@ unsigned int mon_event_config_index_get(u32 evtid)
> }
> }
>
> +struct cntr_config {
> + struct rdt_resource *r;
> + struct rdt_mon_domain *d;
> + enum resctrl_event_id evtid;
> + u32 rmid;
> + u32 closid;
> + u32 cntr_id;
> + u32 val;
> + bool assign;
> +};
I think I am missing something because it is not clear to me why this
new struct is needed. Why not just use union l3_qos_abmc_cfg?
If it is indeed needed it needs better formatting and clear descriptions,
a member like "val" is very generic.
> +
> +static void resctrl_abmc_config_one_amd(void *info)
> +{
> + struct cntr_config *config = info;
> + union l3_qos_abmc_cfg abmc_cfg = { 0 };
> +
reverse fir
> + abmc_cfg.split.cfg_en = 1;
> + abmc_cfg.split.cntr_en = config->assign ? 1 : 0;
> + abmc_cfg.split.cntr_id = config->cntr_id;
> + abmc_cfg.split.bw_src = config->rmid;
> + abmc_cfg.split.bw_type = config->val;
> +
> + wrmsrl(MSR_IA32_L3_QOS_ABMC_CFG, abmc_cfg.full);
> +
> + resctrl_arch_reset_rmid(config->r, config->d, config->closid,
> + config->rmid, config->evtid);
> +}
> +
> static int mbm_config_show(struct seq_file *s, struct rdt_resource *r, u32 evtid)
> {
> struct rdt_mon_domain *dom;
> @@ -1869,6 +1897,36 @@ static ssize_t mbm_local_bytes_config_write(struct kernfs_open_file *of,
> return ret ?: nbytes;
> }
>
> +/*
> + * Send an IPI to the domain to assign the counter to RMID, event pair.
> + */
> +int resctrl_arch_config_cntr(struct rdt_resource *r, struct rdt_mon_domain *d,
> + enum resctrl_event_id evtid, u32 rmid, u32 closid,
> + u32 cntr_id, bool assign)
> +{
> + struct rdt_hw_mon_domain *hw_dom = resctrl_to_arch_mon_dom(d);
> + struct cntr_config config = { 0 };
Please see 29eaa7958367 ("x86/resctrl: Slightly clean-up mbm_config_show()")
> +
> + config.r = r;
> + config.d = d;
> + config.evtid = evtid;
> + config.rmid = rmid;
> + config.closid = closid;
> + config.cntr_id = cntr_id;
> +
> + /* Update the event configuration from the domain */
> + if (evtid == QOS_L3_MBM_TOTAL_EVENT_ID)
> + config.val = hw_dom->mbm_total_cfg;
> + else
> + config.val = hw_dom->mbm_local_cfg;
> +
> + config.assign = assign;
> +
> + smp_call_function_any(&d->hdr.cpu_mask, resctrl_abmc_config_one_amd, &config, 1);
> +
> + return 0;
> +}
> +
> /* rdtgroup information files for one cache resource. */
> static struct rftype res_common_files[] = {
> {
Reinette
Hi Reinette,
On 12/19/2024 5:04 PM, Reinette Chatre wrote:
> (andipan.das@amd.com -> sandipan.das@amd.com to stop sending undeliverable emails)
Yes.
>
> Hi Babu,
>
> On 12/12/24 12:15 PM, Babu Moger wrote:
>> The ABMC feature provides an option to the user to assign a hardware
>> counter to an RMID, event pair and monitor the bandwidth as long as it is
>> assigned. The assigned RMID will be tracked by the hardware until the user
>> unassigns it manually.
>>
>> Configure the counters by writing to the L3_QOS_ABMC_CFG MSR and specifying
>> the counter ID, bandwidth source (RMID), and bandwidth event configuration.
>>
>> Provide the interface to assign the counter ids to RMID.
>
> Until now in this series many patches "introduced interface X" and every
> time it was some new resctrl file that user space interacts with. This
> changelog starts with a context about "user to assign a hardware counter"
> and ends with "Provide the interface", but there is no new user interface
> in this patch. Can this be more specific about what this patch does?
Yes. This should be about resctrl_arch_config_cntr(). How about this?
The ABMC feature provides an option to the user to assign a hardware
counter to an RMID, event pair and monitor the bandwidth as long as it
is assigned. The assigned RMID will be tracked by the hardware until the
user unassigns it manually.
Provide the architecture specific handler to to assign/unassign the
counter. Counters are configured by writing to the L3_QOS_ABMC_CFG MSR
and specifying the counter ID, bandwidth source (RMID), and bandwidth
event configuration.
>
>>
>> The feature details are documented in the APM listed below [1].
>> [1] AMD64 Architecture Programmer's Manual Volume 2: System Programming
>> Publication # 24593 Revision 3.41 section 19.3.3.3 Assignable Bandwidth
>> Monitoring (ABMC).
>>
>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=206537
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> ---
>
> ....
>
>> ---
>> arch/x86/kernel/cpu/resctrl/internal.h | 3 ++
>> arch/x86/kernel/cpu/resctrl/rdtgroup.c | 58 ++++++++++++++++++++++++++
>> 2 files changed, 61 insertions(+)
>>
>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
>> index 35bcf0e5ba7e..849bcfe4ea5b 100644
>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>> @@ -701,5 +701,8 @@ bool closid_allocated(unsigned int closid);
>> int resctrl_find_cleanest_closid(void);
>> void arch_mbm_evt_config_init(struct rdt_hw_mon_domain *hw_dom);
>> unsigned int mon_event_config_index_get(u32 evtid);
>> +int resctrl_arch_config_cntr(struct rdt_resource *r, struct rdt_mon_domain *d,
>> + enum resctrl_event_id evtid, u32 rmid, u32 closid,
>> + u32 cntr_id, bool assign);
>>
>> #endif /* _ASM_X86_RESCTRL_INTERNAL_H */
>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> index 72518e0ec2ec..e895d2415f22 100644
>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> @@ -1686,6 +1686,34 @@ unsigned int mon_event_config_index_get(u32 evtid)
>> }
>> }
>>
>> +struct cntr_config {
>> + struct rdt_resource *r;
>> + struct rdt_mon_domain *d;
>> + enum resctrl_event_id evtid;
>> + u32 rmid;
>> + u32 closid;
>> + u32 cntr_id;
>> + u32 val;
>> + bool assign;
>> +};
>
> I think I am missing something because it is not clear to me why this
> new struct is needed. Why not just use union l3_qos_abmc_cfg?
New struct is needed because we need to call resctrl_arch_reset_rmid()
inside IPI. It requires all these parameters.
void resctrl_arch_reset_rmid(struct rdt_resource *r, struct
rdt_mon_domain *d, u32 closid, u32 rmid,
enum resctrl_event_id eventid);
>
> If it is indeed needed it needs better formatting and clear descriptions,
> a member like "val" is very generic.
Sure. Will change it.
>
>> +
>> +static void resctrl_abmc_config_one_amd(void *info)
>> +{
>> + struct cntr_config *config = info;
>> + union l3_qos_abmc_cfg abmc_cfg = { 0 };
>> +
>
> reverse fir
Sure.
>
>> + abmc_cfg.split.cfg_en = 1;
>> + abmc_cfg.split.cntr_en = config->assign ? 1 : 0;
>> + abmc_cfg.split.cntr_id = config->cntr_id;
>> + abmc_cfg.split.bw_src = config->rmid;
>> + abmc_cfg.split.bw_type = config->val;
>> +
>> + wrmsrl(MSR_IA32_L3_QOS_ABMC_CFG, abmc_cfg.full);
>> +
>> + resctrl_arch_reset_rmid(config->r, config->d, config->closid,
>> + config->rmid, config->evtid);
>> +}
>> +
>> static int mbm_config_show(struct seq_file *s, struct rdt_resource *r, u32 evtid)
>> {
>> struct rdt_mon_domain *dom;
>> @@ -1869,6 +1897,36 @@ static ssize_t mbm_local_bytes_config_write(struct kernfs_open_file *of,
>> return ret ?: nbytes;
>> }
>>
>> +/*
>> + * Send an IPI to the domain to assign the counter to RMID, event pair.
>> + */
>> +int resctrl_arch_config_cntr(struct rdt_resource *r, struct rdt_mon_domain *d,
>> + enum resctrl_event_id evtid, u32 rmid, u32 closid,
>> + u32 cntr_id, bool assign)
>> +{
>> + struct rdt_hw_mon_domain *hw_dom = resctrl_to_arch_mon_dom(d);
>> + struct cntr_config config = { 0 };
>
> Please see 29eaa7958367 ("x86/resctrl: Slightly clean-up mbm_config_show()")
This may not apply here.
x86/resctrl: Slightly clean-up mbm_config_show()
"mon_info' is already zeroed in the list_for_each_entry() loop below.
There is no need to explicitly initialize it here. It just wastes some
space and cycles.
In our case we are not doing memset again.
Thanks
Babu
>
>> +
>> + config.r = r;
>> + config.d = d;
>> + config.evtid = evtid;
>> + config.rmid = rmid;
>> + config.closid = closid;
>> + config.cntr_id = cntr_id;
>> +
>> + /* Update the event configuration from the domain */
>> + if (evtid == QOS_L3_MBM_TOTAL_EVENT_ID)
>> + config.val = hw_dom->mbm_total_cfg;
>> + else
>> + config.val = hw_dom->mbm_local_cfg;
>> +
>> + config.assign = assign;
>> +
>> + smp_call_function_any(&d->hdr.cpu_mask, resctrl_abmc_config_one_amd, &config, 1);
>> +
>> + return 0;
>> +}
>> +
>> /* rdtgroup information files for one cache resource. */
>> static struct rftype res_common_files[] = {
>> {
>
> Reinette
>
Hi Babu,
On 12/20/24 11:22 AM, Moger, Babu wrote:
> Hi Reinette,
>
> On 12/19/2024 5:04 PM, Reinette Chatre wrote:
>> (andipan.das@amd.com -> sandipan.das@amd.com to stop sending undeliverable emails)
>
> Yes.
>
>>
>> Hi Babu,
>>
>> On 12/12/24 12:15 PM, Babu Moger wrote:
>>> The ABMC feature provides an option to the user to assign a hardware
>>> counter to an RMID, event pair and monitor the bandwidth as long as it is
>>> assigned. The assigned RMID will be tracked by the hardware until the user
>>> unassigns it manually.
>>>
>>> Configure the counters by writing to the L3_QOS_ABMC_CFG MSR and specifying
>>> the counter ID, bandwidth source (RMID), and bandwidth event configuration.
>>>
>>> Provide the interface to assign the counter ids to RMID.
>>
>> Until now in this series many patches "introduced interface X" and every
>> time it was some new resctrl file that user space interacts with. This
>> changelog starts with a context about "user to assign a hardware counter"
>> and ends with "Provide the interface", but there is no new user interface
>> in this patch. Can this be more specific about what this patch does?
>
> Yes. This should be about resctrl_arch_config_cntr(). How about this?
>
> The ABMC feature provides an option to the user to assign a hardware
> counter to an RMID, event pair and monitor the bandwidth as long as it is assigned. The assigned RMID will be tracked by the hardware until the user unassigns it manually.
>
> Provide the architecture specific handler to to assign/unassign the counter. Counters are configured by writing to the L3_QOS_ABMC_CFG MSR and specifying the counter ID, bandwidth source (RMID), and bandwidth event configuration.
Again just one sentence appended. The "to to" demonstrates it is another
example of something typed quickly to see if it sticks.
>>> @@ -1686,6 +1686,34 @@ unsigned int mon_event_config_index_get(u32 evtid)
>>> }
>>> }
>>> +struct cntr_config {
>>> + struct rdt_resource *r;
>>> + struct rdt_mon_domain *d;
>>> + enum resctrl_event_id evtid;
>>> + u32 rmid;
>>> + u32 closid;
>>> + u32 cntr_id;
>>> + u32 val;
>>> + bool assign;
>>> +};
>>
>> I think I am missing something because it is not clear to me why this
>> new struct is needed. Why not just use union l3_qos_abmc_cfg?
>
> New struct is needed because we need to call resctrl_arch_reset_rmid() inside IPI. It requires all these parameters.
Could you please answer my question?
>>> @@ -1869,6 +1897,36 @@ static ssize_t mbm_local_bytes_config_write(struct kernfs_open_file *of,
>>> return ret ?: nbytes;
>>> }
>>> +/*
>>> + * Send an IPI to the domain to assign the counter to RMID, event pair.
>>> + */
>>> +int resctrl_arch_config_cntr(struct rdt_resource *r, struct rdt_mon_domain *d,
>>> + enum resctrl_event_id evtid, u32 rmid, u32 closid,
>>> + u32 cntr_id, bool assign)
>>> +{
>>> + struct rdt_hw_mon_domain *hw_dom = resctrl_to_arch_mon_dom(d);
>>> + struct cntr_config config = { 0 };
>>
>> Please see 29eaa7958367 ("x86/resctrl: Slightly clean-up mbm_config_show()")
>
> This may not apply here.
>
> x86/resctrl: Slightly clean-up mbm_config_show()
>
> "mon_info' is already zeroed in the list_for_each_entry() loop below. There is no need to explicitly initialize it here. It just wastes some space and cycles.
>
> In our case we are not doing memset again.
No, but every member is explicitly initialized instead. It may be needed if
union l3_qos_abmc_cfg is used as I asked about earlier where it will be important
to ensure reserve bits are initialized.
Reinette
Hi Reinette,
On 12/20/2024 3:41 PM, Reinette Chatre wrote:
> Hi Babu,
>
> On 12/20/24 11:22 AM, Moger, Babu wrote:
>> Hi Reinette,
>>
>> On 12/19/2024 5:04 PM, Reinette Chatre wrote:
>>> (andipan.das@amd.com -> sandipan.das@amd.com to stop sending undeliverable emails)
>>
>> Yes.
>>
>>>
>>> Hi Babu,
>>>
>>> On 12/12/24 12:15 PM, Babu Moger wrote:
>>>> The ABMC feature provides an option to the user to assign a hardware
>>>> counter to an RMID, event pair and monitor the bandwidth as long as it is
>>>> assigned. The assigned RMID will be tracked by the hardware until the user
>>>> unassigns it manually.
>>>>
>>>> Configure the counters by writing to the L3_QOS_ABMC_CFG MSR and specifying
>>>> the counter ID, bandwidth source (RMID), and bandwidth event configuration.
>>>>
>>>> Provide the interface to assign the counter ids to RMID.
>>>
>>> Until now in this series many patches "introduced interface X" and every
>>> time it was some new resctrl file that user space interacts with. This
>>> changelog starts with a context about "user to assign a hardware counter"
>>> and ends with "Provide the interface", but there is no new user interface
>>> in this patch. Can this be more specific about what this patch does?
>>
>> Yes. This should be about resctrl_arch_config_cntr(). How about this?
>>
>> The ABMC feature provides an option to the user to assign a hardware
>> counter to an RMID, event pair and monitor the bandwidth as long as it is assigned. The assigned RMID will be tracked by the hardware until the user unassigns it manually.
>>
>> Provide the architecture specific handler to to assign/unassign the counter. Counters are configured by writing to the L3_QOS_ABMC_CFG MSR and specifying the counter ID, bandwidth source (RMID), and bandwidth event configuration.
>
> Again just one sentence appended. The "to to" demonstrates it is another
> example of something typed quickly to see if it sticks.
My bad. Will rewrite this.
>
>
>>>> @@ -1686,6 +1686,34 @@ unsigned int mon_event_config_index_get(u32 evtid)
>>>> }
>>>> }
>>>> +struct cntr_config {
>>>> + struct rdt_resource *r;
>>>> + struct rdt_mon_domain *d;
>>>> + enum resctrl_event_id evtid;
>>>> + u32 rmid;
>>>> + u32 closid;
>>>> + u32 cntr_id;
>>>> + u32 val;
>>>> + bool assign;
>>>> +};
>>>
>>> I think I am missing something because it is not clear to me why this
>>> new struct is needed. Why not just use union l3_qos_abmc_cfg?
>>
>> New struct is needed because we need to call resctrl_arch_reset_rmid() inside IPI. It requires all these parameters.
>
> Could you please answer my question?
May be I did not understand your question here.
We need to do couple of things here in the IPI.
1. Configure the counter. This requires the cntr_id, rmid, event config
value and assign(or unassign). This is to populate l3_qos_abmc_cfg and
write the MSR.
2. Reset RMID. This requires rdt_resource, rdt_mon_domain, RMID, CLOSID
and event.
So, I packed all these in a new structure and sent to IPI handler so
that both these actions can be done in IPI.
Can this be simplified?
>
>>>> @@ -1869,6 +1897,36 @@ static ssize_t mbm_local_bytes_config_write(struct kernfs_open_file *of,
>>>> return ret ?: nbytes;
>>>> }
>>>> +/*
>>>> + * Send an IPI to the domain to assign the counter to RMID, event pair.
>>>> + */
>>>> +int resctrl_arch_config_cntr(struct rdt_resource *r, struct rdt_mon_domain *d,
>>>> + enum resctrl_event_id evtid, u32 rmid, u32 closid,
>>>> + u32 cntr_id, bool assign)
>>>> +{
>>>> + struct rdt_hw_mon_domain *hw_dom = resctrl_to_arch_mon_dom(d);
>>>> + struct cntr_config config = { 0 };
>>>
>>> Please see 29eaa7958367 ("x86/resctrl: Slightly clean-up mbm_config_show()")
>>
>> This may not apply here.
>>
>> x86/resctrl: Slightly clean-up mbm_config_show()
>>
>> "mon_info' is already zeroed in the list_for_each_entry() loop below. There is no need to explicitly initialize it here. It just wastes some space and cycles.
>>
>> In our case we are not doing memset again.
>
> No, but every member is explicitly initialized instead. It may be needed if
> union l3_qos_abmc_cfg is used as I asked about earlier where it will be important
> to ensure reserve bits are initialized.
I missed your comment on reserve bits(Searched in this series). General
rule is reserve bits should be written as zeros.
Thanks
Babu
>
> Reinette
>
Hi Babu,
On 12/20/24 2:28 PM, Moger, Babu wrote:
> On 12/20/2024 3:41 PM, Reinette Chatre wrote:
>> On 12/20/24 11:22 AM, Moger, Babu wrote:
>>> On 12/19/2024 5:04 PM, Reinette Chatre wrote:
>>>>> @@ -1686,6 +1686,34 @@ unsigned int mon_event_config_index_get(u32 evtid)
>>>>> }
>>>>> }
>>>>> +struct cntr_config {
>>>>> + struct rdt_resource *r;
>>>>> + struct rdt_mon_domain *d;
>>>>> + enum resctrl_event_id evtid;
>>>>> + u32 rmid;
>>>>> + u32 closid;
>>>>> + u32 cntr_id;
>>>>> + u32 val;
>>>>> + bool assign;
>>>>> +};
>>>>
>>>> I think I am missing something because it is not clear to me why this
>>>> new struct is needed. Why not just use union l3_qos_abmc_cfg?
>>>
>>> New struct is needed because we need to call resctrl_arch_reset_rmid() inside IPI. It requires all these parameters.
>>
>> Could you please answer my question?
>
> May be I did not understand your question here.
>
> We need to do couple of things here in the IPI.
>
> 1. Configure the counter. This requires the cntr_id, rmid, event config value and assign(or unassign). This is to populate l3_qos_abmc_cfg and write the MSR.
>
> 2. Reset RMID. This requires rdt_resource, rdt_mon_domain, RMID, CLOSID and event.
>
> So, I packed all these in a new structure and sent to IPI handler so that both these actions can be done in IPI.
>
> Can this be simplified?
This is all architecture specific code so I think l3_qos_abmc_cfg can be
initialized once and then passed around. Bouncing the individual members of
l3_qos_abmc_cfg through struct cntr_config seems unnecessary to me. More specifically,
would it not make things simpler to make l3_qos_abmc_cfg a member of cntr_config?
>>>>> @@ -1869,6 +1897,36 @@ static ssize_t mbm_local_bytes_config_write(struct kernfs_open_file *of,
>>>>> return ret ?: nbytes;
>>>>> }
>>>>> +/*
>>>>> + * Send an IPI to the domain to assign the counter to RMID, event pair.
>>>>> + */
>>>>> +int resctrl_arch_config_cntr(struct rdt_resource *r, struct rdt_mon_domain *d,
>>>>> + enum resctrl_event_id evtid, u32 rmid, u32 closid,
>>>>> + u32 cntr_id, bool assign)
>>>>> +{
>>>>> + struct rdt_hw_mon_domain *hw_dom = resctrl_to_arch_mon_dom(d);
>>>>> + struct cntr_config config = { 0 };
>>>>
>>>> Please see 29eaa7958367 ("x86/resctrl: Slightly clean-up mbm_config_show()")
>>>
>>> This may not apply here.
>>>
>>> x86/resctrl: Slightly clean-up mbm_config_show()
>>>
>>> "mon_info' is already zeroed in the list_for_each_entry() loop below. There is no need to explicitly initialize it here. It just wastes some space and cycles.
>>>
>>> In our case we are not doing memset again.
>>
>> No, but every member is explicitly initialized instead. It may be needed if
>> union l3_qos_abmc_cfg is used as I asked about earlier where it will be important
>> to ensure reserve bits are initialized.
>
> I missed your comment on reserve bits(Searched in this series). General rule is reserve bits should be written as zeros.
I do not think I am being clear.
Back to original comment: resctrl_arch_config_cntr() zeroes the entire struct and then
initializes every member. I do not think it is necessary to zero the struct if
every member is initialized. If you want to be explicit about the zero initialization
you can do so while initializing the struct only once where it is defined.
See for example, rdtgroup_kn_set_ugid()
Reinette
Hi Reinette,
On 12/20/2024 5:47 PM, Reinette Chatre wrote:
> Hi Babu,
>
> On 12/20/24 2:28 PM, Moger, Babu wrote:
>> On 12/20/2024 3:41 PM, Reinette Chatre wrote:
>>> On 12/20/24 11:22 AM, Moger, Babu wrote:
>>>> On 12/19/2024 5:04 PM, Reinette Chatre wrote:
>
>>>>>> @@ -1686,6 +1686,34 @@ unsigned int mon_event_config_index_get(u32 evtid)
>>>>>> }
>>>>>> }
>>>>>> +struct cntr_config {
>>>>>> + struct rdt_resource *r;
>>>>>> + struct rdt_mon_domain *d;
>>>>>> + enum resctrl_event_id evtid;
>>>>>> + u32 rmid;
>>>>>> + u32 closid;
>>>>>> + u32 cntr_id;
>>>>>> + u32 val;
>>>>>> + bool assign;
>>>>>> +};
>>>>>
>>>>> I think I am missing something because it is not clear to me why this
>>>>> new struct is needed. Why not just use union l3_qos_abmc_cfg?
>>>>
>>>> New struct is needed because we need to call resctrl_arch_reset_rmid() inside IPI. It requires all these parameters.
>>>
>>> Could you please answer my question?
>>
>> May be I did not understand your question here.
>>
>> We need to do couple of things here in the IPI.
>>
>> 1. Configure the counter. This requires the cntr_id, rmid, event config value and assign(or unassign). This is to populate l3_qos_abmc_cfg and write the MSR.
>>
>> 2. Reset RMID. This requires rdt_resource, rdt_mon_domain, RMID, CLOSID and event.
>>
>> So, I packed all these in a new structure and sent to IPI handler so that both these actions can be done in IPI.
>>
>> Can this be simplified?
>
> This is all architecture specific code so I think l3_qos_abmc_cfg can be
> initialized once and then passed around. Bouncing the individual members of
> l3_qos_abmc_cfg through struct cntr_config seems unnecessary to me. More specifically,
> would it not make things simpler to make l3_qos_abmc_cfg a member of cntr_config?
Yes. It can be done.
>
>>>>>> @@ -1869,6 +1897,36 @@ static ssize_t mbm_local_bytes_config_write(struct kernfs_open_file *of,
>>>>>> return ret ?: nbytes;
>>>>>> }
>>>>>> +/*
>>>>>> + * Send an IPI to the domain to assign the counter to RMID, event pair.
>>>>>> + */
>>>>>> +int resctrl_arch_config_cntr(struct rdt_resource *r, struct rdt_mon_domain *d,
>>>>>> + enum resctrl_event_id evtid, u32 rmid, u32 closid,
>>>>>> + u32 cntr_id, bool assign)
>>>>>> +{
>>>>>> + struct rdt_hw_mon_domain *hw_dom = resctrl_to_arch_mon_dom(d);
>>>>>> + struct cntr_config config = { 0 };
>>>>>
>>>>> Please see 29eaa7958367 ("x86/resctrl: Slightly clean-up mbm_config_show()")
>>>>
>>>> This may not apply here.
>>>>
>>>> x86/resctrl: Slightly clean-up mbm_config_show()
>>>>
>>>> "mon_info' is already zeroed in the list_for_each_entry() loop below. There is no need to explicitly initialize it here. It just wastes some space and cycles.
>>>>
>>>> In our case we are not doing memset again.
>>>
>>> No, but every member is explicitly initialized instead. It may be needed if
>>> union l3_qos_abmc_cfg is used as I asked about earlier where it will be important
>>> to ensure reserve bits are initialized.
>>
>> I missed your comment on reserve bits(Searched in this series). General rule is reserve bits should be written as zeros.
>
>
> I do not think I am being clear.
>
> Back to original comment: resctrl_arch_config_cntr() zeroes the entire struct and then
> initializes every member. I do not think it is necessary to zero the struct if
> every member is initialized. If you want to be explicit about the zero initialization
> you can do so while initializing the struct only once where it is defined.
> See for example, rdtgroup_kn_set_ugid()
Yes. I got it. It was not required as we are initializing all the
members of config here.
With adding l3_qos_abmc_cfg inside cntr_config, we may still have to
keep it.
Thanks
Babu
© 2016 - 2025 Red Hat, Inc.