Hi Reinette,
On 9/19/24 12:29, Reinette Chatre wrote:
> Hi Babu,
>
> Subject: "Assign" -> "assign"
Sure.
>
> On 9/4/24 3:21 PM, Babu Moger wrote:
>
>> arch/x86/kernel/cpu/resctrl/rdtgroup.c | 61 ++++++++++++++++++++++++++
>> 1 file changed, 61 insertions(+)
>>
>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> index 21b9ca4ce493..bf94e4e05540 100644
>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> @@ -2866,6 +2866,52 @@ static void schemata_list_destroy(void)
>> }
>> }
>>
>> +/*
>> + * Called when a new group is created. If `mbm_cntr_assign` mode is enabled,
>> + * counters are automatically assigned. Each group requires two counters:
>> + * one for the total event and one for the local event. Due to the limited
>> + * number of counters, assignments may fail in some cases. However, it is
>> + * not necessary to fail the group creation. Users have the option to
>> + * modify the assignments after the group has been created.
>> + */
>> +static int rdtgroup_assign_grp(struct rdtgroup *rdtgrp)
>> +{
>> + struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;
>> + int ret = 0;
>> +
>> + if (!resctrl_arch_mbm_cntr_assign_enabled(r))
>> + return 0;
>> +
>> + if (is_mbm_total_enabled())
>> + ret = rdtgroup_assign_cntr(r, rdtgrp, NULL, QOS_L3_MBM_TOTAL_EVENT_ID);
>> +
>> + if (!ret && is_mbm_local_enabled())
>> + ret = rdtgroup_assign_cntr(r, rdtgrp, NULL, QOS_L3_MBM_LOCAL_EVENT_ID);
>> +
>> + return ret;
>> +}
>> +
>> +/*
>> + * Called when a group is deleted. Counters are unassigned if it was in
>> + * assigned state.
>> + */
>> +static int rdtgroup_unassign_grp(struct rdtgroup *rdtgrp)
>> +{
>> + struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;
>> + int ret = 0;
>> +
>> + if (!resctrl_arch_mbm_cntr_assign_enabled(r))
>> + return 0;
>> +
>> + if (is_mbm_total_enabled())
>> + ret = rdtgroup_unassign_cntr(r, rdtgrp, NULL, QOS_L3_MBM_TOTAL_EVENT_ID);
>> +
>> + if (!ret && is_mbm_local_enabled())
>> + ret = rdtgroup_unassign_cntr(r, rdtgrp, NULL, QOS_L3_MBM_LOCAL_EVENT_ID);
>> +
>> + return ret;
>> +}
>> +
>> static int rdt_get_tree(struct fs_context *fc)
>> {
>> struct rdt_fs_context *ctx = rdt_fc2context(fc);
>> @@ -2925,6 +2971,8 @@ static int rdt_get_tree(struct fs_context *fc)
>> if (ret < 0)
>> goto out_mongrp;
>> rdtgroup_default.mon.mon_data_kn = kn_mondata;
>> +
>> + rdtgroup_assign_grp(&rdtgroup_default);
>> }
>>
>> ret = rdt_pseudo_lock_init();
>> @@ -2955,6 +3003,7 @@ static int rdt_get_tree(struct fs_context *fc)
>> out_psl:
>> rdt_pseudo_lock_release();
>> out_mondata:
>> + rdtgroup_unassign_grp(&rdtgroup_default);
>> if (resctrl_arch_mon_capable())
>> kernfs_remove(kn_mondata);
>> out_mongrp:
>> @@ -3214,6 +3263,8 @@ static void rdt_kill_sb(struct super_block *sb)
>> resctrl_arch_disable_alloc();
>> if (resctrl_arch_mon_capable())
>> resctrl_arch_disable_mon();
>> +
>> + rdtgroup_unassign_grp(&rdtgroup_default);
>> resctrl_mounted = false;
>> kernfs_kill_sb(sb);
>> mutex_unlock(&rdtgroup_mutex);
>> @@ -3805,6 +3856,8 @@ static int rdtgroup_mkdir_mon(struct kernfs_node *parent_kn,
>> goto out_unlock;
>> }
>>
>> + rdtgroup_assign_grp(rdtgrp);
>> +
>> kernfs_activate(rdtgrp->kn);
>>
>> /*
>> @@ -3849,6 +3902,8 @@ static int rdtgroup_mkdir_ctrl_mon(struct kernfs_node *parent_kn,
>> if (ret)
>> goto out_closid_free;
>>
>> + rdtgroup_assign_grp(rdtgrp);
>> +
>> kernfs_activate(rdtgrp->kn);
>>
>> ret = rdtgroup_init_alloc(rdtgrp);
>> @@ -3874,6 +3929,7 @@ static int rdtgroup_mkdir_ctrl_mon(struct kernfs_node *parent_kn,
>> out_del_list:
>> list_del(&rdtgrp->rdtgroup_list);
>> out_rmid_free:
>> + rdtgroup_unassign_grp(rdtgrp);
>> mkdir_rdt_prepare_rmid_free(rdtgrp);
>> out_closid_free:
>> closid_free(closid);
>> @@ -3944,6 +4000,9 @@ static int rdtgroup_rmdir_mon(struct rdtgroup *rdtgrp, cpumask_var_t tmpmask)
>> update_closid_rmid(tmpmask, NULL);
>>
>> rdtgrp->flags = RDT_DELETED;
>> +
>> + rdtgroup_unassign_grp(rdtgrp);
>> +
>> free_rmid(rdtgrp->closid, rdtgrp->mon.rmid);
>>
>> /*
>> @@ -3990,6 +4049,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_grp(rdtgrp);
>> +
>> free_rmid(rdtgrp->closid, rdtgrp->mon.rmid);
>> closid_free(rdtgrp->closid);
>>
>
> Apart from earlier comment about rdtgroup_assign_grp()/rdtgroup_unassign_grp() naming, please also
> take care about how these functions are integrated since it seems to be inconsistent wrt whether it is called
> on mon capable resource. Also, I can see how the counter is removed when CTRL_MON group and MON group are
> explicitly removed but it is not clear to me how when a user removes a CTRL_MON group how the counters
> assigned to its child MON groups are unassigned.
I think we have a problem here while removing. The child MON grou counters
may remain assigned when CTRL_MON is removed. Will fix it. Thanks for
pointing out.
--
Thanks
Babu Moger