When the mba_MBps mount option is used, provide a file in each
ctrl_mon directory to show which memory monitoring event is
being used.
Suggested-by: Reinette Chatre <reinette.chatre@intel.com>
Signed-off-by: Tony Luck <tony.luck@intel.com>
---
arch/x86/kernel/cpu/resctrl/internal.h | 2 ++
arch/x86/kernel/cpu/resctrl/ctrlmondata.c | 25 +++++++++++++++++++++++
arch/x86/kernel/cpu/resctrl/rdtgroup.c | 17 +++++++++++++++
3 files changed, 44 insertions(+)
diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
index a6f051fb2e69..5f3438ca9e2b 100644
--- a/arch/x86/kernel/cpu/resctrl/internal.h
+++ b/arch/x86/kernel/cpu/resctrl/internal.h
@@ -609,6 +609,8 @@ ssize_t rdtgroup_schemata_write(struct kernfs_open_file *of,
char *buf, size_t nbytes, loff_t off);
int rdtgroup_schemata_show(struct kernfs_open_file *of,
struct seq_file *s, void *v);
+int rdtgroup_mba_mbps_event_show(struct kernfs_open_file *of,
+ struct seq_file *s, void *v);
bool rdtgroup_cbm_overlaps(struct resctrl_schema *s, struct rdt_ctrl_domain *d,
unsigned long cbm, int closid, bool exclusive);
unsigned int rdtgroup_cbm_to_size(struct rdt_resource *r, struct rdt_ctrl_domain *d,
diff --git a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
index 200d89a64027..b9ba419e5c88 100644
--- a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
+++ b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
@@ -518,6 +518,31 @@ static int smp_mon_event_count(void *arg)
return 0;
}
+int rdtgroup_mba_mbps_event_show(struct kernfs_open_file *of,
+ struct seq_file *s, void *v)
+{
+ struct rdtgroup *rdtgrp;
+
+ rdtgrp = rdtgroup_kn_lock_live(of->kn);
+
+ if (rdtgrp) {
+ switch (rdtgrp->mba_mbps_event) {
+ case QOS_L3_MBM_LOCAL_EVENT_ID:
+ seq_puts(s, "mbm_local_bytes\n");
+ break;
+ case QOS_L3_MBM_TOTAL_EVENT_ID:
+ seq_puts(s, "mbm_total_bytes\n");
+ break;
+ case QOS_L3_OCCUP_EVENT_ID:
+ break;
+ }
+ }
+
+ rdtgroup_kn_unlock(of->kn);
+
+ return 0;
+}
+
void mon_event_read(struct rmid_read *rr, struct rdt_resource *r,
struct rdt_mon_domain *d, struct rdtgroup *rdtgrp,
cpumask_t *cpumask, int evtid, int first)
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index 5034a3dd0430..3ba81963e981 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -1943,6 +1943,12 @@ static struct rftype res_common_files[] = {
.seq_show = rdtgroup_schemata_show,
.fflags = RFTYPE_CTRL_BASE,
},
+ {
+ .name = "mba_MBps_event",
+ .mode = 0644,
+ .kf_ops = &rdtgroup_kf_single_ops,
+ .seq_show = rdtgroup_mba_mbps_event_show,
+ },
{
.name = "mode",
.mode = 0644,
@@ -2042,6 +2048,15 @@ void __init mbm_config_rftype_init(const char *config)
rft->fflags = RFTYPE_MON_INFO | RFTYPE_RES_CACHE;
}
+static void mba_mbps_event_init(bool enable)
+{
+ struct rftype *rft;
+
+ rft = rdtgroup_get_rftype_by_name("mba_MBps_event");
+ if (rft)
+ rft->fflags = enable ? RFTYPE_CTRL_BASE : 0;
+}
+
/**
* rdtgroup_kn_mode_restrict - Restrict user access to named resctrl file
* @r: The resource group with which the file is associated.
@@ -2371,6 +2386,8 @@ static int set_mba_sc(bool mba_sc)
d->mbps_val[i] = MBA_MAX_MBPS;
}
+ mba_mbps_event_init(mba_sc);
+
return 0;
}
--
2.47.0Hi Tony,
On 10/29/24 10:28 AM, Tony Luck wrote:
> When the mba_MBps mount option is used, provide a file in each
> ctrl_mon directory to show which memory monitoring event is
> being used.
Could the changelog be expanded a bit more to inform reader what
the monitoring event is used for?
I would also like to remind about the expectations documented in
"Changelog" section of Documentation/process/maintainer-tip.rst:
"A good structure is to explain the context, the problem and the solution
in separate paragraphs and this order."
>
> Suggested-by: Reinette Chatre <reinette.chatre@intel.com>
> Signed-off-by: Tony Luck <tony.luck@intel.com>
> ---
> arch/x86/kernel/cpu/resctrl/internal.h | 2 ++
> arch/x86/kernel/cpu/resctrl/ctrlmondata.c | 25 +++++++++++++++++++++++
> arch/x86/kernel/cpu/resctrl/rdtgroup.c | 17 +++++++++++++++
> 3 files changed, 44 insertions(+)
>
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> index a6f051fb2e69..5f3438ca9e2b 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -609,6 +609,8 @@ ssize_t rdtgroup_schemata_write(struct kernfs_open_file *of,
> char *buf, size_t nbytes, loff_t off);
> int rdtgroup_schemata_show(struct kernfs_open_file *of,
> struct seq_file *s, void *v);
> +int rdtgroup_mba_mbps_event_show(struct kernfs_open_file *of,
> + struct seq_file *s, void *v);
> bool rdtgroup_cbm_overlaps(struct resctrl_schema *s, struct rdt_ctrl_domain *d,
> unsigned long cbm, int closid, bool exclusive);
> unsigned int rdtgroup_cbm_to_size(struct rdt_resource *r, struct rdt_ctrl_domain *d,
> diff --git a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
> index 200d89a64027..b9ba419e5c88 100644
> --- a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
> +++ b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
> @@ -518,6 +518,31 @@ static int smp_mon_event_count(void *arg)
> return 0;
> }
>
> +int rdtgroup_mba_mbps_event_show(struct kernfs_open_file *of,
> + struct seq_file *s, void *v)
> +{
> + struct rdtgroup *rdtgrp;
> +
> + rdtgrp = rdtgroup_kn_lock_live(of->kn);
> +
> + if (rdtgrp) {
> + switch (rdtgrp->mba_mbps_event) {
> + case QOS_L3_MBM_LOCAL_EVENT_ID:
> + seq_puts(s, "mbm_local_bytes\n");
> + break;
> + case QOS_L3_MBM_TOTAL_EVENT_ID:
> + seq_puts(s, "mbm_total_bytes\n");
> + break;
> + case QOS_L3_OCCUP_EVENT_ID:
> + break;
Having a value of QOS_L3_OCCUP_EVENT_ID would surely be a kernel bug.
What do you think of a WARN_ON_ONCE()/pr_warn_once() here?
If mba_mbps_event is indeed expected to have a value of "0" to
reflect "uninitialized" then it could also be handled here
to catch any kernel bugs.
> + }
The custom is to return -ENOENT if no rdtgrp.
> + }
> +
> + rdtgroup_kn_unlock(of->kn);
> +
> + return 0;
> +}
> +
> void mon_event_read(struct rmid_read *rr, struct rdt_resource *r,
> struct rdt_mon_domain *d, struct rdtgroup *rdtgrp,
> cpumask_t *cpumask, int evtid, int first)
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index 5034a3dd0430..3ba81963e981 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -1943,6 +1943,12 @@ static struct rftype res_common_files[] = {
> .seq_show = rdtgroup_schemata_show,
> .fflags = RFTYPE_CTRL_BASE,
> },
> + {
> + .name = "mba_MBps_event",
> + .mode = 0644,
Please only support writing to file when appropriate callback exists.
> + .kf_ops = &rdtgroup_kf_single_ops,
> + .seq_show = rdtgroup_mba_mbps_event_show,
> + },
> {
> .name = "mode",
> .mode = 0644,
> @@ -2042,6 +2048,15 @@ void __init mbm_config_rftype_init(const char *config)
> rft->fflags = RFTYPE_MON_INFO | RFTYPE_RES_CACHE;
> }
>
> +static void mba_mbps_event_init(bool enable)
fyi ...
https://lore.kernel.org/all/237409fb566288d9f3dc7568385e6488b62dbba0.1730244116.git.babu.moger@amd.com/
> +{
> + struct rftype *rft;
> +
> + rft = rdtgroup_get_rftype_by_name("mba_MBps_event");
> + if (rft)
> + rft->fflags = enable ? RFTYPE_CTRL_BASE : 0;
I think this sets this file to be created for all CTRL groups, even when not supporting
monitoring?
> +}
> +
> /**
> * rdtgroup_kn_mode_restrict - Restrict user access to named resctrl file
> * @r: The resource group with which the file is associated.
> @@ -2371,6 +2386,8 @@ static int set_mba_sc(bool mba_sc)
> d->mbps_val[i] = MBA_MAX_MBPS;
> }
>
> + mba_mbps_event_init(mba_sc);
> +
> return 0;
> }
>
Reinette
> > +static int set_mba_sc(bool mba_sc)
> > +{
> > + struct rftype *rft;
> > +
> > + rft = rdtgroup_get_rftype_by_name("mba_MBps_event");
> > + if (rft)
> > + rft->fflags = enable ? RFTYPE_CTRL_BASE : 0;
>
> I think this sets this file to be created for all CTRL groups, even when not supporting
> monitoring?
No. The calling sequence is:
rdt_get_tree()
rdt_enable_ctx()
if (ctx->enable_mba_mbps) {
ret = set_mba_sc(true);
if (ret)
goto out_cdpl3;
}
So set_mba_sc() is only called when the mba_MBps mount option has been used. So
mba_mbps_event_init() isn't called.
Note that on unmount of the resctrl file system rdt_kill_sb() calls rdt_disable_ctx()
which calls set_mba_sc(false) which will clear rft->fflags on systems which support
mba_MBps.
-Tony
Hi Tony,
On 11/12/24 3:42 PM, Luck, Tony wrote:
>>> +static int set_mba_sc(bool mba_sc)
>>> +{
>>> + struct rftype *rft;
>>> +
>>> + rft = rdtgroup_get_rftype_by_name("mba_MBps_event");
>>> + if (rft)
>>> + rft->fflags = enable ? RFTYPE_CTRL_BASE : 0;
>>
>> I think this sets this file to be created for all CTRL groups, even when not supporting
>> monitoring?
>
> No. The calling sequence is:
>
> rdt_get_tree()
> rdt_enable_ctx()
>
> if (ctx->enable_mba_mbps) {
> ret = set_mba_sc(true);
> if (ret)
> goto out_cdpl3;
> }
>
> So set_mba_sc() is only called when the mba_MBps mount option has been used. So
> mba_mbps_event_init() isn't called.
>
> Note that on unmount of the resctrl file system rdt_kill_sb() calls rdt_disable_ctx()
> which calls set_mba_sc(false) which will clear rft->fflags on systems which support
> mba_MBps.
It sounds as though you are saying that setting the wrong flags are ok as long as it is
done in some specific calling sequence. Is this correct? Relying on calling sequence
instead of correct flags requires more in-depth knowledge of code flows and makes code
harder to maintain.
Why not just make this "RFTYPE_CTRL_BASE | RFTYPE_MON_BASE" to make it clear that the file
applies to CTRL_MON groups? What am I missing?
Reinette
> >>> +static int set_mba_sc(bool mba_sc)
> >>> +{
> >>> + struct rftype *rft;
> >>> +
> >>> + rft = rdtgroup_get_rftype_by_name("mba_MBps_event");
> >>> + if (rft)
> >>> + rft->fflags = enable ? RFTYPE_CTRL_BASE : 0;
> >>
> >> I think this sets this file to be created for all CTRL groups, even when not supporting
> >> monitoring?
I think I misunderstood you. I though you said the these mba_MBps_event files
would be created even if monitoring is not supported,
But now I wonder what you mean by "all CTRL groups".
> > No. The calling sequence is:
> >
> > rdt_get_tree()
> > rdt_enable_ctx()
> >
> > if (ctx->enable_mba_mbps) {
> > ret = set_mba_sc(true);
> > if (ret)
> > goto out_cdpl3;
> > }
> >
> > So set_mba_sc() is only called when the mba_MBps mount option has been used. So
> > mba_mbps_event_init() isn't called.
> >
> > Note that on unmount of the resctrl file system rdt_kill_sb() calls rdt_disable_ctx()
> > which calls set_mba_sc(false) which will clear rft->fflags on systems which support
> > mba_MBps.
>
> It sounds as though you are saying that setting the wrong flags are ok as long as it is
> done in some specific calling sequence. Is this correct? Relying on calling sequence
> instead of correct flags requires more in-depth knowledge of code flows and makes code
> harder to maintain.
> Why not just make this "RFTYPE_CTRL_BASE | RFTYPE_MON_BASE" to make it clear that the file
> applies to CTRL_MON groups? What am I missing?
The directories which need this new file are the same ones that get a "schemata" file.
The initialization of fflags for the schemata file just uses RFTYPE_CTRL_BASE:
{
.name = "schemata",
.mode = 0644,
.kf_ops = &rdtgroup_kf_single_ops,
.write = rdtgroup_schemata_write,
.seq_show = rdtgroup_schemata_show,
.fflags = RFTYPE_CTRL_BASE,
},
I don't see any files using .fflags = "RFTYPE_CTRL_BASE | RFTYPE_MON_BASE"
-Tony
Hi Tony,
On 11/12/24 4:53 PM, Luck, Tony wrote:
>>>>> +static int set_mba_sc(bool mba_sc)
>>>>> +{
>>>>> + struct rftype *rft;
>>>>> +
>>>>> + rft = rdtgroup_get_rftype_by_name("mba_MBps_event");
>>>>> + if (rft)
>>>>> + rft->fflags = enable ? RFTYPE_CTRL_BASE : 0;
>>>>
>>>> I think this sets this file to be created for all CTRL groups, even when not supporting
>>>> monitoring?
>
> I think I misunderstood you. I though you said the these mba_MBps_event files
> would be created even if monitoring is not supported,
>
> But now I wonder what you mean by "all CTRL groups".
>
>>> No. The calling sequence is:
>>>
>>> rdt_get_tree()
>>> rdt_enable_ctx()
>>>
>>> if (ctx->enable_mba_mbps) {
>>> ret = set_mba_sc(true);
>>> if (ret)
>>> goto out_cdpl3;
>>> }
>>>
>>> So set_mba_sc() is only called when the mba_MBps mount option has been used. So
>>> mba_mbps_event_init() isn't called.
>>>
>>> Note that on unmount of the resctrl file system rdt_kill_sb() calls rdt_disable_ctx()
>>> which calls set_mba_sc(false) which will clear rft->fflags on systems which support
>>> mba_MBps.
>>
>> It sounds as though you are saying that setting the wrong flags are ok as long as it is
>> done in some specific calling sequence. Is this correct? Relying on calling sequence
>> instead of correct flags requires more in-depth knowledge of code flows and makes code
>> harder to maintain.
>> Why not just make this "RFTYPE_CTRL_BASE | RFTYPE_MON_BASE" to make it clear that the file
>> applies to CTRL_MON groups? What am I missing?
>
> The directories which need this new file are the same ones that get a "schemata" file.
Only support for control is required for "schemata" to be created. Monitoring support is not
required for "schemata" to be created. This new file requires both monitoring and control support.
>
> The initialization of fflags for the schemata file just uses RFTYPE_CTRL_BASE:
>
> {
> .name = "schemata",
> .mode = 0644,
> .kf_ops = &rdtgroup_kf_single_ops,
> .write = rdtgroup_schemata_write,
> .seq_show = rdtgroup_schemata_show,
> .fflags = RFTYPE_CTRL_BASE,
> },
>
> I don't see any files using .fflags = "RFTYPE_CTRL_BASE | RFTYPE_MON_BASE"
>
I do not think there is a precedent for this case where a file requires both control and
monitoring support.
Reinette
© 2016 - 2024 Red Hat, Inc.