Re: [PATCH 13/24] x86/resctrl: Allow different CODE/DATA configurations to be staged

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

Hi James,

On 10/30/2020 9:11 AM, James Morse wrote:
> Now that the configuration is staged via an array, allow resctrl to
> stage more than configuration at a time for a single resource and

s/more than/more than one/ ?

> closid.
> 
> To detect that the same schema is being specified twice when the schemata
> file is written, the same slot in the staged_configuration array must be
> used for each schema. Use the conf_type enum directly as an index.
> 
> Signed-off-by: James Morse <james.morse@arm.com>
> ---
>   arch/x86/kernel/cpu/resctrl/ctrlmondata.c | 16 ++++++++++------
>   arch/x86/kernel/cpu/resctrl/rdtgroup.c    |  5 +++--
>   include/linux/resctrl.h                   |  4 +++-
>   3 files changed, 16 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
> index b107c0202cfb..f7152c7fdc1b 100644
> --- a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
> +++ b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c

...

> @@ -266,10 +269,11 @@ int update_domains(struct rdt_resource *r, int closid)
>   	struct resctrl_staged_config *cfg;
>   	struct rdt_hw_domain *hw_dom;
>   	struct msr_param msr_param;
> +	enum resctrl_conf_type t;
>   	cpumask_var_t cpu_mask;
>   	struct rdt_domain *d;
>   	bool mba_sc;
> -	int cpu, i;
> +	int cpu;
>   
>   	if (!zalloc_cpumask_var(&cpu_mask, GFP_KERNEL))
>   		return -ENOMEM;
> @@ -281,8 +285,8 @@ int update_domains(struct rdt_resource *r, int closid)
>   	mba_sc = is_mba_sc(r);
>   	list_for_each_entry(d, &r->domains, list) {
>   		hw_dom = resctrl_to_arch_dom(d);
> -		for (i = 0; i < ARRAY_SIZE(d->staged_config); i++) {
> -			cfg = &hw_dom->resctrl.staged_config[i];
> +		for (t = 0; t < ARRAY_SIZE(d->staged_config); t++) {

Since t is now an enum, it may be easier to read if NUM_CDP_TYPES is 
used instead of ARRAY_SIZE?

> +			cfg = &hw_dom->resctrl.staged_config[t];
>   			if (!cfg->have_new_ctrl)
>   				continue;
>   
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index 1092631ac0b3..5eb14dc9c579 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -2747,6 +2747,7 @@ static u32 cbm_ensure_valid(u32 _val, struct rdt_resource *r)
>   static int __init_one_rdt_domain(struct rdt_domain *d, struct resctrl_schema *s,
>   				 u32 closid)
>   {
> +	enum resctrl_conf_type t = s-> conf_type;

Extra space above in "s-> conf_type".

>   	struct rdt_resource *r_cdp = NULL;
>   	struct resctrl_staged_config *cfg;
>   	struct rdt_domain *d_cdp = NULL;
> @@ -2758,7 +2759,7 @@ static int __init_one_rdt_domain(struct rdt_domain *d, struct resctrl_schema *s,
>   	int i;
>   
>   	rdt_cdp_peer_get(r, d, &r_cdp, &d_cdp);
> -	cfg = &d->staged_config[0];
> +	cfg = &d->staged_config[t];
>   	cfg->have_new_ctrl = false;
>   	cfg->new_ctrl = r->cache.shareable_bits;
>   	used_b = r->cache.shareable_bits;
> @@ -2843,7 +2844,7 @@ static void rdtgroup_init_mba(struct rdt_resource *r, u32 closid)
>   	struct rdt_domain *d;
>   
>   	list_for_each_entry(d, &r->domains, list) {
> -		cfg = &d->staged_config[0];
> +		cfg = &d->staged_config[CDP_BOTH];
>   		cfg->new_ctrl = is_mba_sc(r) ? MBA_MAX_MBPS : r->default_ctrl;
>   		cfg->closid = closid;
>   		cfg->have_new_ctrl = true;
> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
> index 695247c08ba3..e33d6dfce8a1 100644
> --- a/include/linux/resctrl.h
> +++ b/include/linux/resctrl.h
> @@ -25,6 +25,8 @@ enum resctrl_conf_type {
>   	CDP_CODE,
>   	CDP_DATA,
>   };
> +#define NUM_CDP_TYPES	CDP_DATA + 1
> +

There is also the option of making NUM_CDP_TYPES the last member of 
resctrl_conf_type ... and changing its name to CDP_NUM_TYPES to be 
consistent with similar usages (RDT_NUM_GROUP, RDT_NUM_MODES, 
RDT_NUM_RESOURCES)? The problem then is that you need to handle it in 
the switch statement. If you choose to keep this way then please change 
the name to be consistent with the other enums, add an empty line after 
the enum to make checkpatch happy, and add parenthesis around its value.

>   
>   /**
>    * struct resctrl_staged_config - parsed configuration to be applied
> @@ -67,7 +69,7 @@ struct rdt_domain {
>   	int				cqm_work_cpu;
>   
>   	struct pseudo_lock_region	*plr;
> -	struct resctrl_staged_config	staged_config[1];
> +	struct resctrl_staged_config	staged_config[NUM_CDP_TYPES];
>   };
>   
>   /**
> 


Reinette