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

[Moger, Babu]

Boris,

> -----Original Message-----
> From: Borislav Petkov <bp@xxxxxxxxx>
> Sent: Tuesday, November 20, 2018 3:28 AM
> To: Moger, Babu <Babu.Moger@xxxxxxx>
> Cc: tglx@xxxxxxxxxxxxx; mingo@xxxxxxxxxx; corbet@xxxxxxx;
> fenghua.yu@xxxxxxxxx; reinette.chatre@xxxxxxxxx; peterz@xxxxxxxxxxxxx;
> gregkh@xxxxxxxxxxxxxxxxxxx; davem@xxxxxxxxxxxxx; akpm@linux-
> foundation.org; hpa@xxxxxxxxx; x86@xxxxxxxxxx;
> mchehab+samsung@xxxxxxxxxx; arnd@xxxxxxxx;
> kstewart@xxxxxxxxxxxxxxxxxxx; pombredanne@xxxxxxxx;
> rafael@xxxxxxxxxx; kirill.shutemov@xxxxxxxxxxxxxxx; tony.luck@xxxxxxxxx;
> qianyue.zj@xxxxxxxxxxxxxxx; xiaochen.shen@xxxxxxxxx;
> pbonzini@xxxxxxxxxx; Singh, Brijesh <brijesh.singh@xxxxxxx>; Hurwitz,
> Sherry <sherry.hurwitz@xxxxxxx>; dwmw2@xxxxxxxxxxxxx; Lendacky,
> Thomas <Thomas.Lendacky@xxxxxxx>; luto@xxxxxxxxxx; joro@xxxxxxxxxx;
> jannh@xxxxxxxxxx; vkuznets@xxxxxxxxxx; rian@xxxxxxxxxxxx;
> jpoimboe@xxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux-
> doc@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v8 03/13] arch/resctrl: Re-arrange RDT init code
> 
> 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.

Ok. Sure. Will take care of these.

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

Ok. Will change it to rdt_check_quirks.

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

Will remove the comment and rename the function to rdt_check_quirks().

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