Re: [RFC PATCH 03/10] arch/x86: Re-arrange RDT init code

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

Hi Reinette,
Thanks for the review. My response below.

On 10/02/2018 02:21 PM, Reinette Chatre wrote:
> Hi Babu,
> 
> On 9/24/2018 12:19 PM, Moger, Babu wrote:
>> Re-organize the RDT init code. Separate the call sequence for each
>> feature. That way, it is easy to call quirks or features separately
>> for each vendor if there are differences.
>>
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> ---
>>  arch/x86/kernel/cpu/rdt.c | 44 ++++++++++++++++++++++++++++-----------
>>  1 file changed, 32 insertions(+), 12 deletions(-)
>>
>> diff --git a/arch/x86/kernel/cpu/rdt.c b/arch/x86/kernel/cpu/rdt.c
>> index b361c63170d7..736715b81fd8 100644
>> --- a/arch/x86/kernel/cpu/rdt.c
>> +++ b/arch/x86/kernel/cpu/rdt.c
>> @@ -813,10 +813,6 @@ static __init bool get_rdt_alloc_resources(void)
>>  		ret = true;
>>  	}
>>  
>> -	if (rdt_cpu_has(X86_FEATURE_MBA)) {
>> -		if (rdt_get_mem_config(&rdt_resources_all[RDT_RESOURCE_MBA]))
>> -			ret = true;
>> -	}
> 
> The commit message mentions that the call sequence for each feature is
> separated, but here only the MBA feature is separated.

Yes. MBA and quirks are separated. I will fix the commit message. I
overlooked some of the errors returned by these functions. Let me go back
and update this patch. Will keep mostly as is. Only separate MBA and
quirks which are important. Will make sure errors are propagated properly.

> 
> The MBA feature detection is removed above .... (more later)
> 
>>  	return ret;
>>  }
>>  
>> @@ -831,11 +827,12 @@ static __init bool get_rdt_mon_resources(void)
>>  
>>  	if (!rdt_mon_features)
>>  		return false;
>> +	else
>> +		return true;
>>  
>> -	return !rdt_get_mon_l3_config(&rdt_resources_all[RDT_RESOURCE_L3]);
>>  }
>>  
>> -static __init void rdt_quirks(void)
>> +static __init void rdt_quirks_intel(void)
>>  {
>>  	switch (boot_cpu_data.x86_model) {
>>  	case INTEL_FAM6_HASWELL_X:
>> @@ -850,13 +847,22 @@ static __init void rdt_quirks(void)
>>  	}
>>  }
>>  
>> -static __init bool get_rdt_resources(void)
>> +static __init void rdt_quirks(void)
>>  {
>> -	rdt_quirks();
>> -	rdt_alloc_capable = get_rdt_alloc_resources();
>> -	rdt_mon_capable = get_rdt_mon_resources();
>> +	if (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL)
>> +		rdt_quirks_intel();
>> +}
>> +
>> +static __init void rdt_detect_l3_mon(void)
>> +{
>> +	if (rdt_mon_capable)
>> +		rdt_get_mon_l3_config(&rdt_resources_all[RDT_RESOURCE_L3]);
> 
> The possible errors from this configuration is now lost.

Yes. I overlooked it. Same comment as above. Let me go back and update
this patch.
> 
>> +}
>>  
>> -	return (rdt_mon_capable || rdt_alloc_capable);
>> +static __init void rdt_check_mba(void)
>> +{
>> +	if (rdt_cpu_has(X86_FEATURE_MBA))
>> +		rdt_get_mem_config(&rdt_resources_all[RDT_RESOURCE_MBA]);
> 
> Here too the possible failure of this configuration is now lost.

Ditto.. Let me go back and update this patch.

> 
>>  }
>>  
>>  static enum cpuhp_state rdt_online;
>> @@ -866,8 +872,22 @@ static int __init rdt_late_init(void)
>>  	struct rdt_resource *r;
>>  	int state, ret;
>>  
>> -	if (!get_rdt_resources())
>> +	/* Run quirks first */
>> +	rdt_quirks();
>> +
>> +	rdt_alloc_capable = get_rdt_alloc_resources();
>> +	rdt_mon_capable = get_rdt_mon_resources();
>> +
>> +	if (!(rdt_alloc_capable || rdt_mon_capable)) {
>> +		pr_info("RDT allocation or monitoring not detected\n");
> 
> This function ends with a log entry for every resource discovered. Is
> this new log entry needed to indicate that such resources have not been
> found? Could it not just be the absence of the other message?

As this is relatively new feature, so I added this info message. It helped
me debug what went wrong. Otherwise, I don't see anything. I can remove it
if the message is too annoying to the user.

> 
>>  		return -ENODEV;
>> +	}
> 
> ... (continued from above) ... since the MBA feature detection was
> removed from get_rdt_alloc_resources() would the above not cause failure
> on systems that only support MBA?

yes. Let me go back and update this patch.
> 
>> +
>> +	/* Detect l3 monitoring resources */
> 
> I do not think this comment is accurate ... has the monitoring resources
> not been detected earlier in get_rdt_mon_resources() and now they will
> be configured?
> 
>> +	rdt_detect_l3_mon();
>> +
>> +	/* Check for Memory Bandwidth Allocation */
>> +	rdt_check_mba();
> 
> To follow up on above .. the potential failure of these configurations
> are now lost here. Initialization should not continue if these
> configurations failed.

Yes. Let me go back and update this patch.

> 
>>  
>>  	rdt_init_padding();
>>  
>>
> 
> Reinette
>