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. Added support for the files for MON groups only
6. 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.
---
v11:
Patches 1-2: No changes. Added Reviewed-by and fixed tag order. Fixed
the tag order for all the patches.
Patch 3: Corrected the commit messages about RFTYPE_MON_BASE.
Patch 4: Removed only from the text "Files only for CTRL group" in internal.h.
Patches 5-8: No changes.
Patch 9: Commit message change (Reinette). This patch needs Reviewed-by from
Reinette. Rest all have it already.
Patch 10: Removed only from the text "Files for MON group" in internal.h.(Peter)
v10:
Patches 1-2: no code changes. Added "Reviewed-by" from Fenghua.
Patch 3: Added the new flag RFTYPE_MON_BASE [comment for Reinette].
Removed the Reviewed as the patch has changed.
patch 4: No code change. Added "Reviewed-by" from Fenghua.
Patch 5: Removed empty lines in rdt_disable_ctx().
Patch 6: No changes. Added "Reviewed-by".
Patch 7: No changes.
Patch 8: No changes.
Patch 9: New patch. Added support for the files for MON groups only.
Patch 10: Modified. This patch only adds changes for "mon_hw_id" interface.
v9:
Changes since v8:
Split the RMID display in a separate patch. RMID is a special case here as it
should be printed only if monitoring is enabled.
Made rdtgroup_setup_root and rdtgroup_destroy_root as static functions.
Added code to print pid_str in case of task parse error.
v8:
Changes since v7:
Earlier moved both default group initialization and file creation on mount.
Now kept the group initialization as is and only moved the file creation on mount.
Re-organized the RFTYPE flags comments little more to avoid confusion. Thanks
to Reinette for feedback.
Re-factored the rdt_enable_ctx and added rdt_disable_ctx to unwind the errors.
And same call also used in rdt_kill_sb which simplifies the code.
Few other minor text changes.
v7:
Changes since v6:
While moving the default group file creation on mount, I also moved the
initialization of default group data structures. Reinette suggested to move
only the filesystem creation and keep the group initialization as is. Addressed it now.
Added a new function rdt_disable_ctx to unwind the context related features.
Few other minor text changes.
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.
v10:https://lore.kernel.org/lkml/[email protected]/
v9: https://lore.kernel.org/lkml/[email protected]/
v8: https://lore.kernel.org/lkml/[email protected]/
v7: https://lore.kernel.org/lkml/169178429591.1147205.4030367096506551808.stgit@bmoger-ubuntu/
v6: https://lore.kernel.org/lkml/168980872063.1619861.420806535295905172.stgit@bmoger-ubuntu/
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 (10):
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 group file creation to mount
x86/resctrl: Introduce "-o debug" mount option
x86/resctrl: Display CLOSID for resource group
x86/resctrl: Add support for the files for MON groups only
x86/resctrl: Display RMID of resource group
Documentation/arch/x86/resctrl.rst | 22 ++-
arch/x86/kernel/cpu/resctrl/internal.h | 88 +++++++--
arch/x86/kernel/cpu/resctrl/rdtgroup.c | 253 ++++++++++++++++++-------
3 files changed, 281 insertions(+), 82 deletions(-)
--
2.34.1
rdt_enable_ctx() enables the features provided during resctrl mount.
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.
Introduce rdt_disable_ctx() to refactor the error unwinding of
rdt_enable_ctx() to simplify future additions. This also simplifies
cleanup in rdt_kill_sb().
Remove cdp_disable_all() as it is not used anymore after the refactor.
Suggested-by: Reinette Chatre <[email protected]>
Signed-off-by: Babu Moger <[email protected]>
Tested-by: Peter Newman <[email protected]>
Reviewed-by: Peter Newman <[email protected]>
Tested-by: Tan Shaopeng <[email protected]>
Reviewed-by: Tan Shaopeng <[email protected]>
Reviewed-by: Fenghua Yu <[email protected]>
Reviewed-by: Reinette Chatre <[email protected]>
Reviewed-by: Ilpo Järvinen <[email protected]>
---
arch/x86/kernel/cpu/resctrl/rdtgroup.c | 53 ++++++++++++++++----------
1 file changed, 32 insertions(+), 21 deletions(-)
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index 35945b4bf196..3ea874c80c22 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -2290,14 +2290,6 @@ int resctrl_arch_set_cdp_enabled(enum resctrl_res_level l, bool enable)
return 0;
}
-static void cdp_disable_all(void)
-{
- if (resctrl_arch_get_cdp_enabled(RDT_RESOURCE_L3))
- resctrl_arch_set_cdp_enabled(RDT_RESOURCE_L3, false);
- if (resctrl_arch_get_cdp_enabled(RDT_RESOURCE_L2))
- resctrl_arch_set_cdp_enabled(RDT_RESOURCE_L2, false);
-}
-
/*
* We don't allow rdtgroup directories to be created anywhere
* except the root directory. Thus when looking for the rdtgroup
@@ -2377,19 +2369,42 @@ static int mkdir_mondata_all(struct kernfs_node *parent_kn,
struct rdtgroup *prgrp,
struct kernfs_node **mon_data_kn);
+static void rdt_disable_ctx(void)
+{
+ resctrl_arch_set_cdp_enabled(RDT_RESOURCE_L3, false);
+ resctrl_arch_set_cdp_enabled(RDT_RESOURCE_L2, false);
+ set_mba_sc(false);
+}
+
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_done;
+ }
- 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_done:
return ret;
}
@@ -2497,13 +2512,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,11 +2577,8 @@ static int rdt_get_tree(struct fs_context *fc)
kernfs_remove(kn_info);
out_schemata_free:
schemata_list_destroy();
-out_mba:
- if (ctx->enable_mba_mbps)
- set_mba_sc(false);
-out_cdp:
- cdp_disable_all();
+out_ctx:
+ rdt_disable_ctx();
out:
rdt_last_cmd_clear();
mutex_unlock(&rdtgroup_mutex);
@@ -2798,12 +2810,11 @@ static void rdt_kill_sb(struct super_block *sb)
cpus_read_lock();
mutex_lock(&rdtgroup_mutex);
- set_mba_sc(false);
+ rdt_disable_ctx();
/*Put everything back to default values. */
for_each_alloc_capable_rdt_resource(r)
reset_all_ctrls(r);
- cdp_disable_all();
rmdir_all_sub();
rdt_pseudo_lock_release();
rdtgroup_default.mode = RDT_MODE_SHAREABLE;
--
2.34.1
Add "-o debug" option to mount resctrl filesystem in debug mode.
When in debug mode resctrl displays files that have the new
RFTYPE_DEBUG flag to help resctrl debugging.
Signed-off-by: Babu Moger <[email protected]>
Tested-by: Peter Newman <[email protected]>
Reviewed-by: Peter Newman <[email protected]>
Tested-by: Tan Shaopeng <[email protected]>
Reviewed-by: Tan Shaopeng <[email protected]>
Reviewed-by: Fenghua Yu <[email protected]>
Reviewed-by: Reinette Chatre <[email protected]>
Reviewed-by: Ilpo Järvinen <[email protected]>
---
Documentation/arch/x86/resctrl.rst | 5 ++++-
arch/x86/kernel/cpu/resctrl/internal.h | 2 ++
arch/x86/kernel/cpu/resctrl/rdtgroup.c | 18 ++++++++++++++++++
3 files changed, 24 insertions(+), 1 deletion(-)
diff --git a/Documentation/arch/x86/resctrl.rst b/Documentation/arch/x86/resctrl.rst
index 8154e9975d1e..28d35aaa74b4 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 b47a5906f952..d68f947c5a64 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)
@@ -306,6 +307,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 a34657f0bd0c..150105c21fee 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -59,6 +59,8 @@ static void rdtgroup_destroy_root(void);
struct dentry *debugfs_resctrl;
+static bool resctrl_debug;
+
void rdt_last_cmd_clear(void)
{
lockdep_assert_held(&rdtgroup_mutex);
@@ -1874,6 +1876,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);
@@ -2377,6 +2382,8 @@ static void rdt_disable_ctx(void)
resctrl_arch_set_cdp_enabled(RDT_RESOURCE_L3, false);
resctrl_arch_set_cdp_enabled(RDT_RESOURCE_L2, false);
set_mba_sc(false);
+
+ resctrl_debug = false;
}
static int rdt_enable_ctx(struct rdt_fs_context *ctx)
@@ -2401,6 +2408,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:
@@ -2605,6 +2615,7 @@ enum rdt_param {
Opt_cdp,
Opt_cdpl2,
Opt_mba_mbps,
+ Opt_debug,
nr__rdt_params
};
@@ -2612,6 +2623,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),
{}
};
@@ -2637,6 +2649,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;
@@ -3705,6 +3720,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;
}
--
2.34.1
The resctrl task assignment for monitor or control group needs to be
done one at a time. For example:
$mount -t resctrl resctrl /sys/fs/resctrl/
$mkdir /sys/fs/resctrl/ctrl_grp1
$echo 123 > /sys/fs/resctrl/ctrl_grp1/tasks
$echo 456 > /sys/fs/resctrl/ctrl_grp1/tasks
$echo 789 > /sys/fs/resctrl/ctrl_grp1/tasks
This is not user-friendly when dealing with hundreds of tasks.
Support multiple task assignment in one command with tasks ids separated
by commas. For example:
$echo 123,456,789 > /sys/fs/resctrl/ctrl_grp1/tasks
Signed-off-by: Babu Moger <[email protected]>
Tested-by: Peter Newman <[email protected]>
Reviewed-by: Peter Newman <[email protected]>
Tested-by: Tan Shaopeng <[email protected]>
Reviewed-by: Tan Shaopeng <[email protected]>
Reviewed-by: Fenghua Yu <[email protected]>
Reviewed-by: Reinette Chatre <[email protected]>
Reviewed-by: Ilpo Järvinen <[email protected]>
---
Documentation/arch/x86/resctrl.rst | 9 ++++++++-
arch/x86/kernel/cpu/resctrl/rdtgroup.c | 25 ++++++++++++++++++++++---
2 files changed, 30 insertions(+), 4 deletions(-)
diff --git a/Documentation/arch/x86/resctrl.rst b/Documentation/arch/x86/resctrl.rst
index cb05d90111b4..8154e9975d1e 100644
--- a/Documentation/arch/x86/resctrl.rst
+++ b/Documentation/arch/x86/resctrl.rst
@@ -299,7 +299,14 @@ All groups contain the following files:
"tasks":
Reading this file shows the list of all tasks that belong to
this group. Writing a task id to the file will add a task to the
- group. If the group is a CTRL_MON group the task is removed from
+ group. Multiple tasks can be added by separating the task ids
+ with commas. Tasks will be assigned sequentially. Multiple
+ failures are not supported. A single failure encountered while
+ attempting to assign a task will cause the operation to abort and
+ already added tasks before the failure will remain in the group.
+ Failures will be logged to /sys/fs/resctrl/info/last_cmd_status.
+
+ If the group is a CTRL_MON group the task is removed from
whichever previous CTRL_MON group owned the task and also from
any MON group that owned the task. If the group is a MON group,
then the task must already belong to the CTRL_MON parent of this
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index 725344048f85..f0d163950969 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -696,11 +696,10 @@ static ssize_t rdtgroup_tasks_write(struct kernfs_open_file *of,
char *buf, size_t nbytes, loff_t off)
{
struct rdtgroup *rdtgrp;
+ char *pid_str;
int ret = 0;
pid_t pid;
- if (kstrtoint(strstrip(buf), 0, &pid) || pid < 0)
- return -EINVAL;
rdtgrp = rdtgroup_kn_lock_live(of->kn);
if (!rdtgrp) {
rdtgroup_kn_unlock(of->kn);
@@ -715,7 +714,27 @@ static ssize_t rdtgroup_tasks_write(struct kernfs_open_file *of,
goto unlock;
}
- ret = rdtgroup_move_task(pid, rdtgrp, of);
+ while (buf && buf[0] != '\0' && buf[0] != '\n') {
+ pid_str = strim(strsep(&buf, ","));
+
+ if (kstrtoint(pid_str, 0, &pid)) {
+ rdt_last_cmd_printf("Task list parsing error pid %s\n", pid_str);
+ ret = -EINVAL;
+ break;
+ }
+
+ if (pid < 0) {
+ rdt_last_cmd_printf("Invalid pid %d\n", pid);
+ ret = -EINVAL;
+ break;
+ }
+
+ ret = rdtgroup_move_task(pid, rdtgrp, of);
+ if (ret) {
+ rdt_last_cmd_printf("Error while processing task %d\n", pid);
+ break;
+ }
+ }
unlock:
rdtgroup_kn_unlock(of->kn);
--
2.34.1
Files unique to monitoring groups have the RFTYPE_MON flag. When a new
monitoring group is created the resctrl files with flags RFTYPE_BASE
(files common to all resource groups) and RFTYPE_MON (files unique to
monitoring groups) are created to support interacting with the new
monitoring group.
A resource group can support both monitoring and control, also termed
a CTRL_MON resource group. CTRL_MON groups should get both monitoring
and control resctrl files but that is not the case. Only the
RFTYPE_BASE and RFTYPE_CTRL files are created for CTRL_MON groups.
This is not a problem because there are no monitoring specific files
with the RFTYPE_MON flag associated with resource groups.
A later patch introduces the first monitoring specific (RFTYPE_MON)
file for resource groups. Ensure that files with the RFTYPE_MON
flag are created for CTRL_MON groups.
Signed-off-by: Babu Moger <[email protected]>
Tested-by: Peter Newman <[email protected]>
Reviewed-by: Peter Newman <[email protected]>
Tested-by: Tan Shaopeng <[email protected]>
Reviewed-by: Tan Shaopeng <[email protected]>
Reviewed-by: Fenghua Yu <[email protected]>
Reviewed-by: Ilpo Järvinen <[email protected]>
---
arch/x86/kernel/cpu/resctrl/rdtgroup.c | 15 +++++++++++----
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index 953b082cec8a..55d1b90f460e 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -2533,6 +2533,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;
@@ -2563,7 +2564,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;
@@ -3253,8 +3257,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);
@@ -3306,10 +3310,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) {
--
2.34.1
In x86, hardware uses RMID to identify a monitoring group. When a 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.
For example:
$cat /sys/fs/resctrl/mon_groups/mon_grp1/mon_hw_id
3
Signed-off-by: Babu Moger <[email protected]>
Tested-by: Peter Newman <[email protected]>
Reviewed-by: Peter Newman <[email protected]>
Tested-by: Tan Shaopeng <[email protected]>
Reviewed-by: Tan Shaopeng <[email protected]>
Reviewed-by: Fenghua Yu <[email protected]>
Reviewed-by: Reinette Chatre <[email protected]>
Reviewed-by: Ilpo Järvinen <[email protected]>
---
Documentation/arch/x86/resctrl.rst | 4 ++++
arch/x86/kernel/cpu/resctrl/internal.h | 5 +++++
arch/x86/kernel/cpu/resctrl/rdtgroup.c | 23 +++++++++++++++++++++++
3 files changed, 32 insertions(+)
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 eda5da9ab81f..a25407fde6fc 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 for MON group)
+ *
+ * --> RFTYPE_DEBUG (Files to help resctrl debugging)
+ * File: mon_hw_id
+ *
* --> RFTYPE_CTRL (Files for CTRL group)
* Files: mode, schemata, size
*
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index 55d1b90f460e..ef4b18091e5d 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,
+ .fflags = RFTYPE_MON_BASE | RFTYPE_DEBUG,
+ },
{
.name = "schemata",
.mode = 0644,
--
2.34.1
Hi Babu,
On 10/3/2023 4:54 PM, Babu Moger wrote:
> Files unique to monitoring groups have the RFTYPE_MON flag. When a new
> monitoring group is created the resctrl files with flags RFTYPE_BASE
> (files common to all resource groups) and RFTYPE_MON (files unique to
> monitoring groups) are created to support interacting with the new
> monitoring group.
>
> A resource group can support both monitoring and control, also termed
> a CTRL_MON resource group. CTRL_MON groups should get both monitoring
> and control resctrl files but that is not the case. Only the
> RFTYPE_BASE and RFTYPE_CTRL files are created for CTRL_MON groups.
> This is not a problem because there are no monitoring specific files
> with the RFTYPE_MON flag associated with resource groups.
>
> A later patch introduces the first monitoring specific (RFTYPE_MON)
> file for resource groups. Ensure that files with the RFTYPE_MON
> flag are created for CTRL_MON groups.
>
> Signed-off-by: Babu Moger <[email protected]>
> Tested-by: Peter Newman <[email protected]>
> Reviewed-by: Peter Newman <[email protected]>
> Tested-by: Tan Shaopeng <[email protected]>
> Reviewed-by: Tan Shaopeng <[email protected]>
> Reviewed-by: Fenghua Yu <[email protected]>
> Reviewed-by: Ilpo Järvinen <[email protected]>
> ---
Thank you.
Reviewed-by: Reinette Chatre <[email protected]>
I believe this series is ready for inclusion. There is a conflict between
this series and Maciej's non-contiguous work [1] that is also ready for
inclusion. We could wait for outcome of next level review to determine
who will need to rebase. It may help to provide a snippet of the conflict
resolution in anticipation of Maciej's series being merged first (I will
propose the same to Maciej for the scenario of this work merged first).
Reinette
[1] https://lore.kernel.org/lkml/[email protected]/
Hi Reinette,
On 10/6/2023 12:53 PM, Reinette Chatre wrote:
> Hi Babu,
>
> On 10/3/2023 4:54 PM, Babu Moger wrote:
>> Files unique to monitoring groups have the RFTYPE_MON flag. When a new
>> monitoring group is created the resctrl files with flags RFTYPE_BASE
>> (files common to all resource groups) and RFTYPE_MON (files unique to
>> monitoring groups) are created to support interacting with the new
>> monitoring group.
>>
>> A resource group can support both monitoring and control, also termed
>> a CTRL_MON resource group. CTRL_MON groups should get both monitoring
>> and control resctrl files but that is not the case. Only the
>> RFTYPE_BASE and RFTYPE_CTRL files are created for CTRL_MON groups.
>> This is not a problem because there are no monitoring specific files
>> with the RFTYPE_MON flag associated with resource groups.
>>
>> A later patch introduces the first monitoring specific (RFTYPE_MON)
>> file for resource groups. Ensure that files with the RFTYPE_MON
>> flag are created for CTRL_MON groups.
>>
>> Signed-off-by: Babu Moger <[email protected]>
>> Tested-by: Peter Newman <[email protected]>
>> Reviewed-by: Peter Newman <[email protected]>
>> Tested-by: Tan Shaopeng <[email protected]>
>> Reviewed-by: Tan Shaopeng <[email protected]>
>> Reviewed-by: Fenghua Yu <[email protected]>
>> Reviewed-by: Ilpo Järvinen <[email protected]>
>> ---
> Thank you.
>
> Reviewed-by: Reinette Chatre <[email protected]>
>
> I believe this series is ready for inclusion. There is a conflict between
> this series and Maciej's non-contiguous work [1] that is also ready for
> inclusion. We could wait for outcome of next level review to determine
> who will need to rebase. It may help to provide a snippet of the conflict
> resolution in anticipation of Maciej's series being merged first (I will
> propose the same to Maciej for the scenario of this work merged first).
I had a minor comment on Maciej's patch.
https://lore.kernel.org/lkml/[email protected]/
I will respond to his patch 3 with the conflict resolution.
Thanks
Babu
>
> Reinette
>
> [1] https://lore.kernel.org/lkml/[email protected]/
Hi Babu,
On 10/6/2023 1:49 PM, Moger, Babu wrote:
> Hi Reinette,
>
> On 10/6/2023 12:53 PM, Reinette Chatre wrote:
>> Hi Babu,
>>
>> On 10/3/2023 4:54 PM, Babu Moger wrote:
>>> Files unique to monitoring groups have the RFTYPE_MON flag. When a new
>>> monitoring group is created the resctrl files with flags RFTYPE_BASE
>>> (files common to all resource groups) and RFTYPE_MON (files unique to
>>> monitoring groups) are created to support interacting with the new
>>> monitoring group.
>>>
>>> A resource group can support both monitoring and control, also termed
>>> a CTRL_MON resource group. CTRL_MON groups should get both monitoring
>>> and control resctrl files but that is not the case. Only the
>>> RFTYPE_BASE and RFTYPE_CTRL files are created for CTRL_MON groups.
>>> This is not a problem because there are no monitoring specific files
>>> with the RFTYPE_MON flag associated with resource groups.
>>>
>>> A later patch introduces the first monitoring specific (RFTYPE_MON)
>>> file for resource groups. Ensure that files with the RFTYPE_MON
>>> flag are created for CTRL_MON groups.
>>>
>>> Signed-off-by: Babu Moger <[email protected]>
>>> Tested-by: Peter Newman <[email protected]>
>>> Reviewed-by: Peter Newman <[email protected]>
>>> Tested-by: Tan Shaopeng <[email protected]>
>>> Reviewed-by: Tan Shaopeng <[email protected]>
>>> Reviewed-by: Fenghua Yu <[email protected]>
>>> Reviewed-by: Ilpo Järvinen <[email protected]>
>>> ---
>> Thank you.
>>
>> Reviewed-by: Reinette Chatre <[email protected]>
>>
>> I believe this series is ready for inclusion. There is a conflict between
>> this series and Maciej's non-contiguous work [1] that is also ready for
>> inclusion. We could wait for outcome of next level review to determine
>> who will need to rebase. It may help to provide a snippet of the conflict
>> resolution in anticipation of Maciej's series being merged first (I will
>> propose the same to Maciej for the scenario of this work merged first).
>
> I had a minor comment on Maciej's patch.
>
> https://lore.kernel.org/lkml/[email protected]/
ok, thank you for reviewing that work.
> I will respond to his patch 3 with the conflict resolution.
>
Thank you. We'll wait for next level of review to learn how best
to approach this. We do not know which series will be considered/merged
first.
Reinette
On Tue, Oct 03, 2023 at 06:54:25PM -0500, Babu Moger wrote:
> rdt_enable_ctx() enables the features provided during resctrl mount.
>
> 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.
>
> Introduce rdt_disable_ctx() to refactor the error unwinding of
> rdt_enable_ctx() to simplify future additions. This also simplifies
> cleanup in rdt_kill_sb().
>
> Remove cdp_disable_all() as it is not used anymore after the refactor.
Do not talk about *what* the patch is doing in the commit message - that
should be obvious from the diff itself. Rather, concentrate on the *why*
it needs to be done.
Check your whole series.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
Hi, Babu,
On 10/9/23 10:25, Borislav Petkov wrote:
> On Tue, Oct 03, 2023 at 06:54:25PM -0500, Babu Moger wrote:
>> rdt_enable_ctx() enables the features provided during resctrl mount.
>>
>> 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.
>>
>> Introduce rdt_disable_ctx() to refactor the error unwinding of
>> rdt_enable_ctx() to simplify future additions. This also simplifies
>> cleanup in rdt_kill_sb().
>>
>> Remove cdp_disable_all() as it is not used anymore after the refactor.
>
> Do not talk about *what* the patch is doing in the commit message - that
> should be obvious from the diff itself. Rather, concentrate on the *why*
> it needs to be done.
>
> Check your whole series.
When you send the next patch set, please add the change log in each
patch after the "---" instead of in the cover patch only. So Boris and
others clearly know what are changed in each patch.
Thanks.
-Fenghua
Hi Boris,
On 10/9/2023 10:25 AM, Borislav Petkov wrote:
> On Tue, Oct 03, 2023 at 06:54:25PM -0500, Babu Moger wrote:
>> rdt_enable_ctx() enables the features provided during resctrl mount.
>>
>> 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.
>>
>> Introduce rdt_disable_ctx() to refactor the error unwinding of
>> rdt_enable_ctx() to simplify future additions. This also simplifies
>> cleanup in rdt_kill_sb().
>>
>> Remove cdp_disable_all() as it is not used anymore after the refactor.
>
> Do not talk about *what* the patch is doing in the commit message - that
> should be obvious from the diff itself. Rather, concentrate on the *why*
> it needs to be done.
I worked with Babu on this commit message and would appreciate guidance to
get this (and others) right. The second paragraph intends to explain the
current problem with rdt_enable_ctx() with the third paragraph providing an
overview of how the problem will be fixed and why (future code needs to touch
this area).
Is it the fourth paragraph (mentioning cdp_disable_all()) that is annoying? I
can see that it is redundant. Would it be more palatable if the fourth paragraph
is just dropped?
>
> Check your whole series.
>
Reinette
On Tue, Oct 03, 2023 at 06:54:29PM -0500, Babu Moger wrote:
> A later patch introduces the first monitoring specific (RFTYPE_MON)
> file for resource groups. Ensure that files with the RFTYPE_MON
> flag are created for CTRL_MON groups.
You don't need that paragraph here. The "later patch" can talk about it.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Mon, Oct 09, 2023 at 10:59:27AM -0700, Reinette Chatre wrote:
> Is it the fourth paragraph (mentioning cdp_disable_all()) that is annoying? I
> can see that it is redundant. Would it be more palatable if the fourth paragraph
> is just dropped?
Yes, basically you don't want to explain what a patch does as that
should be obvious from the diff. Rather, it should talk about why
a change is being done. Sure, sometimes, you need to talk about the
change in case you want to highlight certain aspects of why the code is
being changed in the first place but explaining in text what is already
visible in the diff is not very useful.
I always give the example about git archeology here: put enough info in
the commit message so that any future reader of it can understand why
the change was done. The "what" of a patch doesn't belong to that text.
I hope that makes more sense.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
Hi Fenghua,
On 10/9/23 12:40, Fenghua Yu wrote:
> Hi, Babu,
>
> On 10/9/23 10:25, Borislav Petkov wrote:
>> On Tue, Oct 03, 2023 at 06:54:25PM -0500, Babu Moger wrote:
>>> rdt_enable_ctx() enables the features provided during resctrl mount.
>>>
>>> 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.
>>>
>>> Introduce rdt_disable_ctx() to refactor the error unwinding of
>>> rdt_enable_ctx() to simplify future additions. This also simplifies
>>> cleanup in rdt_kill_sb().
>>>
>>> Remove cdp_disable_all() as it is not used anymore after the refactor.
>>
>> Do not talk about *what* the patch is doing in the commit message - that
>> should be obvious from the diff itself. Rather, concentrate on the *why*
>> it needs to be done.
>>
>> Check your whole series.
>
> When you send the next patch set, please add the change log in each patch
> after the "---" instead of in the cover patch only. So Boris and others
> clearly know what are changed in each patch.
Sure. Will do in future.
--
Thanks
Babu Moger
On 10/9/23 14:23, Borislav Petkov wrote:
> On Mon, Oct 09, 2023 at 10:59:27AM -0700, Reinette Chatre wrote:
>> Is it the fourth paragraph (mentioning cdp_disable_all()) that is annoying? I
>> can see that it is redundant. Would it be more palatable if the fourth paragraph
>> is just dropped?
>
> Yes, basically you don't want to explain what a patch does as that
> should be obvious from the diff. Rather, it should talk about why
> a change is being done. Sure, sometimes, you need to talk about the
> change in case you want to highlight certain aspects of why the code is
> being changed in the first place but explaining in text what is already
> visible in the diff is not very useful.
>
> I always give the example about git archeology here: put enough info in
> the commit message so that any future reader of it can understand why
> the change was done. The "what" of a patch doesn't belong to that text.
>
> I hope that makes more sense.
>
Sure. Will drop the last paragraph in next revision.
--
Thanks
Babu Moger
Hi Boris,
On 10/9/2023 12:23 PM, Borislav Petkov wrote:
> On Mon, Oct 09, 2023 at 10:59:27AM -0700, Reinette Chatre wrote:
>> Is it the fourth paragraph (mentioning cdp_disable_all()) that is annoying? I
>> can see that it is redundant. Would it be more palatable if the fourth paragraph
>> is just dropped?
>
> Yes, basically you don't want to explain what a patch does as that
> should be obvious from the diff. Rather, it should talk about why
> a change is being done. Sure, sometimes, you need to talk about the
> change in case you want to highlight certain aspects of why the code is
> being changed in the first place but explaining in text what is already
> visible in the diff is not very useful.
>
> I always give the example about git archeology here: put enough info in
> the commit message so that any future reader of it can understand why
> the change was done. The "what" of a patch doesn't belong to that text.
>
> I hope that makes more sense.
>
This is clear. Thank you very much. (I am still working on getting it right
in practice though.)
Reinette
Hi Boris,
On 10/9/2023 1:00 PM, Borislav Petkov wrote:
> On Tue, Oct 03, 2023 at 06:54:29PM -0500, Babu Moger wrote:
>> A later patch introduces the first monitoring specific (RFTYPE_MON)
>> file for resource groups. Ensure that files with the RFTYPE_MON
>> flag are created for CTRL_MON groups.
> You don't need that paragraph here. The "later patch" can talk about it.
Sure. Will do.
Thanks
Babu