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

["Moger, Babu" <Babu.Moger@amd.com>]

On 10/02/2018 05:07 PM, Reinette Chatre wrote:
> 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.

Sure.  Will make this change.

> 
>> +
>> +		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.

Yes.  Will add _intel to these functions.

> 
>>  		}
>>  	}
>>  }
>> 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 ?

Yes. Will fix it.

> 
>> + * @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.

Yes. Will simplify this.

> 
> Reinette
>