Hi Babu, On 10/21/24 7:40 AM, Moger, Babu wrote: > On 10/18/24 10:59, Reinette Chatre wrote: >> On 10/17/24 3:56 PM, Moger, Babu wrote: >>> On 10/15/2024 10:25 PM, Reinette Chatre wrote: >>>> On 10/9/24 10:39 AM, Babu Moger wrote: >> >>>>> + */ >>>>> +int rdtgroup_assign_cntr_event(struct rdt_resource *r, struct rdtgroup *rdtgrp, >>>>> + struct rdt_mon_domain *d, enum resctrl_event_id evtid) >>>>> +{ >>>>> + int index = MBM_EVENT_ARRAY_INDEX(evtid); >>>>> + int cntr_id = rdtgrp->mon.cntr_id[index]; >>>>> + int ret; >>>>> + >>>>> + /* >>>>> + * Allocate a new counter id to the event if the counter is not >>>>> + * assigned already. >>>>> + */ >>>>> + if (cntr_id == MON_CNTR_UNSET) { >>>>> + cntr_id = mbm_cntr_alloc(r); >>>>> + if (cntr_id < 0) { >>>>> + rdt_last_cmd_puts("Out of MBM assignable counters\n"); >>>>> + return -ENOSPC; >>>>> + } >>>>> + rdtgrp->mon.cntr_id[index] = cntr_id; >>>>> + } >>>>> + >>>>> + if (!d) { >>>>> + list_for_each_entry(d, &r->mon_domains, hdr.list) { >>>>> + ret = resctrl_arch_config_cntr(r, d, evtid, rdtgrp->mon.rmid, >>>>> + rdtgrp->closid, cntr_id, true); >>>>> + if (ret) >>>>> + goto out_done_assign; >>>>> + >>>>> + set_bit(cntr_id, d->mbm_cntr_map); >>>> >>>> The code pattern above is repeated four times in this work, twice in >>>> rdtgroup_assign_cntr_event() and twice in rdtgroup_unassign_cntr_event(). This >>>> duplication should be avoided. It can be done in a function that also resets >>>> the architectural state. >>> >>> Are you suggesting to combine rdtgroup_assign_cntr_event() and rdtgroup_unassign_cntr_event()? >> >> No. My comment was about the following pattern that is repeated four times: >> ... >> ret = resctrl_arch_config_cntr(...) >> if (ret) >> ... >> set_bit()/clear_bit() >> ... >> > > ok. > > >>> It can be done. We need a flag to tell if it is a assign or unassign. >> >> There is already a flag that is used by resctrl_arch_config_cntr(), the same parameters >> as resctrl_arch_config_cntr() can be used for a wrapper that just calls >> resctrl_arch_config_cntr() directly and uses that same flag to >> select between set_bit() and clear_bit(). This wrapper can then also include >> the reset of architectural state. > > ok. Got it, It will look like this. > > > +/* > + * Wrapper to configure the counter in a domain. > + */ Please replace comment with a description of what the function does. > +static int rdtgroup_config_cntr(struct rdt_resource *r,struct While it keeps being a challenge to get naming right I do think this can start by replacing "rdtgroup" with "resctrl" (specifically, "rdtgroup_config_cntr() -> resctrl_config_cntr()") because, as seen with the parameters passed, this has nothing to do with rdtgroup. > rdt_mon_domain *d, > + enum resctrl_event_id evtid, u32 rmid, u32 > closid, > + u32 cntr_id, bool assign) > +{ > + int ret; > + > + ret = resctrl_arch_config_cntr(r, d, evtid, rmid, closid, cntr_id, > assign); > + if (ret) > + return ret; > + > + if (assign) > + __set_bit(cntr_id, d->mbm_cntr_map); > + else > + __clear_bit(cntr_id, d->mbm_cntr_map); > + > + /* > + * Reset the architectural state so that reading of hardware > + * counter is not considered as an overflow in next update. > + */ > + resctrl_arch_reset_rmid(r, d, closid, rmid, evtid); > + > + return ret; > +} > + Yes, this looks good. Thank you. Reinette