[PATCH v18 26/33] fs/resctrl: Introduce mbm_assign_on_mkdir to enable assignments on mkdir

Babu Moger posted 33 patches 5 months ago
[PATCH v18 26/33] fs/resctrl: Introduce mbm_assign_on_mkdir to enable assignments on mkdir
Posted by Babu Moger 5 months ago
The "mbm_event" counter assignment mode allows users to assign a hardware
counter to an RMID, event pair and monitor the bandwidth as long as it is
assigned.

Introduce a user-configurable option that determines if a counter will
automatically be assigned to an RMID, event pair when its associated
monitor group is created via mkdir. Accessible when "mbm_event" counter
assignment mode is enabled.

Suggested-by: Peter Newman <peternewman@google.com>
Signed-off-by: Babu Moger <babu.moger@amd.com>
Reviewed-by: Reinette Chatre <reinette.chatre@intel.com>
---
v18: Mismatched quote fix in changelog.
     Added Reviewed-by tag.
     Updated the coverletter to add mbm_assign_on_mkdir.

v17: Added the check resctrl_arch_mbm_cntr_assign_enabled() in
     resctrl_mbm_assign_on_mkdir_show() and resctrl_mbm_assign_on_mkdir_write()
     to make it accessible when mbm_event mode is enabled.
     Added texts in resctrl.rst about the accessability.

v16: Fixed the return in resctrl_mbm_assign_on_mkdir_write().

v15: Fixed the static checker warning in resctrl_mbm_assign_on_mkdir_write() reported in
     https://lore.kernel.org/lkml/dd4a1021-b996-438e-941c-69dfcea5f22a@intel.com/

v14: Added rdtgroup_mutex in resctrl_mbm_assign_on_mkdir_show().
     Updated resctrl.rst for clarity.
     Fixed squashing of few previous changes.
     Added more code documentation.

v13: Added Suggested-by tag.
     Resolved conflicts caused by the recent FS/ARCH code restructure.
     The rdtgroup.c/monitor.c file has now been split between the FS and ARCH directories.

v12: New patch. Added after the discussion on the list.
     https://lore.kernel.org/lkml/CALPaoCh8siZKjL_3yvOYGL4cF_n_38KpUFgHVGbQ86nD+Q2_SA@mail.gmail.com/
---
 Documentation/filesystems/resctrl.rst | 20 ++++++++++
 fs/resctrl/internal.h                 |  6 +++
 fs/resctrl/monitor.c                  | 53 +++++++++++++++++++++++++++
 fs/resctrl/rdtgroup.c                 |  7 ++++
 include/linux/resctrl.h               |  3 ++
 5 files changed, 89 insertions(+)

diff --git a/Documentation/filesystems/resctrl.rst b/Documentation/filesystems/resctrl.rst
index 2e840ef26f68..1de815b3a07b 100644
--- a/Documentation/filesystems/resctrl.rst
+++ b/Documentation/filesystems/resctrl.rst
@@ -355,6 +355,26 @@ with the following files:
 	  # cat /sys/fs/resctrl/info/L3_MON/event_configs/mbm_total_bytes/event_filter
 	   local_reads,local_non_temporal_writes
 
+"mbm_assign_on_mkdir":
+	Exists when "mbm_event" counter assignment mode is supported. Accessible
+	only when "mbm_event" counter assignment mode is enabled.
+
+	Determines if a counter will automatically be assigned to an RMID, MBM event
+	pair when its associated monitor group is created via mkdir. Enabled by default
+	on boot, also when switched from "default" mode to "mbm_event" counter assignment
+	mode. Users can disable this capability by writing to the interface.
+
+	"0":
+		Auto assignment is disabled.
+	"1":
+		Auto assignment is enabled.
+
+	Example::
+
+	  # echo 0 > /sys/fs/resctrl/info/L3_MON/mbm_assign_on_mkdir
+	  # cat /sys/fs/resctrl/info/L3_MON/mbm_assign_on_mkdir
+	  0
+
 "max_threshold_occupancy":
 		Read/write file provides the largest value (in
 		bytes) at which a previously used LLC_occupancy
diff --git a/fs/resctrl/internal.h b/fs/resctrl/internal.h
index 5956570d49fc..9be1e53a73d3 100644
--- a/fs/resctrl/internal.h
+++ b/fs/resctrl/internal.h
@@ -410,6 +410,12 @@ int event_filter_show(struct kernfs_open_file *of, struct seq_file *seq, void *v
 ssize_t event_filter_write(struct kernfs_open_file *of, char *buf, size_t nbytes,
 			   loff_t off);
 
+int resctrl_mbm_assign_on_mkdir_show(struct kernfs_open_file *of,
+				     struct seq_file *s, void *v);
+
+ssize_t resctrl_mbm_assign_on_mkdir_write(struct kernfs_open_file *of, char *buf,
+					  size_t nbytes, loff_t off);
+
 #ifdef CONFIG_RESCTRL_FS_PSEUDO_LOCK
 int rdtgroup_locksetup_enter(struct rdtgroup *rdtgrp);
 
diff --git a/fs/resctrl/monitor.c b/fs/resctrl/monitor.c
index a4bbd45fc58a..b3d33b983c3c 100644
--- a/fs/resctrl/monitor.c
+++ b/fs/resctrl/monitor.c
@@ -1029,6 +1029,57 @@ int event_filter_show(struct kernfs_open_file *of, struct seq_file *seq, void *v
 	return ret;
 }
 
+int resctrl_mbm_assign_on_mkdir_show(struct kernfs_open_file *of, struct seq_file *s,
+				     void *v)
+{
+	struct rdt_resource *r = rdt_kn_parent_priv(of->kn);
+	int ret = 0;
+
+	mutex_lock(&rdtgroup_mutex);
+	rdt_last_cmd_clear();
+
+	if (!resctrl_arch_mbm_cntr_assign_enabled(r)) {
+		rdt_last_cmd_puts("mbm_event counter assignment mode is not enabled\n");
+		ret = -EINVAL;
+		goto out_unlock;
+	}
+
+	seq_printf(s, "%u\n", r->mon.mbm_assign_on_mkdir);
+
+out_unlock:
+	mutex_unlock(&rdtgroup_mutex);
+
+	return ret;
+}
+
+ssize_t resctrl_mbm_assign_on_mkdir_write(struct kernfs_open_file *of, char *buf,
+					  size_t nbytes, loff_t off)
+{
+	struct rdt_resource *r = rdt_kn_parent_priv(of->kn);
+	bool value;
+	int ret;
+
+	ret = kstrtobool(buf, &value);
+	if (ret)
+		return ret;
+
+	mutex_lock(&rdtgroup_mutex);
+	rdt_last_cmd_clear();
+
+	if (!resctrl_arch_mbm_cntr_assign_enabled(r)) {
+		rdt_last_cmd_puts("mbm_event counter assignment mode is not enabled\n");
+		ret = -EINVAL;
+		goto out_unlock;
+	}
+
+	r->mon.mbm_assign_on_mkdir = value;
+
+out_unlock:
+	mutex_unlock(&rdtgroup_mutex);
+
+	return ret ?: nbytes;
+}
+
 /*
  * rdtgroup_assign_cntr() - Assign/unassign the counter ID for the event, RMID
  * pair in the domain.
@@ -1459,6 +1510,8 @@ int resctrl_mon_resource_init(void)
 		resctrl_file_fflags_init("available_mbm_cntrs",
 					 RFTYPE_MON_INFO | RFTYPE_RES_CACHE);
 		resctrl_file_fflags_init("event_filter", RFTYPE_ASSIGN_CONFIG);
+		resctrl_file_fflags_init("mbm_assign_on_mkdir", RFTYPE_MON_INFO |
+					 RFTYPE_RES_CACHE);
 	}
 
 	return 0;
diff --git a/fs/resctrl/rdtgroup.c b/fs/resctrl/rdtgroup.c
index e90bc808fe53..c7ea42c2a3c2 100644
--- a/fs/resctrl/rdtgroup.c
+++ b/fs/resctrl/rdtgroup.c
@@ -1808,6 +1808,13 @@ static struct rftype res_common_files[] = {
 		.seq_show	= rdt_last_cmd_status_show,
 		.fflags		= RFTYPE_TOP_INFO,
 	},
+	{
+		.name		= "mbm_assign_on_mkdir",
+		.mode		= 0644,
+		.kf_ops		= &rdtgroup_kf_single_ops,
+		.seq_show	= resctrl_mbm_assign_on_mkdir_show,
+		.write		= resctrl_mbm_assign_on_mkdir_write,
+	},
 	{
 		.name		= "num_closids",
 		.mode		= 0444,
diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
index 04152654827d..a7d92718b653 100644
--- a/include/linux/resctrl.h
+++ b/include/linux/resctrl.h
@@ -277,12 +277,15 @@ enum resctrl_schema_fmt {
  *			monitoring events can be configured.
  * @num_mbm_cntrs:	Number of assignable counters.
  * @mbm_cntr_assignable:Is system capable of supporting counter assignment?
+ * @mbm_assign_on_mkdir:True if counters should automatically be assigned to MBM
+ *			events of monitor groups created via mkdir.
  */
 struct resctrl_mon {
 	int			num_rmid;
 	unsigned int		mbm_cfg_mask;
 	int			num_mbm_cntrs;
 	bool			mbm_cntr_assignable;
+	bool			mbm_assign_on_mkdir;
 };
 
 /**
-- 
2.34.1
Re: [PATCH v18 26/33] fs/resctrl: Introduce mbm_assign_on_mkdir to enable assignments on mkdir
Posted by Borislav Petkov 5 months ago
On Fri, Sep 05, 2025 at 04:34:25PM -0500, Babu Moger wrote:
> The "mbm_event" counter assignment mode allows users to assign a hardware
> counter to an RMID, event pair and monitor the bandwidth as long as it is
> assigned.
> 
> Introduce a user-configurable option that determines if a counter will
> automatically be assigned to an RMID, event pair when its associated
> monitor group is created via mkdir. Accessible when "mbm_event" counter
> assignment mode is enabled.

This is just a note for the future - you don't have to go change things now:
reading those commit messages back-to-back, there's a lot of boilerplate code
which repeats with each commit message and there's a lot of text talking what
the patch does.

Please tone this down in the future - it is really annoying and doesn't bring
a whole lot by repeating things or explaining the obvious. Just concentrate on
explaining why the patch exists and mention any non-obvious things.

Everything else people can find by searching the net.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette
Re: [PATCH v18 26/33] fs/resctrl: Introduce mbm_assign_on_mkdir to enable assignments on mkdir
Posted by Moger, Babu 5 months ago
Hi Boris,

On 9/11/25 10:08, Borislav Petkov wrote:
> On Fri, Sep 05, 2025 at 04:34:25PM -0500, Babu Moger wrote:
>> The "mbm_event" counter assignment mode allows users to assign a hardware
>> counter to an RMID, event pair and monitor the bandwidth as long as it is
>> assigned.
>>
>> Introduce a user-configurable option that determines if a counter will
>> automatically be assigned to an RMID, event pair when its associated
>> monitor group is created via mkdir. Accessible when "mbm_event" counter
>> assignment mode is enabled.
> 
> This is just a note for the future - you don't have to go change things now:
> reading those commit messages back-to-back, there's a lot of boilerplate code
> which repeats with each commit message and there's a lot of text talking what
> the patch does.
> 
> Please tone this down in the future - it is really annoying and doesn't bring
> a whole lot by repeating things or explaining the obvious. Just concentrate on
> explaining why the patch exists and mention any non-obvious things.

Agreed. Thanks for the note.

-- 
Thanks
Babu Moger
Re: [PATCH v18 26/33] fs/resctrl: Introduce mbm_assign_on_mkdir to enable assignments on mkdir
Posted by Reinette Chatre 5 months ago
Hi Boris,

On 9/11/25 8:08 AM, Borislav Petkov wrote:
> 
> Please tone this down in the future - it is really annoying and doesn't bring
> a whole lot by repeating things or explaining the obvious. Just concentrate on
> explaining why the patch exists and mention any non-obvious things.
> 
> Everything else people can find by searching the net.

Thank you very much for this guidance. You raise two issues: repeating things and too
much text that explains the obvious.

About repeating things: As I see it the annoying repeating results from desire to
follow the "context-problem-solution" changelog script while also ensuring each
patch stands on its own. With these new features many patches share the same context
and then copy&paste results. I see how this can be annoying when going through
the series and I can also see how this is a lazy approach since the context is
not tailored to each patch. Will work on this.

About too much text that explains the obvious: I hear you and will add these criteria
to how changelogs are measured. I do find the criteria a bit subjective though and expect
that I will not get this right immediately and appreciate and welcome your feedback until
I do.

Thank you very much.

Reinette
Re: [PATCH v18 26/33] fs/resctrl: Introduce mbm_assign_on_mkdir to enable assignments on mkdir
Posted by Borislav Petkov 5 months ago
On Thu, Sep 11, 2025 at 09:24:01AM -0700, Reinette Chatre wrote:
> About repeating things: As I see it the annoying repeating results from desire to
> follow the "context-problem-solution" changelog script while also ensuring each
> patch stands on its own. With these new features many patches share the same context
> and then copy&paste results. I see how this can be annoying when going through
> the series and I can also see how this is a lazy approach since the context is
> not tailored to each patch. Will work on this.

Thanks. And I know it makes sense to repeat things to introduce the context
but let's try to keep that at minimum and only when absolutely necessary.

> About too much text that explains the obvious: I hear you and will add these criteria
> to how changelogs are measured. I do find the criteria a bit subjective though and expect
> that I will not get this right immediately and appreciate and welcome your feedback until
> I do.

Yeah, that's fine, don't worry. But it is actually very simple: if it is
visible from the diff itself, then there's no need to state it again in text.
That would be waste of text.

Lemme paste my old git archeology example here in the hope it makes things
more clear. :-)

Do not talk about *what* the patch is doing in the commit message - that
should be obvious from the diff itself. Rather, concentrate on the *why*
it needs to be done.

Imagine one fine day you're doing git archeology, you find the place in
the code about which you want to find out why it was changed the way it 
is now.

You do git annotate <filename> ... find the line, see the commit id and
you do:

git show <commit id>

You read the commit message and there's just gibberish and nothing's
explaining *why* that change was done. And you start scratching your head,
trying to figure out why. Because the damn commit message is not worth the
electrons used to display it with.

This happens to us maintainers at least once a week.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette
Re: [PATCH v18 26/33] fs/resctrl: Introduce mbm_assign_on_mkdir to enable assignments on mkdir
Posted by Reinette Chatre 5 months ago
Hi Boris,

On 9/11/25 9:54 AM, Borislav Petkov wrote:
> On Thu, Sep 11, 2025 at 09:24:01AM -0700, Reinette Chatre wrote:
>> About repeating things: As I see it the annoying repeating results from desire to
>> follow the "context-problem-solution" changelog script while also ensuring each
>> patch stands on its own. With these new features many patches share the same context
>> and then copy&paste results. I see how this can be annoying when going through
>> the series and I can also see how this is a lazy approach since the context is
>> not tailored to each patch. Will work on this.
> 
> Thanks. And I know it makes sense to repeat things to introduce the context
> but let's try to keep that at minimum and only when absolutely necessary.

Will do.
 
>> About too much text that explains the obvious: I hear you and will add these criteria
>> to how changelogs are measured. I do find the criteria a bit subjective though and expect
>> that I will not get this right immediately and appreciate and welcome your feedback until
>> I do.
> 
> Yeah, that's fine, don't worry. But it is actually very simple: if it is
> visible from the diff itself, then there's no need to state it again in text.
> That would be waste of text.
> 
> Lemme paste my old git archeology example here in the hope it makes things
> more clear. :-)
> 
> Do not talk about *what* the patch is doing in the commit message - that
> should be obvious from the diff itself. Rather, concentrate on the *why*
> it needs to be done.
> 
> Imagine one fine day you're doing git archeology, you find the place in
> the code about which you want to find out why it was changed the way it 
> is now.
> 
> You do git annotate <filename> ... find the line, see the commit id and
> you do:
> 
> git show <commit id>
> 
> You read the commit message and there's just gibberish and nothing's
> explaining *why* that change was done. And you start scratching your head,
> trying to figure out why. Because the damn commit message is not worth the
> electrons used to display it with.

Thank you very much. Will use this as changelog benchmark.

 
> This happens to us maintainers at least once a week.
:(

Reinette