[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20181120092740.GA2527@zn.tnic>
Date: Tue, 20 Nov 2018 10:27:40 +0100
From: Borislav Petkov <bp@...en8.de>
To: "Moger, Babu" <Babu.Moger@....com>
Cc: "tglx@...utronix.de" <tglx@...utronix.de>,
"mingo@...hat.com" <mingo@...hat.com>,
"corbet@....net" <corbet@....net>,
"fenghua.yu@...el.com" <fenghua.yu@...el.com>,
"reinette.chatre@...el.com" <reinette.chatre@...el.com>,
"peterz@...radead.org" <peterz@...radead.org>,
"gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
"davem@...emloft.net" <davem@...emloft.net>,
"akpm@...ux-foundation.org" <akpm@...ux-foundation.org>,
"hpa@...or.com" <hpa@...or.com>, "x86@...nel.org" <x86@...nel.org>,
"mchehab+samsung@...nel.org" <mchehab+samsung@...nel.org>,
"arnd@...db.de" <arnd@...db.de>,
"kstewart@...uxfoundation.org" <kstewart@...uxfoundation.org>,
"pombredanne@...b.com" <pombredanne@...b.com>,
"rafael@...nel.org" <rafael@...nel.org>,
"kirill.shutemov@...ux.intel.com" <kirill.shutemov@...ux.intel.com>,
"tony.luck@...el.com" <tony.luck@...el.com>,
"qianyue.zj@...baba-inc.com" <qianyue.zj@...baba-inc.com>,
"xiaochen.shen@...el.com" <xiaochen.shen@...el.com>,
"pbonzini@...hat.com" <pbonzini@...hat.com>,
"Singh, Brijesh" <brijesh.singh@....com>,
"Hurwitz, Sherry" <sherry.hurwitz@....com>,
"dwmw2@...radead.org" <dwmw2@...radead.org>,
"Lendacky, Thomas" <Thomas.Lendacky@....com>,
"luto@...nel.org" <luto@...nel.org>,
"joro@...tes.org" <joro@...tes.org>,
"jannh@...gle.com" <jannh@...gle.com>,
"vkuznets@...hat.com" <vkuznets@...hat.com>,
"rian@...m.mit.edu" <rian@...m.mit.edu>,
"jpoimboe@...hat.com" <jpoimboe@...hat.com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>
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@....com>
> ---
> 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.
Powered by blists - more mailing lists