Re: [PATCH v9 18/26] x86/resctrl: Add the interface to assign/update counter assignment

[Reinette Chatre <reinette.chatre@intel.com>]

Hi Babu,

On 11/20/24 10:05 AM, Moger, Babu wrote:
> Hi Reinette,
> 
> On 11/15/24 18:57, Reinette Chatre wrote:
>> Hi Babu,
>>
>> On 10/29/24 4:21 PM, Babu Moger wrote:
>>> The mbm_cntr_assign mode offers several hardware counters that can be
>>> assigned to an RMID, event pair and monitor the bandwidth as long as it
>>> is assigned.
>>>
>>> Counters are managed at two levels. The global assignment is tracked
>>> using the mbm_cntr_free_map field in the struct resctrl_mon, while
>>> domain-specific assignments are tracked using the mbm_cntr_map field
>>> in the struct rdt_mon_domain. Allocation begins at the global level
>>> and is then applied individually to each domain.
>>>
>>> Introduce an interface to allocate these counters and update the
>>> corresponding domains accordingly.
>>>
>>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>>> ---
>>
>> ...
>>
>>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
>>> index 00f7bf60e16a..cb496bd97007 100644
>>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>>> @@ -717,6 +717,8 @@ unsigned int mon_event_config_index_get(u32 evtid);
>>>  int resctrl_arch_config_cntr(struct rdt_resource *r, struct rdt_mon_domain *d,
>>>  			     enum resctrl_event_id evtid, u32 rmid, u32 closid,
>>>  			     u32 cntr_id, bool assign);
>>> +int rdtgroup_assign_cntr_event(struct rdt_resource *r, struct rdtgroup *rdtgrp,
>>> +			       struct rdt_mon_domain *d, enum resctrl_event_id evtid);
>>>  void rdt_staged_configs_clear(void);
>>>  bool closid_allocated(unsigned int closid);
>>>  int resctrl_find_cleanest_closid(void);
>>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>>> index 1b5529c212f5..bc3752967c44 100644
>>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>>> @@ -1924,6 +1924,93 @@ int resctrl_arch_config_cntr(struct rdt_resource *r, struct rdt_mon_domain *d,
>>>  	return 0;
>>>  }
>>>  
>>> +/*
>>> + * Configure the counter for the event, RMID pair for the domain.
>>> + * Update the bitmap and reset the architectural state.
>>> + */
>>> +static int resctrl_config_cntr(struct rdt_resource *r, struct 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);
>>
>> resctrl_arch_reset_rmid() expects to be run on a CPU that is in the domain
>> @d ... note that after the architectural state is reset it initializes the
>> state by reading the event on the current CPU. By running it here it is
>> run on a random CPU that may not be in the right domain.
> 
> Yes. That is correct.  We can move this part to our earlier
> implementation. We dont need to read the RMID.  We just have to reset the
> counter.
> 
> https://lore.kernel.org/lkml/16d88cc4091cef1999b7ec329364e12dd0dc748d.1728495588.git.babu.moger@amd.com/
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index 9fe419d0c536..bc3654ec3a08 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -2371,6 +2371,13 @@ int resctrl_arch_config_cntr(struct rdt_resource
> *r, struct rdt_mon_domain *d,
>         smp_call_function_any(&d->hdr.cpu_mask, resctrl_abmc_config_one_amd,
>                               &abmc_cfg, 1);
> 
> +       /*
> +        * Reset the architectural state so that reading of hardware
> +        * counter is not considered as an overflow in next update.
> +        */
> +       if (arch_mbm)
> +               memset(arch_mbm, 0, sizeof(struct arch_mbm_state));
> +
>         return 0;
>  }
> 
>

I am not sure what you envision here. One motivation for the move out of
resctrl_arch_config_cntr() was to avoid architectural state being reset twice. For reference,
mbm_config_write_domain()->resctrl_arch_reset_rmid_all(). Will architectural state
be reset twice again?
One thing that I did not notice before is that the non-architectural MBM state is not
reset. Care should be taken to reset this also when considering that there is a plan
to use that MBM state to build a generic rate event for all platforms:
https://lore.kernel.org/all/CALPaoCgFRFgQqG00Uc0GhMHK47bsbtFw6Bxy5O9A_HeYmGa5sA@mail.gmail.com/

Reinette