Re: [PATCH] x86/resctrl: Fix AMD L3 QOS CDP enable/disable

[Babu Moger <babu.moger@amd.com>]

Hi Reinette,


On 11/18/20 4:18 PM, Reinette Chatre wrote:
> Hi Babu,
> 
> On 11/6/2020 12:14 PM, Babu Moger wrote:
>> When the AMD QoS feature CDP(code and data prioritization) is enabled
>> or disabled, the CDP bit in MSR 0000_0C81 is written on one of the
>> cpus in L3 domain(core complex). That is not correct. The CDP bit needs
>> to be updated all the logical cpus in the domain.
> 
> Could you please use CPU instead of cpu throughout, in commit message as
> well as the new code comments?

Sure. Will do.

> 
>>
>> This was not spelled out clearly in the spec earlier. The specification
>> has been updated. The updated specification, "AMD64 Technology Platform
>> Quality of Service Extensions Publication # 56375 Revision: 1.02 Issue
>> Date: October 2020" is available now. Refer the section: Code and Data
>> Prioritization.
>>
>> Fix the issue by adding a new flag arch_needs_update_all in rdt_cache
>> data structure.
> 
> I understand that naming is hard and could be a sticky point. Even so, I
> am concerned that this name is too generic. For example, there are other
> cache settings that are successfully set on a single CPU in the L3 domain
> (the bitmasks for example). This new name and its description in the code
> comments below does not make it clear which cache settings it applies to.
> 
> I interpret this change to mean that the L[23]_QOS_CFG MSR has CPU scope
> while the other L3 QoS configuration registers have the same scope as the
> L3 cache. Could this new variable thus perhaps be named
> "arch_has_per_cpu_cfg"? I considered "arch_has_per_cpu_cdp" but when a new
> field is added to that register it may cause confusion.

Sounds good. Will change it to arch_has_per_cpu_cfg.
> 
>> The documentation can be obtained at the links below:
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdeveloper.amd.com%2Fwp-content%2Fresources%2F56375.pdf&data=04%7C01%7Cbabu.moger%40amd.com%7C3b793291233443b27f6d08d88c0fdc9f%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637413347449195250%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=%2B4AUG%2FbAeq70s0XuZ9J%2FOTTEFO8EypLvcR6yBuWE8U4%3D&reserved=0
>>
>> Link:
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.kernel.org%2Fshow_bug.cgi%3Fid%3D206537&data=04%7C01%7Cbabu.moger%40amd.com%7C3b793291233443b27f6d08d88c0fdc9f%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637413347449195250%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=4lzj2ivqxvFaLC99TOIJEGU3p6CmlBLCPHT80LlKsNE%3D&reserved=0
>>
>>
>> Fixes: 4d05bf71f157 ("x86/resctrl: Introduce AMD QOS feature")
>>
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> ---
>>   arch/x86/kernel/cpu/resctrl/core.c     |    3 +++
>>   arch/x86/kernel/cpu/resctrl/internal.h |    3 +++
>>   arch/x86/kernel/cpu/resctrl/rdtgroup.c |    9 +++++++--
>>   3 files changed, 13 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/x86/kernel/cpu/resctrl/core.c
>> b/arch/x86/kernel/cpu/resctrl/core.c
>> index e5f4ee8f4c3b..142c92a12254 100644
>> --- a/arch/x86/kernel/cpu/resctrl/core.c
>> +++ b/arch/x86/kernel/cpu/resctrl/core.c
>> @@ -570,6 +570,8 @@ static void domain_add_cpu(int cpu, struct
>> rdt_resource *r)
>>         if (d) {
>>           cpumask_set_cpu(cpu, &d->cpu_mask);
>> +        if (r->cache.arch_needs_update_all)
>> +            rdt_domain_reconfigure_cdp(r);
>>           return;
>>       }
>>   @@ -943,6 +945,7 @@ static __init void rdt_init_res_defs_amd(void)
>>               r->rid == RDT_RESOURCE_L2CODE) {
>>               r->cache.arch_has_sparse_bitmaps = true;
>>               r->cache.arch_has_empty_bitmaps = true;
>> +            r->cache.arch_needs_update_all = true;
>>           } else if (r->rid == RDT_RESOURCE_MBA) {
>>               r->msr_base = MSR_IA32_MBA_BW_BASE;
>>               r->msr_update = mba_wrmsr_amd;
> 
> The current pattern is to set these flags on all the architectures. Could
> you thus please set the flag within rdt_init_defs_intel()? I confirmed
> that the scope is the same as the cache domain in Intel RDT so the flag
> should be false.

Yes, Will add that.

> 
>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h
>> b/arch/x86/kernel/cpu/resctrl/internal.h
>> index 80fa997fae60..d23262d59a51 100644
>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>> @@ -360,6 +360,8 @@ struct msr_param {
>>    *            executing entities
>>    * @arch_has_sparse_bitmaps:    True if a bitmap like f00f is valid.
>>    * @arch_has_empty_bitmaps:    True if the '0' bitmap is valid.
>> + * @arch_needs_update_all:    True if arch needs to update the cache
>> + *                settings on all the cpus in the domain.
> 
> Please do update this to make it clear what "cache settings" are referred
> to. Since this is in struct rdt_cache perhaps something like "QOS_CFG
> register for this cache level has CPU scope."

Sure.
> 
>>    */
>>   struct rdt_cache {
>>       unsigned int    cbm_len;
>> @@ -369,6 +371,7 @@ struct rdt_cache {
>>       unsigned int    shareable_bits;
>>       bool        arch_has_sparse_bitmaps;
>>       bool        arch_has_empty_bitmaps;
>> +    bool        arch_needs_update_all;
>>   };
>>     /**
>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> index af323e2e3100..a005e90b373a 100644
>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> @@ -1905,8 +1905,13 @@ static int set_cache_qos_cfg(int level, bool enable)
>>         r_l = &rdt_resources_all[level];
>>       list_for_each_entry(d, &r_l->domains, list) {
>> -        /* Pick one CPU from each domain instance to update MSR */
>> -        cpumask_set_cpu(cpumask_any(&d->cpu_mask), cpu_mask);
>> +        if (r_l->cache.arch_needs_update_all)
>> +            /* Pick all the cpus in the domain instance */
>> +            for_each_cpu(cpu, &d->cpu_mask)
>> +                cpumask_set_cpu(cpu, cpu_mask);
>> +        else
>> +            /* Pick one CPU from each domain instance to update MSR */
>> +            cpumask_set_cpu(cpumask_any(&d->cpu_mask), cpu_mask);
>>       }
>>       cpu = get_cpu();
>>       /* Update QOS_CFG MSR on this cpu if it's in cpu_mask. */
>>
> 
> The solution looks good to me, thank you very much.

Thanks for the review.
-Babu