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
