Hi Babu, On 8/31/2023 4:58 PM, Moger, Babu wrote: > On 8/31/23 12:42, Reinette Chatre wrote: >> On 8/30/2023 4:19 PM, Moger, Babu wrote: >>> On 8/29/23 15:14, Reinette Chatre wrote: >>>> On 8/21/2023 4:30 PM, Babu Moger wrote: >>>> >>>> ... >>>> >>>>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h >>>>> index 2fae6d9e24d3..3fa32c1c2677 100644 >>>>> --- a/arch/x86/kernel/cpu/resctrl/internal.h >>>>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h >>>>> @@ -296,9 +296,15 @@ struct rdtgroup { >>>>> * --> RFTYPE_BASE (Files common for both MON and CTRL groups) >>>>> * Files: cpus, cpus_list, tasks >>>>> * >>>>> + * --> RFTYPE_DEBUG (Files to help resctrl debugging) >>>>> + * File: mon_hw_id >>>>> + * >>>> >>>> This does not look right. I think mon_hw_id should have RFTYPE_MON >>>> (more below). >>> >>> I am not sure about this (more below). >>> >>>> >>>>> * --> RFTYPE_CTRL (Files only for CTRL group) >>>>> * Files: mode, schemata, size >>>>> * >>>>> + * --> RFTYPE_DEBUG (Files to help resctrl debugging) >>>>> + * File: ctrl_hw_id >>>>> + * >>>>> */ >>>>> #define RFTYPE_INFO BIT(0) >>>>> #define RFTYPE_BASE BIT(1) >>>>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c >>>>> index 94bd69f9964c..e0c2479acf49 100644 >>>>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c >>>>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c >>>>> @@ -776,6 +776,38 @@ static int rdtgroup_tasks_show(struct kernfs_open_file *of, >>>>> return ret; >>>>> } >>>>> >>>>> +static int rdtgroup_closid_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->closid); >>>>> + else >>>>> + ret = -ENOENT; >>>>> + rdtgroup_kn_unlock(of->kn); >>>>> + >>>>> + 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 >>>>> >>>>> /* >>>>> @@ -1837,6 +1869,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, >>>>> + .fflags = RFTYPE_BASE | RFTYPE_DEBUG, >>>> >>>> I missed this earlier but I think this also needs a RFTYPE_MON. >>>> Perhaps patch 3 can introduce a new RFTYPE_MON_BASE to not >>>> have the flags of the two new files look too different? >>> >>> We have two types of files in base directory structure. >>> >>> if (rtype == RDTCTRL_GROUP) >>> files = RFTYPE_BASE | RFTYPE_CTRL; >>> else >>> files = RFTYPE_BASE | RFTYPE_MON; >>> >>> 1. RFTYPE_BASE | RFTYPE_CTRL; >>> Files for the control group only. >>> >>> 2. RFTYPE_BASE | RFTYPE_MON; >>> Files for both control and mon groups. However, RFTYPE_MON is not used >>> for any files. It is only RFTYPE_BASE. >>> >>> Because of the check in rdtgroup_add_files it all works fine. >>> For the control group it creates files with RFTYPE_BASE | RFTYPE_CTRL and >>> RFTYPE_BASE. >>> >>> For the mon group it creates files with RFTYPE_BASE only. >> >> This describes current behavior because there are no resctrl >> files in base that are specific to monitoring, mon_hw_id is the >> first. >> >> This does not mean that the new file mon_hw_id should just have >> RFTYPE_BASE because that would result in mon_hw_id being created >> for all control groups, even those that do not support monitoring >> Having mon_hw_id in resctrl for a group that does not support >> monitoring is not correct. >> >> You should be able to reproduce this when booting your system >> with rdt=!cmt,!mbmlocal,!mbmtotal. > > You are right. I reproduced it. > >> >>> >>> Adding FTYPE_MON_BASE will be a problem. >>> >> >> Yes, this change does not just involve assigning the RFTYPE_MON >> to mon_hw_id. As you describe mkdir_rdt_prepare() does not take >> RFTYPE_MON into account when creating the files. Could this not just >> be a straightforward change to have it append RFTYPE_MON to the flags >> of files needing to be created for a CTRL_MON group? This would >> support new resource groups and then the default resource group >> would need to be taken into account also. What am I missing? >> > > It is not straight forward. We have have to handle few more things. > 1. Base directory creation. > 2. Mon directory creation after the base. > heh ... these are not a "few more things" ... these are exactly the items I mentioned: "base directory creation" is taking into account the default resource group and "mon directory creation after the base" are the changes needed in mkdir_rdt_prepare() where RFTYPE_MON is appended to the flags. > I got it working with this patches. We may be able to clean it little > more or we can split also. I think it would make things easier to understand if there is a separate patch that adds support for files with RFTYPE_MON flag. > > diff --git a/arch/x86/kernel/cpu/resctrl/internal.h > b/arch/x86/kernel/cpu/resctrl/internal.h > index 3fa32c1c2677..e2f3197f1c24 100644 > --- a/arch/x86/kernel/cpu/resctrl/internal.h > +++ b/arch/x86/kernel/cpu/resctrl/internal.h > @@ -318,6 +318,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 e0c2479acf49..1f9abab7b9bd 100644 > --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c > +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c > @@ -1874,7 +1874,7 @@ static struct rftype res_common_files[] = { > .mode = 0444, > .kf_ops = &rdtgroup_kf_single_ops, > .seq_show = rdtgroup_rmid_show, > - .fflags = RFTYPE_BASE | RFTYPE_DEBUG, > + .fflags = RFTYPE_MON_BASE | RFTYPE_DEBUG, > }, > { > .name = "schemata", > @@ -2558,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); > + uint flags = RFTYPE_CTRL_BASE; I assume that you may have just copied this from mkdir_rdt_prepare() but I think this should rather match the type as this is used (unsigned long). > struct rdt_domain *dom; > struct rdt_resource *r; > int ret; > @@ -2588,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; > > @@ -3336,6 +3340,9 @@ static int mkdir_rdt_prepare(struct kernfs_node > *parent_kn, > else > files = RFTYPE_BASE | RFTYPE_MON; > > + if (rdt_mon_capable) > + files |= RFTYPE_MON; > + Is this not redundant considering what just happened a few lines above? > ret = rdtgroup_add_files(kn, files); > if (ret) { > rdt_last_cmd_puts("kernfs fill error\n"); > > Reinette