These series adds support few minor features.
1. Support assigning multiple tasks to control/mon groups in one command.
2. Add debug mount option for resctrl interface.
3. Add RMID and CLOSID in resctrl interface when mounted with debug option.
4. Moves the default control group creation during the mount instead of during init.
5. While doing these above changes, found that rftype flags needed some cleanup.
They were named inconsistently. Re-arranged them much more cleanly now and added
few comments. Hope it can help future additions.
---
v6:
Changes since v5:
Moved the default group creation during mount instead of kernel init.
The rdt_root creation moved to rdt_get_tree as suggested by Reinette.
https://lore.kernel.org/lkml/[email protected]/
Needed to modify rdtgroup_setup_root to take care of this.
Re-arraged the patches to move the default group creation earlier.
Others are mostly text changes and few minor changes.
Patches are based on tip/master commit 1a2945f27157825a561be7840023e3664111ab2f
v5:
Changes since v4:
Moved the default group creation during mount instead of kernel init.
Tried to address most of the comments on commit log. Added more context and details.
Addressed feedback about the patch4. Removed the code changes and only kept the comments.
I am ok to drop patch4. But I will wait for the comment on that.
There were lots of comments. Hope I did not miss anything. Even if I missed, it is
not intentional.
v4: Changes since v3
Addressed comments from Reinette and others.
Removed newline requirement when adding tasks.
Dropped one of the changes on flags. Kept the flag names mostly same.
Changed the names of closid and rmid to ctrl_hw_id and mon_hw_id respectively.
James had some concerns about adding these files. Addressed it by making these
files x86 specific.
Tried to address Reinette's comment on patch 7. But due to current code design
I could not do it exact way. But changed it little bit to make it easy debug
file additions in the future.
v3: Changes since v2
Still waiting for more comments. While waiting, addressed few comments from Fenghua.
Added few more texts in the documentation about multiple tasks assignment feature.
Added pid in last_cmd_status when applicable.
Introduced static resctrl_debug to save the debug option.
Few minor text changes.
v2: Changes since v1
a. Removed the changes to add the task's threads automatically. It required
book keeping to handle the failures and gets complicated. Removed that change
for now.
b. Added -o debug option to mount in debug mode(comment from Fenghua)
c. Added debug files rmid and closid. Stephane wanted to rename them more
generic to accommodate ARM. It kind of loses meaning if is renamed differently.
Kept it same for now. Will change if he feels strong about it.
v5: https://lore.kernel.org/lkml/168564586603.527584.10518315376465080920.stgit@bmoger-ubuntu/
v4: https://lore.kernel.org/lkml/168177435378.1758847.8317743523931859131.stgit@bmoger-ubuntu/
v3: https://lore.kernel.org/lkml/167778850105.1053859.14596357862185564029.stgit@bmoger-ubuntu/
v2: https://lore.kernel.org/lkml/167537433143.647488.9641864719195184123.stgit@bmoger-ubuntu/
v1: https://lore.kernel.org/lkml/167278351577.34228.12803395505584557101.stgit@bmoger-ubuntu/
Babu Moger (8):
x86/resctrl: Add multiple tasks to the resctrl group at once
x86/resctrl: Simplify rftype flag definitions
x86/resctrl: Rename rftype flags for consistency
x86/resctrl: Add comments on RFTYPE flags hierarchy
x86/resctrl: Unwind the errors inside rdt_enable_ctx
x86/resctrl: Move default control group creation during mount
x86/resctrl: Introduce "-o debug" mount option
x86/resctrl: Display hardware ids of resource groups
Documentation/arch/x86/resctrl.rst | 21 ++-
arch/x86/kernel/cpu/resctrl/internal.h | 70 ++++++--
arch/x86/kernel/cpu/resctrl/rdtgroup.c | 217 ++++++++++++++++++-------
3 files changed, 236 insertions(+), 72 deletions(-)
--
Add "-o debug" option to mount resctrl filesystem in debug mode. This
option is used for adding extra files to help resctrl debugging.
Signed-off-by: Babu Moger <[email protected]>
---
Documentation/arch/x86/resctrl.rst | 5 ++++-
arch/x86/kernel/cpu/resctrl/internal.h | 2 ++
arch/x86/kernel/cpu/resctrl/rdtgroup.c | 20 ++++++++++++++++++++
3 files changed, 26 insertions(+), 1 deletion(-)
diff --git a/Documentation/arch/x86/resctrl.rst b/Documentation/arch/x86/resctrl.rst
index af234681756e..5a2346d2c561 100644
--- a/Documentation/arch/x86/resctrl.rst
+++ b/Documentation/arch/x86/resctrl.rst
@@ -35,7 +35,7 @@ about the feature from resctrl's info directory.
To use the feature mount the file system::
- # mount -t resctrl resctrl [-o cdp[,cdpl2][,mba_MBps]] /sys/fs/resctrl
+ # mount -t resctrl resctrl [-o cdp[,cdpl2][,mba_MBps][,debug]] /sys/fs/resctrl
mount options are:
@@ -46,6 +46,9 @@ mount options are:
"mba_MBps":
Enable the MBA Software Controller(mba_sc) to specify MBA
bandwidth in MBps
+"debug":
+ Make debug files accessible. Available debug files are annotated with
+ "Available only with debug option".
L2 and L3 CDP are controlled separately.
diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
index 885b656d1088..5bc18accb887 100644
--- a/arch/x86/kernel/cpu/resctrl/internal.h
+++ b/arch/x86/kernel/cpu/resctrl/internal.h
@@ -59,6 +59,7 @@ struct rdt_fs_context {
bool enable_cdpl2;
bool enable_cdpl3;
bool enable_mba_mbps;
+ bool enable_debug;
};
static inline struct rdt_fs_context *rdt_fc2context(struct fs_context *fc)
@@ -296,6 +297,7 @@ struct rdtgroup {
#define RFTYPE_TOP BIT(6)
#define RFTYPE_RES_CACHE BIT(8)
#define RFTYPE_RES_MB BIT(9)
+#define RFTYPE_DEBUG BIT(10)
#define RFTYPE_CTRL_INFO (RFTYPE_INFO | RFTYPE_CTRL)
#define RFTYPE_MON_INFO (RFTYPE_INFO | RFTYPE_MON)
#define RFTYPE_TOP_INFO (RFTYPE_INFO | RFTYPE_TOP)
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index e0570bce76a2..058c1dedb2d7 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -56,6 +56,8 @@ static char last_cmd_status_buf[512];
struct dentry *debugfs_resctrl;
+static bool resctrl_debug;
+
void rdt_last_cmd_clear(void)
{
lockdep_assert_held(&rdtgroup_mutex);
@@ -1871,6 +1873,9 @@ static int rdtgroup_add_files(struct kernfs_node *kn, unsigned long fflags)
lockdep_assert_held(&rdtgroup_mutex);
+ if (resctrl_debug)
+ fflags |= RFTYPE_DEBUG;
+
for (rft = rfts; rft < rfts + len; rft++) {
if (rft->fflags && ((fflags & rft->fflags) == rft->fflags)) {
ret = rdtgroup_add_file(kn, rft);
@@ -2399,6 +2404,9 @@ static int rdt_enable_ctx(struct rdt_fs_context *ctx)
goto out_cdpl3;
}
+ if (ctx->enable_debug)
+ resctrl_debug = true;
+
return 0;
out_cdpl3:
@@ -2591,6 +2599,8 @@ static int rdt_get_tree(struct fs_context *fc)
out_schemata_free:
schemata_list_destroy();
out_ctx:
+ if (ctx->enable_debug)
+ resctrl_debug = false;
if (ctx->enable_mba_mbps)
set_mba_sc(false);
cdp_disable_all();
@@ -2608,6 +2618,7 @@ enum rdt_param {
Opt_cdp,
Opt_cdpl2,
Opt_mba_mbps,
+ Opt_debug,
nr__rdt_params
};
@@ -2615,6 +2626,7 @@ static const struct fs_parameter_spec rdt_fs_parameters[] = {
fsparam_flag("cdp", Opt_cdp),
fsparam_flag("cdpl2", Opt_cdpl2),
fsparam_flag("mba_MBps", Opt_mba_mbps),
+ fsparam_flag("debug", Opt_debug),
{}
};
@@ -2640,6 +2652,9 @@ static int rdt_parse_param(struct fs_context *fc, struct fs_parameter *param)
return -EINVAL;
ctx->enable_mba_mbps = true;
return 0;
+ case Opt_debug:
+ ctx->enable_debug = true;
+ return 0;
}
return -EINVAL;
@@ -2829,6 +2844,8 @@ static void rdt_kill_sb(struct super_block *sb)
set_mba_sc(false);
+ resctrl_debug = false;
+
/*Put everything back to default values. */
for_each_alloc_capable_rdt_resource(r)
reset_all_ctrls(r);
@@ -3710,6 +3727,9 @@ static int rdtgroup_show_options(struct seq_file *seq, struct kernfs_root *kf)
if (is_mba_sc(&rdt_resources_all[RDT_RESOURCE_MBA].r_resctrl))
seq_puts(seq, ",mba_MBps");
+ if (resctrl_debug)
+ seq_puts(seq, ",debug");
+
return 0;
}
The resctrl default control group is created during kernel init time. If
the new files are to be added to the default group during the mount based
on the mount option, then each file needs to be created separately and
call kernfs_activate.
This can avoided if all the files are created during the mount and
destroyed during the umount. Move the root and default group creation
in rdt_get_tree and removal in rdt_kill_sb.
Suggested-by: Reinette Chatre <[email protected]>
Signed-off-by: Babu Moger <[email protected]>
---
arch/x86/kernel/cpu/resctrl/internal.h | 1 +
arch/x86/kernel/cpu/resctrl/rdtgroup.c | 51 ++++++++++++++------------------
2 files changed, 24 insertions(+), 28 deletions(-)
diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
index 783c3e36633c..885b656d1088 100644
--- a/arch/x86/kernel/cpu/resctrl/internal.h
+++ b/arch/x86/kernel/cpu/resctrl/internal.h
@@ -601,5 +601,6 @@ void rdt_domain_reconfigure_cdp(struct rdt_resource *r);
void __init thread_throttle_mode_init(void);
void __init mbm_config_rftype_init(const char *config);
void rdt_staged_configs_clear(void);
+int rdtgroup_setup_root(void);
#endif /* _ASM_X86_RESCTRL_INTERNAL_H */
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index 9a7204f71d2d..e0570bce76a2 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -2512,10 +2512,16 @@ static int rdt_get_tree(struct fs_context *fc)
goto out;
}
- ret = rdt_enable_ctx(ctx);
+ ret = rdtgroup_setup_root();
if (ret)
goto out;
+ ctx->kfc.root = rdt_root;
+
+ ret = rdt_enable_ctx(ctx);
+ if (ret)
+ goto out_root;
+
ret = schemata_list_create();
if (ret) {
schemata_list_destroy();
@@ -2524,6 +2530,12 @@ static int rdt_get_tree(struct fs_context *fc)
closid_init();
+ ret = rdtgroup_add_files(rdtgroup_default.kn, RFTYPE_CTRL_BASE);
+ if (ret)
+ goto out_schemata_free;
+
+ kernfs_activate(rdtgroup_default.kn);
+
ret = rdtgroup_create_info_dir(rdtgroup_default.kn);
if (ret < 0)
goto out_schemata_free;
@@ -2582,6 +2594,9 @@ static int rdt_get_tree(struct fs_context *fc)
if (ctx->enable_mba_mbps)
set_mba_sc(false);
cdp_disable_all();
+out_root:
+ list_del(&rdtgroup_default.rdtgroup_list);
+ kernfs_destroy_root(rdt_root);
out:
rdt_last_cmd_clear();
mutex_unlock(&rdtgroup_mutex);
@@ -2652,7 +2667,6 @@ static int rdt_init_fs_context(struct fs_context *fc)
if (!ctx)
return -ENOMEM;
- ctx->kfc.root = rdt_root;
ctx->kfc.magic = RDTGROUP_SUPER_MAGIC;
fc->fs_private = &ctx->kfc;
fc->ops = &rdt_fs_context_ops;
@@ -2821,7 +2835,9 @@ static void rdt_kill_sb(struct super_block *sb)
cdp_disable_all();
rmdir_all_sub();
rdt_pseudo_lock_release();
- rdtgroup_default.mode = RDT_MODE_SHAREABLE;
+ /* Remove the default group and cleanup the root */
+ list_del(&rdtgroup_default.rdtgroup_list);
+ kernfs_destroy_root(rdt_root);
schemata_list_destroy();
static_branch_disable_cpuslocked(&rdt_alloc_enable_key);
static_branch_disable_cpuslocked(&rdt_mon_enable_key);
@@ -3704,10 +3720,8 @@ static struct kernfs_syscall_ops rdtgroup_kf_syscall_ops = {
.show_options = rdtgroup_show_options,
};
-static int __init rdtgroup_setup_root(void)
+int rdtgroup_setup_root(void)
{
- int ret;
-
rdt_root = kernfs_create_root(&rdtgroup_kf_syscall_ops,
KERNFS_ROOT_CREATE_DEACTIVATED |
KERNFS_ROOT_EXTRA_OPEN_PERM_CHECK,
@@ -3715,28 +3729,17 @@ static int __init rdtgroup_setup_root(void)
if (IS_ERR(rdt_root))
return PTR_ERR(rdt_root);
- mutex_lock(&rdtgroup_mutex);
-
rdtgroup_default.closid = 0;
rdtgroup_default.mon.rmid = 0;
rdtgroup_default.type = RDTCTRL_GROUP;
+ rdtgroup_default.mode = RDT_MODE_SHAREABLE;
INIT_LIST_HEAD(&rdtgroup_default.mon.crdtgrp_list);
list_add(&rdtgroup_default.rdtgroup_list, &rdt_all_groups);
- ret = rdtgroup_add_files(kernfs_root_to_node(rdt_root), RFTYPE_CTRL_BASE);
- if (ret) {
- kernfs_destroy_root(rdt_root);
- goto out;
- }
-
rdtgroup_default.kn = kernfs_root_to_node(rdt_root);
- kernfs_activate(rdtgroup_default.kn);
-out:
- mutex_unlock(&rdtgroup_mutex);
-
- return ret;
+ return 0;
}
static void domain_destroy_mon_state(struct rdt_domain *d)
@@ -3858,13 +3861,9 @@ int __init rdtgroup_init(void)
seq_buf_init(&last_cmd_status, last_cmd_status_buf,
sizeof(last_cmd_status_buf));
- ret = rdtgroup_setup_root();
- if (ret)
- return ret;
-
ret = sysfs_create_mount_point(fs_kobj, "resctrl");
if (ret)
- goto cleanup_root;
+ return ret;
ret = register_filesystem(&rdt_fs_type);
if (ret)
@@ -3897,9 +3896,6 @@ int __init rdtgroup_init(void)
cleanup_mountpoint:
sysfs_remove_mount_point(fs_kobj, "resctrl");
-cleanup_root:
- kernfs_destroy_root(rdt_root);
-
return ret;
}
@@ -3908,5 +3904,4 @@ void __exit rdtgroup_exit(void)
debugfs_remove_recursive(debugfs_resctrl);
unregister_filesystem(&rdt_fs_type);
sysfs_remove_mount_point(fs_kobj, "resctrl");
- kernfs_destroy_root(rdt_root);
}
The rftype flags are bitmaps used for adding files under resctrl
filesystem. Some of these bitmaps have one extra level of indirection
which is not necessary.
Make them all direct definition to be consistent and easier to read.
Reviewed-by: Shaopeng Tan <[email protected]>
Tested-by: Shaopeng Tan <[email protected]>
Signed-off-by: Babu Moger <[email protected]>
Reviewed-by: Reinette Chatre <[email protected]>
---
arch/x86/kernel/cpu/resctrl/internal.h | 9 +++------
arch/x86/kernel/cpu/resctrl/rdtgroup.c | 6 +++++-
2 files changed, 8 insertions(+), 7 deletions(-)
diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
index 85ceaf9a31ac..62767774810d 100644
--- a/arch/x86/kernel/cpu/resctrl/internal.h
+++ b/arch/x86/kernel/cpu/resctrl/internal.h
@@ -243,12 +243,9 @@ struct rdtgroup {
*/
#define RFTYPE_INFO BIT(0)
#define RFTYPE_BASE BIT(1)
-#define RF_CTRLSHIFT 4
-#define RF_MONSHIFT 5
-#define RF_TOPSHIFT 6
-#define RFTYPE_CTRL BIT(RF_CTRLSHIFT)
-#define RFTYPE_MON BIT(RF_MONSHIFT)
-#define RFTYPE_TOP BIT(RF_TOPSHIFT)
+#define RFTYPE_CTRL BIT(4)
+#define RFTYPE_MON BIT(5)
+#define RFTYPE_TOP BIT(6)
#define RFTYPE_RES_CACHE BIT(8)
#define RFTYPE_RES_MB BIT(9)
#define RF_CTRL_INFO (RFTYPE_INFO | RFTYPE_CTRL)
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index 8c91c333f9b3..2f1b9f69326f 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -3242,7 +3242,11 @@ static int mkdir_rdt_prepare(struct kernfs_node *parent_kn,
goto out_destroy;
}
- files = RFTYPE_BASE | BIT(RF_CTRLSHIFT + rtype);
+ if (rtype == RDTCTRL_GROUP)
+ files = RFTYPE_BASE | RFTYPE_CTRL;
+ else
+ files = RFTYPE_BASE | RFTYPE_MON;
+
ret = rdtgroup_add_files(kn, files);
if (ret) {
rdt_last_cmd_puts("kernfs fill error\n");
In x86, hardware uses CLOSID and an RMID to identify a control group and
a monitoring group respectively. When a user creates a control or monitor
group these details are not visible to the user. These details can help
debugging.
Add CLOSID(ctrl_hw_id) and RMID(mon_hw_id) to the control/monitor groups
display in resctrl interface. Users can see these details when resctrl
is mounted with "-o debug" option.
Other architectures do not use "CLOSID" and "RMID". Use the names
ctrl_hw_id and mon_hw_id to refer "CLOSID" and "RMID" respectively in an
effort to keep the naming generic.
For example:
$cat /sys/fs/resctrl/ctrl_grp1/ctrl_hw_id
1
$cat /sys/fs/resctrl/mon_groups/mon_grp1/mon_hw_id
3
Signed-off-by: Babu Moger <[email protected]>
---
Documentation/arch/x86/resctrl.rst | 8 ++++++
arch/x86/kernel/cpu/resctrl/rdtgroup.c | 46 ++++++++++++++++++++++++++++++++
2 files changed, 54 insertions(+)
diff --git a/Documentation/arch/x86/resctrl.rst b/Documentation/arch/x86/resctrl.rst
index 5a2346d2c561..41ad9b1f0c6a 100644
--- a/Documentation/arch/x86/resctrl.rst
+++ b/Documentation/arch/x86/resctrl.rst
@@ -351,6 +351,10 @@ When control is enabled all CTRL_MON groups will also contain:
file. On successful pseudo-locked region creation the mode will
automatically change to "pseudo-locked".
+"ctrl_hw_id":
+ Available only with debug option. The identifier used by hardware
+ for the control group. On x86 this is the CLOSID.
+
When monitoring is enabled all MON groups will also contain:
"mon_data":
@@ -364,6 +368,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/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index 058c1dedb2d7..e8b35013d7c1 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,
+ },
{
.name = "schemata",
.mode = 0644,
@@ -1860,6 +1899,13 @@ static struct rftype res_common_files[] = {
.seq_show = rdtgroup_size_show,
.fflags = RFTYPE_CTRL_BASE,
},
+ {
+ .name = "ctrl_hw_id",
+ .mode = 0444,
+ .kf_ops = &rdtgroup_kf_single_ops,
+ .seq_show = rdtgroup_closid_show,
+ .fflags = RFTYPE_CTRL_BASE | RFTYPE_DEBUG,
+ },
};
rdt_enable_ctx() takes care of enabling the features provided during
resctrl mount. The error unwinding of rdt_enable_ctx is done from the
caller rdt_get_tree. This is not ideal and can cause some error unwinding
to be omitted.
Fix this by moving all the error unwinding inside rdt_enable_ctx.
Suggested-by: Reinette Chatre <[email protected]>
Signed-off-by: Babu Moger <[email protected]>
---
arch/x86/kernel/cpu/resctrl/rdtgroup.c | 31 +++++++++++++++++++++++--------
1 file changed, 23 insertions(+), 8 deletions(-)
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index 3010e3a1394d..9a7204f71d2d 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -2381,15 +2381,31 @@ static int rdt_enable_ctx(struct rdt_fs_context *ctx)
{
int ret = 0;
- if (ctx->enable_cdpl2)
+ if (ctx->enable_cdpl2) {
ret = resctrl_arch_set_cdp_enabled(RDT_RESOURCE_L2, true);
+ if (ret)
+ goto out;
+ }
- if (!ret && ctx->enable_cdpl3)
+ if (ctx->enable_cdpl3) {
ret = resctrl_arch_set_cdp_enabled(RDT_RESOURCE_L3, true);
+ if (ret)
+ goto out_cdpl2;
+ }
- if (!ret && ctx->enable_mba_mbps)
+ if (ctx->enable_mba_mbps) {
ret = set_mba_sc(true);
+ if (ret)
+ goto out_cdpl3;
+ }
+ return 0;
+
+out_cdpl3:
+ resctrl_arch_set_cdp_enabled(RDT_RESOURCE_L3, false);
+out_cdpl2:
+ resctrl_arch_set_cdp_enabled(RDT_RESOURCE_L2, false);
+out:
return ret;
}
@@ -2497,13 +2513,13 @@ static int rdt_get_tree(struct fs_context *fc)
}
ret = rdt_enable_ctx(ctx);
- if (ret < 0)
- goto out_cdp;
+ if (ret)
+ goto out;
ret = schemata_list_create();
if (ret) {
schemata_list_destroy();
- goto out_mba;
+ goto out_ctx;
}
closid_init();
@@ -2562,10 +2578,9 @@ static int rdt_get_tree(struct fs_context *fc)
kernfs_remove(kn_info);
out_schemata_free:
schemata_list_destroy();
-out_mba:
+out_ctx:
if (ctx->enable_mba_mbps)
set_mba_sc(false);
-out_cdp:
cdp_disable_all();
out:
rdt_last_cmd_clear();
resctrl uses RFTYPE flags for creating resctrl directory structure.
Definitions and directory structures are not documented. Add
comments to improve the readability and help future additions.
Signed-off-by: Babu Moger <[email protected]>
---
arch/x86/kernel/cpu/resctrl/internal.h | 48 ++++++++++++++++++++++++++++++++
1 file changed, 48 insertions(+)
diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
index 2051179a3b91..783c3e36633c 100644
--- a/arch/x86/kernel/cpu/resctrl/internal.h
+++ b/arch/x86/kernel/cpu/resctrl/internal.h
@@ -240,6 +240,54 @@ struct rdtgroup {
/*
* Define the file type flags for base and info directories.
+ *
+ * RESCTRL filesystem has two main components
+ * a. info
+ * b. base
+ *
+ * /sys/fs/resctrl/
+ * |
+ * --> info (Top level directory named "info". Contains files that
+ * | provide details on control and monitoring resources.)
+ * |
+ * --> base (Root directory associated with default resource group
+ * as well as directories created by user for MON and CTRL
+ * groups. Contains files to interact with MON and CTRL
+ * groups.)
+ *
+ * info directory structure
+ * ------------------------------------------------------------------
+ * --> RFTYPE_INFO
+ * --> <info> directory
+ * --> RFTYPE_TOP_INFO
+ * Files: last_cmd_status
+ *
+ * --> RFTYPE_MON_INFO
+ * --> <L3_MON> directory
+ * Files: max_threshold_occupancy, mon_features,
+ * num_rmids, mbm_total_bytes_config,
+ * mbm_locat_bytes_config
+ *
+ * --> RFTYPE_CTRL_INFO
+ * --> RFTYPE_RES_CACHE
+ * --> <L2,L3> directory
+ * Files: bit_usage, cbm_mask, min_cbm_bits,
+ * num_closids, shareable_bits
+ *
+ * --> RFTYPE_RES_MB
+ * --> <MB,SMBA> directory
+ * Files: bandwidth_gran, delay_linear,
+ * min_bandwidth, num_closids,
+ * thread_throttle_mode
+ *
+ * base (root) directory structure
+ * ------------------------------------------------------------------
+ * --> RFTYPE_BASE (Files common for both MON and CTRL groups)
+ * Files: cpus, cpus_list, tasks
+ *
+ * --> RFTYPE_CTRL_BASE (Files only for CTRL group)
+ * Files: mode, schemata, size
+ *
*/
#define RFTYPE_INFO BIT(0)
#define RFTYPE_BASE BIT(1)
Hi Babu,
I reviewed this patch series(v6) and it looks fine.
I tested these features and ran the selftests/resctrl test set,
and there is no problem.
<Reviewed-by:[email protected]>
<Tested-by:[email protected]>
Best regards,
Shaopeng TAN
Hi Babu,
On 7/19/2023 4:21 PM, Babu Moger wrote:
> + * ------------------------------------------------------------------
> + * --> RFTYPE_INFO
> + * --> <info> directory
> + * --> RFTYPE_TOP_INFO
> + * Files: last_cmd_status
> + *
> + * --> RFTYPE_MON_INFO
> + * --> <L3_MON> directory
> + * Files: max_threshold_occupancy, mon_features,
> + * num_rmids, mbm_total_bytes_config,
> + * mbm_locat_bytes_config
mbm_locat_bytes_config -> mbm_local_bytes_config
> + *
> + * --> RFTYPE_CTRL_INFO
> + * --> RFTYPE_RES_CACHE
> + * --> <L2,L3> directory
Should this be "directories"?
> + * Files: bit_usage, cbm_mask, min_cbm_bits,
> + * num_closids, shareable_bits
Based on the hierarchy presented the files mentioned here may be expected
to be associated with RFTYPE_CTRL_INFO | RFTYPE_RES_CACHE.
For accuracy it may be better to move num_closids one level higher so
that it is only associated with RFTYPE_CTRL_INFO?
> + *
> + * --> RFTYPE_RES_MB
> + * --> <MB,SMBA> directory
directories?
> + * Files: bandwidth_gran, delay_linear,
> + * min_bandwidth, num_closids,
Please remove space before tab for a clean checkpatch.pl run.
> + * thread_throttle_mode
> + *
> + * base (root) directory structure
Since "base" refers to more than the root directory I think this can
just be "base directory structure".
> + * ------------------------------------------------------------------
> + * --> RFTYPE_BASE (Files common for both MON and CTRL groups)
> + * Files: cpus, cpus_list, tasks
> + *
> + * --> RFTYPE_CTRL_BASE (Files only for CTRL group)
> + * Files: mode, schemata, size
> + *
> */
> #define RFTYPE_INFO BIT(0)
> #define RFTYPE_BASE BIT(1)
>
>
Reinette
Hi Babu,
On 7/19/2023 4:22 PM, Babu Moger wrote:
> rdt_enable_ctx() takes care of enabling the features provided during
"rdt_enable_ctx() takes care of enabling" can just be "rdt_enable_ctx()
enables"
> resctrl mount. The error unwinding of rdt_enable_ctx is done from the
> caller rdt_get_tree. This is not ideal and can cause some error unwinding
> to be omitted.
>
Please consistently use () to indicate function names (in
changelog and subject).
"This is not ideal and can cause some error unwinding to be omitted."
is a bit vague. How about (in a new paragraph):
"Additions to rdt_enable_ctx() are required to also modify error paths
of rdt_enable_ctx() callers to ensure correct unwinding if errors
are encountered after calling rdt_enable_ctx(). This is error prone."
> Fix this by moving all the error unwinding inside rdt_enable_ctx.
"Fix" creates expectation for a "fixes" tag which is not needed here. This
refactors code to simplify future additions.
Even so, I do not think this solution addresses the stated problem (more
below).
>
> Suggested-by: Reinette Chatre <[email protected]>
> Signed-off-by: Babu Moger <[email protected]>
> ---
> arch/x86/kernel/cpu/resctrl/rdtgroup.c | 31 +++++++++++++++++++++++--------
> 1 file changed, 23 insertions(+), 8 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index 3010e3a1394d..9a7204f71d2d 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -2381,15 +2381,31 @@ static int rdt_enable_ctx(struct rdt_fs_context *ctx)
> {
> int ret = 0;
>
> - if (ctx->enable_cdpl2)
> + if (ctx->enable_cdpl2) {
> ret = resctrl_arch_set_cdp_enabled(RDT_RESOURCE_L2, true);
> + if (ret)
> + goto out;
> + }
>
> - if (!ret && ctx->enable_cdpl3)
> + if (ctx->enable_cdpl3) {
> ret = resctrl_arch_set_cdp_enabled(RDT_RESOURCE_L3, true);
> + if (ret)
> + goto out_cdpl2;
> + }
>
> - if (!ret && ctx->enable_mba_mbps)
> + if (ctx->enable_mba_mbps) {
> ret = set_mba_sc(true);
> + if (ret)
> + goto out_cdpl3;
> + }
>
> + return 0;
> +
> +out_cdpl3:
> + resctrl_arch_set_cdp_enabled(RDT_RESOURCE_L3, false);
> +out_cdpl2:
> + resctrl_arch_set_cdp_enabled(RDT_RESOURCE_L2, false);
Be careful here. There is no dependency between L3 and L2 CDP ...
if L3 CDP was enabled it does not mean that L2 CDP was enabled also.
Similarly, if the software controller was enabled it does not mean that
CDP was also enabled.
Since resctrl_arch_set_cdp_enabled() does much more than just change
a flag value I think these should first check if it was enabled
before disabling the feature.
> +out:
> return ret;
> }
>
> @@ -2497,13 +2513,13 @@ static int rdt_get_tree(struct fs_context *fc)
> }
>
> ret = rdt_enable_ctx(ctx);
> - if (ret < 0)
> - goto out_cdp;
> + if (ret)
> + goto out;
>
> ret = schemata_list_create();
> if (ret) {
> schemata_list_destroy();
> - goto out_mba;
> + goto out_ctx;
> }
>
> closid_init();
> @@ -2562,10 +2578,9 @@ static int rdt_get_tree(struct fs_context *fc)
> kernfs_remove(kn_info);
> out_schemata_free:
> schemata_list_destroy();
> -out_mba:
> +out_ctx:
> if (ctx->enable_mba_mbps)
> set_mba_sc(false);
> -out_cdp:
> cdp_disable_all();
> out:
> rdt_last_cmd_clear();
>
The problem statement in the changelog was that rdt_get_tree() is
doing error unwinding of rdt_enable_ctx(). Looking at the above it
seems that the problem remains ... callers of rdt_enable_ctx()
still need to know all internals of that function to do error
unwind correctly. Could it perhaps be made simpler with a new
rdt_disable_ctx() that undoes rdt_enable_ctx()? New additions
to rdt_enable_ctx() would have more clarity where changes are
needed and callers only need to call a single rdt_disable_ctx().
Reinette
Hi Babu,
On 7/19/2023 4:22 PM, Babu Moger wrote:
> @@ -2591,6 +2599,8 @@ static int rdt_get_tree(struct fs_context *fc)
> out_schemata_free:
> schemata_list_destroy();
> out_ctx:
> + if (ctx->enable_debug)
> + resctrl_debug = false;
> if (ctx->enable_mba_mbps)
> set_mba_sc(false);
> cdp_disable_all();
These changes are a red flag to me. Developers still need to
do what patch #4 was aiming to prevent.
Reinette
Hi Babu,
On 7/19/2023 4:22 PM, Babu Moger wrote:
> The resctrl default control group is created during kernel init time. If
> the new files are to be added to the default group during the mount based
> on the mount option, then each file needs to be created separately and
> call kernfs_activate.
>
> This can avoided if all the files are created during the mount and
> destroyed during the umount. Move the root and default group creation
> in rdt_get_tree and removal in rdt_kill_sb.
Please use () to indicate function names.
>
> Suggested-by: Reinette Chatre <[email protected]>
> Signed-off-by: Babu Moger <[email protected]>
> ---
...
> @@ -3704,10 +3720,8 @@ static struct kernfs_syscall_ops rdtgroup_kf_syscall_ops = {
> .show_options = rdtgroup_show_options,
> };
>
> -static int __init rdtgroup_setup_root(void)
> +int rdtgroup_setup_root(void)
> {
> - int ret;
> -
> rdt_root = kernfs_create_root(&rdtgroup_kf_syscall_ops,
> KERNFS_ROOT_CREATE_DEACTIVATED |
> KERNFS_ROOT_EXTRA_OPEN_PERM_CHECK,
> @@ -3715,28 +3729,17 @@ static int __init rdtgroup_setup_root(void)
> if (IS_ERR(rdt_root))
> return PTR_ERR(rdt_root);
>
> - mutex_lock(&rdtgroup_mutex);
> -
> rdtgroup_default.closid = 0;
> rdtgroup_default.mon.rmid = 0;
> rdtgroup_default.type = RDTCTRL_GROUP;
> + rdtgroup_default.mode = RDT_MODE_SHAREABLE;
> INIT_LIST_HEAD(&rdtgroup_default.mon.crdtgrp_list);
>
> list_add(&rdtgroup_default.rdtgroup_list, &rdt_all_groups);
>
> - ret = rdtgroup_add_files(kernfs_root_to_node(rdt_root), RFTYPE_CTRL_BASE);
> - if (ret) {
> - kernfs_destroy_root(rdt_root);
> - goto out;
> - }
> -
> rdtgroup_default.kn = kernfs_root_to_node(rdt_root);
> - kernfs_activate(rdtgroup_default.kn);
>
> -out:
> - mutex_unlock(&rdtgroup_mutex);
> -
> - return ret;
> + return 0;
> }
I am missing something here. Why is it now needed to re-initialize
and add default resource group on every mount of resctrl? I expected
only the kernfs related changes to move.
Reinette
Hi Reinette,
On 8/4/23 15:39, Reinette Chatre wrote:
> Hi Babu,
>
> On 7/19/2023 4:21 PM, Babu Moger wrote:
>> + * ------------------------------------------------------------------
>> + * --> RFTYPE_INFO
>> + * --> <info> directory
>> + * --> RFTYPE_TOP_INFO
>> + * Files: last_cmd_status
>> + *
>> + * --> RFTYPE_MON_INFO
>> + * --> <L3_MON> directory
>> + * Files: max_threshold_occupancy, mon_features,
>> + * num_rmids, mbm_total_bytes_config,
>> + * mbm_locat_bytes_config
>
> mbm_locat_bytes_config -> mbm_local_bytes_config
Good catch. Thanks
>
>> + *
>> + * --> RFTYPE_CTRL_INFO
>> + * --> RFTYPE_RES_CACHE
>> + * --> <L2,L3> directory
>
> Should this be "directories"?
Yes.
>
>> + * Files: bit_usage, cbm_mask, min_cbm_bits,
>> + * num_closids, shareable_bits
>
> Based on the hierarchy presented the files mentioned here may be expected
> to be associated with RFTYPE_CTRL_INFO | RFTYPE_RES_CACHE.
> For accuracy it may be better to move num_closids one level higher so
> that it is only associated with RFTYPE_CTRL_INFO?
>
>> + *
>> + * --> RFTYPE_RES_MB
>> + * --> <MB,SMBA> directory
>
> directories?
Yes.
>
>> + * Files: bandwidth_gran, delay_linear,
>> + * min_bandwidth, num_closids,
>
> Please remove space before tab for a clean checkpatch.pl run.
Sure.
>
>> + * thread_throttle_mode
>> + *
>> + * base (root) directory structure
>
> Since "base" refers to more than the root directory I think this can
> just be "base directory structure".
Sure.
>
>> + * ------------------------------------------------------------------
>> + * --> RFTYPE_BASE (Files common for both MON and CTRL groups)
>> + * Files: cpus, cpus_list, tasks
>> + *
>> + * --> RFTYPE_CTRL_BASE (Files only for CTRL group)
>> + * Files: mode, schemata, size
>> + *
>> */
>> #define RFTYPE_INFO BIT(0)
>> #define RFTYPE_BASE BIT(1)
>>
>>
>
> Reinette
--
Thanks
Babu Moger
Hi Reinette,
On 8/4/23 15:41, Reinette Chatre wrote:
> Hi Babu,
>
> On 7/19/2023 4:22 PM, Babu Moger wrote:
>> rdt_enable_ctx() takes care of enabling the features provided during
>
> "rdt_enable_ctx() takes care of enabling" can just be "rdt_enable_ctx()
> enables"
Sure.
>
>> resctrl mount. The error unwinding of rdt_enable_ctx is done from the
>> caller rdt_get_tree. This is not ideal and can cause some error unwinding
>> to be omitted.
>>
>
> Please consistently use () to indicate function names (in
> changelog and subject).
Sure.
>
> "This is not ideal and can cause some error unwinding to be omitted."
> is a bit vague. How about (in a new paragraph):
> "Additions to rdt_enable_ctx() are required to also modify error paths
> of rdt_enable_ctx() callers to ensure correct unwinding if errors
> are encountered after calling rdt_enable_ctx(). This is error prone."
Sure.
>
>> Fix this by moving all the error unwinding inside rdt_enable_ctx.
>
> "Fix" creates expectation for a "fixes" tag which is not needed here. This
> refactors code to simplify future additions.
Sure.
>
> Even so, I do not think this solution addresses the stated problem (more
> below).
>
>>
>> Suggested-by: Reinette Chatre <[email protected]>
>> Signed-off-by: Babu Moger <[email protected]>
>> ---
>> arch/x86/kernel/cpu/resctrl/rdtgroup.c | 31 +++++++++++++++++++++++--------
>> 1 file changed, 23 insertions(+), 8 deletions(-)
>>
>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> index 3010e3a1394d..9a7204f71d2d 100644
>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> @@ -2381,15 +2381,31 @@ static int rdt_enable_ctx(struct rdt_fs_context *ctx)
>> {
>> int ret = 0;
>>
>> - if (ctx->enable_cdpl2)
>> + if (ctx->enable_cdpl2) {
>> ret = resctrl_arch_set_cdp_enabled(RDT_RESOURCE_L2, true);
>> + if (ret)
>> + goto out;
>> + }
>>
>> - if (!ret && ctx->enable_cdpl3)
>> + if (ctx->enable_cdpl3) {
>> ret = resctrl_arch_set_cdp_enabled(RDT_RESOURCE_L3, true);
>> + if (ret)
>> + goto out_cdpl2;
>> + }
>>
>> - if (!ret && ctx->enable_mba_mbps)
>> + if (ctx->enable_mba_mbps) {
>> ret = set_mba_sc(true);
>> + if (ret)
>> + goto out_cdpl3;
>> + }
>>
>> + return 0;
>> +
>> +out_cdpl3:
>> + resctrl_arch_set_cdp_enabled(RDT_RESOURCE_L3, false);
>> +out_cdpl2:
>> + resctrl_arch_set_cdp_enabled(RDT_RESOURCE_L2, false);
>
> Be careful here. There is no dependency between L3 and L2 CDP ...
> if L3 CDP was enabled it does not mean that L2 CDP was enabled also.
> Similarly, if the software controller was enabled it does not mean that
> CDP was also enabled.
> Since resctrl_arch_set_cdp_enabled() does much more than just change
> a flag value I think these should first check if it was enabled
> before disabling the feature.
Yes. Agree.
>
>> +out:
>> return ret;
>> }
>>
>> @@ -2497,13 +2513,13 @@ static int rdt_get_tree(struct fs_context *fc)
>> }
>>
>> ret = rdt_enable_ctx(ctx);
>> - if (ret < 0)
>> - goto out_cdp;
>> + if (ret)
>> + goto out;
>>
>> ret = schemata_list_create();
>> if (ret) {
>> schemata_list_destroy();
>> - goto out_mba;
>> + goto out_ctx;
>> }
>>
>> closid_init();
>> @@ -2562,10 +2578,9 @@ static int rdt_get_tree(struct fs_context *fc)
>> kernfs_remove(kn_info);
>> out_schemata_free:
>> schemata_list_destroy();
>> -out_mba:
>> +out_ctx:
>> if (ctx->enable_mba_mbps)
>> set_mba_sc(false);
>> -out_cdp:
>> cdp_disable_all();
>> out:
>> rdt_last_cmd_clear();
>>
>
> The problem statement in the changelog was that rdt_get_tree() is
> doing error unwinding of rdt_enable_ctx(). Looking at the above it
> seems that the problem remains ... callers of rdt_enable_ctx()
> still need to know all internals of that function to do error
> unwind correctly. Could it perhaps be made simpler with a new
> rdt_disable_ctx() that undoes rdt_enable_ctx()? New additions
> to rdt_enable_ctx() would have more clarity where changes are
> needed and callers only need to call a single rdt_disable_ctx().
>
Yes. We can do that.
--
Thanks
Babu Moger
Hi Reinette,
On 8/4/23 15:42, Reinette Chatre wrote:
> Hi Babu,
>
> On 7/19/2023 4:22 PM, Babu Moger wrote:
>> The resctrl default control group is created during kernel init time. If
>> the new files are to be added to the default group during the mount based
>> on the mount option, then each file needs to be created separately and
>> call kernfs_activate.
>>
>> This can avoided if all the files are created during the mount and
>> destroyed during the umount. Move the root and default group creation
>> in rdt_get_tree and removal in rdt_kill_sb.
>
> Please use () to indicate function names.
Sure.
>
>>
>> Suggested-by: Reinette Chatre <[email protected]>
>> Signed-off-by: Babu Moger <[email protected]>
>> ---
>
> ...
>
>> @@ -3704,10 +3720,8 @@ static struct kernfs_syscall_ops rdtgroup_kf_syscall_ops = {
>> .show_options = rdtgroup_show_options,
>> };
>>
>> -static int __init rdtgroup_setup_root(void)
>> +int rdtgroup_setup_root(void)
>> {
>> - int ret;
>> -
>> rdt_root = kernfs_create_root(&rdtgroup_kf_syscall_ops,
>> KERNFS_ROOT_CREATE_DEACTIVATED |
>> KERNFS_ROOT_EXTRA_OPEN_PERM_CHECK,
>> @@ -3715,28 +3729,17 @@ static int __init rdtgroup_setup_root(void)
>> if (IS_ERR(rdt_root))
>> return PTR_ERR(rdt_root);
>>
>> - mutex_lock(&rdtgroup_mutex);
>> -
>> rdtgroup_default.closid = 0;
>> rdtgroup_default.mon.rmid = 0;
>> rdtgroup_default.type = RDTCTRL_GROUP;
>> + rdtgroup_default.mode = RDT_MODE_SHAREABLE;
>> INIT_LIST_HEAD(&rdtgroup_default.mon.crdtgrp_list);
>>
>> list_add(&rdtgroup_default.rdtgroup_list, &rdt_all_groups);
>>
>> - ret = rdtgroup_add_files(kernfs_root_to_node(rdt_root), RFTYPE_CTRL_BASE);
>> - if (ret) {
>> - kernfs_destroy_root(rdt_root);
>> - goto out;
>> - }
>> -
>> rdtgroup_default.kn = kernfs_root_to_node(rdt_root);
>> - kernfs_activate(rdtgroup_default.kn);
>>
>> -out:
>> - mutex_unlock(&rdtgroup_mutex);
>> -
>> - return ret;
>> + return 0;
>> }
>
> I am missing something here. Why is it now needed to re-initialize
> and add default resource group on every mount of resctrl? I expected
> only the kernfs related changes to move.
Yes. We can do that. I think I started with the previous version and went
that route. We dont have to change the default group initialization.
Will send new version soon.
Thanks
Babu Moger
On 8/1/23 20:36, Shaopeng Tan (Fujitsu) wrote:
> Hi Babu,
>
> I reviewed this patch series(v6) and it looks fine.
> I tested these features and ran the selftests/resctrl test set,
> and there is no problem.
>
> <Reviewed-by:[email protected]>
> <Tested-by:[email protected]>
>
Thank you.
Babu Moger
Hi Babu,
On 8/8/2023 9:29 AM, Moger, Babu wrote:
> On 8/4/23 15:42, Reinette Chatre wrote:
>> Hi Babu,
>>
>> On 7/19/2023 4:22 PM, Babu Moger wrote:
>>> @@ -2591,6 +2599,8 @@ static int rdt_get_tree(struct fs_context *fc)
>>> out_schemata_free:
>>> schemata_list_destroy();
>>> out_ctx:
>>> + if (ctx->enable_debug)
>>> + resctrl_debug = false;
>>> if (ctx->enable_mba_mbps)
>>> set_mba_sc(false);
>>> cdp_disable_all();
>>
>> These changes are a red flag to me. Developers still need to
>> do what patch #4 was aiming to prevent.
>
> I think you meant patch 5 (x86/resctrl: Unwind the errors inside
> rdt_enable_ctx).
>
> I can take care of this unwind part in rdt_disable_ctx() and call it here.
> https://lore.kernel.org/lkml/[email protected]/#t
>
> Hope that is what you meant.
It is. Thank you for reading what I meant to write.
Reinette
Hi Reinette,
On 8/4/23 15:42, Reinette Chatre wrote:
> Hi Babu,
>
> On 7/19/2023 4:22 PM, Babu Moger wrote:
>> @@ -2591,6 +2599,8 @@ static int rdt_get_tree(struct fs_context *fc)
>> out_schemata_free:
>> schemata_list_destroy();
>> out_ctx:
>> + if (ctx->enable_debug)
>> + resctrl_debug = false;
>> if (ctx->enable_mba_mbps)
>> set_mba_sc(false);
>> cdp_disable_all();
>
> These changes are a red flag to me. Developers still need to
> do what patch #4 was aiming to prevent.
I think you meant patch 5 (x86/resctrl: Unwind the errors inside
rdt_enable_ctx).
I can take care of this unwind part in rdt_disable_ctx() and call it here.
https://lore.kernel.org/lkml/[email protected]/#t
Hope that is what you meant.
--
Thanks
Babu Moger
Reinette,
On 8/4/23 15:39, Reinette Chatre wrote:
> Hi Babu,
>
> On 7/19/2023 4:21 PM, Babu Moger wrote:
>> + * ------------------------------------------------------------------
>> + * --> RFTYPE_INFO
>> + * --> <info> directory
>> + * --> RFTYPE_TOP_INFO
>> + * Files: last_cmd_status
>> + *
>> + * --> RFTYPE_MON_INFO
>> + * --> <L3_MON> directory
>> + * Files: max_threshold_occupancy, mon_features,
>> + * num_rmids, mbm_total_bytes_config,
>> + * mbm_locat_bytes_config
>
> mbm_locat_bytes_config -> mbm_local_bytes_config
>
>> + *
>> + * --> RFTYPE_CTRL_INFO
>> + * --> RFTYPE_RES_CACHE
>> + * --> <L2,L3> directory
>
> Should this be "directories"?
>
>> + * Files: bit_usage, cbm_mask, min_cbm_bits,
>> + * num_closids, shareable_bits
>
> Based on the hierarchy presented the files mentioned here may be expected
> to be associated with RFTYPE_CTRL_INFO | RFTYPE_RES_CACHE.
> For accuracy it may be better to move num_closids one level higher so
> that it is only associated with RFTYPE_CTRL_INFO?
Missed this earlier. Sure.
>
>> + *
>> + * --> RFTYPE_RES_MB
>> + * --> <MB,SMBA> directory
>
> directories?
>
>> + * Files: bandwidth_gran, delay_linear,
>> + * min_bandwidth, num_closids,
>
> Please remove space before tab for a clean checkpatch.pl run.
>
>> + * thread_throttle_mode
>> + *
>> + * base (root) directory structure
>
> Since "base" refers to more than the root directory I think this can
> just be "base directory structure".
>
>> + * ------------------------------------------------------------------
>> + * --> RFTYPE_BASE (Files common for both MON and CTRL groups)
>> + * Files: cpus, cpus_list, tasks
>> + *
>> + * --> RFTYPE_CTRL_BASE (Files only for CTRL group)
>> + * Files: mode, schemata, size
>> + *
>> */
>> #define RFTYPE_INFO BIT(0)
>> #define RFTYPE_BASE BIT(1)
>>
>>
>
> Reinette
--
Thanks
Babu Moger