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. While doing these above changes, found that rftype flags needed some cleanup.
They were named inconsistently. Re-arranged them much more cleanly now.
Hope it can help future additions.
---
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. But I thing it is big problem.
Please comment back if we can do better.
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.
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 (7):
x86/resctrl: Add multiple tasks to the resctrl group at once
x86/resctrl: Remove unnecessary rftype flags
x86/resctrl: Rename rftype flags for consistency
x86/resctrl: Re-arrange RFTYPE flags and add more comments
x86/resctrl: Introduce "-o debug" mount option
x86/resctrl: Display CLOSID and RMID for the resctrl groups
x86/resctrl: Add debug files when mounted with debug option
Documentation/x86/resctrl.rst | 29 +++-
arch/x86/kernel/cpu/resctrl/internal.h | 62 ++++++--
arch/x86/kernel/cpu/resctrl/rdtgroup.c | 188 ++++++++++++++++++++++---
3 files changed, 243 insertions(+), 36 deletions(-)
--
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/clos1
$echo 123 > /sys/fs/resctrl/clos1/tasks
$echo 456 > /sys/fs/resctrl/clos1/tasks
$echo 789 > /sys/fs/resctrl/clos1/tasks
This is not user-friendly when dealing with hundreds of tasks.
It can be improved by supporting the multiple task id assignment in
one command with the tasks separated by commas. For example:
$echo 123,456,789 > /sys/fs/resctrl/clos1/tasks
Signed-off-by: Babu Moger <[email protected]>
---
Documentation/x86/resctrl.rst | 9 ++++++++-
arch/x86/kernel/cpu/resctrl/rdtgroup.c | 31 ++++++++++++++++++++++++++++++-
2 files changed, 38 insertions(+), 2 deletions(-)
diff --git a/Documentation/x86/resctrl.rst b/Documentation/x86/resctrl.rst
index 387ccbcb558f..f28ed1443a6a 100644
--- a/Documentation/x86/resctrl.rst
+++ b/Documentation/x86/resctrl.rst
@@ -292,7 +292,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 in the order it
+ is entered. Failures while assigning the tasks will be aborted
+ immediately and tasks next in the sequence will not be assigned.
+ Users may need to retry them again. Failure details possibly with
+ pid will be logged in /sys/fs/resctrl/info/last_cmd_status file.
+
+ 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 6ad33f355861..df5bd13440b0 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -696,18 +696,41 @@ 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)
+ if (nbytes == 0)
return -EINVAL;
+
+ buf[nbytes - 1] = '\0';
+
rdtgrp = rdtgroup_kn_lock_live(of->kn);
if (!rdtgrp) {
rdtgroup_kn_unlock(of->kn);
return -ENOENT;
}
+
+next:
+ if (!buf || buf[0] == '\0')
+ goto unlock;
+
rdt_last_cmd_clear();
+ pid_str = strim(strsep(&buf, ","));
+
+ if (kstrtoint(pid_str, 0, &pid)) {
+ rdt_last_cmd_printf("Task list parsing error\n");
+ ret = -EINVAL;
+ goto unlock;
+ }
+
+ if (pid < 0) {
+ rdt_last_cmd_printf("Invalid pid %d value\n", pid);
+ ret = -EINVAL;
+ goto unlock;
+ }
+
if (rdtgrp->mode == RDT_MODE_PSEUDO_LOCKED ||
rdtgrp->mode == RDT_MODE_PSEUDO_LOCKSETUP) {
ret = -EINVAL;
@@ -716,6 +739,12 @@ static ssize_t rdtgroup_tasks_write(struct kernfs_open_file *of,
}
ret = rdtgroup_move_task(pid, rdtgrp, of);
+ if (ret) {
+ rdt_last_cmd_printf("Error while processing task %d\n", pid);
+ goto unlock;
+ } else {
+ goto next;
+ }
unlock:
rdtgroup_kn_unlock(of->kn);
Remove unnecessary rftype flags to avoid multiple indirection.
Signed-off-by: Babu Moger <[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 df5bd13440b0..6cd0a8396f30 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -3239,7 +3239,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");
The rftype flags have two different prefixes even though they are used
for the same purpose. Change the prefix to RFTYPE_ for all the flags.
Signed-off-by: Babu Moger <[email protected]>
---
arch/x86/kernel/cpu/resctrl/internal.h | 8 +++---
arch/x86/kernel/cpu/resctrl/rdtgroup.c | 42 ++++++++++++++++----------------
2 files changed, 25 insertions(+), 25 deletions(-)
diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
index 62767774810d..c4fc5a1c630c 100644
--- a/arch/x86/kernel/cpu/resctrl/internal.h
+++ b/arch/x86/kernel/cpu/resctrl/internal.h
@@ -248,10 +248,10 @@ struct rdtgroup {
#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)
-#define RF_MON_INFO (RFTYPE_INFO | RFTYPE_MON)
-#define RF_TOP_INFO (RFTYPE_INFO | RFTYPE_TOP)
-#define RF_CTRL_BASE (RFTYPE_BASE | RFTYPE_CTRL)
+#define RFTYPE_CTRL_INFO (RFTYPE_INFO | RFTYPE_CTRL)
+#define RFTYPE_MON_INFO (RFTYPE_INFO | RFTYPE_MON)
+#define RFTYPE_TOP_INFO (RFTYPE_INFO | RFTYPE_TOP)
+#define RFTYPE_CTRL_BASE (RFTYPE_BASE | RFTYPE_CTRL)
/* 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 6cd0a8396f30..719e29248892 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -1711,77 +1711,77 @@ static struct rftype res_common_files[] = {
.mode = 0444,
.kf_ops = &rdtgroup_kf_single_ops,
.seq_show = rdt_last_cmd_status_show,
- .fflags = RF_TOP_INFO,
+ .fflags = RFTYPE_TOP_INFO,
},
{
.name = "num_closids",
.mode = 0444,
.kf_ops = &rdtgroup_kf_single_ops,
.seq_show = rdt_num_closids_show,
- .fflags = RF_CTRL_INFO,
+ .fflags = RFTYPE_CTRL_INFO,
},
{
.name = "mon_features",
.mode = 0444,
.kf_ops = &rdtgroup_kf_single_ops,
.seq_show = rdt_mon_features_show,
- .fflags = RF_MON_INFO,
+ .fflags = RFTYPE_MON_INFO,
},
{
.name = "num_rmids",
.mode = 0444,
.kf_ops = &rdtgroup_kf_single_ops,
.seq_show = rdt_num_rmids_show,
- .fflags = RF_MON_INFO,
+ .fflags = RFTYPE_MON_INFO,
},
{
.name = "cbm_mask",
.mode = 0444,
.kf_ops = &rdtgroup_kf_single_ops,
.seq_show = rdt_default_ctrl_show,
- .fflags = RF_CTRL_INFO | RFTYPE_RES_CACHE,
+ .fflags = RFTYPE_CTRL_INFO | RFTYPE_RES_CACHE,
},
{
.name = "min_cbm_bits",
.mode = 0444,
.kf_ops = &rdtgroup_kf_single_ops,
.seq_show = rdt_min_cbm_bits_show,
- .fflags = RF_CTRL_INFO | RFTYPE_RES_CACHE,
+ .fflags = RFTYPE_CTRL_INFO | RFTYPE_RES_CACHE,
},
{
.name = "shareable_bits",
.mode = 0444,
.kf_ops = &rdtgroup_kf_single_ops,
.seq_show = rdt_shareable_bits_show,
- .fflags = RF_CTRL_INFO | RFTYPE_RES_CACHE,
+ .fflags = RFTYPE_CTRL_INFO | RFTYPE_RES_CACHE,
},
{
.name = "bit_usage",
.mode = 0444,
.kf_ops = &rdtgroup_kf_single_ops,
.seq_show = rdt_bit_usage_show,
- .fflags = RF_CTRL_INFO | RFTYPE_RES_CACHE,
+ .fflags = RFTYPE_CTRL_INFO | RFTYPE_RES_CACHE,
},
{
.name = "min_bandwidth",
.mode = 0444,
.kf_ops = &rdtgroup_kf_single_ops,
.seq_show = rdt_min_bw_show,
- .fflags = RF_CTRL_INFO | RFTYPE_RES_MB,
+ .fflags = RFTYPE_CTRL_INFO | RFTYPE_RES_MB,
},
{
.name = "bandwidth_gran",
.mode = 0444,
.kf_ops = &rdtgroup_kf_single_ops,
.seq_show = rdt_bw_gran_show,
- .fflags = RF_CTRL_INFO | RFTYPE_RES_MB,
+ .fflags = RFTYPE_CTRL_INFO | RFTYPE_RES_MB,
},
{
.name = "delay_linear",
.mode = 0444,
.kf_ops = &rdtgroup_kf_single_ops,
.seq_show = rdt_delay_linear_show,
- .fflags = RF_CTRL_INFO | RFTYPE_RES_MB,
+ .fflags = RFTYPE_CTRL_INFO | RFTYPE_RES_MB,
},
/*
* Platform specific which (if any) capabilities are provided by
@@ -1800,7 +1800,7 @@ static struct rftype res_common_files[] = {
.kf_ops = &rdtgroup_kf_single_ops,
.write = max_threshold_occ_write,
.seq_show = max_threshold_occ_show,
- .fflags = RF_MON_INFO | RFTYPE_RES_CACHE,
+ .fflags = RFTYPE_MON_INFO | RFTYPE_RES_CACHE,
},
{
.name = "mbm_total_bytes_config",
@@ -1847,7 +1847,7 @@ static struct rftype res_common_files[] = {
.kf_ops = &rdtgroup_kf_single_ops,
.write = rdtgroup_schemata_write,
.seq_show = rdtgroup_schemata_show,
- .fflags = RF_CTRL_BASE,
+ .fflags = RFTYPE_CTRL_BASE,
},
{
.name = "mode",
@@ -1855,14 +1855,14 @@ static struct rftype res_common_files[] = {
.kf_ops = &rdtgroup_kf_single_ops,
.write = rdtgroup_mode_write,
.seq_show = rdtgroup_mode_show,
- .fflags = RF_CTRL_BASE,
+ .fflags = RFTYPE_CTRL_BASE,
},
{
.name = "size",
.mode = 0444,
.kf_ops = &rdtgroup_kf_single_ops,
.seq_show = rdtgroup_size_show,
- .fflags = RF_CTRL_BASE,
+ .fflags = RFTYPE_CTRL_BASE,
},
};
@@ -1919,7 +1919,7 @@ void __init thread_throttle_mode_init(void)
if (!rft)
return;
- rft->fflags = RF_CTRL_INFO | RFTYPE_RES_MB;
+ rft->fflags = RFTYPE_CTRL_INFO | RFTYPE_RES_MB;
}
void __init mbm_config_rftype_init(const char *config)
@@ -1928,7 +1928,7 @@ void __init mbm_config_rftype_init(const char *config)
rft = rdtgroup_get_rftype_by_name(config);
if (rft)
- rft->fflags = RF_MON_INFO | RFTYPE_RES_CACHE;
+ rft->fflags = RFTYPE_MON_INFO | RFTYPE_RES_CACHE;
}
/**
@@ -2063,21 +2063,21 @@ static int rdtgroup_create_info_dir(struct kernfs_node *parent_kn)
if (IS_ERR(kn_info))
return PTR_ERR(kn_info);
- ret = rdtgroup_add_files(kn_info, RF_TOP_INFO);
+ ret = rdtgroup_add_files(kn_info, RFTYPE_TOP_INFO);
if (ret)
goto out_destroy;
/* loop over enabled controls, these are all alloc_capable */
list_for_each_entry(s, &resctrl_schema_all, list) {
r = s->res;
- fflags = r->fflags | RF_CTRL_INFO;
+ fflags = r->fflags | RFTYPE_CTRL_INFO;
ret = rdtgroup_mkdir_info_resdir(s, s->name, fflags);
if (ret)
goto out_destroy;
}
for_each_mon_capable_rdt_resource(r) {
- fflags = r->fflags | RF_MON_INFO;
+ fflags = r->fflags | RFTYPE_MON_INFO;
sprintf(name, "%s_MON", r->name);
ret = rdtgroup_mkdir_info_resdir(r, name, fflags);
if (ret)
@@ -3578,7 +3578,7 @@ static int __init rdtgroup_setup_root(void)
list_add(&rdtgroup_default.rdtgroup_list, &rdt_all_groups);
- ret = rdtgroup_add_files(kernfs_root_to_node(rdt_root), RF_CTRL_BASE);
+ ret = rdtgroup_add_files(kernfs_root_to_node(rdt_root), RFTYPE_CTRL_BASE);
if (ret) {
kernfs_destroy_root(rdt_root);
goto out;
Remove gaps in bit definitions of RFTYPE flags and add more comments
to make it easy for future additions.
Signed-off-by: Babu Moger <[email protected]>
---
arch/x86/kernel/cpu/resctrl/internal.h | 49 +++++++++++++++++++++++++++++---
1 file changed, 44 insertions(+), 5 deletions(-)
diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
index c4fc5a1c630c..75ddbd833fdf 100644
--- a/arch/x86/kernel/cpu/resctrl/internal.h
+++ b/arch/x86/kernel/cpu/resctrl/internal.h
@@ -240,14 +240,53 @@ 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 (Displays information about control and monitoring)
+ * |
+ * --> base (Displays the details on resctrl groups)
+ *
+ * -------------------------------------------------------------
+ * info directory structure
+ * --> RFTYPE_INFO
+ * --> RFTYPE_TOP_INFO
+ * last_cmd_status
+ *
+ * --> RFTYPE_MON_INFO
+ * --> (L2_MON)
+ * --> (L3_MON)
+ * max_threshold_occupancy, mbm_local_bytes_config,
+ * mbm_total_bytes_config, mon_features, num_rmids
+ *
+ * --> RFTYPE_CTRL_INFO
+ * --> RFTYPE_RES_CACHE (L2, L3)
+ * bit_usage, cbm_mask, min_cbm_bits, num_closids,
+ * shareable_bits
+ *
+ * --> RFTYPE_RES_MB (MB, SMBA)
+ * bandwidth_gran, delay_linear, min_bandwidth,
+ * num_closids
+ *
+ * group structure
+ * -----------------------------------------------------------
+ * --> RFTYPE_BASE (Files common for both MON and CTRL groups)
+ * cpus, cpus_list, tasks
+ *
+ * --> RFTYPE_CTRL_BASE (Files only for CTRL group)
+ * mode, schemata, size
*/
#define RFTYPE_INFO BIT(0)
#define RFTYPE_BASE BIT(1)
-#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 RFTYPE_CTRL BIT(2)
+#define RFTYPE_MON BIT(3)
+#define RFTYPE_TOP BIT(4)
+#define RFTYPE_RES_CACHE BIT(5)
+#define RFTYPE_RES_MB BIT(6)
#define RFTYPE_CTRL_INFO (RFTYPE_INFO | RFTYPE_CTRL)
#define RFTYPE_MON_INFO (RFTYPE_INFO | RFTYPE_MON)
#define RFTYPE_TOP_INFO (RFTYPE_INFO | RFTYPE_TOP)
Add "-o debug" option to mount resctrl filesystem in debug mode.
Debug option adds the files for debug purposes.
Signed-off-by: Babu Moger <[email protected]>
---
Documentation/x86/resctrl.rst | 3 +++
arch/x86/kernel/cpu/resctrl/internal.h | 1 +
arch/x86/kernel/cpu/resctrl/rdtgroup.c | 13 +++++++++++++
3 files changed, 17 insertions(+)
diff --git a/Documentation/x86/resctrl.rst b/Documentation/x86/resctrl.rst
index f28ed1443a6a..be443251b484 100644
--- a/Documentation/x86/resctrl.rst
+++ b/Documentation/x86/resctrl.rst
@@ -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 75ddbd833fdf..1eac07ebc31b 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)
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index 719e29248892..0169821bc08c 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);
@@ -2387,6 +2389,9 @@ static int rdt_enable_ctx(struct rdt_fs_context *ctx)
if (!ret && ctx->enable_mba_mbps)
ret = set_mba_sc(true);
+ if (!ret && ctx->enable_debug)
+ resctrl_debug = true;
+
return ret;
}
@@ -2575,6 +2580,7 @@ enum rdt_param {
Opt_cdp,
Opt_cdpl2,
Opt_mba_mbps,
+ Opt_debug,
nr__rdt_params
};
@@ -2582,6 +2588,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),
{}
};
@@ -2607,6 +2614,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;
@@ -3549,6 +3559,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;
}
When a user creates a control or monitor group, the CLOSID or RMID
are not visible to the user. It can help to debug the issues in some
cases. There are only available with "-o debug" option.
Add CLOSID(ctrl_hw_id) and RMID(mon_hw_id) to the control/monitor groups
display in resctrl interface.
$cat /sys/fs/resctrl/clos1/clos_hw_id
1
$cat /sys/fs/resctrl/mon_groups/mon1/mon_hw_id
3
Signed-off-by: Babu Moger <[email protected]>
---
Documentation/x86/resctrl.rst | 17 ++++++++++++
arch/x86/kernel/cpu/resctrl/rdtgroup.c | 44 ++++++++++++++++++++++++++++++++
2 files changed, 61 insertions(+)
diff --git a/Documentation/x86/resctrl.rst b/Documentation/x86/resctrl.rst
index be443251b484..5aff8c2beb08 100644
--- a/Documentation/x86/resctrl.rst
+++ b/Documentation/x86/resctrl.rst
@@ -345,6 +345,14 @@ 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. On x86, reading this file shows
+ the Class of Service (CLOS) id which acts as a resource control
+ tag on which the resources can be throttled. Kernel assigns a new
+ CLOSID a control group is created depending on the available
+ CLOSIDs. Multiple cores(or threads) or processes can share a
+ same CLOSID in a resctrl group.
+
When monitoring is enabled all MON groups will also contain:
"mon_data":
@@ -358,6 +366,15 @@ 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. On x86, reading this file shows
+ the Resource Monitoring ID (RMID) for monitoring the resource
+ utilization. Monitoring is performed by tagging each core (or
+ thread) or process via a RMID. Kernel assigns a new RMID when
+ a group is created depending on the available RMIDs. Multiple
+ cores (or threads) or processes can share a same RMID in a
+ resctrl group.
+
Resource allocation rules
-------------------------
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index 0169821bc08c..15ded0dd5b09 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -782,6 +782,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
/*
@@ -1843,6 +1875,12 @@ 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,
+ },
{
.name = "schemata",
.mode = 0644,
@@ -1866,6 +1904,12 @@ 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,
+ },
};
Add the debug files to the resctrl hierarchy.
Signed-off-by: Babu Moger <[email protected]>
---
arch/x86/kernel/cpu/resctrl/internal.h | 1 +
arch/x86/kernel/cpu/resctrl/rdtgroup.c | 54 +++++++++++++++++++++++++++++++-
2 files changed, 54 insertions(+), 1 deletion(-)
diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
index 1eac07ebc31b..855109abb480 100644
--- a/arch/x86/kernel/cpu/resctrl/internal.h
+++ b/arch/x86/kernel/cpu/resctrl/internal.h
@@ -288,6 +288,7 @@ struct rdtgroup {
#define RFTYPE_TOP BIT(4)
#define RFTYPE_RES_CACHE BIT(5)
#define RFTYPE_RES_MB BIT(6)
+#define RFTYPE_DEBUG BIT(7)
#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 15ded0dd5b09..1ec4359348c2 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -1880,6 +1880,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,
},
{
.name = "schemata",
@@ -1909,6 +1910,7 @@ static struct rftype res_common_files[] = {
.mode = 0444,
.kf_ops = &rdtgroup_kf_single_ops,
.seq_show = rdtgroup_closid_show,
+ .fflags = RFTYPE_CTRL_BASE | RFTYPE_DEBUG,
},
};
@@ -2420,6 +2422,49 @@ static int mkdir_mondata_all(struct kernfs_node *parent_kn,
struct rdtgroup *prgrp,
struct kernfs_node **mon_data_kn);
+static void resctrl_add_debug_files(void)
+{
+ struct rftype *rfts, *rft;
+ int len;
+
+ rfts = res_common_files;
+ len = ARRAY_SIZE(res_common_files);
+
+ lockdep_assert_held(&rdtgroup_mutex);
+
+ for (rft = rfts; rft < rfts + len; rft++) {
+ if (rft->fflags & RFTYPE_DEBUG) {
+ rft->fflags &= ~RFTYPE_DEBUG;
+ rdtgroup_add_file(rdtgroup_default.kn, rft);
+ }
+ }
+
+ kernfs_activate(rdtgroup_default.kn);
+}
+
+static void resctrl_remove_debug_files(void)
+{
+ struct rftype *rfts, *rft;
+ int len;
+
+ rfts = res_common_files;
+ len = ARRAY_SIZE(res_common_files);
+
+ lockdep_assert_held(&rdtgroup_mutex);
+
+ for (rft = rfts; rft < rfts + len; rft++) {
+ if (!strcmp(rft->name, "mon_hw_id")) {
+ rft->fflags |= RFTYPE_DEBUG;
+ kernfs_remove_by_name(rdtgroup_default.kn, rft->name);
+ } else if (!strcmp(rft->name, "ctrl_hw_id")) {
+ rft->fflags |= RFTYPE_DEBUG;
+ kernfs_remove_by_name(rdtgroup_default.kn, rft->name);
+ }
+ }
+
+ kernfs_activate(rdtgroup_default.kn);
+}
+
static int rdt_enable_ctx(struct rdt_fs_context *ctx)
{
int ret = 0;
@@ -2433,8 +2478,10 @@ static int rdt_enable_ctx(struct rdt_fs_context *ctx)
if (!ret && ctx->enable_mba_mbps)
ret = set_mba_sc(true);
- if (!ret && ctx->enable_debug)
+ if (!ret && ctx->enable_debug) {
resctrl_debug = true;
+ resctrl_add_debug_files();
+ }
return ret;
}
@@ -2851,6 +2898,11 @@ static void rdt_kill_sb(struct super_block *sb)
set_mba_sc(false);
+ if (resctrl_debug) {
+ resctrl_remove_debug_files();
+ resctrl_debug = false;
+ }
+
/*Put everything back to default values. */
for_each_alloc_capable_rdt_resource(r)
reset_all_ctrls(r);
On 4/18/23 06:34, Babu Moger wrote:
> +"ctrl_hw_id":
> + Available only with debug option. On x86, reading this file shows
> + the Class of Service (CLOS) id which acts as a resource control
> + tag on which the resources can be throttled. Kernel assigns a new
> + CLOSID a control group is created depending on the available
> + CLOSIDs. Multiple cores(or threads) or processes can share a
> + same CLOSID in a resctrl group.
> +
> <snipped>...
> +"mon_hw_id":
> + Available only with debug option. On x86, reading this file shows
> + the Resource Monitoring ID (RMID) for monitoring the resource
> + utilization. Monitoring is performed by tagging each core (or
> + thread) or process via a RMID. Kernel assigns a new RMID when
> + a group is created depending on the available RMIDs. Multiple
> + cores (or threads) or processes can share a same RMID in a
> + resctrl group.
> +
Is CONFIG_DEBUG=y required?
--
An old man doll... just what I always wanted! - Clara
On 4/17/23 21:22, Bagas Sanjaya wrote:
> On 4/18/23 06:34, Babu Moger wrote:
>> +"ctrl_hw_id":
>> + Available only with debug option. On x86, reading this file shows
>> + the Class of Service (CLOS) id which acts as a resource control
>> + tag on which the resources can be throttled. Kernel assigns a new
>> + CLOSID a control group is created depending on the available
>> + CLOSIDs. Multiple cores(or threads) or processes can share a
>> + same CLOSID in a resctrl group.
>> +
>> <snipped>...
>> +"mon_hw_id":
>> + Available only with debug option. On x86, reading this file shows
>> + the Resource Monitoring ID (RMID) for monitoring the resource
>> + utilization. Monitoring is performed by tagging each core (or
>> + thread) or process via a RMID. Kernel assigns a new RMID when
>> + a group is created depending on the available RMIDs. Multiple
>> + cores (or threads) or processes can share a same RMID in a
>> + resctrl group.
>> +
>
> Is CONFIG_DEBUG=y required?
No. Available with resctrl dubug option.
Thanks
Babu Moger
On Mon, 17 Apr 2023, Babu Moger wrote:
> The rftype flags have two different prefixes even though they are used
> for the same purpose. Change the prefix to RFTYPE_ for all the flags.
>
> Signed-off-by: Babu Moger <[email protected]>
> ---
> arch/x86/kernel/cpu/resctrl/internal.h | 8 +++---
> arch/x86/kernel/cpu/resctrl/rdtgroup.c | 42 ++++++++++++++++----------------
> 2 files changed, 25 insertions(+), 25 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> index 62767774810d..c4fc5a1c630c 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -248,10 +248,10 @@ struct rdtgroup {
> #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)
> -#define RF_MON_INFO (RFTYPE_INFO | RFTYPE_MON)
> -#define RF_TOP_INFO (RFTYPE_INFO | RFTYPE_TOP)
> -#define RF_CTRL_BASE (RFTYPE_BASE | RFTYPE_CTRL)
> +#define RFTYPE_CTRL_INFO (RFTYPE_INFO | RFTYPE_CTRL)
> +#define RFTYPE_MON_INFO (RFTYPE_INFO | RFTYPE_MON)
> +#define RFTYPE_TOP_INFO (RFTYPE_INFO | RFTYPE_TOP)
> +#define RFTYPE_CTRL_BASE (RFTYPE_BASE | RFTYPE_CTRL)
>
> /* List of all resource groups */
> extern struct list_head rdt_all_groups;
This needs to be changed as well:
* @fflags: File specific RF_* or RFTYPE_* flags
--
i.
On Mon, 17 Apr 2023, Babu Moger wrote:
> 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/clos1
> $echo 123 > /sys/fs/resctrl/clos1/tasks
> $echo 456 > /sys/fs/resctrl/clos1/tasks
> $echo 789 > /sys/fs/resctrl/clos1/tasks
>
> This is not user-friendly when dealing with hundreds of tasks.
>
> It can be improved by supporting the multiple task id assignment in
> one command with the tasks separated by commas. For example:
>
> $echo 123,456,789 > /sys/fs/resctrl/clos1/tasks
>
> Signed-off-by: Babu Moger <[email protected]>
> ---
> Documentation/x86/resctrl.rst | 9 ++++++++-
> arch/x86/kernel/cpu/resctrl/rdtgroup.c | 31 ++++++++++++++++++++++++++++++-
> 2 files changed, 38 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/x86/resctrl.rst b/Documentation/x86/resctrl.rst
> index 387ccbcb558f..f28ed1443a6a 100644
> --- a/Documentation/x86/resctrl.rst
> +++ b/Documentation/x86/resctrl.rst
> @@ -292,7 +292,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 in the order it
> + is entered.
"Tasks ... it is ..." doesn't sound correct.
> Failures while assigning the tasks will be aborted
> + immediately and tasks next in the sequence will not be assigned.
> + Users may need to retry them again. Failure details possibly with
> + pid will be logged in /sys/fs/resctrl/info/last_cmd_status file.
> +
> + 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 6ad33f355861..df5bd13440b0 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -696,18 +696,41 @@ 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)
> + if (nbytes == 0)
> return -EINVAL;
> +
> + buf[nbytes - 1] = '\0';
> +
> rdtgrp = rdtgroup_kn_lock_live(of->kn);
> if (!rdtgrp) {
> rdtgroup_kn_unlock(of->kn);
> return -ENOENT;
> }
> +
> +next:
> + if (!buf || buf[0] == '\0')
> + goto unlock;
> +
> rdt_last_cmd_clear();
>
> + pid_str = strim(strsep(&buf, ","));
> +
> + if (kstrtoint(pid_str, 0, &pid)) {
> + rdt_last_cmd_printf("Task list parsing error\n");
> + ret = -EINVAL;
> + goto unlock;
> + }
> +
> + if (pid < 0) {
> + rdt_last_cmd_printf("Invalid pid %d value\n", pid);
> + ret = -EINVAL;
> + goto unlock;
> + }
> +
> if (rdtgrp->mode == RDT_MODE_PSEUDO_LOCKED ||
> rdtgrp->mode == RDT_MODE_PSEUDO_LOCKSETUP) {
> ret = -EINVAL;
> @@ -716,6 +739,12 @@ static ssize_t rdtgroup_tasks_write(struct kernfs_open_file *of,
> }
>
> ret = rdtgroup_move_task(pid, rdtgrp, of);
> + if (ret) {
> + rdt_last_cmd_printf("Error while processing task %d\n", pid);
> + goto unlock;
> + } else {
> + goto next;
> + }
Why is this not changed into a while () loop??
>
> unlock:
> rdtgroup_kn_unlock(of->kn);
>
>
--
i.
On Mon, 17 Apr 2023, Babu Moger wrote:
> Add the debug files to the resctrl hierarchy.
>
> Signed-off-by: Babu Moger <[email protected]>
> ---
> arch/x86/kernel/cpu/resctrl/internal.h | 1 +
> arch/x86/kernel/cpu/resctrl/rdtgroup.c | 54 +++++++++++++++++++++++++++++++-
> 2 files changed, 54 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> index 1eac07ebc31b..855109abb480 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -288,6 +288,7 @@ struct rdtgroup {
> #define RFTYPE_TOP BIT(4)
> #define RFTYPE_RES_CACHE BIT(5)
> #define RFTYPE_RES_MB BIT(6)
> +#define RFTYPE_DEBUG BIT(7)
> #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 15ded0dd5b09..1ec4359348c2 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -1880,6 +1880,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,
> },
> {
> .name = "schemata",
> @@ -1909,6 +1910,7 @@ static struct rftype res_common_files[] = {
> .mode = 0444,
> .kf_ops = &rdtgroup_kf_single_ops,
> .seq_show = rdtgroup_closid_show,
> + .fflags = RFTYPE_CTRL_BASE | RFTYPE_DEBUG,
> },
>
> };
> @@ -2420,6 +2422,49 @@ static int mkdir_mondata_all(struct kernfs_node *parent_kn,
> struct rdtgroup *prgrp,
> struct kernfs_node **mon_data_kn);
>
> +static void resctrl_add_debug_files(void)
> +{
> + struct rftype *rfts, *rft;
> + int len;
> +
> + rfts = res_common_files;
> + len = ARRAY_SIZE(res_common_files);
> +
> + lockdep_assert_held(&rdtgroup_mutex);
> +
> + for (rft = rfts; rft < rfts + len; rft++) {
> + if (rft->fflags & RFTYPE_DEBUG) {
> + rft->fflags &= ~RFTYPE_DEBUG;
I don't fully follow why you need to play with ->fflags like this.
Is it for the ->fflags test in rdtgroup_add_files()? Can't you just do
some extra masking there for RFTYPE_DEBUG based on resctrl_debug which
you already keep?
> + rdtgroup_add_file(rdtgroup_default.kn, rft);
> + }
> + }
> +
> + kernfs_activate(rdtgroup_default.kn);
> +}
> +
> +static void resctrl_remove_debug_files(void)
> +{
> + struct rftype *rfts, *rft;
> + int len;
> +
> + rfts = res_common_files;
> + len = ARRAY_SIZE(res_common_files);
> +
> + lockdep_assert_held(&rdtgroup_mutex);
> +
> + for (rft = rfts; rft < rfts + len; rft++) {
> + if (!strcmp(rft->name, "mon_hw_id")) {
> + rft->fflags |= RFTYPE_DEBUG;
> + kernfs_remove_by_name(rdtgroup_default.kn, rft->name);
> + } else if (!strcmp(rft->name, "ctrl_hw_id")) {
> + rft->fflags |= RFTYPE_DEBUG;
> + kernfs_remove_by_name(rdtgroup_default.kn, rft->name);
> + }
> + }
> +
> + kernfs_activate(rdtgroup_default.kn);
> +}
> +
> static int rdt_enable_ctx(struct rdt_fs_context *ctx)
> {
> int ret = 0;
> @@ -2433,8 +2478,10 @@ static int rdt_enable_ctx(struct rdt_fs_context *ctx)
> if (!ret && ctx->enable_mba_mbps)
> ret = set_mba_sc(true);
>
> - if (!ret && ctx->enable_debug)
> + if (!ret && ctx->enable_debug) {
> resctrl_debug = true;
> + resctrl_add_debug_files();
> + }
>
> return ret;
> }
> @@ -2851,6 +2898,11 @@ static void rdt_kill_sb(struct super_block *sb)
>
> set_mba_sc(false);
>
> + if (resctrl_debug) {
> + resctrl_remove_debug_files();
> + resctrl_debug = false;
Logically, this false assignment belongs to the earlier patch.
--
i.
Hi Jarvinen,
On 4/19/23 07:44, Ilpo Järvinen wrote:
> On Mon, 17 Apr 2023, Babu Moger wrote:
>
>> The rftype flags have two different prefixes even though they are used
>> for the same purpose. Change the prefix to RFTYPE_ for all the flags.
>>
>> Signed-off-by: Babu Moger <[email protected]>
>> ---
>> arch/x86/kernel/cpu/resctrl/internal.h | 8 +++---
>> arch/x86/kernel/cpu/resctrl/rdtgroup.c | 42 ++++++++++++++++----------------
>> 2 files changed, 25 insertions(+), 25 deletions(-)
>>
>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
>> index 62767774810d..c4fc5a1c630c 100644
>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>> @@ -248,10 +248,10 @@ struct rdtgroup {
>> #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)
>> -#define RF_MON_INFO (RFTYPE_INFO | RFTYPE_MON)
>> -#define RF_TOP_INFO (RFTYPE_INFO | RFTYPE_TOP)
>> -#define RF_CTRL_BASE (RFTYPE_BASE | RFTYPE_CTRL)
>> +#define RFTYPE_CTRL_INFO (RFTYPE_INFO | RFTYPE_CTRL)
>> +#define RFTYPE_MON_INFO (RFTYPE_INFO | RFTYPE_MON)
>> +#define RFTYPE_TOP_INFO (RFTYPE_INFO | RFTYPE_TOP)
>> +#define RFTYPE_CTRL_BASE (RFTYPE_BASE | RFTYPE_CTRL)
>>
>> /* List of all resource groups */
>> extern struct list_head rdt_all_groups;
>
> This needs to be changed as well:
>
> * @fflags: File specific RF_* or RFTYPE_* flags
Yes. Thanks for pointing that. Will correct it in next revision.
--
Thanks
Babu Moger
Hi Jarvinen,
On 4/19/23 07:58, Ilpo Järvinen wrote:
> On Mon, 17 Apr 2023, Babu Moger wrote:
>
>> 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/clos1
>> $echo 123 > /sys/fs/resctrl/clos1/tasks
>> $echo 456 > /sys/fs/resctrl/clos1/tasks
>> $echo 789 > /sys/fs/resctrl/clos1/tasks
>>
>> This is not user-friendly when dealing with hundreds of tasks.
>>
>> It can be improved by supporting the multiple task id assignment in
>> one command with the tasks separated by commas. For example:
>>
>> $echo 123,456,789 > /sys/fs/resctrl/clos1/tasks
>>
>> Signed-off-by: Babu Moger <[email protected]>
>> ---
>> Documentation/x86/resctrl.rst | 9 ++++++++-
>> arch/x86/kernel/cpu/resctrl/rdtgroup.c | 31 ++++++++++++++++++++++++++++++-
>> 2 files changed, 38 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/x86/resctrl.rst b/Documentation/x86/resctrl.rst
>> index 387ccbcb558f..f28ed1443a6a 100644
>> --- a/Documentation/x86/resctrl.rst
>> +++ b/Documentation/x86/resctrl.rst
>> @@ -292,7 +292,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 in the order it
>> + is entered.
>
> "Tasks ... it is ..." doesn't sound correct.
How about "Tasks will be assigned sequentially in the order they are entered."
>
>> Failures while assigning the tasks will be aborted
>> + immediately and tasks next in the sequence will not be assigned.
>> + Users may need to retry them again. Failure details possibly with
>> + pid will be logged in /sys/fs/resctrl/info/last_cmd_status file.
>> +
>> + 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 6ad33f355861..df5bd13440b0 100644
>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> @@ -696,18 +696,41 @@ 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)
>> + if (nbytes == 0)
>> return -EINVAL;
>> +
>> + buf[nbytes - 1] = '\0';
>> +
>> rdtgrp = rdtgroup_kn_lock_live(of->kn);
>> if (!rdtgrp) {
>> rdtgroup_kn_unlock(of->kn);
>> return -ENOENT;
>> }
>> +
>> +next:
>> + if (!buf || buf[0] == '\0')
>> + goto unlock;
>> +
>> rdt_last_cmd_clear();
>>
>> + pid_str = strim(strsep(&buf, ","));
>> +
>> + if (kstrtoint(pid_str, 0, &pid)) {
>> + rdt_last_cmd_printf("Task list parsing error\n");
>> + ret = -EINVAL;
>> + goto unlock;
>> + }
>> +
>> + if (pid < 0) {
>> + rdt_last_cmd_printf("Invalid pid %d value\n", pid);
>> + ret = -EINVAL;
>> + goto unlock;
>> + }
>> +
>> if (rdtgrp->mode == RDT_MODE_PSEUDO_LOCKED ||
>> rdtgrp->mode == RDT_MODE_PSEUDO_LOCKSETUP) {
>> ret = -EINVAL;
>> @@ -716,6 +739,12 @@ static ssize_t rdtgroup_tasks_write(struct kernfs_open_file *of,
>> }
>>
>> ret = rdtgroup_move_task(pid, rdtgrp, of);
>> + if (ret) {
>> + rdt_last_cmd_printf("Error while processing task %d\n", pid);
>> + goto unlock;
>> + } else {
>> + goto next;
>> + }
>
> Why is this not changed into a while () loop??
Yes. I can change that. It might look bit more cleaner.
>
>>
>> unlock:
>> rdtgroup_kn_unlock(of->kn);
>>
>>
>
>
--
Thanks
Babu Moger
On 4/19/23 08:20, Ilpo Järvinen wrote:
> On Mon, 17 Apr 2023, Babu Moger wrote:
>
>> Add the debug files to the resctrl hierarchy.
>>
>> Signed-off-by: Babu Moger <[email protected]>
>> ---
>> arch/x86/kernel/cpu/resctrl/internal.h | 1 +
>> arch/x86/kernel/cpu/resctrl/rdtgroup.c | 54 +++++++++++++++++++++++++++++++-
>> 2 files changed, 54 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
>> index 1eac07ebc31b..855109abb480 100644
>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>> @@ -288,6 +288,7 @@ struct rdtgroup {
>> #define RFTYPE_TOP BIT(4)
>> #define RFTYPE_RES_CACHE BIT(5)
>> #define RFTYPE_RES_MB BIT(6)
>> +#define RFTYPE_DEBUG BIT(7)
>> #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 15ded0dd5b09..1ec4359348c2 100644
>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> @@ -1880,6 +1880,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,
>> },
>> {
>> .name = "schemata",
>> @@ -1909,6 +1910,7 @@ static struct rftype res_common_files[] = {
>> .mode = 0444,
>> .kf_ops = &rdtgroup_kf_single_ops,
>> .seq_show = rdtgroup_closid_show,
>> + .fflags = RFTYPE_CTRL_BASE | RFTYPE_DEBUG,
>> },
>>
>> };
>> @@ -2420,6 +2422,49 @@ static int mkdir_mondata_all(struct kernfs_node *parent_kn,
>> struct rdtgroup *prgrp,
>> struct kernfs_node **mon_data_kn);
>>
>> +static void resctrl_add_debug_files(void)
>> +{
>> + struct rftype *rfts, *rft;
>> + int len;
>> +
>> + rfts = res_common_files;
>> + len = ARRAY_SIZE(res_common_files);
>> +
>> + lockdep_assert_held(&rdtgroup_mutex);
>> +
>> + for (rft = rfts; rft < rfts + len; rft++) {
>> + if (rft->fflags & RFTYPE_DEBUG) {
>> + rft->fflags &= ~RFTYPE_DEBUG;
>
> I don't fully follow why you need to play with ->fflags like this.
Yes. It is because of this check.
if (rft->fflags && ((fflags & rft->fflags) == rft->fflags)) {
I am not sure about "== rft->fflags" check. If I remove this check then I
may not have to do all this trick. I can try that.
> Is it for the ->fflags test in rdtgroup_add_files()? Can't you just do
> some extra masking there for RFTYPE_DEBUG based on resctrl_debug which
> you already keep?
Yes. Let me experiment little bit.
Thanks
Babu
>
>> + rdtgroup_add_file(rdtgroup_default.kn, rft);
>> + }
>> + }
>> +
>> + kernfs_activate(rdtgroup_default.kn);
>> +}
>> +
>> +static void resctrl_remove_debug_files(void)
>> +{
>> + struct rftype *rfts, *rft;
>> + int len;
>> +
>> + rfts = res_common_files;
>> + len = ARRAY_SIZE(res_common_files);
>> +
>> + lockdep_assert_held(&rdtgroup_mutex);
>> +
>> + for (rft = rfts; rft < rfts + len; rft++) {
>> + if (!strcmp(rft->name, "mon_hw_id")) {
>> + rft->fflags |= RFTYPE_DEBUG;
>> + kernfs_remove_by_name(rdtgroup_default.kn, rft->name);
>> + } else if (!strcmp(rft->name, "ctrl_hw_id")) {
>> + rft->fflags |= RFTYPE_DEBUG;
>> + kernfs_remove_by_name(rdtgroup_default.kn, rft->name);
>> + }
>> + }
>> +
>> + kernfs_activate(rdtgroup_default.kn);
>> +}
>> +
>> static int rdt_enable_ctx(struct rdt_fs_context *ctx)
>> {
>> int ret = 0;
>> @@ -2433,8 +2478,10 @@ static int rdt_enable_ctx(struct rdt_fs_context *ctx)
>> if (!ret && ctx->enable_mba_mbps)
>> ret = set_mba_sc(true);
>>
>> - if (!ret && ctx->enable_debug)
>> + if (!ret && ctx->enable_debug) {
>> resctrl_debug = true;
>> + resctrl_add_debug_files();
>> + }
>>
>> return ret;
>> }
>> @@ -2851,6 +2898,11 @@ static void rdt_kill_sb(struct super_block *sb)
>>
>> set_mba_sc(false);
>>
>> + if (resctrl_debug) {
>> + resctrl_remove_debug_files();
>> + resctrl_debug = false;
>
> Logically, this false assignment belongs to the earlier patch.
>
--
Thanks
Babu Moger
On 4/19/23 08:20, Ilpo Järvinen wrote:
> On Mon, 17 Apr 2023, Babu Moger wrote:
>
>> Add the debug files to the resctrl hierarchy.
>>
>> Signed-off-by: Babu Moger <[email protected]>
>> ---
>> arch/x86/kernel/cpu/resctrl/internal.h | 1 +
>> arch/x86/kernel/cpu/resctrl/rdtgroup.c | 54 +++++++++++++++++++++++++++++++-
>> 2 files changed, 54 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
>> index 1eac07ebc31b..855109abb480 100644
>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>> @@ -288,6 +288,7 @@ struct rdtgroup {
>> #define RFTYPE_TOP BIT(4)
>> #define RFTYPE_RES_CACHE BIT(5)
>> #define RFTYPE_RES_MB BIT(6)
>> +#define RFTYPE_DEBUG BIT(7)
>> #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 15ded0dd5b09..1ec4359348c2 100644
>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> @@ -1880,6 +1880,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,
>> },
>> {
>> .name = "schemata",
>> @@ -1909,6 +1910,7 @@ static struct rftype res_common_files[] = {
>> .mode = 0444,
>> .kf_ops = &rdtgroup_kf_single_ops,
>> .seq_show = rdtgroup_closid_show,
>> + .fflags = RFTYPE_CTRL_BASE | RFTYPE_DEBUG,
>> },
>>
>> };
>> @@ -2420,6 +2422,49 @@ static int mkdir_mondata_all(struct kernfs_node *parent_kn,
>> struct rdtgroup *prgrp,
>> struct kernfs_node **mon_data_kn);
>>
>> +static void resctrl_add_debug_files(void)
>> +{
>> + struct rftype *rfts, *rft;
>> + int len;
>> +
>> + rfts = res_common_files;
>> + len = ARRAY_SIZE(res_common_files);
>> +
>> + lockdep_assert_held(&rdtgroup_mutex);
>> +
>> + for (rft = rfts; rft < rfts + len; rft++) {
>> + if (rft->fflags & RFTYPE_DEBUG) {
>> + rft->fflags &= ~RFTYPE_DEBUG;
>
> I don't fully follow why you need to play with ->fflags like this.
>
> Is it for the ->fflags test in rdtgroup_add_files()? Can't you just do
> some extra masking there for RFTYPE_DEBUG based on resctrl_debug which
> you already keep?
>
>> + rdtgroup_add_file(rdtgroup_default.kn, rft);
>> + }
>> + }
>> +
>> + kernfs_activate(rdtgroup_default.kn);
>> +}
>> +
>> +static void resctrl_remove_debug_files(void)
>> +{
>> + struct rftype *rfts, *rft;
>> + int len;
>> +
>> + rfts = res_common_files;
>> + len = ARRAY_SIZE(res_common_files);
>> +
>> + lockdep_assert_held(&rdtgroup_mutex);
>> +
>> + for (rft = rfts; rft < rfts + len; rft++) {
>> + if (!strcmp(rft->name, "mon_hw_id")) {
>> + rft->fflags |= RFTYPE_DEBUG;
>> + kernfs_remove_by_name(rdtgroup_default.kn, rft->name);
>> + } else if (!strcmp(rft->name, "ctrl_hw_id")) {
>> + rft->fflags |= RFTYPE_DEBUG;
>> + kernfs_remove_by_name(rdtgroup_default.kn, rft->name);
>> + }
>> + }
>> +
>> + kernfs_activate(rdtgroup_default.kn);
>> +}
>> +
>> static int rdt_enable_ctx(struct rdt_fs_context *ctx)
>> {
>> int ret = 0;
>> @@ -2433,8 +2478,10 @@ static int rdt_enable_ctx(struct rdt_fs_context *ctx)
>> if (!ret && ctx->enable_mba_mbps)
>> ret = set_mba_sc(true);
>>
>> - if (!ret && ctx->enable_debug)
>> + if (!ret && ctx->enable_debug) {
>> resctrl_debug = true;
>> + resctrl_add_debug_files();
>> + }
>>
>> return ret;
>> }
>> @@ -2851,6 +2898,11 @@ static void rdt_kill_sb(struct super_block *sb)
>>
>> set_mba_sc(false);
>>
>> + if (resctrl_debug) {
>> + resctrl_remove_debug_files();
>> + resctrl_debug = false;
>
> Logically, this false assignment belongs to the earlier patch.
Ok. Sure
--
Thanks
Babu Moger
On 4/19/23 08:20, Ilpo Järvinen wrote:
> On Mon, 17 Apr 2023, Babu Moger wrote:
>
>> Add the debug files to the resctrl hierarchy.
>>
>> Signed-off-by: Babu Moger <[email protected]>
>> ---
>> arch/x86/kernel/cpu/resctrl/internal.h | 1 +
>> arch/x86/kernel/cpu/resctrl/rdtgroup.c | 54 +++++++++++++++++++++++++++++++-
>> 2 files changed, 54 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
>> index 1eac07ebc31b..855109abb480 100644
>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>> @@ -288,6 +288,7 @@ struct rdtgroup {
>> #define RFTYPE_TOP BIT(4)
>> #define RFTYPE_RES_CACHE BIT(5)
>> #define RFTYPE_RES_MB BIT(6)
>> +#define RFTYPE_DEBUG BIT(7)
>> #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 15ded0dd5b09..1ec4359348c2 100644
>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> @@ -1880,6 +1880,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,
>> },
>> {
>> .name = "schemata",
>> @@ -1909,6 +1910,7 @@ static struct rftype res_common_files[] = {
>> .mode = 0444,
>> .kf_ops = &rdtgroup_kf_single_ops,
>> .seq_show = rdtgroup_closid_show,
>> + .fflags = RFTYPE_CTRL_BASE | RFTYPE_DEBUG,
>> },
>>
>> };
>> @@ -2420,6 +2422,49 @@ static int mkdir_mondata_all(struct kernfs_node *parent_kn,
>> struct rdtgroup *prgrp,
>> struct kernfs_node **mon_data_kn);
>>
>> +static void resctrl_add_debug_files(void)
>> +{
>> + struct rftype *rfts, *rft;
>> + int len;
>> +
>> + rfts = res_common_files;
>> + len = ARRAY_SIZE(res_common_files);
>> +
>> + lockdep_assert_held(&rdtgroup_mutex);
>> +
>> + for (rft = rfts; rft < rfts + len; rft++) {
>> + if (rft->fflags & RFTYPE_DEBUG) {
>> + rft->fflags &= ~RFTYPE_DEBUG;
>
> I don't fully follow why you need to play with ->fflags like this.
>
> Is it for the ->fflags test in rdtgroup_add_files()? Can't you just do
> some extra masking there for RFTYPE_DEBUG based on resctrl_debug which
> you already keep?
Actually with this change, I can remove all these tricks here.
I don't have to change the check "if (rft->fflags && ((fflags &
rft->fflags) == rft->fflags)) {"
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index 1ec4359348c2..b560c44817bb 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -1925,6 +1925,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);
>
>> + rdtgroup_add_file(rdtgroup_default.kn, rft);
>> + }
>> + }
>> +
>> + kernfs_activate(rdtgroup_default.kn);
>> +}
>> +
>> +static void resctrl_remove_debug_files(void)
>> +{
>> + struct rftype *rfts, *rft;
>> + int len;
>> +
>> + rfts = res_common_files;
>> + len = ARRAY_SIZE(res_common_files);
>> +
>> + lockdep_assert_held(&rdtgroup_mutex);
>> +
>> + for (rft = rfts; rft < rfts + len; rft++) {
>> + if (!strcmp(rft->name, "mon_hw_id")) {
>> + rft->fflags |= RFTYPE_DEBUG;
>> + kernfs_remove_by_name(rdtgroup_default.kn, rft->name);
>> + } else if (!strcmp(rft->name, "ctrl_hw_id")) {
>> + rft->fflags |= RFTYPE_DEBUG;
>> + kernfs_remove_by_name(rdtgroup_default.kn, rft->name);
>> + }
>> + }
>> +
>> + kernfs_activate(rdtgroup_default.kn);
>> +}
>> +
>> static int rdt_enable_ctx(struct rdt_fs_context *ctx)
>> {
>> int ret = 0;
>> @@ -2433,8 +2478,10 @@ static int rdt_enable_ctx(struct rdt_fs_context *ctx)
>> if (!ret && ctx->enable_mba_mbps)
>> ret = set_mba_sc(true);
>>
>> - if (!ret && ctx->enable_debug)
>> + if (!ret && ctx->enable_debug) {
>> resctrl_debug = true;
>> + resctrl_add_debug_files();
>> + }
>>
>> return ret;
>> }
>> @@ -2851,6 +2898,11 @@ static void rdt_kill_sb(struct super_block *sb)
>>
>> set_mba_sc(false);
>>
>> + if (resctrl_debug) {
>> + resctrl_remove_debug_files();
>> + resctrl_debug = false;
>
> Logically, this false assignment belongs to the earlier patch.
>
--
Thanks
Babu Moger
On Wed, 19 Apr 2023, Moger, Babu wrote:
>
>
> On 4/19/23 08:20, Ilpo J?rvinen wrote:
> > On Mon, 17 Apr 2023, Babu Moger wrote:
> >
> >> Add the debug files to the resctrl hierarchy.
> >>
> >> Signed-off-by: Babu Moger <[email protected]>
> >> ---
> >> arch/x86/kernel/cpu/resctrl/internal.h | 1 +
> >> arch/x86/kernel/cpu/resctrl/rdtgroup.c | 54 +++++++++++++++++++++++++++++++-
> >> 2 files changed, 54 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> >> index 1eac07ebc31b..855109abb480 100644
> >> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> >> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> >> @@ -288,6 +288,7 @@ struct rdtgroup {
> >> #define RFTYPE_TOP BIT(4)
> >> #define RFTYPE_RES_CACHE BIT(5)
> >> #define RFTYPE_RES_MB BIT(6)
> >> +#define RFTYPE_DEBUG BIT(7)
> >> #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 15ded0dd5b09..1ec4359348c2 100644
> >> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> >> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> >> @@ -1880,6 +1880,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,
> >> },
> >> {
> >> .name = "schemata",
> >> @@ -1909,6 +1910,7 @@ static struct rftype res_common_files[] = {
> >> .mode = 0444,
> >> .kf_ops = &rdtgroup_kf_single_ops,
> >> .seq_show = rdtgroup_closid_show,
> >> + .fflags = RFTYPE_CTRL_BASE | RFTYPE_DEBUG,
> >> },
> >>
> >> };
> >> @@ -2420,6 +2422,49 @@ static int mkdir_mondata_all(struct kernfs_node *parent_kn,
> >> struct rdtgroup *prgrp,
> >> struct kernfs_node **mon_data_kn);
> >>
> >> +static void resctrl_add_debug_files(void)
> >> +{
> >> + struct rftype *rfts, *rft;
> >> + int len;
> >> +
> >> + rfts = res_common_files;
> >> + len = ARRAY_SIZE(res_common_files);
> >> +
> >> + lockdep_assert_held(&rdtgroup_mutex);
> >> +
> >> + for (rft = rfts; rft < rfts + len; rft++) {
> >> + if (rft->fflags & RFTYPE_DEBUG) {
> >> + rft->fflags &= ~RFTYPE_DEBUG;
> >
> > I don't fully follow why you need to play with ->fflags like this.
> >
> > Is it for the ->fflags test in rdtgroup_add_files()? Can't you just do
> > some extra masking there for RFTYPE_DEBUG based on resctrl_debug which
> > you already keep?
>
> Actually with this change, I can remove all these tricks here.
> I don't have to change the check "if (rft->fflags && ((fflags &
> rft->fflags) == rft->fflags)) {"
>
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index 1ec4359348c2..b560c44817bb 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -1925,6 +1925,9 @@ static int rdtgroup_add_files(struct kernfs_node
> *kn, unsigned long fflags)
>
> lockdep_assert_held(&rdtgroup_mutex);
>
> + if (resctrl_debug)
> + fflags |= RFTYPE_DEBUG;
Yes, looks good.
It matches to the idea I had in my mind but doesn't require putting it
into the if condition itself.
> +
> for (rft = rfts; rft < rfts + len; rft++) {
> if (rft->fflags && ((fflags & rft->fflags) == rft->fflags)) {
> ret = rdtgroup_add_file(kn, rft);
>
>
> >
> >> + rdtgroup_add_file(rdtgroup_default.kn, rft);
> >> + }
> >> + }
--
i.
On Wed, 19 Apr 2023, Moger, Babu wrote:
> Hi Jarvinen,
>
> On 4/19/23 07:58, Ilpo Järvinen wrote:
> > On Mon, 17 Apr 2023, Babu Moger wrote:
> >
> >> 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/clos1
> >> $echo 123 > /sys/fs/resctrl/clos1/tasks
> >> $echo 456 > /sys/fs/resctrl/clos1/tasks
> >> $echo 789 > /sys/fs/resctrl/clos1/tasks
> >>
> >> This is not user-friendly when dealing with hundreds of tasks.
> >>
> >> It can be improved by supporting the multiple task id assignment in
> >> one command with the tasks separated by commas. For example:
> >>
> >> $echo 123,456,789 > /sys/fs/resctrl/clos1/tasks
> >>
> >> Signed-off-by: Babu Moger <[email protected]>
> >> ---
> >> Documentation/x86/resctrl.rst | 9 ++++++++-
> >> arch/x86/kernel/cpu/resctrl/rdtgroup.c | 31 ++++++++++++++++++++++++++++++-
> >> 2 files changed, 38 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/Documentation/x86/resctrl.rst b/Documentation/x86/resctrl.rst
> >> index 387ccbcb558f..f28ed1443a6a 100644
> >> --- a/Documentation/x86/resctrl.rst
> >> +++ b/Documentation/x86/resctrl.rst
> >> @@ -292,7 +292,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 in the order it
> >> + is entered.
> >
> > "Tasks ... it is ..." doesn't sound correct.
>
> How about "Tasks will be assigned sequentially in the order they are entered."
It sounds better.
--
i.
[AMD Official Use Only - General]
> -----Original Message-----
> From: Ilpo J?rvinen <[email protected]>
> Sent: Thursday, April 20, 2023 3:59 AM
> To: Moger, Babu <[email protected]>
> Cc: [email protected]; Reinette Chatre <[email protected]>;
> [email protected]; [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; Das1, Sandipan <[email protected]>;
> [email protected]; [email protected]; [email protected];
> LKML <[email protected]>; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected]
> Subject: Re: [PATCH v4 7/7] x86/resctrl: Add debug files when mounted with
> debug option
>
> On Wed, 19 Apr 2023, Moger, Babu wrote:
>
> >
> >
> > On 4/19/23 08:20, Ilpo J?rvinen wrote:
> > > On Mon, 17 Apr 2023, Babu Moger wrote:
> > >
> > >> Add the debug files to the resctrl hierarchy.
> > >>
> > >> Signed-off-by: Babu Moger <[email protected]>
> > >> ---
> > >> arch/x86/kernel/cpu/resctrl/internal.h | 1 +
> > >> arch/x86/kernel/cpu/resctrl/rdtgroup.c | 54
> +++++++++++++++++++++++++++++++-
> > >> 2 files changed, 54 insertions(+), 1 deletion(-)
> > >>
> > >> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h
> > >> b/arch/x86/kernel/cpu/resctrl/internal.h
> > >> index 1eac07ebc31b..855109abb480 100644
> > >> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> > >> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> > >> @@ -288,6 +288,7 @@ struct rdtgroup {
> > >> #define RFTYPE_TOP BIT(4)
> > >> #define RFTYPE_RES_CACHE BIT(5)
> > >> #define RFTYPE_RES_MB BIT(6)
> > >> +#define RFTYPE_DEBUG BIT(7)
> > >> #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 15ded0dd5b09..1ec4359348c2 100644
> > >> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> > >> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> > >> @@ -1880,6 +1880,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,
> > >> },
> > >> {
> > >> .name = "schemata",
> > >> @@ -1909,6 +1910,7 @@ static struct rftype res_common_files[] = {
> > >> .mode = 0444,
> > >> .kf_ops = &rdtgroup_kf_single_ops,
> > >> .seq_show = rdtgroup_closid_show,
> > >> + .fflags = RFTYPE_CTRL_BASE | RFTYPE_DEBUG,
> > >> },
> > >>
> > >> };
> > >> @@ -2420,6 +2422,49 @@ static int mkdir_mondata_all(struct
> kernfs_node *parent_kn,
> > >> struct rdtgroup *prgrp,
> > >> struct kernfs_node **mon_data_kn);
> > >>
> > >> +static void resctrl_add_debug_files(void) {
> > >> + struct rftype *rfts, *rft;
> > >> + int len;
> > >> +
> > >> + rfts = res_common_files;
> > >> + len = ARRAY_SIZE(res_common_files);
> > >> +
> > >> + lockdep_assert_held(&rdtgroup_mutex);
> > >> +
> > >> + for (rft = rfts; rft < rfts + len; rft++) {
> > >> + if (rft->fflags & RFTYPE_DEBUG) {
> > >> + rft->fflags &= ~RFTYPE_DEBUG;
> > >
> > > I don't fully follow why you need to play with ->fflags like this.
> > >
> > > Is it for the ->fflags test in rdtgroup_add_files()? Can't you just
> > > do some extra masking there for RFTYPE_DEBUG based on resctrl_debug
> > > which you already keep?
> >
> > Actually with this change, I can remove all these tricks here.
> > I don't have to change the check "if (rft->fflags && ((fflags &
> > rft->fflags) == rft->fflags)) {"
> >
> > diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> > b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> > index 1ec4359348c2..b560c44817bb 100644
> > --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> > +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> > @@ -1925,6 +1925,9 @@ static int rdtgroup_add_files(struct kernfs_node
> > *kn, unsigned long fflags)
> >
> > lockdep_assert_held(&rdtgroup_mutex);
> >
> > + if (resctrl_debug)
> > + fflags |= RFTYPE_DEBUG;
>
> Yes, looks good.
>
> It matches to the idea I had in my mind but doesn't require putting it into the if
> condition itself.
Without if condition? How? Let me know.
Thanks
Babu
On Fri, 21 Apr 2023, Moger, Babu wrote:
> > -----Original Message-----
> > From: Ilpo J?rvinen <[email protected]>
> > Sent: Thursday, April 20, 2023 3:59 AM
> > To: Moger, Babu <[email protected]>
> > Cc: [email protected]; Reinette Chatre <[email protected]>;
> > [email protected]; [email protected]; [email protected]; [email protected];
> > [email protected]; [email protected]; [email protected];
> > [email protected]; [email protected]; [email protected];
> > [email protected]; [email protected];
> > [email protected]; [email protected]; [email protected];
> > [email protected]; [email protected];
> > [email protected]; [email protected];
> > [email protected]; Das1, Sandipan <[email protected]>;
> > [email protected]; [email protected]; [email protected];
> > LKML <[email protected]>; [email protected];
> > [email protected]; [email protected]; [email protected];
> > [email protected]; [email protected]; [email protected]
> > Subject: Re: [PATCH v4 7/7] x86/resctrl: Add debug files when mounted with
> > debug option
> >
> > On Wed, 19 Apr 2023, Moger, Babu wrote:
> >
> > >
> > >
> > > On 4/19/23 08:20, Ilpo J?rvinen wrote:
> > > > On Mon, 17 Apr 2023, Babu Moger wrote:
> > > >
> > > >> Add the debug files to the resctrl hierarchy.
> > > >>
> > > >> Signed-off-by: Babu Moger <[email protected]>
> > > >> ---
> > > >> arch/x86/kernel/cpu/resctrl/internal.h | 1 +
> > > >> arch/x86/kernel/cpu/resctrl/rdtgroup.c | 54
> > +++++++++++++++++++++++++++++++-
> > > >> 2 files changed, 54 insertions(+), 1 deletion(-)
> > > >>
> > > >> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h
> > > >> b/arch/x86/kernel/cpu/resctrl/internal.h
> > > >> index 1eac07ebc31b..855109abb480 100644
> > > >> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> > > >> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> > > >> @@ -288,6 +288,7 @@ struct rdtgroup {
> > > >> #define RFTYPE_TOP BIT(4)
> > > >> #define RFTYPE_RES_CACHE BIT(5)
> > > >> #define RFTYPE_RES_MB BIT(6)
> > > >> +#define RFTYPE_DEBUG BIT(7)
> > > >> #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 15ded0dd5b09..1ec4359348c2 100644
> > > >> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> > > >> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> > > >> @@ -1880,6 +1880,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,
> > > >> },
> > > >> {
> > > >> .name = "schemata",
> > > >> @@ -1909,6 +1910,7 @@ static struct rftype res_common_files[] = {
> > > >> .mode = 0444,
> > > >> .kf_ops = &rdtgroup_kf_single_ops,
> > > >> .seq_show = rdtgroup_closid_show,
> > > >> + .fflags = RFTYPE_CTRL_BASE | RFTYPE_DEBUG,
> > > >> },
> > > >>
> > > >> };
> > > >> @@ -2420,6 +2422,49 @@ static int mkdir_mondata_all(struct
> > kernfs_node *parent_kn,
> > > >> struct rdtgroup *prgrp,
> > > >> struct kernfs_node **mon_data_kn);
> > > >>
> > > >> +static void resctrl_add_debug_files(void) {
> > > >> + struct rftype *rfts, *rft;
> > > >> + int len;
> > > >> +
> > > >> + rfts = res_common_files;
> > > >> + len = ARRAY_SIZE(res_common_files);
> > > >> +
> > > >> + lockdep_assert_held(&rdtgroup_mutex);
> > > >> +
> > > >> + for (rft = rfts; rft < rfts + len; rft++) {
> > > >> + if (rft->fflags & RFTYPE_DEBUG) {
> > > >> + rft->fflags &= ~RFTYPE_DEBUG;
> > > >
> > > > I don't fully follow why you need to play with ->fflags like this.
> > > >
> > > > Is it for the ->fflags test in rdtgroup_add_files()? Can't you just
> > > > do some extra masking there for RFTYPE_DEBUG based on resctrl_debug
> > > > which you already keep?
> > >
> > > Actually with this change, I can remove all these tricks here.
> > > I don't have to change the check "if (rft->fflags && ((fflags &
> > > rft->fflags) == rft->fflags)) {"
> > >
> > > diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> > > b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> > > index 1ec4359348c2..b560c44817bb 100644
> > > --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> > > +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> > > @@ -1925,6 +1925,9 @@ static int rdtgroup_add_files(struct kernfs_node
> > > *kn, unsigned long fflags)
> > >
> > > lockdep_assert_held(&rdtgroup_mutex);
> > >
> > > + if (resctrl_debug)
> > > + fflags |= RFTYPE_DEBUG;
> >
> > Yes, looks good.
> >
> > It matches to the idea I had in my mind but doesn't require putting it
> > into the if condition itself.
>
> Without if condition? How? Let me know.
I was referring to the if condition within the loop, not to doing it
without some conditional (I had an (resctrl_debug ? RFTYPE_DEBUG : 0)
construct in my mind).
To remove if, it would, of course, be possible to use another static
file-level variable but it doesn't seem justified. I think what you
proposed is fine for this use and looks clean.
--
i.
Hi Babu,
On 4/17/2023 4:33 PM, Babu Moger wrote:
> 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. While doing these above changes, found that rftype flags needed some cleanup.
> They were named inconsistently. Re-arranged them much more cleanly now.
> Hope it can help future additions.
>
> ---
> 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. But I thing it is big problem.
> Please comment back if we can do better.
From what I understand the primary concern was the naming of the
files, which you address in this version.
A second point I saw was a request for insight into why user space may need this
(James recommended obfuscation when value is only shared between kernel interfaces).
You did answer this in your response and since there was no follow-up I currently
assume that this has been answered.
Unless we hear otherwise from James I thus believe that his feedback is
addressed.
> 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.
Could you please elaborate? It actually looks like you may be headed there next
according to:
https://lore.kernel.org/lkml/[email protected]/
Reinette
Hi Babu,
On 4/17/2023 4:34 PM, Babu Moger wrote:
> The resctrl task assignment for MONITOR or CONTROL group needs to be
> done one at a time. For example:
Why all caps for monitor and control? If the intention is to use
the terms for these groups then it may be easier to use the same
terms as in the documentation, or you could just not use all caps
like you do in later patches.
>
> $mount -t resctrl resctrl /sys/fs/resctrl/
> $mkdir /sys/fs/resctrl/clos1
> $echo 123 > /sys/fs/resctrl/clos1/tasks
> $echo 456 > /sys/fs/resctrl/clos1/tasks
> $echo 789 > /sys/fs/resctrl/clos1/tasks
>
> This is not user-friendly when dealing with hundreds of tasks.
>
> It can be improved by supporting the multiple task id assignment in
> one command with the tasks separated by commas. For example:
Please use imperative mood (see Documentation/process/maintainer-tip.rst).
Something like:
"Improve multiple task id assignment ...."
>
> $echo 123,456,789 > /sys/fs/resctrl/clos1/tasks
>
> Signed-off-by: Babu Moger <[email protected]>
> ---
> Documentation/x86/resctrl.rst | 9 ++++++++-
> arch/x86/kernel/cpu/resctrl/rdtgroup.c | 31 ++++++++++++++++++++++++++++++-
> 2 files changed, 38 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/x86/resctrl.rst b/Documentation/x86/resctrl.rst
> index 387ccbcb558f..f28ed1443a6a 100644
> --- a/Documentation/x86/resctrl.rst
> +++ b/Documentation/x86/resctrl.rst
> @@ -292,7 +292,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 in the order it
I think the "in the order it is entered." can be dropped so that it just
reads (note tense change): "Tasks are assigned sequentially."
> + is entered. Failures while assigning the tasks will be aborted
> + immediately and tasks next in the sequence will not be assigned.
Multiple failures are not supported. A single failure encountered while
attempting to assign a single task will cause the operation to abort.
> + Users may need to retry them again. Failure details possibly with
I am not sure about this guidance. From what I can tell a failure could be
either that the pid does not exist or that the move is illegal. A retry
would not achieve a different outcome. I think you may thus mean that
the tasks that followed a task that could not be moved, but in that
case the "retry" is not clear to me.
> + pid will be logged in /sys/fs/resctrl/info/last_cmd_status file.
Would it not always print the failing pid (if error was encountered while
attempting to move a task) ? Maybe just drop that so that it reads
"Failure details will be logged to ..." (adding file seems unnecessary).
> +
> + 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 6ad33f355861..df5bd13440b0 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -696,18 +696,41 @@ 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)
> + if (nbytes == 0)
> return -EINVAL;
> +
> + buf[nbytes - 1] = '\0';
> +
This seems like another remnant of the schemata write code that
expects that the buffer ends with a '\n'. Since this code does not
have this requirement the above may have unintended consequences if
a tool provides a buffer that does not end with '\n'.
I think you just want to ensure that the buffer is properly terminated
and from what I understand when looking at kernfs_fop_write_iter() this
is already taken care of.
> rdtgrp = rdtgroup_kn_lock_live(of->kn);
> if (!rdtgrp) {
> rdtgroup_kn_unlock(of->kn);
> return -ENOENT;
> }
> +
> +next:
> + if (!buf || buf[0] == '\0')
> + goto unlock;
> +
> rdt_last_cmd_clear();
Why is this buffer cleared on processing of each pid?
>
> + pid_str = strim(strsep(&buf, ","));
> +
> + if (kstrtoint(pid_str, 0, &pid)) {
> + rdt_last_cmd_printf("Task list parsing error\n");
rdt_last_cmd_puts()?
> + ret = -EINVAL;
> + goto unlock;
> + }
> +
> + if (pid < 0) {
> + rdt_last_cmd_printf("Invalid pid %d value\n", pid);
> + ret = -EINVAL;
> + goto unlock;
> + }
> +
> if (rdtgrp->mode == RDT_MODE_PSEUDO_LOCKED ||
> rdtgrp->mode == RDT_MODE_PSEUDO_LOCKSETUP) {
> ret = -EINVAL;
The above code has nothing to do with the pid so repeating its
execution does not seem necessary.
> @@ -716,6 +739,12 @@ static ssize_t rdtgroup_tasks_write(struct kernfs_open_file *of,
> }
>
> ret = rdtgroup_move_task(pid, rdtgrp, of);
> + if (ret) {
> + rdt_last_cmd_printf("Error while processing task %d\n", pid);
> + goto unlock;
> + } else {
> + goto next;
> + }
>
> unlock:
> rdtgroup_kn_unlock(of->kn);
>
>
Reinette
Hi Babu,
On 4/17/2023 4:34 PM, Babu Moger wrote:
> Add "-o debug" option to mount resctrl filesystem in debug mode.
> Debug option adds the files for debug purposes.
>
Could this changelog please be expanded to explain what "debug mode"
is, why is it necessary, and what will it be used for?
The changelog mentions "adds the files for debug purposes" but does
not explain what is meant by "the files" nor what these
files may look like or what "debug purposes" may be.
Reinette
Hi Babu,
On 4/17/2023 4:34 PM, Babu Moger wrote:
> Remove gaps in bit definitions of RFTYPE flags and add more comments
Why is it necessary to remove gaps in the bit definitions?
Reinette
Hi Babu,
On 4/17/2023 4:34 PM, Babu Moger wrote:
> Remove unnecessary rftype flags to avoid multiple indirection.
Could you please elaborate what makes these flags unnecessary?
Reinette
Hi Babu,
On 4/17/2023 4:34 PM, Babu Moger wrote:
> The rftype flags have two different prefixes even though they are used
Could this changelog please start with some context about what the
rftype flags are? Please note that the changelogs are required
to comply to the x86 requirements as documented in:
Documentation/process/maintainer-tip.rst (there is a whole
section on changelogs).
After a context the problem could be made specific with something
like "rftype flags use the RF_ as well as RFTYPE_ prefixes."
Reinette
Hi Babu,
On 4/17/2023 4:34 PM, Babu Moger wrote:
> When a user creates a control or monitor group, the CLOSID or RMID
> are not visible to the user. It can help to debug the issues in some
> cases. There are only available with "-o debug" option.
Please see: Documentation/process/maintainer-tip.rst
"It's also useful to structure the changelog into several paragraphs and not
lump everything together into a single one. A good structure is to explain
the context, the problem and the solution in separate paragraphs and this
order."
>
> Add CLOSID(ctrl_hw_id) and RMID(mon_hw_id) to the control/monitor groups
Please highlight that CLOSID and RMID are x86 concepts.
> display in resctrl interface.
> $cat /sys/fs/resctrl/clos1/clos_hw_id
> 1
This example does not match what the patch does (clos_hw_id -> ctrl_hw_id).
I also think this change would be more palatable (to non x86 audience) if
the example resource group has a generic (non-x86 concept) name.
> $cat /sys/fs/resctrl/mon_groups/mon1/mon_hw_id
> 3
>
> Signed-off-by: Babu Moger <[email protected]>
> ---
> Documentation/x86/resctrl.rst | 17 ++++++++++++
> arch/x86/kernel/cpu/resctrl/rdtgroup.c | 44 ++++++++++++++++++++++++++++++++
> 2 files changed, 61 insertions(+)
>
> diff --git a/Documentation/x86/resctrl.rst b/Documentation/x86/resctrl.rst
> index be443251b484..5aff8c2beb08 100644
> --- a/Documentation/x86/resctrl.rst
> +++ b/Documentation/x86/resctrl.rst
> @@ -345,6 +345,14 @@ 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. On x86, reading this file shows
> + the Class of Service (CLOS) id which acts as a resource control
> + tag on which the resources can be throttled. Kernel assigns a new
> + CLOSID a control group is created depending on the available
> + CLOSIDs. Multiple cores(or threads) or processes can share a
> + same CLOSID in a resctrl group.
Please keep other content from the documentation in mind when making
this change. CLOSID is already documented, including the fact that it
is a limited resource. Please see content under: "Notes on cache occupancy
monitoring and control" where it, for example, states that "The number
of CLOSid and RMID are limited by the hardware."
Considering this the above could just read:
"Available only with debug option. The identifier used by hardware
for the control group. On x86 this is the CLOSID."
Similar feedback to the "mon_hw_id" portion.
Reinette
[AMD Official Use Only - General]
Hi Reinette,
> -----Original Message-----
> From: Reinette Chatre <[email protected]>
> Sent: Thursday, May 4, 2023 1:54 PM
> To: Moger, Babu <[email protected]>; [email protected];
> [email protected]; [email protected]; [email protected]
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; Das1, Sandipan
> <[email protected]>; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]
> Subject: Re: [PATCH v4 0/7] x86/resctrl: Miscellaneous resctrl features
>
> Hi Babu,
>
> On 4/17/2023 4:33 PM, Babu Moger wrote:
> > 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. While doing these above changes, found that rftype flags needed some
> cleanup.
> > They were named inconsistently. Re-arranged them much more cleanly
> now.
> > Hope it can help future additions.
> >
> > ---
> > 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. But I thing it is big
> problem.
> > Please comment back if we can do better.
>
> From what I understand the primary concern was the naming of the files, which
> you address in this version.
>
> A second point I saw was a request for insight into why user space may need
> this (James recommended obfuscation when value is only shared between
> kernel interfaces).
> You did answer this in your response and since there was no follow-up I
> currently assume that this has been answered.
>
> Unless we hear otherwise from James I thus believe that his feedback is
> addressed.
Ok. Sounds good.
>
> > 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.
>
> Could you please elaborate? It actually looks like you may be headed there next
> according to:
> https://lore.kernel.org/lkml/933d8ae2-d8b7-7436-5918-
> [email protected]/
Sorry, I was talking about this comment.
https://lore.kernel.org/lkml/[email protected]/
I tried to address it here.
https://lore.kernel.org/lkml/168177451010.1758847.568218491528297451.stgit@bmoger-ubuntu/
This may not be the exact way you mentioned. Reason is, adding the flags before rdtgroup_add_files cannot be done. It does not update the resctrl root filesystem.
These files have to added by calling rdtgroup_add_file and kernfs_activate in rdt_enable_ctx.
Thanks
Babu
[AMD Official Use Only - General]
Hi Reinette,
> -----Original Message-----
> From: Reinette Chatre <[email protected]>
> Sent: Thursday, May 4, 2023 1:58 PM
> To: Moger, Babu <[email protected]>; [email protected];
> [email protected]; [email protected]; [email protected]
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; Das1, Sandipan
> <[email protected]>; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]
> Subject: Re: [PATCH v4 1/7] x86/resctrl: Add multiple tasks to the resctrl group
> at once
>
> Hi Babu,
>
> On 4/17/2023 4:34 PM, Babu Moger wrote:
> > The resctrl task assignment for MONITOR or CONTROL group needs to be
> > done one at a time. For example:
>
> Why all caps for monitor and control? If the intention is to use the terms for
> these groups then it may be easier to use the same terms as in the
> documentation, or you could just not use all caps like you do in later patches.
Sure.
>
> >
> > $mount -t resctrl resctrl /sys/fs/resctrl/
> > $mkdir /sys/fs/resctrl/clos1
> > $echo 123 > /sys/fs/resctrl/clos1/tasks
> > $echo 456 > /sys/fs/resctrl/clos1/tasks
> > $echo 789 > /sys/fs/resctrl/clos1/tasks
> >
> > This is not user-friendly when dealing with hundreds of tasks.
> >
> > It can be improved by supporting the multiple task id assignment in
> > one command with the tasks separated by commas. For example:
>
> Please use imperative mood (see Documentation/process/maintainer-tip.rst).
>
> Something like:
> "Improve multiple task id assignment ...."
How about:
"Improve the assignment by supporting multiple task id assignment in
one command with the tasks separated by commas."
>
> >
> > $echo 123,456,789 > /sys/fs/resctrl/clos1/tasks
> >
> > Signed-off-by: Babu Moger <[email protected]>
> > ---
> > Documentation/x86/resctrl.rst | 9 ++++++++-
> > arch/x86/kernel/cpu/resctrl/rdtgroup.c | 31
> ++++++++++++++++++++++++++++++-
> > 2 files changed, 38 insertions(+), 2 deletions(-)
> >
> > diff --git a/Documentation/x86/resctrl.rst
> > b/Documentation/x86/resctrl.rst index 387ccbcb558f..f28ed1443a6a
> > 100644
> > --- a/Documentation/x86/resctrl.rst
> > +++ b/Documentation/x86/resctrl.rst
> > @@ -292,7 +292,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 in the order it
>
> I think the "in the order it is entered." can be dropped so that it just reads (note
> tense change): "Tasks are assigned sequentially."
Ok. Sure
>
> > + is entered. Failures while assigning the tasks will be aborted
> > + immediately and tasks next in the sequence will not be assigned.
>
> Multiple failures are not supported. A single failure encountered while
> attempting to assign a single task will cause the operation to abort.
Ok. Sure
>
> > + Users may need to retry them again. Failure details possibly with
>
> I am not sure about this guidance. From what I can tell a failure could be either
> that the pid does not exist or that the move is illegal. A retry would not achieve
> a different outcome. I think you may thus mean that the tasks that followed a
> task that could not be moved, but in that case the "retry" is not clear to me.
Ok. Will drop "retry" sentence.
>
> > + pid will be logged in /sys/fs/resctrl/info/last_cmd_status file.
>
> Would it not always print the failing pid (if error was encountered while
Not always. In this case it does not print the pid,
rdt_last_cmd_puts("Can't move task to different control group\n");
return -EINVAL;
> attempting to move a task) ? Maybe just drop that so that it reads "Failure
> details will be logged to ..." (adding file seems unnecessary).
Ok
>
>
> > +
> > + 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 6ad33f355861..df5bd13440b0 100644
> > --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> > +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> > @@ -696,18 +696,41 @@ 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)
> > + if (nbytes == 0)
> > return -EINVAL;
> > +
> > + buf[nbytes - 1] = '\0';
> > +
>
> This seems like another remnant of the schemata write code that expects that
> the buffer ends with a '\n'. Since this code does not have this requirement the
> above may have unintended consequences if a tool provides a buffer that does
> not end with '\n'.
> I think you just want to ensure that the buffer is properly terminated and from
> what I understand when looking at kernfs_fop_write_iter() this is already taken
> care of.
Sure. Will check. Then I will have to change the check below to if (!buf).
>
> > rdtgrp = rdtgroup_kn_lock_live(of->kn);
> > if (!rdtgrp) {
> > rdtgroup_kn_unlock(of->kn);
> > return -ENOENT;
> > }
> > +
> > +next:
> > + if (!buf || buf[0] == '\0')
> > + goto unlock;
> > +
> > rdt_last_cmd_clear();
>
> Why is this buffer cleared on processing of each pid?
Will check.
>
> >
> > + pid_str = strim(strsep(&buf, ","));
> > +
> > + if (kstrtoint(pid_str, 0, &pid)) {
> > + rdt_last_cmd_printf("Task list parsing error\n");
>
> rdt_last_cmd_puts()?
Sure.
>
> > + ret = -EINVAL;
> > + goto unlock;
> > + }
> > +
> > + if (pid < 0) {
> > + rdt_last_cmd_printf("Invalid pid %d value\n", pid);
> > + ret = -EINVAL;
> > + goto unlock;
> > + }
> > +
> > if (rdtgrp->mode == RDT_MODE_PSEUDO_LOCKED ||
> > rdtgrp->mode == RDT_MODE_PSEUDO_LOCKSETUP) {
> > ret = -EINVAL;
>
> The above code has nothing to do with the pid so repeating its execution does
> not seem necessary.
Will remove..
Thanks
Babu
>
> > @@ -716,6 +739,12 @@ static ssize_t rdtgroup_tasks_write(struct
> kernfs_open_file *of,
> > }
> >
> > ret = rdtgroup_move_task(pid, rdtgrp, of);
> > + if (ret) {
> > + rdt_last_cmd_printf("Error while processing task %d\n", pid);
> > + goto unlock;
> > + } else {
> > + goto next;
> > + }
> >
> > unlock:
> > rdtgroup_kn_unlock(of->kn);
> >
> >
>
> Reinette
Hi Babu,
On 5/5/2023 8:43 AM, Moger, Babu wrote:
> [AMD Official Use Only - General]
>
> Hi Reinette,
>
>> -----Original Message-----
>> From: Reinette Chatre <[email protected]>
>> Sent: Thursday, May 4, 2023 1:54 PM
>> To: Moger, Babu <[email protected]>; [email protected];
>> [email protected]; [email protected]; [email protected]
>> Cc: [email protected]; [email protected]; [email protected];
>> [email protected]; [email protected]; [email protected];
>> [email protected]; [email protected];
>> [email protected]; [email protected];
>> [email protected]; [email protected]; [email protected];
>> [email protected]; [email protected];
>> [email protected]; [email protected]; Das1, Sandipan
>> <[email protected]>; [email protected]; [email protected];
>> [email protected]; [email protected];
>> [email protected]; [email protected]; [email protected];
>> [email protected]; [email protected]; [email protected];
>> [email protected]
>> Subject: Re: [PATCH v4 0/7] x86/resctrl: Miscellaneous resctrl features
>>
...
>>> 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.
>>
>> Could you please elaborate? It actually looks like you may be headed there next
>> according to:
>> https://lore.kernel.org/lkml/933d8ae2-d8b7-7436-5918-
>> [email protected]/
>
> Sorry, I was talking about this comment.
> https://lore.kernel.org/lkml/[email protected]/
>
> I tried to address it here.
> https://lore.kernel.org/lkml/168177451010.1758847.568218491528297451.stgit@bmoger-ubuntu/
>
> This may not be the exact way you mentioned. Reason is, adding the
> flags before rdtgroup_add_files cannot be done. It does not update
> the resctrl root filesystem. These files have to added by calling
> rdtgroup_add_file and kernfs_activate in rdt_enable_ctx.
I think what you are referring to is files not appearing in the default
resource group? From what I can tell the files should appear when new resource
groups are created. Could the default resource group's files not also
be added on resctrl mount (moved from rdtgroup_setup_root() to rdt_get_tree())?
I do not see why the files belonging to the default group are required to be
added before resctrl mount and with the move the resctrl_debug flag can
continue to be set in rdt_enable_ctx() and available to rdtgroup_add_files()
when adding the default resource group's files.
To me this sounds simpler since there is no duplicate file add/remove code,
and the debug files are just another file type.
Reinette
[AMD Official Use Only - General]
Hi Reinette,
> -----Original Message-----
> From: Reinette Chatre <[email protected]>
> Sent: Friday, May 5, 2023 12:47 PM
> To: Moger, Babu <[email protected]>; [email protected];
> [email protected]; [email protected]; [email protected]
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; Das1, Sandipan
> <[email protected]>; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]
> Subject: Re: [PATCH v4 0/7] x86/resctrl: Miscellaneous resctrl features
>
> Hi Babu,
>
> On 5/5/2023 8:43 AM, Moger, Babu wrote:
> > [AMD Official Use Only - General]
> >
> > Hi Reinette,
> >
> >> -----Original Message-----
> >> From: Reinette Chatre <[email protected]>
> >> Sent: Thursday, May 4, 2023 1:54 PM
> >> To: Moger, Babu <[email protected]>; [email protected];
> >> [email protected]; [email protected]; [email protected]
> >> Cc: [email protected]; [email protected];
> >> [email protected]; [email protected]; [email protected];
> >> [email protected]; [email protected];
> >> [email protected]; [email protected];
> >> [email protected]; [email protected];
> [email protected];
> >> [email protected]; [email protected];
> >> [email protected];
> >> [email protected]; [email protected]; Das1, Sandipan
> >> <[email protected]>; [email protected]; [email protected];
> >> [email protected]; [email protected];
> >> [email protected]; [email protected];
> >> [email protected]; [email protected];
> >> [email protected]; [email protected];
> >> [email protected]
> >> Subject: Re: [PATCH v4 0/7] x86/resctrl: Miscellaneous resctrl
> >> features
> >>
>
> ...
>
> >>> 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.
> >>
> >> Could you please elaborate? It actually looks like you may be headed
> >> there next according to:
> >> https://lore.kernel.org/lkml/933d8ae2-d8b7-7436-5918-
> >> [email protected]/
> >
> > Sorry, I was talking about this comment.
> > https://lore.kernel.org/lkml/fef12c9e-7d6f-bcd4-f92e-e776eb9e673b@inte
> > l.com/
> >
> > I tried to address it here.
> > https://lore.kernel.org/lkml/168177451010.1758847.568218491528297451.s
> > tgit@bmoger-ubuntu/
> >
> > This may not be the exact way you mentioned. Reason is, adding the
> > flags before rdtgroup_add_files cannot be done. It does not update the
> > resctrl root filesystem. These files have to added by calling
> > rdtgroup_add_file and kernfs_activate in rdt_enable_ctx.
> I think what you are referring to is files not appearing in the default resource
> group? From what I can tell the files should appear when new resource groups
> are created. Could the default resource group's files not also be added on
> resctrl mount (moved from rdtgroup_setup_root() to rdt_get_tree())?
Yes. I can try that.
> I do not see why the files belonging to the default group are required to be
> added before resctrl mount and with the move the resctrl_debug flag can
> continue to be set in rdt_enable_ctx() and available to rdtgroup_add_files()
> when adding the default resource group's files.
>
> To me this sounds simpler since there is no duplicate file add/remove code, and
> the debug files are just another file type.
Yes. If it works then It will make code simple. Will try and let you know.
Thanks
Babu
[AMD Official Use Only - General]
Hi Reinette,
> -----Original Message-----
> From: Reinette Chatre <[email protected]>
> Sent: Thursday, May 4, 2023 1:59 PM
> To: Moger, Babu <[email protected]>; [email protected];
> [email protected]; [email protected]; [email protected]
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; Das1, Sandipan
> <[email protected]>; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]
> Subject: Re: [PATCH v4 2/7] x86/resctrl: Remove unnecessary rftype flags
>
> Hi Babu,
>
> On 4/17/2023 4:34 PM, Babu Moger wrote:
> > Remove unnecessary rftype flags to avoid multiple indirection.
>
> Could you please elaborate what makes these flags unnecessary?
Yea. Probably should not say unnecessary.
How about this ?
"rftype flags have two prefixes, RFTYPE_ and RF_. Remove the flag names with suffix RF_ and avoid indirection."
Thanks
Babu
Hi Babu,
On 5/5/2023 11:31 AM, Moger, Babu wrote:
> [AMD Official Use Only - General]
>
> Hi Reinette,
>
>> -----Original Message-----
>> From: Reinette Chatre <[email protected]>
>> Sent: Thursday, May 4, 2023 1:59 PM
>> To: Moger, Babu <[email protected]>; [email protected];
>> [email protected]; [email protected]; [email protected]
>> Cc: [email protected]; [email protected]; [email protected];
>> [email protected]; [email protected]; [email protected];
>> [email protected]; [email protected];
>> [email protected]; [email protected];
>> [email protected]; [email protected]; [email protected];
>> [email protected]; [email protected];
>> [email protected]; [email protected]; Das1, Sandipan
>> <[email protected]>; [email protected]; [email protected];
>> [email protected]; [email protected];
>> [email protected]; [email protected]; [email protected];
>> [email protected]; [email protected]; [email protected];
>> [email protected]
>> Subject: Re: [PATCH v4 2/7] x86/resctrl: Remove unnecessary rftype flags
>>
>> Hi Babu,
>>
>> On 4/17/2023 4:34 PM, Babu Moger wrote:
>>> Remove unnecessary rftype flags to avoid multiple indirection.
>>
>> Could you please elaborate what makes these flags unnecessary?
>
> Yea. Probably should not say unnecessary.
> How about this ?
> "rftype flags have two prefixes, RFTYPE_ and RF_. Remove the flag names with suffix RF_ and avoid indirection."
I do not think that having a different prefix is a motivation
for a flag to be removed. Having a different prefix could
rather be a motivation for a flag to be renamed to a consistent
name.
Could you please describe the problem being solved?
That will help to clarify if this patch is really needed.
Reinette
Hi Babu,
On 5/5/2023 10:09 AM, Moger, Babu wrote:
> [AMD Official Use Only - General]
>
> Hi Reinette,
>
>> -----Original Message-----
>> From: Reinette Chatre <[email protected]>
>> Sent: Thursday, May 4, 2023 1:58 PM
>> To: Moger, Babu <[email protected]>; [email protected];
>> [email protected]; [email protected]; [email protected]
>> Cc: [email protected]; [email protected]; [email protected];
>> [email protected]; [email protected]; [email protected];
>> [email protected]; [email protected];
>> [email protected]; [email protected];
>> [email protected]; [email protected]; [email protected];
>> [email protected]; [email protected];
>> [email protected]; [email protected]; Das1, Sandipan
>> <[email protected]>; [email protected]; [email protected];
>> [email protected]; [email protected];
>> [email protected]; [email protected]; [email protected];
>> [email protected]; [email protected]; [email protected];
>> [email protected]
>> Subject: Re: [PATCH v4 1/7] x86/resctrl: Add multiple tasks to the resctrl group
>> at once
>>
>> Hi Babu,
>>
>> On 4/17/2023 4:34 PM, Babu Moger wrote:
>>> The resctrl task assignment for MONITOR or CONTROL group needs to be
>>> done one at a time. For example:
>>
>> Why all caps for monitor and control? If the intention is to use the terms for
>> these groups then it may be easier to use the same terms as in the
>> documentation, or you could just not use all caps like you do in later patches.
>
> Sure.
>>
>>>
>>> $mount -t resctrl resctrl /sys/fs/resctrl/
>>> $mkdir /sys/fs/resctrl/clos1
>>> $echo 123 > /sys/fs/resctrl/clos1/tasks
>>> $echo 456 > /sys/fs/resctrl/clos1/tasks
>>> $echo 789 > /sys/fs/resctrl/clos1/tasks
>>>
>>> This is not user-friendly when dealing with hundreds of tasks.
>>>
>>> It can be improved by supporting the multiple task id assignment in
>>> one command with the tasks separated by commas. For example:
>>
>> Please use imperative mood (see Documentation/process/maintainer-tip.rst).
>>
>> Something like:
>> "Improve multiple task id assignment ...."
>
> How about:
> "Improve the assignment by supporting multiple task id assignment in
> one command with the tasks separated by commas."
The double use of 'assignment' can be confusing. This is also a
changelog where a clear context->problem->solution format can help.
If your changelog is clear regarding the context and problem then it
can end with brief solution description like:
"Support multiple task assignment in one command with tasks ids separated
by commas. For example: " (and also please use a non-x86 term for the group
name in your example)
>>> $echo 123,456,789 > /sys/fs/resctrl/clos1/tasks
>>>
...
>>> + pid will be logged in /sys/fs/resctrl/info/last_cmd_status file.
>>
>> Would it not always print the failing pid (if error was encountered while
>
> Not always. In this case it does not print the pid,
> rdt_last_cmd_puts("Can't move task to different control group\n");
> return -EINVAL;
>
What you quote above adds the relevant text to the last_cmd_status buffer ...
and later (see below) more text is added to the buffer that contains the
pid, no?
...
>>> struct rdtgroup *rdtgrp;
>>> + char *pid_str;
>>> int ret = 0;
>>> pid_t pid;
>>>
>>> - if (kstrtoint(strstrip(buf), 0, &pid) || pid < 0)
>>> + if (nbytes == 0)
>>> return -EINVAL;
>>> +
>>> + buf[nbytes - 1] = '\0';
>>> +
>>
>> This seems like another remnant of the schemata write code that expects that
>> the buffer ends with a '\n'. Since this code does not have this requirement the
>> above may have unintended consequences if a tool provides a buffer that does
>> not end with '\n'.
>> I think you just want to ensure that the buffer is properly terminated and from
>> what I understand when looking at kernfs_fop_write_iter() this is already taken
>> care of.
>
> Sure. Will check. Then I will have to change the check below to if (!buf).
Please check what kernfs_fop_write_iter() does. From what I can tell it does
exactly what you are trying to do above, but without overwriting
part of the string that user space provides.
I thus do not think that the later check needs to change. From what I understand
it is used to handle the scenario if user space provides a string like "pid,"
(last character is the separator). Please do confirm that the code can handle
any variations that user space may throw at it.
>>> @@ -716,6 +739,12 @@ static ssize_t rdtgroup_tasks_write(struct
>> kernfs_open_file *of,
>>> }
>>>
>>> ret = rdtgroup_move_task(pid, rdtgrp, of);
>>> + if (ret) {
>>> + rdt_last_cmd_printf("Error while processing task %d\n", pid);
Note here the pid is added to the buffer that is printed when user space
views last_cmd_status. I think this is the first time two lines of error may
be added to the buffer so you could double check all works as expected.
Reinette
[AMD Official Use Only - General]
Hi Reinette,
> -----Original Message-----
> From: Reinette Chatre <[email protected]>
> Sent: Friday, May 5, 2023 1:54 PM
> To: Moger, Babu <[email protected]>; [email protected];
> [email protected]; [email protected]; [email protected]
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; Das1, Sandipan
> <[email protected]>; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]
> Subject: Re: [PATCH v4 2/7] x86/resctrl: Remove unnecessary rftype flags
>
> Hi Babu,
>
> On 5/5/2023 11:31 AM, Moger, Babu wrote:
> > [AMD Official Use Only - General]
> >
> > Hi Reinette,
> >
> >> -----Original Message-----
> >> From: Reinette Chatre <[email protected]>
> >> Sent: Thursday, May 4, 2023 1:59 PM
> >> To: Moger, Babu <[email protected]>; [email protected];
> >> [email protected]; [email protected]; [email protected]
> >> Cc: [email protected]; [email protected];
> >> [email protected]; [email protected]; [email protected];
> >> [email protected]; [email protected];
> >> [email protected]; [email protected];
> >> [email protected]; [email protected];
> [email protected];
> >> [email protected]; [email protected];
> >> [email protected];
> >> [email protected]; [email protected]; Das1, Sandipan
> >> <[email protected]>; [email protected]; [email protected];
> >> [email protected]; [email protected];
> >> [email protected]; [email protected];
> >> [email protected]; [email protected];
> >> [email protected]; [email protected];
> >> [email protected]
> >> Subject: Re: [PATCH v4 2/7] x86/resctrl: Remove unnecessary rftype
> >> flags
> >>
> >> Hi Babu,
> >>
> >> On 4/17/2023 4:34 PM, Babu Moger wrote:
> >>> Remove unnecessary rftype flags to avoid multiple indirection.
> >>
> >> Could you please elaborate what makes these flags unnecessary?
> >
> > Yea. Probably should not say unnecessary.
> > How about this ?
> > "rftype flags have two prefixes, RFTYPE_ and RF_. Remove the flag names
> with suffix RF_ and avoid indirection."
>
> I do not think that having a different prefix is a motivation for a flag to be
> removed. Having a different prefix could rather be a motivation for a flag to be
> renamed to a consistent name.
>
> Could you please describe the problem being solved?
Motivation for these changes.
1. Remove the naming inconsistency
2. Remove the indirection
3. Make it easier for adding new flags
Thanks
Babu
> That will help to clarify if this patch is really needed.
>
> Reinette
Hi Reinette,
On 5/4/2023 2:00 PM, Reinette Chatre wrote:
> Hi Babu,
>
> On 4/17/2023 4:34 PM, Babu Moger wrote:
>> Remove gaps in bit definitions of RFTYPE flags and add more comments
> Why is it necessary to remove gaps in the bit definitions?
Removing the gaps is not necessary definitely. I thought adding comments
will help adding new flags in the future.
If you want me to drop this whole patch, I am fine with it.
Thanks
Babu
On 5/4/2023 2:02 PM, Reinette Chatre wrote:
> Hi Babu,
>
> On 4/17/2023 4:34 PM, Babu Moger wrote:
>> Add "-o debug" option to mount resctrl filesystem in debug mode.
>> Debug option adds the files for debug purposes.
>>
> Could this changelog please be expanded to explain what "debug mode"
> is, why is it necessary, and what will it be used for?
>
> The changelog mentions "adds the files for debug purposes" but does
> not explain what is meant by "the files" nor what these
> files may look like or what "debug purposes" may be.
Sure. Will add the more details.
Thanks
Babu
Hi Babu,
On 5/5/2023 1:40 PM, Moger, Babu wrote:
> Hi Reinette,
>
> On 5/4/2023 2:00 PM, Reinette Chatre wrote:
>> Hi Babu,
>>
>> On 4/17/2023 4:34 PM, Babu Moger wrote:
>>> Remove gaps in bit definitions of RFTYPE flags and add more comments
>> Why is it necessary to remove gaps in the bit definitions?
>
> Removing the gaps is not necessary definitely. I thought adding
> comments will help adding new flags in the future.
>
I agree that removing the gaps are not necessary.
> If you want me to drop this whole patch, I am fine with it.>
The comments may be useful. If you decide to keep it please review
it for consistency. The comments should not increase confusion.
For example,
* in one instance you refer to "info" and "base" as components, in
another you refer to them as directories, which is confusing since
there is a "info" directory but no "base" directory.
* related to previous item, the comments start by referring to the
"info" and "base" components but then the comments switch to
describing a "info directory structure and "group structure"
* the separator (---) is used above a header in one instance and
below a header in another
* in some places you use the syntax:
--> <flag name> (<dir name>, <dir name>)
in other places you use:
--> <flag name>
--> (<dir name>)
--> (<dir name>)
Reinette
Hi Babu,
On 5/5/2023 12:04 PM, Moger, Babu wrote:
> [AMD Official Use Only - General]
>
> Hi Reinette,
>
>> -----Original Message-----
>> From: Reinette Chatre <[email protected]>
>> Sent: Friday, May 5, 2023 1:54 PM
>> To: Moger, Babu <[email protected]>; [email protected];
>> [email protected]; [email protected]; [email protected]
>> Cc: [email protected]; [email protected]; [email protected];
>> [email protected]; [email protected]; [email protected];
>> [email protected]; [email protected];
>> [email protected]; [email protected];
>> [email protected]; [email protected]; [email protected];
>> [email protected]; [email protected];
>> [email protected]; [email protected]; Das1, Sandipan
>> <[email protected]>; [email protected]; [email protected];
>> [email protected]; [email protected];
>> [email protected]; [email protected]; [email protected];
>> [email protected]; [email protected]; [email protected];
>> [email protected]
>> Subject: Re: [PATCH v4 2/7] x86/resctrl: Remove unnecessary rftype flags
(is it possible to trim the headers in your replies?)
...
>>>> On 4/17/2023 4:34 PM, Babu Moger wrote:
>>>>> Remove unnecessary rftype flags to avoid multiple indirection.
>>>>
>>>> Could you please elaborate what makes these flags unnecessary?
>>>
>>> Yea. Probably should not say unnecessary.
>>> How about this ?
>>> "rftype flags have two prefixes, RFTYPE_ and RF_. Remove the flag names
>> with suffix RF_ and avoid indirection."
>>
>> I do not think that having a different prefix is a motivation for a flag to be
>> removed. Having a different prefix could rather be a motivation for a flag to be
>> renamed to a consistent name.
>>
>> Could you please describe the problem being solved?
>
> Motivation for these changes.
> 1. Remove the naming inconsistency
> 2. Remove the indirection
> 3. Make it easier for adding new flags
>
Could you please create a coherent changelog that follows the guidance
of the maintainer doc? Some details about why the indirection needs to be
removed and how this makes it easier to add new flags?
Reinette
Hi Babu,
On 5/5/2023 1:16 PM, Moger, Babu wrote:
> Hi Reinette,
>
> On 5/4/2023 2:00 PM, Reinette Chatre wrote:
>> Hi Babu,
>>
>> On 4/17/2023 4:34 PM, Babu Moger wrote:
>>> The rftype flags have two different prefixes even though they are used
>> Could this changelog please start with some context about what the
>> rftype flags are? Please note that the changelogs are required
>> to comply to the x86 requirements as documented in:
>> Documentation/process/maintainer-tip.rst (there is a whole
>> section on changelogs).
> Thanks
>>
>> After a context the problem could be made specific with something
>> like "rftype flags use the RF_ as well as RFTYPE_ prefixes."
> I can say start with "rftype flags are used for creating the files
> under resctrl filesystem. These flags use the RF_ as well as RFTYPE_
> prefixes.
>
> Change the prefix to RFTYPE_ for all the flags to be consistent.">
I would be careful with the term "rftype flags" since it is
not clear what is meant by this. Note that there are RF_*, RFTYPE_*,
as well as RFTYPE_FLAGS* flags and the reader may easily think that
you refer to the latter.
"resctrl associates flags with its files so that files can be chosen
based on the resource, whether it is info or base, and if it is control
or monitor type file. These flags use the RF_ as well as RFTYPE_ prefixes.
Change ..."
(A shortcut I used was to look up the commit that introduced the change
and borrow some text from it.)
Reinette
Hi Reinette,
On 5/4/2023 2:04 PM, Reinette Chatre wrote:
> Hi Babu,
>
> On 4/17/2023 4:34 PM, Babu Moger wrote:
>> When a user creates a control or monitor group, the CLOSID or RMID
>> are not visible to the user. It can help to debug the issues in some
>> cases. There are only available with "-o debug" option.
> Please see: Documentation/process/maintainer-tip.rst
>
> "It's also useful to structure the changelog into several paragraphs and not
> lump everything together into a single one. A good structure is to explain
> the context, the problem and the solution in separate paragraphs and this
> order."
ok Sure.
>> Add CLOSID(ctrl_hw_id) and RMID(mon_hw_id) to the control/monitor groups
> Please highlight that CLOSID and RMID are x86 concepts.
ok Sure.
>
>> display in resctrl interface.
>> $cat /sys/fs/resctrl/clos1/clos_hw_id
>> 1
> This example does not match what the patch does (clos_hw_id -> ctrl_hw_id).
My bad. Will fix it.
> I also think this change would be more palatable (to non x86 audience) if
> the example resource group has a generic (non-x86 concept) name.
ok. In this example the clos1 name sounds x86 specific. I can change it
to ctrl_grp1. Hope this is what you meant.
>
>> $cat /sys/fs/resctrl/mon_groups/mon1/mon_hw_id
>> 3
>>
>> Signed-off-by: Babu Moger <[email protected]>
>> ---
>> Documentation/x86/resctrl.rst | 17 ++++++++++++
>> arch/x86/kernel/cpu/resctrl/rdtgroup.c | 44 ++++++++++++++++++++++++++++++++
>> 2 files changed, 61 insertions(+)
>>
>> diff --git a/Documentation/x86/resctrl.rst b/Documentation/x86/resctrl.rst
>> index be443251b484..5aff8c2beb08 100644
>> --- a/Documentation/x86/resctrl.rst
>> +++ b/Documentation/x86/resctrl.rst
>> @@ -345,6 +345,14 @@ 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. On x86, reading this file shows
>> + the Class of Service (CLOS) id which acts as a resource control
>> + tag on which the resources can be throttled. Kernel assigns a new
>> + CLOSID a control group is created depending on the available
>> + CLOSIDs. Multiple cores(or threads) or processes can share a
>> + same CLOSID in a resctrl group.
> Please keep other content from the documentation in mind when making
> this change. CLOSID is already documented, including the fact that it
> is a limited resource. Please see content under: "Notes on cache occupancy
> monitoring and control" where it, for example, states that "The number
> of CLOSid and RMID are limited by the hardware."
>
> Considering this the above could just read:
> "Available only with debug option. The identifier used by hardware
> for the control group. On x86 this is the CLOSID."
ok. Sure.
>
> Similar feedback to the "mon_hw_id" portion.
Sure.
Thanks
Babu
Hi Babu,
On 5/5/2023 2:45 PM, Moger, Babu wrote:
> On 5/4/2023 2:04 PM, Reinette Chatre wrote:
Thank you for trimming the header in replies.
>> On 4/17/2023 4:34 PM, Babu Moger wrote:
>>> When a user creates a control or monitor group, the CLOSID or RMID
>>> are not visible to the user. It can help to debug the issues in some
>>> cases. There are only available with "-o debug" option.
>> Please see: Documentation/process/maintainer-tip.rst
>>
>> "It's also useful to structure the changelog into several paragraphs and not
>> lump everything together into a single one. A good structure is to explain
>> the context, the problem and the solution in separate paragraphs and this
>> order."
> ok Sure.
>>> Add CLOSID(ctrl_hw_id) and RMID(mon_hw_id) to the control/monitor groups
>> Please highlight that CLOSID and RMID are x86 concepts.
> ok Sure.
>>
>>> display in resctrl interface.
>>> $cat /sys/fs/resctrl/clos1/clos_hw_id
>>> 1
>> This example does not match what the patch does (clos_hw_id -> ctrl_hw_id).
> My bad. Will fix it.
>> I also think this change would be more palatable (to non x86 audience) if
>> the example resource group has a generic (non-x86 concept) name.
>
> ok. In this example the clos1 name sounds x86 specific. I can change it to ctrl_grp1. Hope this is what you meant.
Yes, that is what I meant. ctrl_grp1 sounds good.
Thank you
Reinette
Hi Reinette,
On 5/5/23 13:49, Reinette Chatre wrote:
> Hi Babu,
>
> On 5/5/2023 10:09 AM, Moger, Babu wrote:
>> [AMD Official Use Only - General]
>>
>> Hi Reinette,
>>
>>> -----Original Message-----
>>> From: Reinette Chatre <[email protected]>
>>> Sent: Thursday, May 4, 2023 1:58 PM
>>> To: Moger, Babu <[email protected]>; [email protected];
>>> [email protected]; [email protected]; [email protected]
>>> Cc: [email protected]; [email protected]; [email protected];
>>> [email protected]; [email protected]; [email protected];
>>> [email protected]; [email protected];
>>> [email protected]; [email protected];
>>> [email protected]; [email protected]; [email protected];
>>> [email protected]; [email protected];
>>> [email protected]; [email protected]; Das1, Sandipan
>>> <[email protected]>; [email protected]; [email protected];
>>> [email protected]; [email protected];
>>> [email protected]; [email protected]; [email protected];
>>> [email protected]; [email protected]; [email protected];
>>> [email protected]
>>> Subject: Re: [PATCH v4 1/7] x86/resctrl: Add multiple tasks to the resctrl group
>>> at once
>>>
>>> Hi Babu,
>>>
>>> On 4/17/2023 4:34 PM, Babu Moger wrote:
>>>> The resctrl task assignment for MONITOR or CONTROL group needs to be
>>>> done one at a time. For example:
>>>
>>> Why all caps for monitor and control? If the intention is to use the terms for
>>> these groups then it may be easier to use the same terms as in the
>>> documentation, or you could just not use all caps like you do in later patches.
>>
>> Sure.
>>>
>>>>
>>>> $mount -t resctrl resctrl /sys/fs/resctrl/
>>>> $mkdir /sys/fs/resctrl/clos1
>>>> $echo 123 > /sys/fs/resctrl/clos1/tasks
>>>> $echo 456 > /sys/fs/resctrl/clos1/tasks
>>>> $echo 789 > /sys/fs/resctrl/clos1/tasks
>>>>
>>>> This is not user-friendly when dealing with hundreds of tasks.
>>>>
>>>> It can be improved by supporting the multiple task id assignment in
>>>> one command with the tasks separated by commas. For example:
>>>
>>> Please use imperative mood (see Documentation/process/maintainer-tip.rst).
>>>
>>> Something like:
>>> "Improve multiple task id assignment ...."
>>
>> How about:
>> "Improve the assignment by supporting multiple task id assignment in
>> one command with the tasks separated by commas."
>
> The double use of 'assignment' can be confusing. This is also a
> changelog where a clear context->problem->solution format can help.
> If your changelog is clear regarding the context and problem then it
> can end with brief solution description like:
>
> "Support multiple task assignment in one command with tasks ids separated
> by commas. For example: " (and also please use a non-x86 term for the group
> name in your example)
Sure.
>
>>>> $echo 123,456,789 > /sys/fs/resctrl/clos1/tasks
>>>>
>
> ...
>
>>>> + pid will be logged in /sys/fs/resctrl/info/last_cmd_status file.
>>>
>>> Would it not always print the failing pid (if error was encountered while
>>
>> Not always. In this case it does not print the pid,
>> rdt_last_cmd_puts("Can't move task to different control group\n");
>> return -EINVAL;
>>
>
> What you quote above adds the relevant text to the last_cmd_status buffer ...
> and later (see below) more text is added to the buffer that contains the
> pid, no?
Yes. That is correct.
>
> ...
>
>>>> struct rdtgroup *rdtgrp;
>>>> + char *pid_str;
>>>> int ret = 0;
>>>> pid_t pid;
>>>>
>>>> - if (kstrtoint(strstrip(buf), 0, &pid) || pid < 0)
>>>> + if (nbytes == 0)
>>>> return -EINVAL;
>>>> +
>>>> + buf[nbytes - 1] = '\0';
>>>> +
>>>
>>> This seems like another remnant of the schemata write code that expects that
>>> the buffer ends with a '\n'. Since this code does not have this requirement the
>>> above may have unintended consequences if a tool provides a buffer that does
>>> not end with '\n'.
>>> I think you just want to ensure that the buffer is properly terminated and from
>>> what I understand when looking at kernfs_fop_write_iter() this is already taken
>>> care of.
>>
>> Sure. Will check. Then I will have to change the check below to if (!buf).
>
> Please check what kernfs_fop_write_iter() does. From what I can tell it does
> exactly what you are trying to do above, but without overwriting
> part of the string that user space provides.
> I thus do not think that the later check needs to change. From what I understand
> it is used to handle the scenario if user space provides a string like "pid,"
> (last character is the separator). Please do confirm that the code can handle
> any variations that user space may throw at it.
Sure. Thanks
Babu
>
>>>> @@ -716,6 +739,12 @@ static ssize_t rdtgroup_tasks_write(struct
>>> kernfs_open_file *of,
>>>> }
>>>>
>>>> ret = rdtgroup_move_task(pid, rdtgrp, of);
>>>> + if (ret) {
>>>> + rdt_last_cmd_printf("Error while processing task %d\n", pid);
>
> Note here the pid is added to the buffer that is printed when user space
> views last_cmd_status. I think this is the first time two lines of error may
> be added to the buffer so you could double check all works as expected.
>
> Reinette
--
Thanks
Babu Moger
On 5/5/23 16:24, Reinette Chatre wrote:
> Hi Babu,
>
> On 5/5/2023 1:16 PM, Moger, Babu wrote:
>> Hi Reinette,
>>
>> On 5/4/2023 2:00 PM, Reinette Chatre wrote:
>>> Hi Babu,
>>>
>>> On 4/17/2023 4:34 PM, Babu Moger wrote:
>>>> The rftype flags have two different prefixes even though they are used
>>> Could this changelog please start with some context about what the
>>> rftype flags are? Please note that the changelogs are required
>>> to comply to the x86 requirements as documented in:
>>> Documentation/process/maintainer-tip.rst (there is a whole
>>> section on changelogs).
>> Thanks
>>>
>>> After a context the problem could be made specific with something
>>> like "rftype flags use the RF_ as well as RFTYPE_ prefixes."
>
>> I can say start with "rftype flags are used for creating the files
>> under resctrl filesystem. These flags use the RF_ as well as RFTYPE_
>> prefixes.
>>
>> Change the prefix to RFTYPE_ for all the flags to be consistent.">
>
> I would be careful with the term "rftype flags" since it is
> not clear what is meant by this. Note that there are RF_*, RFTYPE_*,
> as well as RFTYPE_FLAGS* flags and the reader may easily think that
> you refer to the latter.
>
> "resctrl associates flags with its files so that files can be chosen
> based on the resource, whether it is info or base, and if it is control
> or monitor type file. These flags use the RF_ as well as RFTYPE_ prefixes.
>
> Change ..."
Will change it.
>
> (A shortcut I used was to look up the commit that introduced the change
> and borrow some text from it.)
Sure.
Thanks
Babu Moger
Hi Reinette,
On 5/5/23 16:24, Reinette Chatre wrote:
> Hi Babu,
>
> On 5/5/2023 1:40 PM, Moger, Babu wrote:
>> Hi Reinette,
>>
>> On 5/4/2023 2:00 PM, Reinette Chatre wrote:
>>> Hi Babu,
>>>
>>> On 4/17/2023 4:34 PM, Babu Moger wrote:
>>>> Remove gaps in bit definitions of RFTYPE flags and add more comments
>>> Why is it necessary to remove gaps in the bit definitions?
>>
>> Removing the gaps is not necessary definitely. I thought adding
>> comments will help adding new flags in the future.
>>
>
> I agree that removing the gaps are not necessary.
ok.
>
>> If you want me to drop this whole patch, I am fine with it.>
>
> The comments may be useful. If you decide to keep it please review
> it for consistency. The comments should not increase confusion.
> For example,
> * in one instance you refer to "info" and "base" as components, in
> another you refer to them as directories, which is confusing since
> there is a "info" directory but no "base" directory.
> * related to previous item, the comments start by referring to the
> "info" and "base" components but then the comments switch to
> describing a "info directory structure and "group structure"
> * the separator (---) is used above a header in one instance and
> below a header in another
> * in some places you use the syntax:
> --> <flag name> (<dir name>, <dir name>)
> in other places you use:
> --> <flag name>
> --> (<dir name>)
> --> (<dir name>)
>
>
sure. Will address this next revision.
Thanks
Babu
Hi Reinette,
On 5/5/23 16:28, Reinette Chatre wrote:
> Hi Babu,
>
> On 5/5/2023 12:04 PM, Moger, Babu wrote:
>> [AMD Official Use Only - General]
>>
>> Hi Reinette,
>>
>>> -----Original Message-----
>>> From: Reinette Chatre <[email protected]>
>>> Sent: Friday, May 5, 2023 1:54 PM
>>> To: Moger, Babu <[email protected]>; [email protected];
>>> [email protected]; [email protected]; [email protected]
>>> Cc: [email protected]; [email protected]; [email protected];
>>> [email protected]; [email protected]; [email protected];
>>> [email protected]; [email protected];
>>> [email protected]; [email protected];
>>> [email protected]; [email protected]; [email protected];
>>> [email protected]; [email protected];
>>> [email protected]; [email protected]; Das1, Sandipan
>>> <[email protected]>; [email protected]; [email protected];
>>> [email protected]; [email protected];
>>> [email protected]; [email protected]; [email protected];
>>> [email protected]; [email protected]; [email protected];
>>> [email protected]
>>> Subject: Re: [PATCH v4 2/7] x86/resctrl: Remove unnecessary rftype flags
>
> (is it possible to trim the headers in your replies?)
>
You mean Company Header.. Sure.. Will do.
> ...
>
>>>>> On 4/17/2023 4:34 PM, Babu Moger wrote:
>>>>>> Remove unnecessary rftype flags to avoid multiple indirection.
>>>>>
>>>>> Could you please elaborate what makes these flags unnecessary?
>>>>
>>>> Yea. Probably should not say unnecessary.
>>>> How about this ?
>>>> "rftype flags have two prefixes, RFTYPE_ and RF_. Remove the flag names
>>> with suffix RF_ and avoid indirection."
>>>
>>> I do not think that having a different prefix is a motivation for a flag to be
>>> removed. Having a different prefix could rather be a motivation for a flag to be
>>> renamed to a consistent name.
>>>
>>> Could you please describe the problem being solved?
>>
>> Motivation for these changes.
>> 1. Remove the naming inconsistency
>> 2. Remove the indirection
>> 3. Make it easier for adding new flags
>>
>
> Could you please create a coherent changelog that follows the guidance
> of the maintainer doc? Some details about why the indirection needs to be
> removed and how this makes it easier to add new flags?
Sure. Will try to address it next revision.
--
Thanks
Babu Moger
On 5/5/23 16:28, Reinette Chatre wrote:
>
> (is it possible to trim the headers in your replies?)
Sure. Got it. Thanks
Babu
Hi Babu,
> 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. While doing these above changes, found that rftype flags needed some
> cleanup.
> They were named inconsistently. Re-arranged them much more cleanly
> now.
> Hope it can help future additions.
I tested these features and ran the selftests/resctrl test set,
it seems to be fine.
Maybe the next version will come soon with some fixes,
I will test it again.
<Tested-by:[email protected]>
Best regards,
Shaopeng TAN