Re: [PATCH v2] x86/resctrl: Clear the stale staged config after the configuration is completed
From: Shawn Wang
Date: Thu Oct 20 2022 - 01:56:16 EST
Hi Reinette,
Sorry for replying now due to other things.
On 10/12/2022 7:48 AM, Reinette Chatre wrote:
Hi Shawn,
Thank you very much for working on getting this fixed.
On 10/9/2022 1:36 AM, Shawn Wang wrote:
As a temporary storage, array staged_config in struct rdt_domain is not
staged_config -> staged_config[]
cleared after it has been used. The stale value in staged_config could
staged_config -> staged_config[]
(please make the above change in rest of changelog, doing so makes it
easier to read)
cause a MSR access error.
a MSR -> an MSR
If resctrl is mounted with CDP enabled and then remounted with CDP
disabled, the value of staged_config in domainX via mkdir changes as
follows:
CDP enabled:
mkdir <resource control group>
domainX.staged_config[CDP_NONE].have_new_ctrl = false
domainX.staged_config[CDP_NONE].new_ctrl = 0
domainX.staged_config[CDP_CODE].have_new_ctrl = true
domainX.staged_config[CDP_CODE].new_ctrl = <default mask>
domainX.staged_config[CDP_DATA].have_new_ctrl = true
domainX.staged_config[CDP_CODE].new_ctrl = <default mask>
Apologies, you copied a copy&paste error from me ... the last one
should be CDP_DATA.
unmount/remount resctrl (CDP disabled):
mkdir <resource control group>
domainX.staged_config[CDP_NONE].have_new_ctrl = true
domainX.staged_config[CDP_NONE].new_ctrl = <default mask>
domainX.staged_config[CDP_CODE].have_new_ctrl = true /* stale */
domainX.staged_config[CDP_CODE].new_ctrl = <default mask> /* stale */
domainX.staged_config[CDP_DATA].have_new_ctrl = true /* stale */
domainX.staged_config[CDP_CODE].new_ctrl = <default mask> /* stale */
(same typo here)
When creating a new resource control group, hardware will be configured by
resctrl_arch_update_domains():
rdtgroup_mkdir()->rdtgroup_mkdir_ctrl_mon()->rdtgroup_init_alloc()->resctrl_arch_update_domains()
Since resctrl_arch_update_domains() iterates and updates all
resctrl_conf_type whose have_new_ctrl is true, it will continue to update
the stale CDP_CODE and CDP_DATA configurations when CDP is disabled.
Based on the above analysis, an error can be reproduced on a system with
16 usable CLOSIDs for a 15-way L3 Cache (MBA should be disabled if the
number of CLOSIDs for MB is less than 16.) :
mount -t resctrl resctrl -o cdp /sys/fs/resctrl
mkdir /sys/fs/resctrl/p{1..7}
umount /sys/fs/resctrl/
mount -t resctrl resctrl /sys/fs/resctrl
mkdir /sys/fs/resctrl/p{1..8}
dmesg will generate the following error:
I think it would be helpful to connect this reproducer to the
explanation of what happens. Maybe just something like:
"Upon creating directory "p8", to which CLOSID 8 is automatically
assigned, the following error is generated:"
Please consider what is the most useful information found in the backtrace
and only include that. Interesting enough, there is a very related example
of what information is useful in Documentation/process/submitting-patches.rst
(section "Backtraces in commit messages").
[ 6180.939345] unchecked MSR access error: WRMSR to 0xca0 (tried to write
0x00000000000007ff) at rIP: 0xffffffff82249142 (cat_wrmsr+0x32/0x60)
[ 6180.951983] Call Trace:
[ 6180.954516] <IRQ>
[ 6180.956619] __flush_smp_call_function_queue+0x11d/0x170
[ 6180.962028] __sysvec_call_function+0x24/0xd0
[ 6180.966485] sysvec_call_function+0x89/0xc0
[ 6180.970760] </IRQ>
[ 6180.972947] <TASK>
[ 6180.975131] asm_sysvec_call_function+0x16/0x20
[ 6180.979757] RIP: 0010:cpuidle_enter_state+0xcd/0x400
[ 6180.984821] Code: 49 89 c5 0f 1f 44 00 00 31 ff e8 1e e5 77 ff 45 84
ff 74 12 9c 58 f6 c4 02 0f 85 13 03 00 00 31 ff e8 67 70 7d ff fb 45 85
f6 <0f> 88 75 01 00 00 49 63 c6 4c 2b 2c 24 48 8d 14 40 48 8d 14 90 49
[ 6181.003710] RSP: 0018:ffffffff83a03e48 EFLAGS: 00000202
[ 6181.009028] RAX: ffff943400800000 RBX: 0000000000000001 RCX: 000000000000001f
[ 6181.016261] RDX: 0000000000000000 RSI: ffffffff83795059 RDI: ffffffff837c101e
[ 6181.023490] RBP: ffff9434c9352000 R08: 0000059f1cb1a05e R09: 0000000000000008
[ 6181.030717] R10: 0000000000000001 R11: 0000000000005c66 R12: ffffffff83bbf3a0
[ 6181.037944] R13: 0000059f1cb1a05e R14: 0000000000000001 R15: 0000000000000000
[ 6181.045202] ? cpuidle_enter_state+0xb2/0x400
[ 6181.049678] cpuidle_enter+0x24/0x40
[ 6181.053370] do_idle+0x1dd/0x260
[ 6181.056713] cpu_startup_entry+0x14/0x20
[ 6181.060753] rest_init+0xbb/0xc0
[ 6181.064097] arch_call_rest_init+0x5/0xa
[ 6181.068137] start_kernel+0x668/0x691
[ 6181.071914] secondary_startup_64_no_verify+0xe0/0xeb
[ 6181.077086] </TASK>
As Reinette Chatre explained:
https://lore.kernel.org/all/2728c354-ac75-be4c-66ad-86ebd9c50248@xxxxxxxxx/
"
The value of interest here is the register it tries to write to ... 0xca0.
On a system with 16 CLOSIDs the range of registers available to set the
CBM would be 0xc90 to 0xc9f that corresponds to CLOSID 0 to CLOSID 15.
The error is an attempt to write to an unsupported register - there
appears to have been an attempt to configure non-existent CLOSID 16.
Above becomes an issue when the resource group being created is for a
CLOSID # that is more than half of the CLOSIDs supported. In the
reproducer the issue was encountered when creating resource group for
CLOSID 8 on a system that supports 16 CLOSIDs.
In this case get_config_index() called from resctrl_arch_update_domains()
will return 16 and 17 when processing this resource group and that
translated to an invalid register - 0xca0 in this scenario.
"
I am not sure if adding this full quote is necessary to explain the issue.
Maybe you could just summarize it and move it to before the "Based on the
above analysis ..."
Thank you very much for your suggestion, I will revise it in the new
version.
Fix this issue by clearing the staged configs when the configuration is
completed.
Fixes: 75408e43509ed ("x86/resctrl: Allow different CODE/DATA configurations to be staged")
Cc: <stable@xxxxxxxxxxxxxxx>
Signed-off-by: Shawn Wang <shawnwang@xxxxxxxxxxxxxxxxx>
Suggested-by: Xin Hao <xhao@xxxxxxxxxxxxxxxxx>
---
Changes since v1:
- Move the clearing from schemata_list_destroy() to resctrl_arch_update_domains().
- Update the commit message suggested by Reiniette Chatre.
- Add stable tag suggested by James Morse.
---
arch/x86/kernel/cpu/resctrl/ctrlmondata.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
index 1dafbdc5ac31..2c719da5544f 100644
--- a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
+++ b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
@@ -338,6 +338,8 @@ int resctrl_arch_update_domains(struct rdt_resource *r, u32 closid)
msr_param.high = max(msr_param.high, idx + 1);
}
}
+ /* Clear the stale staged config */
+ memset(d->staged_config, 0, sizeof(d->staged_config));
}
if (cpumask_empty(cpu_mask))
Please also ensure that the temporary storage is cleared if there is an
early exist because of failure. Please do not duplicate the memset() code
but instead move it to a common exit location.
There are two different resctrl_arch_update_domains() function call paths:
1.rdtgroup_mkdir()->rdtgroup_mkdir_ctrl_mon()->rdtgroup_init_alloc()->resctrl_arch_update_domains()
2.rdtgroup_schemata_write()->resctrl_arch_update_domains()
Perhaps there is no common exit location if we want to clear
staged_config[] after every call of resctrl_arch_update_domains().
How about doing the cleanup at the end of rdtgroup_schemata_write() and
rdtgroup_mkdir_ctrl_mon() respectively?
Thank you,
Shawn