Hi Reinette,
On 7/12/24 17:07, Reinette Chatre wrote:
> Hi Babu,
>
> On 7/3/24 2:48 PM, Babu Moger wrote:
>> Hardware provides a set of counters when the ABMC feature is supported.
>> These counters are used for enabling the events in resctrl group when
>> the feature is enabled.
>>
>> Introduce mbm_cntrs_free_map bitmap to track available and free counters.
>>
>> Signed-off-by: Babu Moger <babu.moger@xxxxxxx>
>> ---
>> v5:
>> Updated the comments and commit log.
>> Few renames
>> num_cntrs_free_map -> mbm_cntrs_free_map
>> num_cntrs_init -> mbm_cntrs_init
>> Added initialization in rdt_get_tree because the default ABMC
>> enablement happens during the init.
>>
>> v4: Changed the name to num_cntrs where applicable.
>> Used bitmap apis.
>> Added more comments for the globals.
>>
>> v3: Changed the bitmap name to assign_cntrs_free_map. Removed abmc
>> from the name.
>>
>> v2: Changed the bitmap name to assignable_counter_free_map from
>> abmc_counter_free_map.
>> ---
>> arch/x86/kernel/cpu/resctrl/rdtgroup.c | 29 ++++++++++++++++++++++++--
>> 1 file changed, 27 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> index 4f47f52e01c2..b3d3fa048f15 100644
>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> @@ -185,6 +185,23 @@ bool closid_allocated(unsigned int closid)
>> return !test_bit(closid, &closid_free_map);
>> }
>> +/*
>> + * Counter bitmap and its length for tracking available counters.
>> + * ABMC feature provides set of hardware counters for enabling events.
>> + * Each event takes one hardware counter. Kernel needs to keep track
>
> What is meant with "Kernel" here? It looks to be the fs code but the
> implementation has both fs and arch code reaching into the counter
> management. This should not be the case, either the fs code or the
> arch code needs to manage the counters, not both.
Yes. This needs to be done at FS code.
>
>> + * of number of available counters.
>> + */
>> +static unsigned long mbm_cntrs_free_map;
>
> With the lengths involved this needs a proper DECLARE_BITMAP()
Sure.
>
>> +static unsigned int mbm_cntrs_free_map_len;
>> +
>> +static void mbm_cntrs_init(void)
>> +{
>> + struct rdt_resource *r =
>> &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;
>> +
>> + bitmap_fill(&mbm_cntrs_free_map, r->mon.num_mbm_cntrs);
>> + mbm_cntrs_free_map_len = r->mon.num_mbm_cntrs;
>> +}
>> +
>> /**
>> * rdtgroup_mode_by_closid - Return mode of resource group with closid
>> * @closid: closid if the resource group
>> @@ -2466,6 +2483,12 @@ static int _resctrl_abmc_enable(struct
>> rdt_resource *r, bool enable)
>> {
>> struct rdt_mon_domain *d;
>> + /*
>> + * Clear all the previous assignments while switching the monitor
>> + * mode.
>> + */
>> + mbm_cntrs_init();
>> +
>
> If the counters are managed by fs code then the arch code should not be
> doing this. If needed the fs code should init the counters before calling
> the arch helpers.
Yes. We cannot make this call from here. I need to move this call to FS
layer before coming to this code. Thanks for good point.
>
>> /*
>> * Hardware counters will reset after switching the monitor mode.
>> * Reset the architectural state so that reading of hardware
>> @@ -2724,10 +2747,10 @@ static void schemata_list_destroy(void)
>> static int rdt_get_tree(struct fs_context *fc)
>> {
>> + struct rdt_resource *r =
>> &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;
>> struct rdt_fs_context *ctx = rdt_fc2context(fc);
>> unsigned long flags = RFTYPE_CTRL_BASE;
>> struct rdt_mon_domain *dom;
>> - struct rdt_resource *r;
>> int ret;
>> cpus_read_lock();
>> @@ -2756,6 +2779,9 @@ static int rdt_get_tree(struct fs_context *fc)
>> closid_init();
>> + if (r->mon.abmc_capable)
>> + mbm_cntrs_init();
>> +
>> if (resctrl_arch_mon_capable())
>> flags |= RFTYPE_MON;
>> @@ -2800,7 +2826,6 @@ static int rdt_get_tree(struct fs_context *fc)
>> resctrl_mounted = true;
>> if (is_mbm_enabled()) {
>> - r = &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;
>> list_for_each_entry(dom, &r->mon_domains, hdr.list)
>> mbm_setup_overflow_handler(dom, MBM_OVERFLOW_INTERVAL,
>> RESCTRL_PICK_ANY_CPU);
>
> Reinette
>
--
Thanks
Babu Moger
