Re: [PATCH v8 18/25] x86/resctrl: Add the interface to unassign a MBM counter

[Reinette Chatre]

Hi Babu,

On 10/17/24 4:11 PM, Moger, Babu wrote:
> On 10/15/2024 10:29 PM, Reinette Chatre wrote:
>> On 10/9/24 10:39 AM, Babu Moger wrote:

>>> +    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, false);
>>> +            if (ret)
>>> +                goto out_done_unassign;
>>> +
>>> +            clear_bit(cntr_id, d->mbm_cntr_map);
>>> +        }
>>> +    } else {
>>> +        ret = resctrl_arch_config_cntr(r, d, evtid, rdtgrp->mon.rmid,
>>> +                           rdtgrp->closid, cntr_id, false);
>>> +        if (ret)
>>> +            goto out_done_unassign;
>>> +
>>> +        clear_bit(cntr_id, d->mbm_cntr_map);
>>
>> Please see comment to previous patch about the duplicate snippets. Snippets can be
>> replaced with single function that also resets architectural state.
> 
> Sure.
> 
> will combine rdtgroup_assign_cntr_event() and
> rdtgroup_unassign_cntr_event().

That is not what I suggested. I attempted to clarify in response to patch
with original feedback:
https://lore.kernel.org/all/c36e0c76-1666-4a31-984e-1ee6aed2e414@xxxxxxxxx/

> 
> I need to rename the function. How about resctrl_configure_cntr_event()?
> 
> 
>>
>>> +    }
>>> +
>>> +    /* Update the counter bitmap */
>>
>> What is the update?
> 
> Clear the counter bitmap.

Could you please update the comment to be more specific? What is
written can be seen from code.

Reinette