lkml.org 
[lkml]   [2024]   [May]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [RFC PATCH v3 03/17] x86/resctrl: Detect Assignable Bandwidth Monitoring feature details
    From
    Hi Babu,

    On 3/28/2024 6:06 PM, Babu Moger wrote:
    > ABMC feature details are reported via CPUID Fn8000_0020_EBX_x5.
    > Bits Description
    > 15:0 MAX_ABMC Maximum Supported Assignable Bandwidth
    > Monitoring Counter ID + 1
    >
    > The feature details are documented in 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@amd.com>
    > Link: https://bugzilla.kernel.org/show_bug.cgi?id=206537
    > ---
    > v3: Removed changes related to mon_features.
    > Moved rdt_cpu_has to core.c and added new function resctrl_arch_has_abmc.
    > Also moved the fields mbm_assign_capable and mbm_assign_cntrs to
    > rdt_resource. (James)
    >
    > v2: Changed the field name to mbm_assign_capable from abmc_capable.
    > ---
    > arch/x86/kernel/cpu/resctrl/core.c | 17 +++++++++++++++++
    > arch/x86/kernel/cpu/resctrl/internal.h | 1 +
    > arch/x86/kernel/cpu/resctrl/monitor.c | 3 +++
    > include/linux/resctrl.h | 12 ++++++++++++
    > 4 files changed, 33 insertions(+)
    >
    > diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
    > index 57a8c6f30dd6..bb82b392cf5d 100644
    > --- a/arch/x86/kernel/cpu/resctrl/core.c
    > +++ b/arch/x86/kernel/cpu/resctrl/core.c
    > @@ -740,6 +740,23 @@ bool __init rdt_cpu_has(int flag)
    > return ret;
    > }
    >
    > +inline bool __init resctrl_arch_has_abmc(struct rdt_resource *r)
    > +{
    > + bool ret = rdt_cpu_has(X86_FEATURE_ABMC);
    > + u32 eax, ebx, ecx, edx;
    > +
    > + if (ret) {
    > + /*
    > + * Query CPUID_Fn80000020_EBX_x05 for number of
    > + * ABMC counters
    > + */
    > + cpuid_count(0x80000020, 5, &eax, &ebx, &ecx, &edx);
    > + r->mbm_assign_cntrs = (ebx & 0xFFFF) + 1;
    > + }
    > +
    > + return ret;
    > +}

    It is not clear to me why this function is needed. I went back to
    read James' comment and it sounds to me as though he expected it
    to be called from non-arch code ... but this is only called
    from rdt_get_mon_l3_config() which is very much architecture specific
    and will remain in arch/x86 where rdt_cpu_has() will be accessible.

    > +
    > static __init bool get_mem_config(void)
    > {
    > struct rdt_hw_resource *hw_res = &rdt_resources_all[RDT_RESOURCE_MBA];
    > diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
    > index c99f26ebe7a6..c4ae6f3993aa 100644
    > --- a/arch/x86/kernel/cpu/resctrl/internal.h
    > +++ b/arch/x86/kernel/cpu/resctrl/internal.h
    > @@ -584,6 +584,7 @@ void free_rmid(u32 closid, u32 rmid);
    > int rdt_get_mon_l3_config(struct rdt_resource *r);
    > void __exit rdt_put_mon_l3_config(void);
    > bool __init rdt_cpu_has(int flag);
    > +bool __init resctrl_arch_has_abmc(struct rdt_resource *r);
    > void mon_event_count(void *info);
    > int rdtgroup_mondata_show(struct seq_file *m, void *arg);
    > void mon_event_read(struct rmid_read *rr, struct rdt_resource *r,
    > diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
    > index c34a35ec0f03..e5938bf53d5a 100644
    > --- a/arch/x86/kernel/cpu/resctrl/monitor.c
    > +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
    > @@ -1055,6 +1055,9 @@ int __init rdt_get_mon_l3_config(struct rdt_resource *r)
    > mbm_local_event.configurable = true;
    > mbm_config_rftype_init("mbm_local_bytes_config");
    > }
    > +
    > + if (resctrl_arch_has_abmc(r))
    > + r->mbm_assign_capable = ABMC_ASSIGN;
    > }

    This is confusing to me in two ways:
    (a) why need different layers of abstraction to initialize r->mbm_assign_capable
    and r->mbm_assign_cntrs? Can they not just be assigned at the same time?
    (b) r->mbm_assign_capable is a bool ... but it is assigned an enum? Why is
    this enum needed for this?

    >
    > l3_mon_evt_init(r);
    > diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
    > index a365f67131ec..a1ee9afabff3 100644
    > --- a/include/linux/resctrl.h
    > +++ b/include/linux/resctrl.h
    > @@ -150,6 +150,14 @@ struct resctrl_membw {
    > struct rdt_parse_data;
    > struct resctrl_schema;
    >
    > +/**
    > + * enum mbm_assign_type - The type of assignable monitoring.
    > + * @ABMC_ASSIGN: Assignable Bandwidth Monitoring Counters.
    > + */
    > +enum mbm_assign_type {
    > + ABMC_ASSIGN = 0x01,
    > +};
    > +

    Either the resource is mbm_assign_capable or not ... it is not clear
    to me why an enum is needed.

    > /**
    > * struct rdt_resource - attributes of a resctrl resource
    > * @rid: The index of the resource
    > @@ -168,6 +176,8 @@ struct resctrl_schema;
    > * @evt_list: List of monitoring events
    > * @fflags: flags to choose base and info files
    > * @cdp_capable: Is the CDP feature available on this resource
    > + * @mbm_assign_capable: Does system capable of supporting monitor assignment?

    "Does system capable" -> "Is system capable"?

    > + * @mbm_assign_cntrs: Maximum number of assignable counters
    > */
    > struct rdt_resource {
    > int rid;
    > @@ -188,6 +198,8 @@ struct rdt_resource {
    > struct list_head evt_list;
    > unsigned long fflags;
    > bool cdp_capable;
    > + bool mbm_assign_capable;
    > + u32 mbm_assign_cntrs;
    > };

    Please check tabs vs spaces (in this whole series please).

    I'm thinking that a new "MBM specific" struct within
    struct rdt_resource will be helpful to clearly separate the MBM related
    data. This will be similar to struct resctrl_cache for
    cache allocation and struct resctrl_membw for memory bandwidth
    allocation.

    >
    > /**

    Reinette

    \
     
     \ /
      Last update: 2024-05-27 18:15    [W:4.207 / U:0.028 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site