Re: [PATCH 03/24] x86/resctrl: Add resctrl_arch_get_num_closid()

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

Hi James,

On 10/30/2020 9:10 AM, James Morse wrote:
> resctrl chooses whether to enable CDP, once it does, half the number
> of closid are available. MPAM doesn't behave like this, an in-kernel user
> of MPAM could be 'using CDP' while resctrl is not.
> 
> To move the 'half the closids' behaviour to be part of the core code,
> each schema would have a num_closids. This may be different from the
> single resources num_closid if CDP is in use.
> 
> Add a helper to read the resource's num_closid, this should return the
> number of closid that the resource supports, regardless of whether CDP
> is in use.
> 
> For now return the hw_res->num_closid, which is already adjusted for CDP.
> Once the CODE/DATA/BOTH resources are merged, resctrl can make the
> adjustment when copying the value to the schema's num_closid.
> 
> Signed-off-by: James Morse <james.morse@arm.com>
> ---
>   arch/x86/kernel/cpu/resctrl/core.c        |  5 +++++
>   arch/x86/kernel/cpu/resctrl/ctrlmondata.c |  9 +++------
>   arch/x86/kernel/cpu/resctrl/rdtgroup.c    | 14 +++++---------
>   include/linux/resctrl.h                   |  3 +++
>   4 files changed, 16 insertions(+), 15 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
> index 97040a54cc9a..5d5b566c4359 100644
> --- a/arch/x86/kernel/cpu/resctrl/core.c
> +++ b/arch/x86/kernel/cpu/resctrl/core.c
> @@ -443,6 +443,11 @@ struct rdt_domain *get_domain_from_cpu(int cpu, struct rdt_resource *r)
>   	return NULL;
>   }
>   
> +u32 resctrl_arch_get_num_closid(struct rdt_resource *r)
> +{
> +	return resctrl_to_arch_res(r)->num_closid;
> +}
> +

Helper returns the value but also changes the type. Could you please add 
motivation for this in a comment?


>   void rdt_ctrl_update(void *arg)
>   {
>   	struct msr_param *m = arg;
> diff --git a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
> index 2e7466659af3..14ea6a40993f 100644
> --- a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
> +++ b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
> @@ -286,12 +286,11 @@ int update_domains(struct rdt_resource *r, int closid)
>   static int rdtgroup_parse_resource(char *resname, char *tok,
>   				   struct rdtgroup *rdtgrp)
>   {
> -	struct rdt_hw_resource *hw_res;
>   	struct rdt_resource *r;
>   
>   	for_each_alloc_enabled_rdt_resource(r) {
> -		hw_res = resctrl_to_arch_res(r);
> -		if (!strcmp(resname, r->name) && rdtgrp->closid < hw_res->num_closid)
> +		if (!strcmp(resname, r->name) &&
> +		     rdtgrp->closid < resctrl_arch_get_num_closid(r))

Please let alignment match open parenthesis.

>   			return parse_line(tok, r, rdtgrp);
>   	}
>   	rdt_last_cmd_printf("Unknown or unsupported resource name '%s'\n", resname);
> @@ -400,7 +399,6 @@ static void show_doms(struct seq_file *s, struct rdt_resource *r, int closid)
>   int rdtgroup_schemata_show(struct kernfs_open_file *of,
>   			   struct seq_file *s, void *v)
>   {
> -	struct rdt_hw_resource *hw_res;
>   	struct rdtgroup *rdtgrp;
>   	struct rdt_resource *r;
>   	int ret = 0;
> @@ -425,8 +423,7 @@ int rdtgroup_schemata_show(struct kernfs_open_file *of,
>   		} else {
>   			closid = rdtgrp->closid;
>   			for_each_alloc_enabled_rdt_resource(r) {
> -				hw_res = resctrl_to_arch_res(r);
> -				if (closid < hw_res->num_closid)
> +				if (closid < resctrl_arch_get_num_closid(r))
>   					show_doms(s, r, closid);
>   			}
>   		}
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index b55861ff4e34..df10135f021e 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -100,15 +100,13 @@ int closids_supported(void)
>   
>   static void closid_init(void)
>   {
> -	struct rdt_hw_resource *hw_res;
> +	u32 rdt_min_closid = 32;
>   	struct rdt_resource *r;
> -	int rdt_min_closid = 32;
>   
>   	/* Compute rdt_min_closid across all resources */
> -	for_each_alloc_enabled_rdt_resource(r) {
> -		hw_res = resctrl_to_arch_res(r);
> -		rdt_min_closid = min(rdt_min_closid, hw_res->num_closid);
> -	}
> +	for_each_alloc_enabled_rdt_resource(r)
> +		rdt_min_closid = min(rdt_min_closid,
> +				     resctrl_arch_get_num_closid(r));
>   
>   	closid_free_map = BIT_MASK(rdt_min_closid) - 1;
>   
> @@ -847,10 +845,8 @@ static int rdt_num_closids_show(struct kernfs_open_file *of,
>   				struct seq_file *seq, void *v)
>   {
>   	struct rdt_resource *r = of->kn->parent->priv;
> -	struct rdt_hw_resource *hw_res;
>   
> -	hw_res = resctrl_to_arch_res(r);
> -	seq_printf(seq, "%d\n", hw_res->num_closid);
> +	seq_printf(seq, "%d\n", resctrl_arch_get_num_closid(r));
>   	return 0;

Now that this type is changed this will need to be %u?

>   }
>   
> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
> index f5af59b8f2a9..dfb0f32b73a1 100644
> --- a/include/linux/resctrl.h
> +++ b/include/linux/resctrl.h
> @@ -163,4 +163,7 @@ struct rdt_resource {
>   
>   };
>   
> +/* The number of closid supported by this resource regardless of CDP */
> +u32 resctrl_arch_get_num_closid(struct rdt_resource *r);
> +
>   #endif /* _RESCTRL_H */
> 


The purpose of this change is unclear and introducing confusion. It 
introduces a helper that returns the num_closid associated with a 
resource but it does not use the helper in all the cases where this 
value is needed. After this change some code uses this new helper while 
other code continue to access the value directly.

Could you please elaborate in the commit message why this helper is not 
used everywhere and how the places that were changed were chosen? Seems 
like the places that _don't_ need the (eventual) value of 
resctrl_arch_get_num_closid() are changed (so that it is easier to move 
to the schema's num_closid later) while the places that actually do need 
the helper are not changed?

Reinette