Re: [PATCH v6 15/22] x86/resctrl: Add the interface to assign a hardware counter

["Moger, Babu" <bmoger@amd.com>]

Hi Reinette,

On 8/16/2024 4:41 PM, Reinette Chatre wrote:
> Hi Babu,
> 
> On 8/6/24 3:00 PM, Babu Moger wrote:
>> The ABMC feature provides an option to the user to assign a hardware
> 
> This patch is a mix of resctrl fs and arch code, could each piece please
> be desribed clearly?

I will separate them. That is probably better.

> 
>> counter to an RMID and monitor the bandwidth as long as it is assigned.
>> The assigned RMID will be tracked by the hardware until the user 
>> unassigns
>> it manually.
>>
>> Counters are configured by writing to L3_QOS_ABMC_CFG MSR and
>> specifying the counter id, bandwidth source, and bandwidth types.
>>
>> Provide the interface to assign the counter ids to RMID.
>>
>> The feature details are documented in the APM listed below [1].
>> [1] AMD64 Architecture Programmer's Manual Volume 2: System Programming
>>      Publication # 24593 Revision 3.41 section 19.3.3.3 Assignable 
>> Bandwidth
>>      Monitoring (ABMC).
>>
>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=206537
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> ---
>> v6: Removed mbm_cntr_alloc() from this patch to keep fs and arch code
>>      separate.
>>      Added code to update the counter assignment at domain level.
>>
>> v5: Few name changes to match cntr_id.
>>      Changed the function names to
>>        rdtgroup_assign_cntr
>>        resctr_arch_assign_cntr
>>        More comments on commit log.
>>        Added function summary.
>>
>> v4: Commit message update.
>>        User bitmap APIs where applicable.
>>        Changed the interfaces considering MPAM(arm).
>>        Added domain specific assignment.
>>
>> v3: Removed the static from the prototype of rdtgroup_assign_abmc.
>>        The function is not called directly from user anymore. These
>>        changes are related to global assignment interface.
>>
>> v2: Minor text changes in commit message.
>> ---
>>   arch/x86/kernel/cpu/resctrl/internal.h |  4 ++
>>   arch/x86/kernel/cpu/resctrl/rdtgroup.c | 97 ++++++++++++++++++++++++++
>>   2 files changed, 101 insertions(+)
>>
>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h 
>> b/arch/x86/kernel/cpu/resctrl/internal.h
>> index d93082b65d69..4e8109dee174 100644
>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>> @@ -685,6 +685,10 @@ int mbm_cntr_alloc(struct rdt_resource *r);
>>   void mbm_cntr_free(u32 cntr_id);
>>   void resctrl_mbm_evt_config_init(struct rdt_hw_mon_domain *hw_dom);
>>   unsigned int mon_event_config_index_get(u32 evtid);
>> +int resctrl_arch_assign_cntr(struct rdt_mon_domain *d, enum 
>> resctrl_event_id evtid,
>> +                 u32 rmid, u32 cntr_id, u32 closid, bool assign);
>> +int rdtgroup_assign_cntr(struct rdtgroup *rdtgrp, enum 
>> resctrl_event_id evtid);
>> +int rdtgroup_alloc_cntr(struct rdtgroup *rdtgrp, int index);
>>   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 60696b248b56..1ee91a7293a8 100644
>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> @@ -1864,6 +1864,103 @@ static ssize_t 
>> mbm_local_bytes_config_write(struct kernfs_open_file *of,
>>       return ret ?: nbytes;
>>   }
>> +static void rdtgroup_abmc_cfg(void *info)
> 
> This has nothing to do with a resctrl group (arch code has no insight 
> into the groups anyway).
> Maybe an arch specific name like "resctrl_abmc_config_one_amd()" to 
> match earlier
> "resctrl_abmc_set_one_amd()"?
> 

Sure.

> 
>> +{
>> +    u64 *msrval = info;
>> +
>> +    wrmsrl(MSR_IA32_L3_QOS_ABMC_CFG, *msrval);
>> +}
>> +
>> +/*
>> + * Send an IPI to the domain to assign the counter id to RMID.
>> + */
>> +int resctrl_arch_assign_cntr(struct rdt_mon_domain *d, enum 
>> resctrl_event_id evtid,
>> +                 u32 rmid, u32 cntr_id, u32 closid, bool assign)
>> +{
>> +    struct rdt_hw_mon_domain *hw_dom = resctrl_to_arch_mon_dom(d);
>> +    union l3_qos_abmc_cfg abmc_cfg = { 0 };
>> +    struct arch_mbm_state *arch_mbm;
>> +
>> +    abmc_cfg.split.cfg_en = 1;
>> +    abmc_cfg.split.cntr_en = assign ? 1 : 0;
>> +    abmc_cfg.split.cntr_id = cntr_id;
>> +    abmc_cfg.split.bw_src = rmid;
>> +
>> +    /* Update the event configuration from the domain */
>> +    if (evtid == QOS_L3_MBM_TOTAL_EVENT_ID) {
>> +        abmc_cfg.split.bw_type = hw_dom->mbm_total_cfg;
>> +        arch_mbm = &hw_dom->arch_mbm_total[rmid];
>> +    } else {
>> +        abmc_cfg.split.bw_type = hw_dom->mbm_local_cfg;
>> +        arch_mbm = &hw_dom->arch_mbm_local[rmid];
>> +    }
>> +
>> +    smp_call_function_any(&d->hdr.cpu_mask, rdtgroup_abmc_cfg, 
>> &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;
>> +}
>> +
>> +/* Allocate a new counter id if the event is unassigned */
>> +int rdtgroup_alloc_cntr(struct rdtgroup *rdtgrp, int index)
>> +{
>> +    struct rdt_resource *r = 
>> &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;
>> +    int cntr_id;
>> +
>> +    /* Nothing to do if event has been assigned already */
>> +    if (rdtgrp->mon.cntr_id[index] != MON_CNTR_UNSET) {
>> +        rdt_last_cmd_puts("ABMC counter is assigned already\n");
> 
> This is resctrl fs code. Please replace the arch specific messages
> ("ABMC") with resctrl fs terms.

Sure.

> 
>> +        return 0;
>> +    }
>> +
>> +    /*
>> +     * Allocate a new counter id and update domains
>> +     */
>> +    cntr_id = mbm_cntr_alloc(r);
>> +    if (cntr_id < 0) {
>> +        rdt_last_cmd_puts("Out of ABMC counters\n");
> 
> here also.

Sure.

> 
>> +        return -ENOSPC;
>> +    }
>> +
>> +    rdtgrp->mon.cntr_id[index] = cntr_id;
>> +
>> +    return 0;
>> +}
>> +
>> +/*
>> + * Assign a hardware counter to the group and assign the counter
>> + * all the domains in the group. It will try to allocate the mbm
>> + * counter if the counter is available.
>> + */
>> +int rdtgroup_assign_cntr(struct rdtgroup *rdtgrp, enum 
>> resctrl_event_id evtid)
>> +{
>> +    struct rdt_resource *r = 
>> &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;
>> +    struct rdt_mon_domain *d;
>> +    int index;
>> +
>> +    index = mon_event_config_index_get(evtid);
> 
> After going through MPAM series this no longer looks correct. As the 
> name of this
> function implies this is an index unique to the monitor event 
> configuration feature
> and as the MPAM series highlights, it is unique to the architecture, not 
> something
> that is visible to resctrl fs. resctrl fs uses the event IDs and it is 
> only when the
> fs makes a request to the architecture that this translation comes into 
> play.
> 
> With this change, what is the architecture specific "mon event config 
> index" now
> becomes part of resctrl fs used for something totally different from mon 
> event
> configuration.
> 
> I think we should separate this to make sure we distinguish between an 
> architectural
> translation and a resctrl fs translation, the array index is not the 
> same as the architecture
> specific "mov event config index".
> 
> How about we start with something simple that is defined by resctrl fs? 
> for example:
> #define MBM_EVENT_ARRAY_INDEX(_event) (_event  - 2)

Yes. Good point. We can do that.

> 
> 
>> +    if (index == INVALID_CONFIG_INDEX)
>> +        return -EINVAL;
>> +
>> +    if (rdtgroup_alloc_cntr(rdtgrp, index))
>> +        return -EINVAL;
>> +
> 
> hmmm ... so rdtgroup_alloc_cntr() returns 0 if the counter is assigned 
> already, and
> in this case the configuration is done again even if counter was already 
> assigned.
> Is this intended?

I didn't think thru this. Yea. It is not required as far as I can see.
Will address it.

> 
> rdtgroup_assign_cntr() seems to be almost identical to 
> rdtgroup_assign_update()
> that has protection against the above from happening. It looks like 
> these two
> functions can be merged into one?

Yes. We can do that.

> 
>> +    list_for_each_entry(d, &r->mon_domains, hdr.list) {
>> +        resctrl_arch_assign_cntr(d, evtid, rdtgrp->mon.rmid,
>> +                     rdtgrp->mon.cntr_id[index],
> 
> There currently seems to be a mismatch between functions needing to
> access this ID directly as above in some cases while also needing to
> use helpers like rdtgroup_alloc_cntr().

I think I need to merge rdtgroup_assign_cntr(), rdtgroup_assign_update()
and rdtgroup_alloc_cntr. It will probably make it clear.
> 
> Also, as James indicated, resctrl_arch_assign_cntr() may fail on Arm
> so this needs error checking even though the x86 implementation always
> returns success.

Sure. Will do.

> 
>> +                     rdtgrp->closid, true);
>> +        set_bit(rdtgrp->mon.cntr_id[index], d->mbm_cntr_map);
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>>   /* rdtgroup information files for one cache resource. */
>>   static struct rftype res_common_files[] = {
>>       {
> 
> Reinette
> 
Thanks
- Babu Moger