Re: [PATCH v2] x86/resctrl: Clear the stale staged config after the configuration is completed

Shawn Wang
2022-10-21T08:23:26Z

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[].

I think maybe it is better to put the clearing work where
rdtgroup_schemata_write() and rdtgroup_init_alloc() exit.

(Sorry, I mistakenly wrote rdtgroup_init_alloc() to
rdtgroup_mkdir_ctrl_mon() in my last reply.)

Thank you,

Shawn