Messages in this thread | | | Subject | Re: [PATCH] x86/resctrl: fix an imbalance in domain_remove_cpu | From | Qian Cai <> | Date | Tue, 10 Dec 2019 13:06:20 -0500 |
| |
> On Dec 10, 2019, at 2:55 AM, Ryan Chen <yu.chen.surf@gmail.com> wrote: > > Hi Qian, > > On Sun, Dec 8, 2019 at 12:14 PM Qian Cai <cai@lca.pw> wrote: >> >> domain_add_cpu() calls domain_setup_mon_state() only when r->mon_capable >> is true where it will initialize d->mbm_over. However, >> domain_remove_cpu() calls cancel_delayed_work(&d->mbm_over) without >> checking r->mon_capable. Hence, it triggers a debugobjects warning when >> offlining CPUs because those timer debugobjects are never initialized. >> > Could you elaborate a little more on the failure symptom? > If I understand correctly, the error you described was due to > r->mon_capable set to false while is_mbm_enabled() returns true? > Which means on this platform rdt_mon_features is non zero? > And in get_rdt_mon_resources() it will invoke rdt_get_mon_l3_config(), > however the only possible failure to do not set r->mon_capable is that it > failed in dom_data_init() due to kcalloc() failure? Then the logic in > get_rdt_resources() is that it will ignore the return error if rdt allocate > feature is supported on this platform? If this is the case, the r->mon_capable > is not an indicator for whether the overflow thread has been created, right? > Can we simply remove the check of r->mon_capable in domain_add_cpu() and > invoke domain_setup_mon_state() directly?
Actually,
domain_add_cpu r->name = L3, r->alloc_capable = 1, r->mon_capable = 1 domain_add_cpu r->name = L3DATA, r->alloc_capable = 1, r->mon_capable = 0 domain_add_cpu r->name = L3CODE, r->alloc_capable = 1, r->mon_capable = 0
rdt_get_mon_l3_config() will only set r->mon_capable = 1 for L3.
>> ODEBUG: assert_init not available (active state 0) object type: >> timer_list hint: 0x0 >> WARNING: CPU: 143 PID: 789 at lib/debugobjects.c:484 >> debug_print_object+0xfe/0x140 >> Hardware name: HP Synergy 680 Gen9/Synergy 680 Gen9 Compute Module, BIOS >> I40 05/23/2018 >> RIP: 0010:debug_print_object+0xfe/0x140 >> Call Trace: >> debug_object_assert_init+0x1f5/0x240 >> del_timer+0x6f/0xf0 >> try_to_grab_pending+0x42/0x3c0 >> cancel_delayed_work+0x7d/0x150 >> resctrl_offline_cpu+0x3c0/0x520 >> cpuhp_invoke_callback+0x197/0x1120 >> cpuhp_thread_fun+0x252/0x2f0 >> smpboot_thread_fn+0x255/0x440 >> kthread+0x1e6/0x210 >> ret_from_fork+0x3a/0x50 >> >> Fixes: e33026831bdb ("x86/intel_rdt/mbm: Handle counter overflow") >> Signed-off-by: Qian Cai <cai@lca.pw> >> --- >> arch/x86/kernel/cpu/resctrl/core.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c >> index 03eb90d00af0..89049b343c7a 100644 >> --- a/arch/x86/kernel/cpu/resctrl/core.c >> +++ b/arch/x86/kernel/cpu/resctrl/core.c >> @@ -618,7 +618,7 @@ static void domain_remove_cpu(int cpu, struct rdt_resource *r) >> if (static_branch_unlikely(&rdt_mon_enable_key)) >> rmdir_mondata_subdir_allrdtgrp(r, d->id); >> list_del(&d->list); >> - if (is_mbm_enabled()) >> + if (r->mon_capable && is_mbm_enabled()) >> cancel_delayed_work(&d->mbm_over); > Humm, it looks like there are two places within this function > invoked cancel_delayed_work(&d->mbm_over), > why not adding the check for both of them?
Here it only check L3, so it will skip correctly for L3DATA and L3CODE to not call cancel_delayed_work(). Recalled the above that only L3 will have r->capable set.
if (r == &rdt_resources_all[RDT_RESOURCE_L3]) { if (is_mbm_enabled() && cpu == d->mbm_work_cpu) { cancel_delayed_work(&d->mbm_over);
Hence, r->mon_capable check seems redundant here.
| |