Re: [PATCH 2/7] x86/resctrl: Remove hard code of RDT_RESOURCE_L3 in monitor.c

["Yu, Fenghua" <fenghua.yu@intel.com>]

Hi, Tony,

On 1/26/23 10:41, Tony Luck wrote:
> Scope of monitoring may be scoped at L3 cache granularity (legacy) or
> at the node level (systems with Sub NUMA Cluster enabled).
> 
> Save the struct rdt_resource pointer that was used to initialize
> the monitor sections of code and use that value instead of the
> hard-coded RDT_RESOURCE_L3.
> 
> No functional change.
> 
> Signed-off-by: Tony Luck <tony.luck@intel.com>
> ---
>   arch/x86/kernel/cpu/resctrl/monitor.c | 18 ++++++++++++------
>   1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
> index 77538abeb72a..d05bbd4f6b2d 100644
> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
> @@ -30,6 +30,8 @@ struct rmid_entry {
>   	struct list_head		list;
>   };
>   
> +static struct rdt_resource *mon_resource;
> +
>   /**
>    * @rmid_free_lru    A least recently used list of free RMIDs
>    *     These RMIDs are guaranteed to have an occupancy less than the
> @@ -251,7 +253,7 @@ int resctrl_arch_rmid_read(struct rdt_resource *r, struct rdt_domain *d,
>    */
>   void __check_limbo(struct rdt_domain *d, bool force_free)
>   {
> -	struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;
> +	struct rdt_resource *r = mon_resource;
>   	struct rmid_entry *entry;
>   	u32 crmid = 1, nrmid;
>   	bool rmid_dirty;
> @@ -316,7 +318,7 @@ int alloc_rmid(void)
>   
>   static void add_rmid_to_limbo(struct rmid_entry *entry)
>   {
> -	struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;
> +	struct rdt_resource *r = mon_resource;
>   	struct rdt_domain *d;
>   	int cpu, err;
>   	u64 val = 0;
> @@ -633,7 +635,7 @@ void cqm_handle_limbo(struct work_struct *work)
>   
>   	mutex_lock(&rdtgroup_mutex);
>   
> -	r = &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;
> +	r = mon_resource;
>   	d = container_of(work, struct rdt_domain, cqm_limbo.work);
>   
>   	__check_limbo(d, false);
> @@ -669,7 +671,7 @@ void mbm_handle_overflow(struct work_struct *work)
>   	if (!static_branch_likely(&rdt_mon_enable_key))
>   		goto out_unlock;
>   
> -	r = &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;
> +	r = mon_resource;
>   	d = container_of(work, struct rdt_domain, mbm_over.work);
>   
>   	list_for_each_entry(prgrp, &rdt_all_groups, rdtgroup_list) {
> @@ -747,9 +749,11 @@ static struct mon_evt mbm_local_event = {
>   /*
>    * Initialize the event list for the resource.
>    *
> - * Note that MBM events are also part of RDT_RESOURCE_L3 resource
> + * Note that MBM events can either be part of RDT_RESOURCE_L3 resource
>    * because as per the SDM the total and local memory bandwidth
> - * are enumerated as part of L3 monitoring.
> + * are enumerated as part of L3 monitoring, or they may be per NUMA
> + * node on systems with sub-NUMA cluster enabled and are then in the
> + * RDT_RESOURCE_NODE resource.

"RDT_RESOURCE_NODE" is not defined yet. It will be defined in patch #3.
Maybe better to move this comment change to patch #3 to avoid confusion
on RDT_RESOURCE_NODE.

Further, the current comment calls out MBM because MBM is not obviously
related to L3. But with SNC, I think we need to call out SNC node for
all monitor events, not just MBM.

Maybe something like this?
    /*
     * Initialize the event list for the resource.
     *
     * Monitor events can either be part of RDT_RESOURCE_L3 resource,
     * or they may be per NUMA node on systems with sub-NUMA cluster
     * enabled and are then in the RDT_RESOURCE_NODE resource.
     *
     * Note that MBM events are also part of RDT_RESOURCE_L3 or
     * RDT_RESOURCE_NODE resource
     * because as per the SDM the total and local memory bandwidth
     * are enumerated as part of L3 monitoring.
     */

>    */
>   static void l3_mon_evt_init(struct rdt_resource *r)
>   {
> @@ -761,6 +765,8 @@ static void l3_mon_evt_init(struct rdt_resource *r)
>   		list_add_tail(&mbm_total_event.list, &r->evt_list);
>   	if (is_mbm_local_enabled())
>   		list_add_tail(&mbm_local_event.list, &r->evt_list);
> +
> +	mon_resource = r;
>   }
>   
>   int rdt_get_mon_l3_config(struct rdt_resource *r)

Thanks.

-Fenghua