Re: [PATCH v9 9/9] x86/resctrl: Display RMID of resource group

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

Hi Fenghua,

On 9/11/23 13:14, Fenghua Yu wrote:
> Hi, Babu,
> 
> On 9/7/23 16:51, Babu Moger wrote:
>> In x86, hardware uses RMID to identify a a monitoring group. When a
> 
> s/a a/a/

Sure.
Thanks for the review. Reinette has already addressed other comments.
Thanks
Babu

> 
>> user creates a monitor group these details are not visible. These details
>> can help resctrl debugging.
>>
>> Add RMID(mon_hw_id) to the monitor groups display in resctrl interface.
>> Users can see these details when resctrl is mounted with "-o debug" option.
>>
>> Other architectures do not use "RMID". Use the name mon_hw_id to refer
>> to "RMID" in an effort to keep the naming generic.
>>
>> Add the flag RFTYPE_MON_BASE, which contains the files required only
>> for the MON group.
>>
>> For example:
>>   $cat /sys/fs/resctrl/mon_groups/mon_grp1/mon_hw_id
>>   3
>>
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> ---
>>   Documentation/arch/x86/resctrl.rst     |  4 +++
>>   arch/x86/kernel/cpu/resctrl/internal.h |  6 ++++
>>   arch/x86/kernel/cpu/resctrl/rdtgroup.c | 38 +++++++++++++++++++++++---
>>   3 files changed, 44 insertions(+), 4 deletions(-)
>>
>> diff --git a/Documentation/arch/x86/resctrl.rst
>> b/Documentation/arch/x86/resctrl.rst
>> index 54691c8b832d..98b0eb509ed4 100644
>> --- a/Documentation/arch/x86/resctrl.rst
>> +++ b/Documentation/arch/x86/resctrl.rst
>> @@ -369,6 +369,10 @@ When monitoring is enabled all MON groups will also
>> contain:
>>       the sum for all tasks in the CTRL_MON group and all tasks in
>>       MON groups. Please see example section for more details on usage.
>>   +"mon_hw_id":
>> +    Available only with debug option. The identifier used by hardware
>> +    for the monitor group. On x86 this is the RMID.
>> +
>>   Resource allocation rules
>>   -------------------------
>>   diff --git a/arch/x86/kernel/cpu/resctrl/internal.h
>> b/arch/x86/kernel/cpu/resctrl/internal.h
>> index ccdbed615d41..b4910892b0a6 100644
>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>> @@ -296,6 +296,11 @@ struct rdtgroup {
>>    *    --> RFTYPE_BASE (Files common for both MON and CTRL groups)
>>    *        Files: cpus, cpus_list, tasks
>>    *
>> + *        --> RFTYPE_MON (Files only for MON group)
>> + *
>> + *            --> RFTYPE_DEBUG (Files to help resctrl debugging)
>> + *                File: mon_hw_id
>> + *
>>    *        --> RFTYPE_CTRL (Files only for CTRL group)
>>    *            Files: mode, schemata, size
>>    *
>> @@ -315,6 +320,7 @@ struct rdtgroup {
>>   #define RFTYPE_MON_INFO            (RFTYPE_INFO | RFTYPE_MON)
>>   #define RFTYPE_TOP_INFO            (RFTYPE_INFO | RFTYPE_TOP)
>>   #define RFTYPE_CTRL_BASE        (RFTYPE_BASE | RFTYPE_CTRL)
>> +#define RFTYPE_MON_BASE            (RFTYPE_BASE | RFTYPE_MON)
>>     /* List of all resource groups */
>>   extern struct list_head rdt_all_groups;
>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> index 8be0fb323ad3..fc830ffce82a 100644
>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> @@ -795,6 +795,22 @@ static int rdtgroup_closid_show(struct
>> kernfs_open_file *of,
>>       return ret;
>>   }
>>   +static int rdtgroup_rmid_show(struct kernfs_open_file *of,
>> +                  struct seq_file *s, void *v)
>> +{
>> +    struct rdtgroup *rdtgrp;
>> +    int ret = 0;
>> +
>> +    rdtgrp = rdtgroup_kn_lock_live(of->kn);
>> +    if (rdtgrp)
>> +        seq_printf(s, "%u\n", rdtgrp->mon.rmid);
>> +    else
>> +        ret = -ENOENT;
>> +    rdtgroup_kn_unlock(of->kn);
>> +
>> +    return ret;
>> +}
>> +
>>   #ifdef CONFIG_PROC_CPU_RESCTRL
>>     /*
>> @@ -1856,6 +1872,13 @@ static struct rftype res_common_files[] = {
>>           .seq_show    = rdtgroup_tasks_show,
>>           .fflags        = RFTYPE_BASE,
>>       },
>> +    {
>> +        .name        = "mon_hw_id",
>> +        .mode        = 0444,
>> +        .kf_ops        = &rdtgroup_kf_single_ops,
>> +        .seq_show    = rdtgroup_rmid_show,
> 
> Similar to showing ctrl_hw_id, is it better to rename "rdtgroup_rmid_show"
> as "rdtgroup_mon_hw_id_show" for arch neutral naming?
> 
>> +        .fflags        = RFTYPE_MON_BASE | RFTYPE_DEBUG,
>> +    },
>>       {
>>           .name        = "schemata",
>>           .mode        = 0644,
>> @@ -2535,6 +2558,7 @@ static void schemata_list_destroy(void)
>>   static int rdt_get_tree(struct fs_context *fc)
>>   {
>>       struct rdt_fs_context *ctx = rdt_fc2context(fc);
>> +    unsigned long flags = RFTYPE_CTRL_BASE;
>>       struct rdt_domain *dom;
>>       struct rdt_resource *r;
>>       int ret;
>> @@ -2565,7 +2589,10 @@ static int rdt_get_tree(struct fs_context *fc)
>>         closid_init();
>>   -    ret = rdtgroup_add_files(rdtgroup_default.kn, RFTYPE_CTRL_BASE);
>> +    if (rdt_mon_capable)
>> +        flags |= RFTYPE_MON;
>> +
>> +    ret = rdtgroup_add_files(rdtgroup_default.kn, flags);
>>       if (ret)
>>           goto out_schemata_free;
>>   @@ -3255,8 +3282,8 @@ static int mkdir_rdt_prepare(struct kernfs_node
>> *parent_kn,
>>                    enum rdt_group_type rtype, struct rdtgroup **r)
>>   {
>>       struct rdtgroup *prdtgrp, *rdtgrp;
>> +    unsigned long files = 0;
>>       struct kernfs_node *kn;
>> -    uint files = 0;
>>       int ret;
>>         prdtgrp = rdtgroup_kn_lock_live(parent_kn);
>> @@ -3308,10 +3335,13 @@ static int mkdir_rdt_prepare(struct kernfs_node
>> *parent_kn,
>>           goto out_destroy;
>>       }
>>   -    if (rtype == RDTCTRL_GROUP)
>> +    if (rtype == RDTCTRL_GROUP) {
>>           files = RFTYPE_BASE | RFTYPE_CTRL;
>> -    else
>> +        if (rdt_mon_capable)
>> +            files |= RFTYPE_MON;
>> +    } else {
>>           files = RFTYPE_BASE | RFTYPE_MON;
>> +    }
>>         ret = rdtgroup_add_files(kn, files);
>>       if (ret) {
> 
> Thanks.
> 
> -Fenghua

-- 
Thanks
Babu Moger