Re: [PATCH v6 06/18] fs/resctrl: Introduce kernel mode states for resctrl
From: Moger, Babu
Date: Fri Oct 16 2026 - 11:17:39 EST
Hi Reinette,
On 10/15/2026 9:37 AM, Reinette Chatre wrote:
Hi Babu,
nit: "for resctrl" in subject is redundant.
ok.
On 10/1/26 8:10 AM, Babu Moger wrote:
The filesystem tracks supported kernel modes, the active mode, and the
control and monitoring association for the assigned group.
Please let the context describe the context at the time of the patch, not how
things will work after entire series is applied.
Please use consistent terminology, this series switches between "assigned group"
and "backing group" to refer to the same group.
Will use the "backing group". That seems more clearer.
Ack.
Please use the changelog to guide the reader to understand the architecture
and how things fit together. Many changelogs are very cryptic without explanation
of what the patch does and why.
Introduce struct resctrl_kmode_cfg and initialize the default configuration
from the platform's capabilities.
Please check all changelogs to ensure they all are able to provide an
answer to "why". More specifically, please consider:
https://lore.kernel.org/lkml/20250911165433.GBaML-yTUZHkywuJIe@fat_crate.local/
Ack
...
Signed-off-by: Babu Moger <babu.moger@xxxxxxx>
---
---
fs/resctrl/internal.h | 29 +++++++++++++++++++++++++++++
fs/resctrl/rdtgroup.c | 25 +++++++++++++++++++++++++
2 files changed, 54 insertions(+)
diff --git a/fs/resctrl/internal.h b/fs/resctrl/internal.h
index e62a277dee85..98f04a2a762b 100644
--- a/fs/resctrl/internal.h
+++ b/fs/resctrl/internal.h
@@ -314,6 +314,35 @@ struct mbm_state {
u32 prev_bw;
};
+/**
+ * enum kmode_state - Control or monitoring association
What does "control or monitoring association" mean? This is becoming
an overloaded term used to mean different things and becoming confusing.
For comparison, earlier patch introduced *another*
"control and monitoring association" enum:
enum resctrl_kernel_mode - Kernel mode control and monitoring association
Sure.
+ * @KMODE_INHERIT: Inherit from the user-space task.
+ * @KMODE_ASSIGN: Use the backing group's CLOSID or RMID.
+ */
+enum kmode_state {
+ KMODE_INHERIT,
+ KMODE_ASSIGN
+};
+
+/**
+ * struct resctrl_kmode_cfg - Kernel mode configuration
+ * @kmode_sup: Bitmap of supported kernel modes.
+ * @kmode_cur: Currently selected kernel mode.
Adding this comment during a second pass ... please consider later motivations
about naming of these fields. This field is introduced as "kmode_cur" but when
considering the language used in rest of series "active" seems more appropriate.
@kmode_sup could also just be @supported (the "kmode" is redundant).
Sure. Will change kmode_cur to "current".
+ * @ctrl: Control association for %RESCTRL_GLOBAL_ENABLE_PER_CPU.
+ * Kept while %RESCTRL_INHERIT_USER is selected.
The "Kept while ..." does not seem to match implementation. Looks like
resctrl_kmode_switch() always resets it to KMODE_ASSIGN?
Yes. Will remove the line.
At this point the reader is left to decipher the architecture via cryptic
changelogs and inaccurate comments. It is very difficult to parse.
+ * @mon: Monitoring association for %RESCTRL_GLOBAL_ENABLE_PER_CPU.
+ * Kept while %RESCTRL_INHERIT_USER is selected.
Similar comment about "Kept ..."
Yes. Will remove the text.
+ * @rdtgrp: Resource group backing %RESCTRL_GLOBAL_ENABLE_PER_CPU.
+ * %NULL while that mode is not selected.
+ */
+struct resctrl_kmode_cfg {
+ DECLARE_BITMAP(kmode_sup, RESCTRL_NUM_KERNEL_MODES);
+ enum resctrl_kernel_mode kmode_cur;
+ enum kmode_state ctrl;
+ enum kmode_state mon;
+ struct rdtgroup *rdtgrp;
+};
+
extern struct mutex rdtgroup_mutex;
static inline const char *rdt_kn_name(const struct kernfs_node *kn)
diff --git a/fs/resctrl/rdtgroup.c b/fs/resctrl/rdtgroup.c
index 68be9b903ac6..7c39792be1fa 100644
--- a/fs/resctrl/rdtgroup.c
+++ b/fs/resctrl/rdtgroup.c
@@ -76,6 +76,9 @@ static void rdtgroup_destroy_root(void);
struct dentry *debugfs_resctrl;
+/* Kernel mode for this system. */
+static struct resctrl_kmode_cfg resctrl_kmode;
+
/*
* Memory bandwidth monitoring event to use for the default CTRL_MON group
* and each new CTRL_MON group created by the user. Only relevant when
@@ -2297,6 +2300,26 @@ static void io_alloc_init(void)
}
}
+/*
+ * Set RESCTRL_INHERIT_USER as the default when the platform supports
+ * allocation or monitoring. Otherwise leave kernel mode unsupported.
This (and why) would be useful to describe in changelog.
Ack.
+ */
+static void resctrl_kmode_init(void)
+{
+ if (!resctrl_arch_alloc_capable() && !resctrl_arch_mon_capable())
+ return;
+
This check seems unnecessary? resctrl_kmode_init() will only be called
on a system that is capable of allocation or monitoring.
The planned AET changes will change this behavior see
https://lore.kernel.org/lkml/20260928221509.68002-16-tony.luck@xxxxxxxxx/
but in that case it still seems as though initializing the kernel mode as
below always needs to be done.
That begs the question why is resctrl_kmode_init() needed at all? Why not use
a static initializer?
Sure. Will do.
I find resctrl_kmode_init() to be potentially confusing since it does *not*
actually initialize resctrl_kmode. This is only possible to see two patches
later where get_rdt_resources() is changed so that x86 sets its supported
modes and this runs *before* resctrl_kmode_init().
If anyone decides one day "this initialization code is missing a bitmap_zero()"
then it will break PLZA.
Sounds good. Will remove resctrl_kmode_init() and statically initialize resctrl_kmode.
Thanks
Babu