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

[Reinette Chatre <reinette.chatre@intel.com>]

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