Hi 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@xxxxxxxxx>
> Signed-off-by: Tony Luck <tony.luck@xxxxxxxxx>
> ---
> 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@xxxxxxx/
> +{
> + 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
