Re: [PATCH v4 04/39] x86/resctrl: Use schema type to determine how to parse schema values

From: Reinette Chatre
Date: Tue Aug 13 2024 - 23:57:50 EST


Hi James,

On 8/2/24 10:28 AM, James Morse wrote:
diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
index 9ca542a8e2d4..57c88e1c2adf 100644
--- a/arch/x86/kernel/cpu/resctrl/core.c
+++ b/arch/x86/kernel/cpu/resctrl/core.c
@@ -72,7 +72,7 @@ struct rdt_hw_resource rdt_resources_all[] = {
.mon_scope = RESCTRL_L3_CACHE,
.ctrl_domains = ctrl_domain_init(RDT_RESOURCE_L3),
.mon_domains = mon_domain_init(RDT_RESOURCE_L3),
- .parse_ctrlval = parse_cbm,
+ .schema_fmt = RESCTRL_SCHEMA_BITMAP,
.format_str = "%d=%0*x",
},
.msr_base = MSR_IA32_L3_CBM_BASE,
@@ -85,7 +85,7 @@ struct rdt_hw_resource rdt_resources_all[] = {
.name = "L2",
.ctrl_scope = RESCTRL_L2_CACHE,
.ctrl_domains = ctrl_domain_init(RDT_RESOURCE_L2),
- .parse_ctrlval = parse_cbm,
+ .schema_fmt = RESCTRL_SCHEMA_BITMAP,
.format_str = "%d=%0*x",
},
.msr_base = MSR_IA32_L2_CBM_BASE,
@@ -98,7 +98,11 @@ struct rdt_hw_resource rdt_resources_all[] = {
.name = "MB",
.ctrl_scope = RESCTRL_L3_CACHE,
.ctrl_domains = ctrl_domain_init(RDT_RESOURCE_MBA),
- .parse_ctrlval = parse_bw,
+ /*
+ * MBA schema_fmt is modified by
+ * __rdt_get_mem_config_amd()
+ */
+ .schema_fmt = RESCTRL_SCHEMA_PERCENTAGE,
.format_str = "%d=%*u",
},
},
@@ -109,7 +113,7 @@ struct rdt_hw_resource rdt_resources_all[] = {
.name = "SMBA",
.ctrl_scope = RESCTRL_L3_CACHE,
.ctrl_domains = ctrl_domain_init(RDT_RESOURCE_SMBA),
- .parse_ctrlval = parse_bw,
+ .schema_fmt = RESCTRL_SCHEMA_MBPS,
.format_str = "%d=%*u",
},
},

...

@@ -195,6 +204,19 @@ int parse_cbm(struct rdt_parse_data *data, struct resctrl_schema *s,
return 0;
}
+static ctrlval_parser_t *get_parser(struct rdt_resource *r)
+{
+ switch (r->schema_fmt) {
+ case RESCTRL_SCHEMA_BITMAP:
+ return &parse_cbm;
+ case RESCTRL_SCHEMA_PERCENTAGE:
+ case RESCTRL_SCHEMA_MBPS:
+ return &parse_bw;
+ }
+
+ return NULL;
+}
+
/*
* For each domain in this resource we expect to find a series of:
* id=mask

...

@@ -192,6 +191,18 @@ enum resctrl_scope {
RESCTRL_L3_NODE,
};
+/**
+ * enum resctrl_schema_fmt - The format user-space provides for a schema.
+ * @RESCTRL_SCHEMA_BITMAP: The schema is a bitmap in hex.
+ * @RESCTRL_SCHEMA_PERCENTAGE: The schema is a decimal percentage value.
+ * @RESCTRL_SCHEMA_MBPS: The schema is a decimal MBps value.
+ */
+enum resctrl_schema_fmt {
+ RESCTRL_SCHEMA_BITMAP,
+ RESCTRL_SCHEMA_PERCENTAGE,
+ RESCTRL_SCHEMA_MBPS,
+};
+

I believe that the choice of RESCTRL_SCHEMA_PERCENTAGE and RESCTRL_SCHEMA_MBPS has
potential for significant confusion. The closest place to where user space can enter
a MBps value (which is actually MiBps) is on Intel when resctrl is mounted with mba_MBps,
and as per above it will have the "RESCTRL_SCHEMA_PERCENTAGE" format. What is considered
here as RESCTRL_SCHEMA_MBPS also cannot really be considered as "MBPS" since it is used to
cover AMD's values that are "multiples of one eighth GB/s". Any new resource that
_actually_ uses MBPS will thus not be able to use RESCTRL_SCHEMA_MBPS.

Considering that RESCTRL_SCHEMA_PERCENTAGE and RESCTRL_SCHEMA_MBPS use the same parser,
could "RESCTRL_SCHEMA_RANGE" be more fitting? I acknowledge that it is very generic and better
ideas are welcome. A "range" does seem to be appropriate considering the later patch (x86/resctrl:
Add max_bw to struct resctrl_membw) that codes an explicit max.

Reinette