Re: [RFC PATCH 03/10] arch/x86: Re-arrange RDT init code
On 10/03/2018 01:54 PM, Reinette Chatre wrote:
> Hi Babu,
>
> On 10/2/2018 4:41 PM, Moger, Babu wrote:
>> On 10/02/2018 02:21 PM, Reinette Chatre wrote:
>>> On 9/24/2018 12:19 PM, Moger, Babu wrote:
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.
>
> This log entry is made after detection of resources/features supported
> by the system. A user would find more information in the
> presence/absence of the relevant CPU feature flags in /proc/cpuinfo.
Ok. Understood. I have dropped this log entry.
>
> Reinette
>
Re: [RFC PATCH 03/10] arch/x86: Re-arrange RDT init code
Hi Babu,
On 10/2/2018 4:41 PM, Moger, Babu wrote:
> On 10/02/2018 02:21 PM, Reinette Chatre wrote:
>> On 9/24/2018 12:19 PM, Moger, Babu wrote:
>>> 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.
This log entry is made after detection of resources/features supported
by the system. A user would find more information in the
presence/absence of the relevant CPU feature flags in /proc/cpuinfo.
Reinette
Re: [RFC PATCH 03/10] arch/x86: Re-arrange RDT init code
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
>> ---
>> 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
>
Re: [RFC PATCH 03/10] arch/x86: Re-arrange RDT init code
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
> ---
> 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.
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.
> +}
>
> - 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.
> }
>
> 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?
> 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?
> +
> + /* 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.
>
> rdt_init_padding();
>
>
Reinette
[RFC PATCH 03/10] arch/x86: Re-arrange RDT init code
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
---
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;
- }
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]);
+}
- 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]);
}
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");
return -ENODEV;
+ }
+
+ /* Detect l3 monitoring resources */
+ rdt_detect_l3_mon();
+
+ /* Check for Memory Bandwidth Allocation */
+ rdt_check_mba();
rdt_init_padding();
--
2.17.1
