To support multiple architectures, the resctrl code needs to be split
into a 'fs' specific part in core code, and an arch-specific backend.
It should be difficult for the arch-specific backends to diverge,
supporting slightly different ABIs for user-space. For example,
generating, parsing and validating the schema configuration values
should be done in what becomes the core code to prevent divergence.
Today, the schema emerge from which entries in the rdt_resources_all
array the arch code has chosen to enable.
Start by creating a struct resctrl_schema, which will eventually hold
the name and type of configuration values for resctrl.
Reviewed-by: Jamie Iles <[email protected]>
Signed-off-by: James Morse <[email protected]>
---
Changes since v1:
* Renamed resctrl_all_schema list
* Used schemata_list as a prefix to make these easier to search for
* Added kerneldoc string
* Removed 'pending configuration' reference in commit message
---
arch/x86/kernel/cpu/resctrl/internal.h | 1 +
arch/x86/kernel/cpu/resctrl/rdtgroup.c | 43 +++++++++++++++++++++++++-
include/linux/resctrl.h | 9 ++++++
3 files changed, 52 insertions(+), 1 deletion(-)
diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
index d3e47fd51e3a..8a9da490134b 100644
--- a/arch/x86/kernel/cpu/resctrl/internal.h
+++ b/arch/x86/kernel/cpu/resctrl/internal.h
@@ -106,6 +106,7 @@ extern unsigned int resctrl_cqm_threshold;
extern bool rdt_alloc_capable;
extern bool rdt_mon_capable;
extern unsigned int rdt_mon_features;
+extern struct list_head resctrl_schema_all;
enum rdt_group_type {
RDTCTRL_GROUP = 0,
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index 8c29304d3e01..73a695e7096d 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -39,6 +39,9 @@ static struct kernfs_root *rdt_root;
struct rdtgroup rdtgroup_default;
LIST_HEAD(rdt_all_groups);
+/* list of entries for the schemata file */
+LIST_HEAD(resctrl_schema_all);
+
/* Kernel fs node for "info" directory under root */
static struct kernfs_node *kn_info;
@@ -2109,6 +2112,35 @@ static int rdt_enable_ctx(struct rdt_fs_context *ctx)
return ret;
}
+static int schemata_list_create(void)
+{
+ struct rdt_resource *r;
+ struct resctrl_schema *s;
+
+ for_each_alloc_enabled_rdt_resource(r) {
+ s = kzalloc(sizeof(*s), GFP_KERNEL);
+ if (!s)
+ return -ENOMEM;
+
+ s->res = r;
+
+ INIT_LIST_HEAD(&s->list);
+ list_add(&s->list, &resctrl_schema_all);
+ }
+
+ return 0;
+}
+
+static void schemata_list_destroy(void)
+{
+ struct resctrl_schema *s, *tmp;
+
+ list_for_each_entry_safe(s, tmp, &resctrl_schema_all, list) {
+ list_del(&s->list);
+ kfree(s);
+ }
+}
+
static int rdt_get_tree(struct fs_context *fc)
{
struct rdt_fs_context *ctx = rdt_fc2context(fc);
@@ -2130,11 +2162,17 @@ static int rdt_get_tree(struct fs_context *fc)
if (ret < 0)
goto out_cdp;
+ ret = schemata_list_create();
+ if (ret) {
+ schemata_list_destroy();
+ goto out_mba;
+ }
+
closid_init();
ret = rdtgroup_create_info_dir(rdtgroup_default.kn);
if (ret < 0)
- goto out_mba;
+ goto out_schemata_free;
if (rdt_mon_capable) {
ret = mongroup_create_dir(rdtgroup_default.kn,
@@ -2184,6 +2222,8 @@ static int rdt_get_tree(struct fs_context *fc)
kernfs_remove(kn_mongrp);
out_info:
kernfs_remove(kn_info);
+out_schemata_free:
+ schemata_list_destroy();
out_mba:
if (ctx->enable_mba_mbps)
set_mba_sc(false);
@@ -2425,6 +2465,7 @@ static void rdt_kill_sb(struct super_block *sb)
rmdir_all_sub();
rdt_pseudo_lock_release();
rdtgroup_default.mode = RDT_MODE_SHAREABLE;
+ schemata_list_destroy();
static_branch_disable_cpuslocked(&rdt_alloc_enable_key);
static_branch_disable_cpuslocked(&rdt_mon_enable_key);
static_branch_disable_cpuslocked(&rdt_enable_key);
diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
index be6f5df78e31..092ff0c13b9b 100644
--- a/include/linux/resctrl.h
+++ b/include/linux/resctrl.h
@@ -154,4 +154,13 @@ struct rdt_resource {
};
+/**
+ * struct resctrl_schema - configuration abilities of a resource presented to user-space
+ * @list: Member of resctrl's schema list
+ * @res: The rdt_resource for this entry
+ */
+struct resctrl_schema {
+ struct list_head list;
+ struct rdt_resource *res;
+};
#endif /* _RESCTRL_H */
--
2.30.0
Hi James,
On 3/12/2021 9:58 AM, James Morse wrote:
> To support multiple architectures, the resctrl code needs to be split
> into a 'fs' specific part in core code, and an arch-specific backend.
>
> It should be difficult for the arch-specific backends to diverge,
> supporting slightly different ABIs for user-space. For example,
> generating, parsing and validating the schema configuration values
> should be done in what becomes the core code to prevent divergence.
> Today, the schema emerge from which entries in the rdt_resources_all
rdt_resources_all -> rdt_resources_all[]
> array the arch code has chosen to enable.
Throughout this series the commit messages do not have a consistent
format. On top of this some commit messages uses terms like "Today" or
"Previously" to document the context of the change ... sometimes in the
middle of a commit message like here after the solution has been
presented. In a long series like this it does make things increasingly
harder to follow. There is an established commit message format in the
x86 area that makes communicating changes much easier to do. Quoting
Thomas ([1]) "A good structure is to explain the context, the problem
and the solution in separate paragraphs and this order." Following this
format makes changes much easier to communicate in a commit message and
would definitely help this series during the next level of review by the
contributors to [1].
[1] https://lore.kernel.org/lkml/[email protected]/
>
> Start by creating a struct resctrl_schema, which will eventually hold
This term "eventually" shows up a lot in the commit messages of this
series and causing some trouble because sometimes what it refers to is
done in this series but sometimes what it refers to is _not_ done in
this series. For example, the changes mentioned here are indeed made in
this series but the changes mentioned in patch 6 as "eventually" are
not. This concerns me about how many gaps are created by these changes.
> the name and type of configuration values for resctrl.
The future members are mentioned but the one introduced here and why is not.
...
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index 8c29304d3e01..73a695e7096d 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -39,6 +39,9 @@ static struct kernfs_root *rdt_root;
> struct rdtgroup rdtgroup_default;
> LIST_HEAD(rdt_all_groups);
>
> +/* list of entries for the schemata file */
> +LIST_HEAD(resctrl_schema_all);
> +
> /* Kernel fs node for "info" directory under root */
> static struct kernfs_node *kn_info;
>
> @@ -2109,6 +2112,35 @@ static int rdt_enable_ctx(struct rdt_fs_context *ctx)
> return ret;
> }
>
> +static int schemata_list_create(void)
> +{
> + struct rdt_resource *r;
> + struct resctrl_schema *s;
Please maintain reverse fir tree format.
> +
> + for_each_alloc_enabled_rdt_resource(r) {
> + s = kzalloc(sizeof(*s), GFP_KERNEL);
> + if (!s)
> + return -ENOMEM;
> +
> + s->res = r;
> +
> + INIT_LIST_HEAD(&s->list);
> + list_add(&s->list, &resctrl_schema_all);
> + }
> +
> + return 0;
> +}
...
> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
> index be6f5df78e31..092ff0c13b9b 100644
> --- a/include/linux/resctrl.h
> +++ b/include/linux/resctrl.h
> @@ -154,4 +154,13 @@ struct rdt_resource {
>
> };
>
> +/**
> + * struct resctrl_schema - configuration abilities of a resource presented to user-space
> + * @list: Member of resctrl's schema list
> + * @res: The rdt_resource for this entry
Could this description be improved? It merely states what can be seen
from the code.
Thank you
Reinette