[v5] x86/resctrl : Support AMD Assignable Bandwidth Monitoring Counters (ABMC)

[PATCH v5 10/20] x86/resctrl: Introduce mbm_total_cfg and mbm_local_cfg

Posted by Babu Moger 1 year, 7 months ago

If the BMEC (Bandwidth Monitoring Event Configuration) feature is
supported, the bandwidth events can be configured to track specific
events. The event configuration is domain specific. ABMC (Assignable
Bandwidth Monitoring Counters) feature needs event configuration
information to assign hardware counter to an RMID. Event configurations
are not stored in resctrl but instead always read from or written to
hardware directly when prompted by user space.

Read the event configuration from the hardware during the domain
initialization. Save the configuration information in the rdt_hw_domain,
so it can be used for counter assignment.

Signed-off-by: Babu Moger <babu.moger@amd.com>
---
v5: Exported mon_event_config_index_get.
    Renamed arch_domain_mbm_evt_config to resctrl_arch_mbm_evt_config.

v4: Read the configuration information from the hardware to initialize.
    Added few commit messages.
    Fixed the tab spaces.

v3: Minor changes related to rebase in mbm_config_write_domain.

v2: No changes.
---
 arch/x86/kernel/cpu/resctrl/core.c     |  2 ++
 arch/x86/kernel/cpu/resctrl/internal.h |  6 ++++++
 arch/x86/kernel/cpu/resctrl/monitor.c  | 22 ++++++++++++++++++++++
 arch/x86/kernel/cpu/resctrl/rdtgroup.c |  2 +-
 4 files changed, 31 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
index ff5cb693b396..6265ef8b610f 100644
--- a/arch/x86/kernel/cpu/resctrl/core.c
+++ b/arch/x86/kernel/cpu/resctrl/core.c
@@ -619,6 +619,8 @@ static void domain_add_cpu_mon(int cpu, struct rdt_resource *r)
 
 	arch_mon_domain_online(r, d);
 
+	resctrl_arch_mbm_evt_config(hw_dom);
+
 	if (arch_domain_mbm_alloc(r->mon.num_rmid, hw_dom)) {
 		mon_domain_free(hw_dom);
 		return;
diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
index 0ce9797f80fe..4cb1a5d014a3 100644
--- a/arch/x86/kernel/cpu/resctrl/internal.h
+++ b/arch/x86/kernel/cpu/resctrl/internal.h
@@ -401,6 +401,8 @@ struct rdt_hw_ctrl_domain {
  * @d_resctrl:	Properties exposed to the resctrl file system
  * @arch_mbm_total:	arch private state for MBM total bandwidth
  * @arch_mbm_local:	arch private state for MBM local bandwidth
+ * @mbm_total_cfg:	MBM total bandwidth configuration
+ * @mbm_local_cfg:	MBM local bandwidth configuration
  *
  * Members of this structure are accessed via helpers that provide abstraction.
  */
@@ -408,6 +410,8 @@ struct rdt_hw_mon_domain {
 	struct rdt_mon_domain		d_resctrl;
 	struct arch_mbm_state		*arch_mbm_total;
 	struct arch_mbm_state		*arch_mbm_local;
+	u32				mbm_total_cfg;
+	u32				mbm_local_cfg;
 };
 
 static inline struct rdt_hw_ctrl_domain *resctrl_to_arch_ctrl_dom(struct rdt_ctrl_domain *r)
@@ -662,6 +666,8 @@ void __check_limbo(struct rdt_mon_domain *d, bool force_free);
 void rdt_domain_reconfigure_cdp(struct rdt_resource *r);
 void __init resctrl_file_fflags_init(const char *config,
 				     unsigned long fflags);
+void resctrl_arch_mbm_evt_config(struct rdt_hw_mon_domain *hw_dom);
+unsigned int mon_event_config_index_get(u32 evtid);
 void rdt_staged_configs_clear(void);
 bool closid_allocated(unsigned int closid);
 int resctrl_find_cleanest_closid(void);
diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
index 7a93a6d2b2de..b96b0a8bd7d3 100644
--- a/arch/x86/kernel/cpu/resctrl/monitor.c
+++ b/arch/x86/kernel/cpu/resctrl/monitor.c
@@ -1256,6 +1256,28 @@ int __init rdt_get_mon_l3_config(struct rdt_resource *r)
 	return 0;
 }
 
+void resctrl_arch_mbm_evt_config(struct rdt_hw_mon_domain *hw_dom)
+{
+	unsigned int index;
+	u64 msrval;
+
+	/*
+	 * Read the configuration registers QOS_EVT_CFG_n, where <n> is
+	 * the BMEC event number (EvtID).
+	 */
+	if (mbm_total_event.configurable) {
+		index = mon_event_config_index_get(QOS_L3_MBM_TOTAL_EVENT_ID);
+		rdmsrl(MSR_IA32_EVT_CFG_BASE + index, msrval);
+		hw_dom->mbm_total_cfg = msrval & MAX_EVT_CONFIG_BITS;
+	}
+
+	if (mbm_local_event.configurable) {
+		index = mon_event_config_index_get(QOS_L3_MBM_LOCAL_EVENT_ID);
+		rdmsrl(MSR_IA32_EVT_CFG_BASE + index, msrval);
+		hw_dom->mbm_local_cfg = msrval & MAX_EVT_CONFIG_BITS;
+	}
+}
+
 void __exit rdt_put_mon_l3_config(void)
 {
 	dom_data_exit();
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index b3d3fa048f15..b2b751741dd8 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -1606,7 +1606,7 @@ struct mon_config_info {
  *         1 for evtid == QOS_L3_MBM_LOCAL_EVENT_ID
  *         INVALID_CONFIG_INDEX for invalid evtid
  */
-static inline unsigned int mon_event_config_index_get(u32 evtid)
+unsigned int mon_event_config_index_get(u32 evtid)
 {
 	switch (evtid) {
 	case QOS_L3_MBM_TOTAL_EVENT_ID:
-- 
2.34.1

Re: [PATCH v5 10/20] x86/resctrl: Introduce mbm_total_cfg and mbm_local_cfg

Posted by Reinette Chatre 1 year, 7 months ago

Hi Babu,

On 7/3/24 2:48 PM, Babu Moger wrote:
> If the BMEC (Bandwidth Monitoring Event Configuration) feature is
> supported, the bandwidth events can be configured to track specific
> events. The event configuration is domain specific. ABMC (Assignable
> Bandwidth Monitoring Counters) feature needs event configuration
> information to assign hardware counter to an RMID. Event configurations
> are not stored in resctrl but instead always read from or written to
> hardware directly when prompted by user space.
> 
> Read the event configuration from the hardware during the domain
> initialization. Save the configuration information in the rdt_hw_domain,

rdt_hw_domain -> rdt_hw_mon_domain

> so it can be used for counter assignment.
> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
> v5: Exported mon_event_config_index_get.
>      Renamed arch_domain_mbm_evt_config to resctrl_arch_mbm_evt_config.
> 
> v4: Read the configuration information from the hardware to initialize.
>      Added few commit messages.
>      Fixed the tab spaces.
> 
> v3: Minor changes related to rebase in mbm_config_write_domain.
> 
> v2: No changes.
> ---
>   arch/x86/kernel/cpu/resctrl/core.c     |  2 ++
>   arch/x86/kernel/cpu/resctrl/internal.h |  6 ++++++
>   arch/x86/kernel/cpu/resctrl/monitor.c  | 22 ++++++++++++++++++++++
>   arch/x86/kernel/cpu/resctrl/rdtgroup.c |  2 +-
>   4 files changed, 31 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
> index ff5cb693b396..6265ef8b610f 100644
> --- a/arch/x86/kernel/cpu/resctrl/core.c
> +++ b/arch/x86/kernel/cpu/resctrl/core.c
> @@ -619,6 +619,8 @@ static void domain_add_cpu_mon(int cpu, struct rdt_resource *r)
>   
>   	arch_mon_domain_online(r, d);
>   
> +	resctrl_arch_mbm_evt_config(hw_dom);
> +

This does not look to be an arch call called by the fs code so special
naming does not seem to be required? If it _was_ an arch callback then
it cannot take a HW resource as parameter since the fs code does not have
access to that.


>   	if (arch_domain_mbm_alloc(r->mon.num_rmid, hw_dom)) {
>   		mon_domain_free(hw_dom);
>   		return;
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> index 0ce9797f80fe..4cb1a5d014a3 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -401,6 +401,8 @@ struct rdt_hw_ctrl_domain {
>    * @d_resctrl:	Properties exposed to the resctrl file system
>    * @arch_mbm_total:	arch private state for MBM total bandwidth
>    * @arch_mbm_local:	arch private state for MBM local bandwidth
> + * @mbm_total_cfg:	MBM total bandwidth configuration
> + * @mbm_local_cfg:	MBM local bandwidth configuration
>    *
>    * Members of this structure are accessed via helpers that provide abstraction.
>    */
> @@ -408,6 +410,8 @@ struct rdt_hw_mon_domain {
>   	struct rdt_mon_domain		d_resctrl;
>   	struct arch_mbm_state		*arch_mbm_total;
>   	struct arch_mbm_state		*arch_mbm_local;
> +	u32				mbm_total_cfg;
> +	u32				mbm_local_cfg;
>   };
>   
>   static inline struct rdt_hw_ctrl_domain *resctrl_to_arch_ctrl_dom(struct rdt_ctrl_domain *r)
> @@ -662,6 +666,8 @@ void __check_limbo(struct rdt_mon_domain *d, bool force_free);
>   void rdt_domain_reconfigure_cdp(struct rdt_resource *r);
>   void __init resctrl_file_fflags_init(const char *config,
>   				     unsigned long fflags);
> +void resctrl_arch_mbm_evt_config(struct rdt_hw_mon_domain *hw_dom);
> +unsigned int mon_event_config_index_get(u32 evtid);
>   void rdt_staged_configs_clear(void);
>   bool closid_allocated(unsigned int closid);
>   int resctrl_find_cleanest_closid(void);
> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
> index 7a93a6d2b2de..b96b0a8bd7d3 100644
> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
> @@ -1256,6 +1256,28 @@ int __init rdt_get_mon_l3_config(struct rdt_resource *r)
>   	return 0;
>   }
>   
> +void resctrl_arch_mbm_evt_config(struct rdt_hw_mon_domain *hw_dom)

A function is expected to have a verb in its name and the verb here seems to be
"config", which does not seem appropriate and creates confusion with
resctrl_arch_event_config_set(). How about resctrl_arch_mbm_evt_config_init()
with proper initializer of the config values to also cover case when events are
not configurable (INVALID_CONFIG_VALUE introduced in next patch?) ?

> +{
> +	unsigned int index;
> +	u64 msrval;
> +
> +	/*
> +	 * Read the configuration registers QOS_EVT_CFG_n, where <n> is
> +	 * the BMEC event number (EvtID).
> +	 */
> +	if (mbm_total_event.configurable) {
> +		index = mon_event_config_index_get(QOS_L3_MBM_TOTAL_EVENT_ID);
> +		rdmsrl(MSR_IA32_EVT_CFG_BASE + index, msrval);
> +		hw_dom->mbm_total_cfg = msrval & MAX_EVT_CONFIG_BITS;
> +	}
> +
> +	if (mbm_local_event.configurable) {
> +		index = mon_event_config_index_get(QOS_L3_MBM_LOCAL_EVENT_ID);
> +		rdmsrl(MSR_IA32_EVT_CFG_BASE + index, msrval);
> +		hw_dom->mbm_local_cfg = msrval & MAX_EVT_CONFIG_BITS;
> +	}
> +}
> +
>   void __exit rdt_put_mon_l3_config(void)
>   {
>   	dom_data_exit();
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index b3d3fa048f15..b2b751741dd8 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -1606,7 +1606,7 @@ struct mon_config_info {
>    *         1 for evtid == QOS_L3_MBM_LOCAL_EVENT_ID
>    *         INVALID_CONFIG_INDEX for invalid evtid
>    */
> -static inline unsigned int mon_event_config_index_get(u32 evtid)
> +unsigned int mon_event_config_index_get(u32 evtid)
>   {
>   	switch (evtid) {
>   	case QOS_L3_MBM_TOTAL_EVENT_ID:

Reinette

Re: [PATCH v5 10/20] x86/resctrl: Introduce mbm_total_cfg and mbm_local_cfg

Posted by Moger, Babu 1 year, 6 months ago

Hi Reinette,

On 7/12/24 17:08, Reinette Chatre wrote:
> Hi Babu,
> 
> On 7/3/24 2:48 PM, Babu Moger wrote:
>> If the BMEC (Bandwidth Monitoring Event Configuration) feature is
>> supported, the bandwidth events can be configured to track specific
>> events. The event configuration is domain specific. ABMC (Assignable
>> Bandwidth Monitoring Counters) feature needs event configuration
>> information to assign hardware counter to an RMID. Event configurations
>> are not stored in resctrl but instead always read from or written to
>> hardware directly when prompted by user space.
>>
>> Read the event configuration from the hardware during the domain
>> initialization. Save the configuration information in the rdt_hw_domain,
> 
> rdt_hw_domain -> rdt_hw_mon_domain

Sure.

> 
>> so it can be used for counter assignment.
>>
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> ---
>> v5: Exported mon_event_config_index_get.
>>      Renamed arch_domain_mbm_evt_config to resctrl_arch_mbm_evt_config.
>>
>> v4: Read the configuration information from the hardware to initialize.
>>      Added few commit messages.
>>      Fixed the tab spaces.
>>
>> v3: Minor changes related to rebase in mbm_config_write_domain.
>>
>> v2: No changes.
>> ---
>>   arch/x86/kernel/cpu/resctrl/core.c     |  2 ++
>>   arch/x86/kernel/cpu/resctrl/internal.h |  6 ++++++
>>   arch/x86/kernel/cpu/resctrl/monitor.c  | 22 ++++++++++++++++++++++
>>   arch/x86/kernel/cpu/resctrl/rdtgroup.c |  2 +-
>>   4 files changed, 31 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/kernel/cpu/resctrl/core.c
>> b/arch/x86/kernel/cpu/resctrl/core.c
>> index ff5cb693b396..6265ef8b610f 100644
>> --- a/arch/x86/kernel/cpu/resctrl/core.c
>> +++ b/arch/x86/kernel/cpu/resctrl/core.c
>> @@ -619,6 +619,8 @@ static void domain_add_cpu_mon(int cpu, struct
>> rdt_resource *r)
>>         arch_mon_domain_online(r, d);
>>   +    resctrl_arch_mbm_evt_config(hw_dom);
>> +
> 
> This does not look to be an arch call called by the fs code so special
> naming does not seem to be required? If it _was_ an arch callback then

Yes. Correct.

> it cannot take a HW resource as parameter since the fs code does not have
> access to that.
> 
> 
>>       if (arch_domain_mbm_alloc(r->mon.num_rmid, hw_dom)) {
>>           mon_domain_free(hw_dom);
>>           return;
>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h
>> b/arch/x86/kernel/cpu/resctrl/internal.h
>> index 0ce9797f80fe..4cb1a5d014a3 100644
>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>> @@ -401,6 +401,8 @@ struct rdt_hw_ctrl_domain {
>>    * @d_resctrl:    Properties exposed to the resctrl file system
>>    * @arch_mbm_total:    arch private state for MBM total bandwidth
>>    * @arch_mbm_local:    arch private state for MBM local bandwidth
>> + * @mbm_total_cfg:    MBM total bandwidth configuration
>> + * @mbm_local_cfg:    MBM local bandwidth configuration
>>    *
>>    * Members of this structure are accessed via helpers that provide
>> abstraction.
>>    */
>> @@ -408,6 +410,8 @@ struct rdt_hw_mon_domain {
>>       struct rdt_mon_domain        d_resctrl;
>>       struct arch_mbm_state        *arch_mbm_total;
>>       struct arch_mbm_state        *arch_mbm_local;
>> +    u32                mbm_total_cfg;
>> +    u32                mbm_local_cfg;
>>   };
>>     static inline struct rdt_hw_ctrl_domain
>> *resctrl_to_arch_ctrl_dom(struct rdt_ctrl_domain *r)
>> @@ -662,6 +666,8 @@ void __check_limbo(struct rdt_mon_domain *d, bool
>> force_free);
>>   void rdt_domain_reconfigure_cdp(struct rdt_resource *r);
>>   void __init resctrl_file_fflags_init(const char *config,
>>                        unsigned long fflags);
>> +void resctrl_arch_mbm_evt_config(struct rdt_hw_mon_domain *hw_dom);
>> +unsigned int mon_event_config_index_get(u32 evtid);
>>   void rdt_staged_configs_clear(void);
>>   bool closid_allocated(unsigned int closid);
>>   int resctrl_find_cleanest_closid(void);
>> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c
>> b/arch/x86/kernel/cpu/resctrl/monitor.c
>> index 7a93a6d2b2de..b96b0a8bd7d3 100644
>> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
>> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
>> @@ -1256,6 +1256,28 @@ int __init rdt_get_mon_l3_config(struct
>> rdt_resource *r)
>>       return 0;
>>   }
>>   +void resctrl_arch_mbm_evt_config(struct rdt_hw_mon_domain *hw_dom)
> 
> A function is expected to have a verb in its name and the verb here seems
> to be
> "config", which does not seem appropriate and creates confusion with
> resctrl_arch_event_config_set(). How about resctrl_arch_mbm_evt_config_init()
> with proper initializer of the config values to also cover case when
> events are
> not configurable (INVALID_CONFIG_VALUE introduced in next patch?) ?

Sorry. I am not clear on this comment. Can you please elaborate?

> 
>> +{
>> +    unsigned int index;
>> +    u64 msrval;
>> +
>> +    /*
>> +     * Read the configuration registers QOS_EVT_CFG_n, where <n> is
>> +     * the BMEC event number (EvtID).
>> +     */
>> +    if (mbm_total_event.configurable) {
>> +        index = mon_event_config_index_get(QOS_L3_MBM_TOTAL_EVENT_ID);
>> +        rdmsrl(MSR_IA32_EVT_CFG_BASE + index, msrval);
>> +        hw_dom->mbm_total_cfg = msrval & MAX_EVT_CONFIG_BITS;
>> +    }
>> +
>> +    if (mbm_local_event.configurable) {
>> +        index = mon_event_config_index_get(QOS_L3_MBM_LOCAL_EVENT_ID);
>> +        rdmsrl(MSR_IA32_EVT_CFG_BASE + index, msrval);
>> +        hw_dom->mbm_local_cfg = msrval & MAX_EVT_CONFIG_BITS;
>> +    }
>> +}
>> +
>>   void __exit rdt_put_mon_l3_config(void)
>>   {
>>       dom_data_exit();
>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> index b3d3fa048f15..b2b751741dd8 100644
>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> @@ -1606,7 +1606,7 @@ struct mon_config_info {
>>    *         1 for evtid == QOS_L3_MBM_LOCAL_EVENT_ID
>>    *         INVALID_CONFIG_INDEX for invalid evtid
>>    */
>> -static inline unsigned int mon_event_config_index_get(u32 evtid)
>> +unsigned int mon_event_config_index_get(u32 evtid)
>>   {
>>       switch (evtid) {
>>       case QOS_L3_MBM_TOTAL_EVENT_ID:
> 
> Reinette
> 

-- 
Thanks
Babu Moger

Re: [PATCH v5 10/20] x86/resctrl: Introduce mbm_total_cfg and mbm_local_cfg

Posted by Reinette Chatre 1 year, 6 months ago

Hi Babu,

On 7/16/24 12:21 PM, Moger, Babu wrote:
> On 7/12/24 17:08, Reinette Chatre wrote:
>> On 7/3/24 2:48 PM, Babu Moger wrote:

>>> @@ -662,6 +666,8 @@ void __check_limbo(struct rdt_mon_domain *d, bool
>>> force_free);
>>>    void rdt_domain_reconfigure_cdp(struct rdt_resource *r);
>>>    void __init resctrl_file_fflags_init(const char *config,
>>>                         unsigned long fflags);
>>> +void resctrl_arch_mbm_evt_config(struct rdt_hw_mon_domain *hw_dom);
>>> +unsigned int mon_event_config_index_get(u32 evtid);
>>>    void rdt_staged_configs_clear(void);
>>>    bool closid_allocated(unsigned int closid);
>>>    int resctrl_find_cleanest_closid(void);
>>> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c
>>> b/arch/x86/kernel/cpu/resctrl/monitor.c
>>> index 7a93a6d2b2de..b96b0a8bd7d3 100644
>>> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
>>> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
>>> @@ -1256,6 +1256,28 @@ int __init rdt_get_mon_l3_config(struct
>>> rdt_resource *r)
>>>        return 0;
>>>    }
>>>    +void resctrl_arch_mbm_evt_config(struct rdt_hw_mon_domain *hw_dom)
>>
>> A function is expected to have a verb in its name and the verb here seems
>> to be
>> "config", which does not seem appropriate and creates confusion with
>> resctrl_arch_event_config_set(). How about resctrl_arch_mbm_evt_config_init()
>> with proper initializer of the config values to also cover case when
>> events are
>> not configurable (INVALID_CONFIG_VALUE introduced in next patch?) ?
> 
> Sorry. I am not clear on this comment. Can you please elaborate?

This comment has two parts.

First, there is the naming of the function.
The name of the function should reflect what the function does and I
believe that resctrl_arch_mbm_evt_config() is close enough to
resctrl_arch_event_config_set() to cause confusion while also lacking
an expected verb in the function name (since "config" should not be
considered a verb here) . I proposed resctrl_arch_mbm_evt_config_init()
as a new function name that has the "init" verb to indicate that this
function "init"ializes the MBM config values.

Second, there is the work done by the function.
In this implementation the function initializes
rdt_hw_mon_domain->mbm_total_cfg and rdt_hw_mon_domain->mbm_local_cfg
when the events are configurable. I proposed that as an initializer
the function can be expected to initialize rdt_hw_mon_domain->mbm_total_cfg
and rdt_hw_mon_domain->mbm_local_cfg whether the events are configurable
or not. In the latter case they can be initialized with INVALID_CONFIG_VALUE?

Reinette

Re: [PATCH v5 10/20] x86/resctrl: Introduce mbm_total_cfg and mbm_local_cfg

Posted by Moger, Babu 1 year, 6 months ago

Hi Reinette,

On 7/16/2024 3:42 PM, Reinette Chatre wrote:
> Hi Babu,
> 
> On 7/16/24 12:21 PM, Moger, Babu wrote:
>> On 7/12/24 17:08, Reinette Chatre wrote:
>>> On 7/3/24 2:48 PM, Babu Moger wrote:
> 
>>>> @@ -662,6 +666,8 @@ void __check_limbo(struct rdt_mon_domain *d, bool
>>>> force_free);
>>>>    void rdt_domain_reconfigure_cdp(struct rdt_resource *r);
>>>>    void __init resctrl_file_fflags_init(const char *config,
>>>>                         unsigned long fflags);
>>>> +void resctrl_arch_mbm_evt_config(struct rdt_hw_mon_domain *hw_dom);
>>>> +unsigned int mon_event_config_index_get(u32 evtid);
>>>>    void rdt_staged_configs_clear(void);
>>>>    bool closid_allocated(unsigned int closid);
>>>>    int resctrl_find_cleanest_closid(void);
>>>> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c
>>>> b/arch/x86/kernel/cpu/resctrl/monitor.c
>>>> index 7a93a6d2b2de..b96b0a8bd7d3 100644
>>>> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
>>>> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
>>>> @@ -1256,6 +1256,28 @@ int __init rdt_get_mon_l3_config(struct
>>>> rdt_resource *r)
>>>>        return 0;
>>>>    }
>>>>    +void resctrl_arch_mbm_evt_config(struct rdt_hw_mon_domain *hw_dom)
>>>
>>> A function is expected to have a verb in its name and the verb here 
>>> seems
>>> to be
>>> "config", which does not seem appropriate and creates confusion with
>>> resctrl_arch_event_config_set(). How about 
>>> resctrl_arch_mbm_evt_config_init()
>>> with proper initializer of the config values to also cover case when
>>> events are
>>> not configurable (INVALID_CONFIG_VALUE introduced in next patch?) ?
>>
>> Sorry. I am not clear on this comment. Can you please elaborate?
> 
> This comment has two parts.
> 
> First, there is the naming of the function.
> The name of the function should reflect what the function does and I
> believe that resctrl_arch_mbm_evt_config() is close enough to
> resctrl_arch_event_config_set() to cause confusion while also lacking
> an expected verb in the function name (since "config" should not be
> considered a verb here) . I proposed resctrl_arch_mbm_evt_config_init()
> as a new function name that has the "init" verb to indicate that this
> function "init"ializes the MBM config values.

Yes. Make sense.
> 
> Second, there is the work done by the function.
> In this implementation the function initializes
> rdt_hw_mon_domain->mbm_total_cfg and rdt_hw_mon_domain->mbm_local_cfg
> when the events are configurable. I proposed that as an initializer
> the function can be expected to initialize rdt_hw_mon_domain->mbm_total_cfg
> and rdt_hw_mon_domain->mbm_local_cfg whether the events are configurable
> or not. In the latter case they can be initialized with 
> INVALID_CONFIG_VALUE?

Yes. Thanks for the clarifications.

-- 
- Babu Moger