Messages in this thread | | | Date | Fri, 21 Oct 2022 11:05:28 -0700 | Subject | Re: [PATCH v2] x86/resctrl: Clear the stale staged config after the configuration is completed | From | Reinette Chatre <> |
| |
Hi Shawn,
On 10/21/2022 1:22 AM, Shawn Wang wrote: > Hi Reinette, > > On 10/21/2022 12:35 AM, Reinette Chatre wrote: > > ... > >>>>> 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(). >> >> I was referring to a common exit out of resctrl_arch_update_domains(). >> >> Look at how resctrl_arch_update_domains() behaves with this change: >> >> resctrl_arch_update_domains() >> { >> ... >> >> if (!zalloc_cpumask_var(&cpu_mask, GFP_KERNEL)) >> return -ENOMEM; >> >> ... >> list_for_each_entry(d, &r->domains, list) { >> ... >> memset(d->staged_config, 0, sizeof(d->staged_config)); >> } >> >> >> ... >> done: >> free_cpumask_var(cpu_mask); >> >> return 0; >> } >> >> >> The goal of this fix is to ensure that staged_config[] is cleared on >> return from resctrl_arch_update_domains() so that there is no stale >> data in staged_config[] when resctrl_arch_update_domains() is called >> again. >> >> Considering this, I can see two scenarios in the above solution where >> staged_config[] is not cleared on exit from resctrl_arch_update_domains(): > > It may not be enough to just clear staged_config[] when > resctrl_arch_update_domains() exits. I think the fix needs to make > sure staged_config[] can be cleared where it is set. > > The modification of staged_config[] comes from two paths: > > Path 1: > rdtgroup_schemata_write() { > ... > rdtgroup_parse_resource() // set staged_config[] > ... > resctrl_arch_update_domains() // clear staged_config[] > ... > } > > Path 2: > rdtgroup_init_alloc() { > ... > rdtgroup_init_mba()/rdtgroup_init_cat() // set staged_config[] > ... > resctrl_arch_update_domains() // clear staged_config[] > ... > } > > If we clear staged_config[] in resctrl_arch_update_domains(), goto > statement for error handling between setting staged_config[] and > calling resctrl_arch_update_domains() will be ignored. This can still > remain the stale staged_config[]. ah - indeed. Thank you for catching that.
> > I think maybe it is better to put the clearing work where > rdtgroup_schemata_write() and rdtgroup_init_alloc() exit. >
It may be more robust to let rdtgroup_init_alloc() follow how rdtgroup_schemata_write() already ensures that it is working with a clean state by clearing staged_config[] before placing its staged config within.
Reinette
| |