lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <16e22ce2-6567-458f-a9a6-6bd79dfb9a23@amd.com>
Date: Thu, 20 Jun 2024 08:48:41 -0500
From: "Moger, Babu" <babu.moger@....com>
To: Reinette Chatre <reinette.chatre@...el.com>, corbet@....net,
 fenghua.yu@...el.com, tglx@...utronix.de, mingo@...hat.com, bp@...en8.de,
 dave.hansen@...ux.intel.com
Cc: x86@...nel.org, hpa@...or.com, paulmck@...nel.org, rdunlap@...radead.org,
 tj@...nel.org, peterz@...radead.org, yanjiewtw@...il.com,
 kim.phillips@....com, lukas.bulwahn@...il.com, seanjc@...gle.com,
 jmattson@...gle.com, leitao@...ian.org, jpoimboe@...nel.org,
 rick.p.edgecombe@...el.com, kirill.shutemov@...ux.intel.com,
 jithu.joseph@...el.com, kai.huang@...el.com, kan.liang@...ux.intel.com,
 daniel.sneddon@...ux.intel.com, pbonzini@...hat.com, sandipan.das@....com,
 ilpo.jarvinen@...ux.intel.com, peternewman@...gle.com,
 maciej.wieczor-retman@...el.com, linux-doc@...r.kernel.org,
 linux-kernel@...r.kernel.org, eranian@...gle.com, james.morse@....com
Subject: Re: [PATCH v4 15/19] x86/resctrl: Add the interface to unassign ABMC
 counter

Hi Reinette,

On 6/13/24 20:49, Reinette Chatre wrote:
> Hi Babu,
> 
> On 5/24/24 5:23 AM, Babu Moger wrote:
>> Hardware provides a limited number of ABMC counters. Once all the
>> counters are exhausted, counters need to be freed for new assignments.
>>
>> Provide the interface to unassign the counter.
> 
> Please write a proper changelog. This needs information that explains
> what this patch does and why.

Sure. Will elaborate on this.
> 
>>
>> 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).
>>
>> Signed-off-by: Babu Moger <babu.moger@....com>
>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=206537
>> ---
>> v4: Added domain specific unassign feature.
>>      Few name changes.
>>
>> v3: Removed the static from the prototype of rdtgroup_unassign_abmc.
>>      The function is not called directly from user anymore. These
>>      changes are related to global assignment interface.
>>
>> v2: No changes.
>> ---
>>   arch/x86/kernel/cpu/resctrl/internal.h |  2 ++
>>   arch/x86/kernel/cpu/resctrl/rdtgroup.c | 42 ++++++++++++++++++++++++++
>>   2 files changed, 44 insertions(+)
>>
>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h
>> b/arch/x86/kernel/cpu/resctrl/internal.h
>> index a88c8fc5e4df..e16244895350 100644
>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>> @@ -660,6 +660,8 @@ void arch_domain_mbm_evt_config(struct rdt_hw_domain
>> *hw_dom);
>>   int resctrl_arch_assign(struct rdt_domain *d, u32 evtid, u32 rmid,
>>               u32 ctr_id, u32 closid, bool enable);
>>   int resctrl_grp_assign(struct rdtgroup *rdtgrp, u32 evtid);
>> +int resctrl_grp_unassign(struct rdtgroup *rdtgrp, u32 evtid);
>> +void num_cntrs_free(u32 ctr_id);
>>   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 48df76499a04..5ea1e58c7201 100644
>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> @@ -216,6 +216,11 @@ static int assign_cntrs_alloc(void)
>>       return ctr_id;
>>   }
>>   +void num_cntrs_free(u32 ctr_id)
> 
> The name does not reflect what it does. It neither frees the "num_cntrs"
> information
> nor does it free "num_cntrs" of counters. How about "mon_cntr_free()"?

Sure. If we change the name of num_cntrs to mbm_cntrs, then we can change
to mbm_cntr_free().
Here is the comments on name change.
https://lore.kernel.org/lkml/62fe683f-3a4c-4280-8770-d2aaff478d33@amd.com/


> 
> 
>> +{
>> +    __set_bit(ctr_id, &num_cntrs_free_map);
>> +}
>> +
>>   /**
>>    * rdtgroup_mode_by_closid - Return mode of resource group with closid
>>    * @closid: closid if the resource group
>> @@ -1931,6 +1936,43 @@ int resctrl_grp_assign(struct rdtgroup *rdtgrp,
>> u32 evtid)
>>       return 0;
>>   }
>>   +int resctrl_grp_unassign(struct rdtgroup *rdtgrp, u32 evtid)
> 
> Same comment wrt namespace. Also this function needs a description.

Sure.

> 
>> +{
>> +    struct rdt_resource *r =
>> &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;
>> +    struct rdt_domain *d;
>> +    u32 mon_state;
>> +    int index;
>> +
>> +    index = mon_event_config_index_get(evtid);
>> +    if (index == INVALID_CONFIG_INDEX) {
>> +        pr_warn_once("Invalid event id %d\n", evtid);
>> +        return -EINVAL;
>> +    }
>> +
>> +    if (evtid == QOS_L3_MBM_TOTAL_EVENT_ID) {
>> +        mon_state = ASSIGN_TOTAL;
>> +    } else if (evtid == QOS_L3_MBM_LOCAL_EVENT_ID) {
>> +        mon_state = ASSIGN_LOCAL;
>> +    } else {
>> +        rdt_last_cmd_puts("Invalid event id\n");
>> +        return -EINVAL;
>> +    }
>> +
>> +    if (rdtgrp->mon.mon_state & mon_state) {
>> +        list_for_each_entry(d, &r->domains, list)
>> +            resctrl_arch_assign(d, evtid, rdtgrp->mon.rmid,
>> +                        rdtgrp->mon.ctr_id[index],
>> +                        rdtgrp->closid, 0);
>> +
>> +        /* Update the counter bitmap */
>> +        num_cntrs_free(rdtgrp->mon.ctr_id[index]);
>> +    }
>> +
>> +    rdtgrp->mon.mon_state &= ~mon_state;
>> +
>> +    return 0;
>> +}
>> +
>>   /* rdtgroup information files for one cache resource. */
>>   static struct rftype res_common_files[] = {
>>       {
> 
> 
> Reinette
> 

-- 
Thanks
Babu Moger

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ