Re: [RFC PATCH 07/10] arch/x86: Bring few more functions into the resource structure

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

Hi Babu,

On 9/24/2018 12:19 PM, Moger, Babu wrote:
> Bring all resource functions that are different between the vendors
> into resource structure and initialize them dynamically.
> 
> Implement these functions separately for each vendors.
> update_mba_bw : Feedback loop bandwidth update functionality is not
>                 needed for AMD.
> cbm_validate  : Cache bitmask validate function. AMD allows
>                 non-contiguous masks. So, use separate functions for
>                 Intel and AMD.
> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
>  arch/x86/kernel/cpu/rdt.c             | 17 +++++++++++++----
>  arch/x86/kernel/cpu/rdt.h             | 19 +++++++++++++------
>  arch/x86/kernel/cpu/rdt_ctrlmondata.c |  4 ++--
>  arch/x86/kernel/cpu/rdt_monitor.c     | 10 +++++++---
>  4 files changed, 35 insertions(+), 15 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/rdt.c b/arch/x86/kernel/cpu/rdt.c
> index 6dec45bf81d6..ae26b9b3fafa 100644
> --- a/arch/x86/kernel/cpu/rdt.c
> +++ b/arch/x86/kernel/cpu/rdt.c
> @@ -867,10 +867,19 @@ static __init void rdt_init_res_defs_intel(void)
>  	struct rdt_resource *r;
>  
>  	for_each_rdt_resource(r) {
> -		if (r->rid == RDT_RESOURCE_MBA) {
> -			r->msr_base = IA32_MBA_THRTL_BASE;
> -			r->msr_update = mba_wrmsr;
> -			r->parse_ctrlval = parse_bw;
> +		if ((r->rid == RDT_RESOURCE_L3) ||
> +		    (r->rid == RDT_RESOURCE_L3DATA) ||
> +		    (r->rid == RDT_RESOURCE_L3CODE) ||
> +		    (r->rid == RDT_RESOURCE_L2) ||
> +		    (r->rid == RDT_RESOURCE_L2DATA) ||
> +		    (r->rid == RDT_RESOURCE_L2CODE))
> +			r->cbm_validate = cbm_validate;

Same comment here about naming as in patch 6. Later cbm_validate_amd
would appear while this remains - to help reduce confusion it may help
to rename this function to cbm_validate_intel at this time.

> +
> +		else if (r->rid == RDT_RESOURCE_MBA) {
> +			 r->msr_base = IA32_MBA_THRTL_BASE;
> +			 r->msr_update = mba_wrmsr;
> +			 r->parse_ctrlval = parse_bw;
> +			 r->update_mba_bw = update_mba_bw;

Same comment about naming.

>  		}
>  	}
>  }
> diff --git a/arch/x86/kernel/cpu/rdt.h b/arch/x86/kernel/cpu/rdt.h
> index 2569c10c37f4..7205157d359b 100644
> --- a/arch/x86/kernel/cpu/rdt.h
> +++ b/arch/x86/kernel/cpu/rdt.h
> @@ -386,9 +386,9 @@ static inline bool is_mbm_event(int e)
>   * struct rdt_resource - attributes of an RDT resource
>   * @rid:		The index of the resource
>   * @alloc_enabled:	Is allocation enabled on this machine
> - * @mon_enabled:		Is monitoring enabled for this feature
> + * @mon_enabled:	Is monitoring enabled for this feature
>   * @alloc_capable:	Is allocation available on this machine
> - * @mon_capable:		Is monitor feature available on this machine
> + * @mon_capable:	Is monitor feature available on this machine
>   * @name:		Name to use in "schemata" file
>   * @num_closid:		Number of CLOSIDs available
>   * @cache_level:	Which cache level defines scope of this resource
> @@ -400,10 +400,12 @@ static inline bool is_mbm_event(int e)
>   * @cache:		Cache allocation related data
>   * @format_str:		Per resource format string to show domain value
>   * @parse_ctrlval:	Per resource function pointer to parse control values
> - * @evt_list:			List of monitoring events
> - * @num_rmid:			Number of RMIDs available
> - * @mon_scale:			cqm counter * mon_scale = occupancy in bytes
> - * @fflags:			flags to choose base and info files
> + * @update_mba_bw:	Feedback loop for MBA software controllerer function

controllerer -> controller ?

> + * @cbm_validate	Cache bitmask validate function
> + * @evt_list:		List of monitoring events
> + * @num_rmid:		Number of RMIDs available
> + * @mon_scale:		cqm counter * mon_scale = occupancy in bytes
> + * @fflags:		flags to choose base and info files
>   */
>  struct rdt_resource {
>  	int			rid;
> @@ -425,6 +427,9 @@ struct rdt_resource {
>  	const char		*format_str;
>  	int (*parse_ctrlval)	(void *data, struct rdt_resource *r,
>  				 struct rdt_domain *d);
> +	void (*update_mba_bw)   (struct rdtgroup *rgrp,
> +				 struct rdt_domain *dom_mbm);
> +	bool (*cbm_validate)    (char *buf, u32 *data, struct rdt_resource *r);
>  	struct list_head	evt_list;
>  	int			num_rmid;
>  	unsigned int		mon_scale;
> @@ -562,5 +567,7 @@ void cqm_setup_limbo_handler(struct rdt_domain *dom, unsigned long delay_ms);
>  void cqm_handle_limbo(struct work_struct *work);
>  bool has_busy_rmid(struct rdt_resource *r, struct rdt_domain *d);
>  void __check_limbo(struct rdt_domain *d, bool force_free);
> +void update_mba_bw(struct rdtgroup *rgrp, struct rdt_domain *dom_mbm);
> +bool cbm_validate(char *buf, u32 *data, struct rdt_resource *r);
>  
>  #endif /* _ASM_X86_RDT_H */
> diff --git a/arch/x86/kernel/cpu/rdt_ctrlmondata.c b/arch/x86/kernel/cpu/rdt_ctrlmondata.c
> index 0565c564b297..5a282b6c4bd7 100644
> --- a/arch/x86/kernel/cpu/rdt_ctrlmondata.c
> +++ b/arch/x86/kernel/cpu/rdt_ctrlmondata.c
> @@ -88,7 +88,7 @@ int parse_bw(void *_buf, struct rdt_resource *r, struct rdt_domain *d)
>   *	are allowed (e.g. FFFFH, 0FF0H, 003CH, etc.).
>   * Additionally Haswell requires at least two bits set.
>   */
> -static bool cbm_validate(char *buf, u32 *data, struct rdt_resource *r)
> +bool cbm_validate(char *buf, u32 *data, struct rdt_resource *r)
>  {
>  	unsigned long first_bit, zero_bit, val;
>  	unsigned int cbm_len = r->cache.cbm_len;
> @@ -153,7 +153,7 @@ int parse_cbm(void *_data, struct rdt_resource *r, struct rdt_domain *d)
>  		return -EINVAL;
>  	}
>  
> -	if (!cbm_validate(data->buf, &cbm_val, r))
> +	if ((r->cbm_validate) && !(r->cbm_validate(data->buf, &cbm_val, r)))

I do not think you need all of those brackets.

Reinette