Re: [PATCH v2 03/24] x86/resctrl: Add a separate schema list for resctrl

2021-03-31 Thread Reinette Chatre 

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/20181107171149.165693...@linutronix.de/




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(>list);
+   list_add(>list, _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

[PATCH v2 03/24] x86/resctrl: Add a separate schema list for resctrl

2021-03-12 Thread James Morse 
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 
Signed-off-by: James Morse 
---
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(>list);
+   list_add(>list, _schema_all);
+   }
+
+   return 0;
+}
+
+static void schemata_list_destroy(void)
+{
+   struct resctrl_schema *s, *tmp;
+
+   list_for_each_entry_safe(s, tmp, _schema_all, list) {
+   list_del(>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 +,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(_alloc_enable_key);
static_branch_disable_cpuslocked(_mon_enable_key);
static_branch_disable_cpuslocked(_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_headlist;
+   struct rdt_resource *res;
+};
 #endif /* _RESCTRL_H */
-- 
2.30.0