Hi Babu, On 11/21/24 4:26 PM, Moger, Babu wrote: > On 11/21/2024 6:22 PM, Moger, Babu wrote: >> On 11/18/2024 11:18 AM, Reinette Chatre wrote: >>> On 10/29/24 4:21 PM, Babu Moger wrote: >>>> @@ -3871,6 +3920,8 @@ static int rdtgroup_mkdir_mon(struct kernfs_node *parent_kn, >>>> goto out_unlock; >>>> } >>>> + rdtgroup_assign_cntrs(rdtgrp); >>>> + >>>> kernfs_activate(rdtgrp->kn); >>>> /* >>>> @@ -3915,6 +3966,8 @@ static int rdtgroup_mkdir_ctrl_mon(struct kernfs_node *parent_kn, >>>> if (ret) >>>> goto out_closid_free; >>>> + rdtgroup_assign_cntrs(rdtgrp); >>>> + >>>> kernfs_activate(rdtgrp->kn); >>>> ret = rdtgroup_init_alloc(rdtgrp); >>> >>> Please compare the above two hunks with earlier "x86/resctrl: Introduce cntr_id in mongroup for assignments". >>> Earlier patch initializes the counters within mkdir_rdt_prepare_rmid_alloc() while the above >>> hunk assigns the counters after mkdir_rdt_prepare_rmid_alloc() is called. Could this fragmentation be avoided >>> with init done once within mkdir_rdt_prepare_rmid_alloc()? >> >> It seems more appropriate to call rdtgroup_cntr_id_init() inside mkdir_rdt_prepare(). This will initialize the cntr_id to MON_CNTR_UNSET. >> >> And then call rdtgroup_assign_cntrs() inside mkdir_rdt_prepare_rmid_alloc(). >> >> what do you think? Taking a closer look this seems most appropriate. mkdir_rdt_prepare() is where the resource groupreset is created and all fields initialized, control and monitoring (irrespective of monitoring enabled). Doing the MON_CNTR_UNSET initalization in that central place seems good. Yes, and then assigning the counters in mkdir_rdt_prepare_rmid_alloc() sounds good. >>>> @@ -3940,6 +3993,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_cntrs(rdtgrp); >>>> mkdir_rdt_prepare_rmid_free(rdtgrp); >>>> out_closid_free: >>>> closid_free(closid); >>>> @@ -4010,6 +4064,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); >>>> /* >>>> @@ -4056,6 +4113,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); >>> >>> There is a potential problem here. rdtgroup_unassign_cntrs() attempts to remove counter from >>> all domains associated with the resource group. This may fail in any of the domains that results >>> in the counter not being marked as free in the global map and not reset the counter in the >>> resource group ... but the resource group is removed right after calling rdtgroup_unassign_cntrs(). >>> In this scenario there is thus a counter that is considered to be in use but not assigned to any >>> resource group. >>> >>>>> From what I can tell there is a difference here between default resource group and the others: >>> on remount of resctrl the default resource group will maintain knowledge of the counter that could >>> not be unassigned. This means that unmount/remount of resctrl does not provide a real "clean slate" >>> when it comes to counter assignment. Is this intended? >>> >> >> Yes. Agree. It is not intended. >> >> How about adding bitmap_zero() inside rdt_get_tree() to address this problem? Also need to reset the cntr_id in rdt_kill_sb(). > > I meant reset the cntr_id for the default group in rdt_kill_sb(). Doing the cntr_id reset like this matches the custom is to reset to defaults in rdt_kill_sb(). I am not sure what you envision with the bitmap_zero() in rdt_get_tree() ... I wonder if it may not just be simpler to call mbm_cntr_reset() from rdt_kill_sb()? This does raise the question if mbm_cntr_reset() should reset architectural state. I do not think it does harm, the state will just be reset again when the mon dirs are created? Reinette
