Hi Babu,
On 12/12/24 12:15 PM, Babu Moger wrote:
> @@ -1604,33 +1645,11 @@ unsigned int mon_event_config_index_get(u32 evtid)
> }
> }
>
> -static void mon_event_config_read(void *info)
> -{
> - struct mon_config_info *mon_info = info;
> - unsigned int index;
> - u64 msrval;
> -
> - index = mon_event_config_index_get(mon_info->evtid);
> - if (index == INVALID_CONFIG_INDEX) {
> - pr_warn_once("Invalid event id %d\n", mon_info->evtid);
> - return;
> - }
> - rdmsrl(MSR_IA32_EVT_CFG_BASE + index, msrval);
> -
> - /* Report only the valid event configuration bits */
> - mon_info->mon_config = msrval & MAX_EVT_CONFIG_BITS;
> -}
> -
> -static void mondata_config_read(struct rdt_mon_domain *d, struct mon_config_info *mon_info)
> -{
> - smp_call_function_any(&d->hdr.cpu_mask, mon_event_config_read, mon_info, 1);
> -}
> -
> static int mbm_config_show(struct seq_file *s, struct rdt_resource *r, u32 evtid)
> {
> - struct mon_config_info mon_info;
> struct rdt_mon_domain *dom;
> bool sep = false;
> + u32 val;
Could this variable name be more descriptive? For example, mon_config, or config_val as
used in mbm_config_write_domain()?
...
> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
> index f11d6fdfd977..c8ab3d7a0dab 100644
> --- a/include/linux/resctrl.h
> +++ b/include/linux/resctrl.h
> @@ -118,6 +118,18 @@ struct rdt_mon_domain {
> int cqm_work_cpu;
> };
>
> +/**
> + * struct mon_config_info - Monitoring event configuratiin details
configuratiin -> configuration
... but actually, the motivation for moving this struct here was
to make it available for an arch to interpret the data passed
via resctrl_arch_mon_event_config_set(). This patch passes data
in this struct but a later patch modifies
resctrl_arch_mon_event_config_set() to not use struct anymore ...
and then leaves struct mon_config_info here.
Even so, considering Boris's preference this is no longer needed.
> + * @d: Domain for the event
> + * @evtid: Event type
> + * @mon_config: Event configuration value
> + */
> +struct mon_config_info {
> + struct rdt_mon_domain *d;
> + enum resctrl_event_id evtid;
> + u32 mon_config;
> +};
> +
> /**
> * struct resctrl_cache - Cache allocation related data
> * @cbm_len: Length of the cache bit mask
> @@ -352,6 +364,10 @@ 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);
> +u32 resctrl_arch_mon_event_config_get(struct rdt_mon_domain *d,
> + enum resctrl_event_id eventid);
> +
Please move to internal header file instead and consider this for
all changes to include/linux/resctrl.h
> extern unsigned int resctrl_rmid_realloc_threshold;
> extern unsigned int resctrl_rmid_realloc_limit;
>
Reinette
