Re: [PATCH v4 01/31] x86,fs/resctrl: Drop rdt_mon_features variable

[Reinette Chatre <reinette.chatre@intel.com>]

Hi Tony,

On 4/28/25 5:33 PM, Tony Luck wrote:
> The fs/arch boundary is a little muddy for adding new monitor features.

It is not possible to accurately interpret what is meant with "little muddy".
Please add specific information that can be verified/reasoned about.

> 
> Clean it up by making the mon_evt structure the source of all information
> about each event. In this case replace the bitmap of enabled monitor
> features with an "enabled" bit in the mon_evt structure.

bit -> boolean?

> 
> Change architecture code to inform file system code which events are
> available on a system with resctrl_enable_mon_event().

(nit: no need to mention that a patch changes code, it should be implied.)

This could be, "An architecture uses resctrl_enable_mon_event() to inform
resctrl fs which events are enabled on the system."

(I think we need to be cautious about the "available" vs "enabled"
distinction.)

> 
> Replace the event and architecture specific:
> 	resctrl_arch_is_llc_occupancy_enabled()
> 	resctrl_arch_is_mbm_total_enabled()
> 	resctrl_arch_is_mbm_local_enabled()
> functions with calls to resctrl_is_mon_event_enabled() with the
> appropriate QOS_L3_* enum resctrl_event_id.

No mention or motivation for the new array. I think the new array is an
improvement and now it begs the question whether rdt_resource::evt_list is
still needed? It seems to me that any usage of rdt_resource::evt_list can
use the new mon_event_all[] instead?
With struct mon_evt being independent like before this
patch it almost seems as though it prepared for multiple resources to
support the same event (do you know history here?). This appears to already
be thwarted by rdt_mon_features though ... although theoretically it could
have been "rdt_l3_mon_features".
Even so, with patch #4 adding the resource ID all event information is
centralized. Only potential issue may be if multiple resources use the
same event ... but since the existing event IDs already have resource
name embedded this does not seem to be of concern?

> 
> Signed-off-by: Tony Luck <tony.luck@intel.com>
> ---

...

> @@ -866,14 +879,13 @@ static struct mon_evt mbm_local_event = {
>   */
>  static void l3_mon_evt_init(struct rdt_resource *r)
>  {
> +	enum resctrl_event_id evt;
> +
>  	INIT_LIST_HEAD(&r->evt_list);
>  
> -	if (resctrl_arch_is_llc_occupancy_enabled())
> -		list_add_tail(&llc_occupancy_event.list, &r->evt_list);
> -	if (resctrl_arch_is_mbm_total_enabled())
> -		list_add_tail(&mbm_total_event.list, &r->evt_list);
> -	if (resctrl_arch_is_mbm_local_enabled())
> -		list_add_tail(&mbm_local_event.list, &r->evt_list);
> +	for (evt = 0; evt < QOS_NUM_EVENTS; evt++)
> +		if (mon_event_all[evt].enabled)
> +			list_add_tail(&mon_event_all[evt].list, &r->evt_list);
>  }

This hunk can create confusion with it adding "all enabled events" to
a single resource. I understand that at this point only L3 supports monitoring
and this works ok, but in the context of this work it creates a caveat early
in series that needs to be fixed later (patch #4). This wrangling becomes
unnecessary if removing rdt_resource::evt_list.

Reinette