Re: [PATCH v8 03/13] arch/resctrl: Re-arrange RDT init code

[Borislav Petkov]

On Fri, Nov 16, 2018 at 08:54:26PM +0000, Moger, Babu wrote:
> Separate the call sequence for rdt_quirks and MBA feature.
> This is in preparation to handle vendor differences in these
> call sequences.
> 
> Signed-off-by: Babu Moger <babu.moger@xxxxxxx>
> ---
>  arch/x86/kernel/cpu/resctrl.c | 27 +++++++++++++++++++++------
>  1 file changed, 21 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl.c b/arch/x86/kernel/cpu/resctrl.c
> index 5d526dc45751..4cea275c7c57 100644
> --- a/arch/x86/kernel/cpu/resctrl.c
> +++ b/arch/x86/kernel/cpu/resctrl.c
> @@ -794,6 +794,14 @@ static bool __init rdt_cpu_has(int flag)
>  	return ret;
>  }

Just nitpicks:

> +static __init bool rdt_mba_config(void)

That function doesn't have a verb in its name but it needs to have one
stating what it does. You could do

mv rdt_get_mem_config() -> __rdt_get_mem_config()
mv rdt_mba_config() -> rdt_get_mem_config()

to have the hierarchy of what calls what. And then the AMD variant will
be called __rdt_get_mem_config_amd().

Also, those are all static functions so you can just as well drop the
"rdt" prefix, I'd say.

> +{
> +	if (rdt_cpu_has(X86_FEATURE_MBA))
> +		return rdt_get_mem_config(&rdt_resources_all[RDT_RESOURCE_MBA]);
> +
> +	return false;
> +}
> +
>  static __init bool get_rdt_alloc_resources(void)
>  {
>  	bool ret = false;
> @@ -818,10 +826,9 @@ 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;
> -	}
> +	if (rdt_mba_config())
> +		ret = true;
> +
>  	return ret;
>  }
>  
> @@ -840,7 +847,7 @@ static __init bool get_rdt_mon_resources(void)
>  	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:
> @@ -855,9 +862,14 @@ static __init void rdt_quirks(void)
>  	}
>  }
>  
> +static __init void rdt_quirks(void)

Those functions also need to have a verb in the name stating what they
do.

> +{
> +	if (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL)
> +		rdt_quirks_intel();
> +}
> +
>  static __init bool get_rdt_resources(void)
>  {
> -	rdt_quirks();
>  	rdt_alloc_capable = get_rdt_alloc_resources();
>  	rdt_mon_capable = get_rdt_mon_resources();
>  
> @@ -871,6 +883,9 @@ static int __init resctrl_late_init(void)
>  	struct rdt_resource *r;
>  	int state, ret;
>  
> +	/* Run quirks first */
> +	rdt_quirks();

If the comment wasn't there, seeing "rdt_quirks();" doesn't say much and
makes me go look at what that function does.

> +
>  	if (!get_rdt_resources())

Unlike here, where it is clear that this gets the rdt resources.

Thx.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.