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

[Reinette Chatre <reinette.chatre@intel.com>]

Hi James,

On 10/30/2020 9:11 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
> array the arch code has chosen to enable.
> 
> Start by creating a struct resctrl_schema, which will eventually hold
> the name and pending configuration values for resctrl.

Looking ahead I am not able to identify the "pending configuration 
values" that will eventually be held in resctrl_schema. With entire 
series applied there is the name, type, num_closid, and pointer to the 
resource.

> 
> Signed-off-by: James Morse <james.morse@arm.com>
> ---
>   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 f7aab9245259..682e84aebd14 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_all_schema;

Considering the relationship with rdt_resources_all it seems more 
consistent to call it 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 df10135f021e..f79a5e548138 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_all_schema);
> +
>   /* Kernel fs node for "info" directory under root */
>   static struct kernfs_node *kn_info;
>   
> @@ -2117,6 +2120,35 @@ static int rdt_enable_ctx(struct rdt_fs_context *ctx)
>   	return ret;
>   }
>   
> +static int create_schemata_list(void)
> +{

Could you please use the new namespace (schema_list_) as a prefix 
instead of suffix? That will make it easier to search.

> +	struct resctrl_schema *s;
> +	struct rdt_resource *r;
> +
> +	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_all_schema);
> +	}
> +
> +	return 0;
> +}
> +
> +static void destroy_schemata_list(void)
> +{
> +	struct resctrl_schema *s, *tmp;
> +
> +	list_for_each_entry_safe(s, tmp, &resctrl_all_schema, list) {
> +		list_del(&s->list);
> +		kfree(s);
> +	}
> +}
> +
>   static int rdt_get_tree(struct fs_context *fc)
>   {
>   	struct rdt_fs_context *ctx = rdt_fc2context(fc);

...

> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
> index dfb0f32b73a1..de6cbc725753 100644
> --- a/include/linux/resctrl.h
> +++ b/include/linux/resctrl.h
> @@ -163,6 +163,15 @@ struct rdt_resource {
>   
>   };
>   
> +/**

Missing a kernel-doc description here

> + * @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;
> +};
> +
>   /* The number of closid supported by this resource regardless of CDP */
>   u32 resctrl_arch_get_num_closid(struct rdt_resource *r);
>   
> 

Reinette