Assign/unassign counters on resctrl group creation/deletion. If the
counters are exhausted, report the warnings and continue. It is not
required to fail group creation for assignment failures. Users have
the option to modify the assignments later.

Signed-off-by: Babu Moger <babu.moger@amd.com>
---
v5: Removed the code to enable/disable ABMC during the mount.
    That will be another patch.
    Added arch callers to get the arch specific data.
    Renamed fuctions to match the other abmc function.
    Added code comments for assignment failures.

v4: Few name changes based on the upstream discussion.
    Commit message update.

v3: This is a new patch. Patch addresses the upstream comment to enable
    ABMC feature by default if the feature is available.
---
 arch/x86/kernel/cpu/resctrl/rdtgroup.c | 78 ++++++++++++++++++++++++++
 1 file changed, 78 insertions(+)

diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index ffde30b36c1a..475a0c7b2a25 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -2910,6 +2910,46 @@ static void schemata_list_destroy(void)
 	}
 }
 
+/*
+ * Called when new group is created. Assign the counters if ABMC is
+ * already enabled. Two counters are required per group, one for total
+ * event and one for local event. With limited number of counters,
+ * the assignments can fail in some cases. But, it is not required to
+ * fail the group creation. Users have the option to modify the
+ * assignments after the group creation.
+ */
+static int rdtgroup_assign_cntrs(struct rdtgroup *rdtgrp)
+{
+	int ret = 0;
+
+	if (!resctrl_arch_get_abmc_enabled())
+		return 0;
+
+	if (is_mbm_total_enabled())
+		ret = rdtgroup_assign_cntr(rdtgrp, QOS_L3_MBM_TOTAL_EVENT_ID);
+
+	if (!ret && is_mbm_local_enabled())
+		ret = rdtgroup_assign_cntr(rdtgrp, QOS_L3_MBM_LOCAL_EVENT_ID);
+
+	return ret;
+}
+
+static int rdtgroup_unassign_cntrs(struct rdtgroup *rdtgrp)
+{
+	int ret = 0;
+
+	if (!resctrl_arch_get_abmc_enabled())
+		return 0;
+
+	if (is_mbm_total_enabled())
+		ret = rdtgroup_unassign_cntr(rdtgrp, QOS_L3_MBM_TOTAL_EVENT_ID);
+
+	if (!ret && is_mbm_local_enabled())
+		ret = rdtgroup_unassign_cntr(rdtgrp, QOS_L3_MBM_LOCAL_EVENT_ID);
+
+	return ret;
+}
+
 static int rdt_get_tree(struct fs_context *fc)
 {
 	struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;
@@ -2972,6 +3012,16 @@ static int rdt_get_tree(struct fs_context *fc)
 		if (ret < 0)
 			goto out_mongrp;
 		rdtgroup_default.mon.mon_data_kn = kn_mondata;
+
+		/*
+		 * Assign the counters if ABMC is already enabled.
+		 * With limited number of counters, the assignments can
+		 * fail in some cases. But, it is not required to fail
+		 * the group creation. Users have the option to modify
+		 * the assignments after the group creation.
+		 */
+		if (rdtgroup_assign_cntrs(&rdtgroup_default) < 0)
+			rdt_last_cmd_puts("Monitor assignment failed\n");
 	}
 
 	ret = rdt_pseudo_lock_init();
@@ -3246,6 +3296,8 @@ static void rdt_kill_sb(struct super_block *sb)
 	cpus_read_lock();
 	mutex_lock(&rdtgroup_mutex);
 
+	rdtgroup_unassign_cntrs(&rdtgroup_default);
+
 	rdt_disable_ctx();
 
 	/*Put everything back to default values. */
@@ -3850,6 +3902,16 @@ static int rdtgroup_mkdir_mon(struct kernfs_node *parent_kn,
 		goto out_unlock;
 	}
 
+	/*
+	 * Assign the counters if ABMC is already enabled.
+	 * With the limited number of counters, there can be cases
+	 * only on assignment succeed. It is not required to fail
+	 * here in that case. Users have the option to modify the
+	 * assignments later.
+	 */
+	if (rdtgroup_assign_cntrs(rdtgrp) < 0)
+		rdt_last_cmd_puts("Monitor assignment failed\n");
+
 	kernfs_activate(rdtgrp->kn);
 
 	/*
@@ -3894,6 +3956,17 @@ static int rdtgroup_mkdir_ctrl_mon(struct kernfs_node *parent_kn,
 	if (ret)
 		goto out_closid_free;
 
+	/*
+	 * Assign the counters if ABMC is already enabled.
+	 * With the limited number of counters, there can be cases
+	 * only on assignment succeed. It is not required to fail
+	 * here in that case. Users have the option to assign the
+	 * counter later.
+	 */
+
+	if (rdtgroup_assign_cntrs(rdtgrp) < 0)
+		rdt_last_cmd_puts("Monitor assignment failed\n");
+
 	kernfs_activate(rdtgrp->kn);
 
 	ret = rdtgroup_init_alloc(rdtgrp);
@@ -3989,6 +4062,9 @@ static int rdtgroup_rmdir_mon(struct rdtgroup *rdtgrp, cpumask_var_t tmpmask)
 	update_closid_rmid(tmpmask, NULL);
 
 	rdtgrp->flags = RDT_DELETED;
+
+	rdtgroup_unassign_cntrs(rdtgrp);
+
 	free_rmid(rdtgrp->closid, rdtgrp->mon.rmid);
 
 	/*
@@ -4035,6 +4111,8 @@ static int rdtgroup_rmdir_ctrl(struct rdtgroup *rdtgrp, cpumask_var_t tmpmask)
 	cpumask_or(tmpmask, tmpmask, &rdtgrp->cpu_mask);
 	update_closid_rmid(tmpmask, NULL);
 
+	rdtgroup_unassign_cntrs(rdtgrp);
+
 	free_rmid(rdtgrp->closid, rdtgrp->mon.rmid);
 	closid_free(rdtgrp->closid);
 
-- 
2.34.1

Hi Babu,

On Wed, Jul 3, 2024 at 2:50 PM Babu Moger <babu.moger@amd.com> wrote:

> @@ -3894,6 +3956,17 @@ static int rdtgroup_mkdir_ctrl_mon(struct kernfs_node *parent_kn,
>         if (ret)
>                 goto out_closid_free;
>
> +       /*
> +        * Assign the counters if ABMC is already enabled.
> +        * With the limited number of counters, there can be cases
> +        * only on assignment succeed. It is not required to fail
> +        * here in that case. Users have the option to assign the
> +        * counter later.
> +        */
> +
> +       if (rdtgroup_assign_cntrs(rdtgrp) < 0)
> +               rdt_last_cmd_puts("Monitor assignment failed\n");

Supposing rdtgroup_init_alloc() below fails, would you want to release
the counters allocated here?

> +
>         kernfs_activate(rdtgrp->kn);
>
>         ret = rdtgroup_init_alloc(rdtgrp);

-Peter

Hi Peter,

On 7/26/2024 6:22 PM, Peter Newman wrote:
> Hi Babu,
> 
> On Wed, Jul 3, 2024 at 2:50 PM Babu Moger <babu.moger@amd.com> wrote:
> 
>> @@ -3894,6 +3956,17 @@ static int rdtgroup_mkdir_ctrl_mon(struct kernfs_node *parent_kn,
>>          if (ret)
>>                  goto out_closid_free;
>>
>> +       /*
>> +        * Assign the counters if ABMC is already enabled.
>> +        * With the limited number of counters, there can be cases
>> +        * only on assignment succeed. It is not required to fail
>> +        * here in that case. Users have the option to assign the
>> +        * counter later.
>> +        */
>> +
>> +       if (rdtgroup_assign_cntrs(rdtgrp) < 0)
>> +               rdt_last_cmd_puts("Monitor assignment failed\n");
> 
> Supposing rdtgroup_init_alloc() below fails, would you want to release
> the counters allocated here?

Yes. Sure. Fix it in v6.
> 
>> +
>>          kernfs_activate(rdtgrp->kn);
>>
>>          ret = rdtgroup_init_alloc(rdtgrp);
> 
> -Peter
> 

-- 
- Babu Moger

Hi Babu,

On 7/3/24 2:48 PM, Babu Moger wrote:
> Assign/unassign counters on resctrl group creation/deletion. If the
> counters are exhausted, report the warnings and continue. It is not
> required to fail group creation for assignment failures. Users have
> the option to modify the assignments later.
> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
> v5: Removed the code to enable/disable ABMC during the mount.
>      That will be another patch.
>      Added arch callers to get the arch specific data.
>      Renamed fuctions to match the other abmc function.
>      Added code comments for assignment failures.
> 
> v4: Few name changes based on the upstream discussion.
>      Commit message update.
> 
> v3: This is a new patch. Patch addresses the upstream comment to enable
>      ABMC feature by default if the feature is available.
> ---
>   arch/x86/kernel/cpu/resctrl/rdtgroup.c | 78 ++++++++++++++++++++++++++
>   1 file changed, 78 insertions(+)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index ffde30b36c1a..475a0c7b2a25 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -2910,6 +2910,46 @@ static void schemata_list_destroy(void)
>   	}
>   }
>   
> +/*
> + * Called when new group is created. Assign the counters if ABMC is
> + * already enabled. Two counters are required per group, one for total
> + * event and one for local event. With limited number of counters,
> + * the assignments can fail in some cases. But, it is not required to
> + * fail the group creation. Users have the option to modify the
> + * assignments after the group creation.
> + */
> +static int rdtgroup_assign_cntrs(struct rdtgroup *rdtgrp)
> +{
> +	int ret = 0;
> +
> +	if (!resctrl_arch_get_abmc_enabled())
> +		return 0;
> +
> +	if (is_mbm_total_enabled())
> +		ret = rdtgroup_assign_cntr(rdtgrp, QOS_L3_MBM_TOTAL_EVENT_ID);
> +
> +	if (!ret && is_mbm_local_enabled())
> +		ret = rdtgroup_assign_cntr(rdtgrp, QOS_L3_MBM_LOCAL_EVENT_ID);
> +
> +	return ret;
> +}
> +
> +static int rdtgroup_unassign_cntrs(struct rdtgroup *rdtgrp)
> +{
> +	int ret = 0;
> +
> +	if (!resctrl_arch_get_abmc_enabled())
> +		return 0;
> +
> +	if (is_mbm_total_enabled())
> +		ret = rdtgroup_unassign_cntr(rdtgrp, QOS_L3_MBM_TOTAL_EVENT_ID);
> +
> +	if (!ret && is_mbm_local_enabled())
> +		ret = rdtgroup_unassign_cntr(rdtgrp, QOS_L3_MBM_LOCAL_EVENT_ID);
> +
> +	return ret;
> +}
> +
>   static int rdt_get_tree(struct fs_context *fc)
>   {
>   	struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;
> @@ -2972,6 +3012,16 @@ static int rdt_get_tree(struct fs_context *fc)
>   		if (ret < 0)
>   			goto out_mongrp;
>   		rdtgroup_default.mon.mon_data_kn = kn_mondata;
> +
> +		/*
> +		 * Assign the counters if ABMC is already enabled.
> +		 * With limited number of counters, the assignments can
> +		 * fail in some cases. But, it is not required to fail
> +		 * the group creation. Users have the option to modify
> +		 * the assignments after the group creation.
> +		 */

The function has detailed comments - it seems unnecessary to me that the
same comments are duplicated at each call site.

> +		if (rdtgroup_assign_cntrs(&rdtgroup_default) < 0)
> +			rdt_last_cmd_puts("Monitor assignment failed\n");

rdtgroup_assign_cntrs() already prints message, why print another? Error
handling can then be dropped.

>   	}
>   
>   	ret = rdt_pseudo_lock_init();
> @@ -3246,6 +3296,8 @@ static void rdt_kill_sb(struct super_block *sb)
>   	cpus_read_lock();
>   	mutex_lock(&rdtgroup_mutex);
>   
> +	rdtgroup_unassign_cntrs(&rdtgroup_default);
> +

This seems appropriate to be in the "Put everything back to default values"
section.

>   	rdt_disable_ctx();
>   
>   	/*Put everything back to default values. */
> @@ -3850,6 +3902,16 @@ static int rdtgroup_mkdir_mon(struct kernfs_node *parent_kn,
>   		goto out_unlock;
>   	}
>   
> +	/*
> +	 * Assign the counters if ABMC is already enabled.
> +	 * With the limited number of counters, there can be cases
> +	 * only on assignment succeed. It is not required to fail
> +	 * here in that case. Users have the option to modify the
> +	 * assignments later.
> +	 */
> +	if (rdtgroup_assign_cntrs(rdtgrp) < 0)
> +		rdt_last_cmd_puts("Monitor assignment failed\n");
> +
>   	kernfs_activate(rdtgrp->kn);
>   
>   	/*
> @@ -3894,6 +3956,17 @@ static int rdtgroup_mkdir_ctrl_mon(struct kernfs_node *parent_kn,
>   	if (ret)
>   		goto out_closid_free;
>   
> +	/*
> +	 * Assign the counters if ABMC is already enabled.
> +	 * With the limited number of counters, there can be cases
> +	 * only on assignment succeed. It is not required to fail
> +	 * here in that case. Users have the option to assign the
> +	 * counter later.
> +	 */
> +
> +	if (rdtgroup_assign_cntrs(rdtgrp) < 0)
> +		rdt_last_cmd_puts("Monitor assignment failed\n");
> +
>   	kernfs_activate(rdtgrp->kn);
>   
>   	ret = rdtgroup_init_alloc(rdtgrp);
> @@ -3989,6 +4062,9 @@ static int rdtgroup_rmdir_mon(struct rdtgroup *rdtgrp, cpumask_var_t tmpmask)
>   	update_closid_rmid(tmpmask, NULL);
>   
>   	rdtgrp->flags = RDT_DELETED;
> +
> +	rdtgroup_unassign_cntrs(rdtgrp);
> +
>   	free_rmid(rdtgrp->closid, rdtgrp->mon.rmid);
>   
>   	/*
> @@ -4035,6 +4111,8 @@ static int rdtgroup_rmdir_ctrl(struct rdtgroup *rdtgrp, cpumask_var_t tmpmask)
>   	cpumask_or(tmpmask, tmpmask, &rdtgrp->cpu_mask);
>   	update_closid_rmid(tmpmask, NULL);
>   
> +	rdtgroup_unassign_cntrs(rdtgrp);
> +
>   	free_rmid(rdtgrp->closid, rdtgrp->mon.rmid);
>   	closid_free(rdtgrp->closid);
>   

Reinette

Hi Reinette,


On 7/12/24 17:10, Reinette Chatre wrote:
> Hi Babu,
> 
> On 7/3/24 2:48 PM, Babu Moger wrote:
>> Assign/unassign counters on resctrl group creation/deletion. If the
>> counters are exhausted, report the warnings and continue. It is not
>> required to fail group creation for assignment failures. Users have
>> the option to modify the assignments later.
>>
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> ---
>> v5: Removed the code to enable/disable ABMC during the mount.
>>      That will be another patch.
>>      Added arch callers to get the arch specific data.
>>      Renamed fuctions to match the other abmc function.
>>      Added code comments for assignment failures.
>>
>> v4: Few name changes based on the upstream discussion.
>>      Commit message update.
>>
>> v3: This is a new patch. Patch addresses the upstream comment to enable
>>      ABMC feature by default if the feature is available.
>> ---
>>   arch/x86/kernel/cpu/resctrl/rdtgroup.c | 78 ++++++++++++++++++++++++++
>>   1 file changed, 78 insertions(+)
>>
>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> index ffde30b36c1a..475a0c7b2a25 100644
>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> @@ -2910,6 +2910,46 @@ static void schemata_list_destroy(void)
>>       }
>>   }
>>   +/*
>> + * Called when new group is created. Assign the counters if ABMC is
>> + * already enabled. Two counters are required per group, one for total
>> + * event and one for local event. With limited number of counters,
>> + * the assignments can fail in some cases. But, it is not required to
>> + * fail the group creation. Users have the option to modify the
>> + * assignments after the group creation.
>> + */
>> +static int rdtgroup_assign_cntrs(struct rdtgroup *rdtgrp)
>> +{
>> +    int ret = 0;
>> +
>> +    if (!resctrl_arch_get_abmc_enabled())
>> +        return 0;
>> +
>> +    if (is_mbm_total_enabled())
>> +        ret = rdtgroup_assign_cntr(rdtgrp, QOS_L3_MBM_TOTAL_EVENT_ID);
>> +
>> +    if (!ret && is_mbm_local_enabled())
>> +        ret = rdtgroup_assign_cntr(rdtgrp, QOS_L3_MBM_LOCAL_EVENT_ID);
>> +
>> +    return ret;
>> +}
>> +
>> +static int rdtgroup_unassign_cntrs(struct rdtgroup *rdtgrp)
>> +{
>> +    int ret = 0;
>> +
>> +    if (!resctrl_arch_get_abmc_enabled())
>> +        return 0;
>> +
>> +    if (is_mbm_total_enabled())
>> +        ret = rdtgroup_unassign_cntr(rdtgrp, QOS_L3_MBM_TOTAL_EVENT_ID);
>> +
>> +    if (!ret && is_mbm_local_enabled())
>> +        ret = rdtgroup_unassign_cntr(rdtgrp, QOS_L3_MBM_LOCAL_EVENT_ID);
>> +
>> +    return ret;
>> +}
>> +
>>   static int rdt_get_tree(struct fs_context *fc)
>>   {
>>       struct rdt_resource *r =
>> &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;
>> @@ -2972,6 +3012,16 @@ static int rdt_get_tree(struct fs_context *fc)
>>           if (ret < 0)
>>               goto out_mongrp;
>>           rdtgroup_default.mon.mon_data_kn = kn_mondata;
>> +
>> +        /*
>> +         * Assign the counters if ABMC is already enabled.
>> +         * With limited number of counters, the assignments can
>> +         * fail in some cases. But, it is not required to fail
>> +         * the group creation. Users have the option to modify
>> +         * the assignments after the group creation.
>> +         */
> 
> The function has detailed comments - it seems unnecessary to me that the
> same comments are duplicated at each call site.

Sure. Will remove duplicates.

> 
>> +        if (rdtgroup_assign_cntrs(&rdtgroup_default) < 0)
>> +            rdt_last_cmd_puts("Monitor assignment failed\n");
> 
> rdtgroup_assign_cntrs() already prints message, why print another? Error
> handling can then be dropped.

Sure.

> 
>>       }
>>         ret = rdt_pseudo_lock_init();
>> @@ -3246,6 +3296,8 @@ static void rdt_kill_sb(struct super_block *sb)
>>       cpus_read_lock();
>>       mutex_lock(&rdtgroup_mutex);
>>   +    rdtgroup_unassign_cntrs(&rdtgroup_default);
>> +
> 
> This seems appropriate to be in the "Put everything back to default values"
> section.

Sure. Will move it down.

> 
>>       rdt_disable_ctx();
>>         /*Put everything back to default values. */
>> @@ -3850,6 +3902,16 @@ static int rdtgroup_mkdir_mon(struct kernfs_node
>> *parent_kn,
>>           goto out_unlock;
>>       }
>>   +    /*
>> +     * Assign the counters if ABMC is already enabled.
>> +     * With the limited number of counters, there can be cases
>> +     * only on assignment succeed. It is not required to fail
>> +     * here in that case. Users have the option to modify the
>> +     * assignments later.
>> +     */
>> +    if (rdtgroup_assign_cntrs(rdtgrp) < 0)
>> +        rdt_last_cmd_puts("Monitor assignment failed\n");
>> +
>>       kernfs_activate(rdtgrp->kn);
>>         /*
>> @@ -3894,6 +3956,17 @@ static int rdtgroup_mkdir_ctrl_mon(struct
>> kernfs_node *parent_kn,
>>       if (ret)
>>           goto out_closid_free;
>>   +    /*
>> +     * Assign the counters if ABMC is already enabled.
>> +     * With the limited number of counters, there can be cases
>> +     * only on assignment succeed. It is not required to fail
>> +     * here in that case. Users have the option to assign the
>> +     * counter later.
>> +     */
>> +
>> +    if (rdtgroup_assign_cntrs(rdtgrp) < 0)
>> +        rdt_last_cmd_puts("Monitor assignment failed\n");
>> +
>>       kernfs_activate(rdtgrp->kn);
>>         ret = rdtgroup_init_alloc(rdtgrp);
>> @@ -3989,6 +4062,9 @@ static int rdtgroup_rmdir_mon(struct rdtgroup
>> *rdtgrp, cpumask_var_t tmpmask)
>>       update_closid_rmid(tmpmask, NULL);
>>         rdtgrp->flags = RDT_DELETED;
>> +
>> +    rdtgroup_unassign_cntrs(rdtgrp);
>> +
>>       free_rmid(rdtgrp->closid, rdtgrp->mon.rmid);
>>         /*
>> @@ -4035,6 +4111,8 @@ static int rdtgroup_rmdir_ctrl(struct rdtgroup
>> *rdtgrp, cpumask_var_t tmpmask)
>>       cpumask_or(tmpmask, tmpmask, &rdtgrp->cpu_mask);
>>       update_closid_rmid(tmpmask, NULL);
>>   +    rdtgroup_unassign_cntrs(rdtgrp);
>> +
>>       free_rmid(rdtgrp->closid, rdtgrp->mon.rmid);
>>       closid_free(rdtgrp->closid);
>>   
> 
> Reinette
> 

-- 
Thanks
Babu Moger