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/25/2022 8:30 AM, Shawn Wang wrote:
> Hi Reinette,
> 
> On 10/25/2022 12:45 AM, Reinette Chatre wrote:
>> Hi Shawn,
>>
>> On 10/23/2022 7:31 PM, Shawn Wang wrote:
>>> On 10/22/2022 2:05 AM, Reinette Chatre wrote:
>>>
>>> ...
>>>
>>>>> 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.
>>>>
>>>
>>> I want to make sure, do you mean just ignore the stale value and
>>> place the clearing work before staged_config[] is used? If so, maybe
>>> the only thing the fix should do is to add memset() to
>>> rdtgroup_init_alloc().>
>>
>> No, let us not leave stale data lying around.
>>
>> The idea is that the function calling resctrl_arch_update_domains() is
>> responsible for initializing staged_config[] correctly and completely.
>> To confirm, yes, the idea is to clear the staged_config[] in
>> rdtgroup_init_alloc() before resctrl_arch_update_domains() is called
>> to follow how it is currently done in rdtgroup_schemata_write().
>>
>> But, as you indicate, by itself this would leave stale data lying around.
>>
>> The solution that you suggested earlier, to put the clearing work where
>> rdtgroup_schemata_write() and rdtgroup_init_alloc() exit, is most logical.
>> That makes the code symmetrical in that staged_config[] is cleared
>> where it is initialized and no stale data is left lying around. What was
>> not clear to me is how this would look in the end. Were you planning to
>> keep the staged_config[] clearing within rdtgroup_schemata_write() but
>> not do so in rdtgroup_init_alloc()? rdtgroup_schemata_write() and
>> rdtgroup_init_alloc() has to follow the same pattern to reduce confusion.
>>
>> So, to be more robust, how about:
>>
>> /* Clear staged_config[] to make sure working from a clean slate */
>> resctrl_arch_update_domains()
>> /* Clear staged_config[] to not leave stale data lying around */
>>
> 
> Thank you for your explanation, and it makes sense to me. But this will
> require 4 memset() loops, how about putting the clearing work in
> a separate function in rdtgroup.c, like rdt_last_cmd_clear():

Yes, thanks for avoiding duplicating code.

> 
> void staged_configs_clear(void) {
>     struct resctrl_schema *s;
>     struct rdt_domain *dom;
> 
>     lockdep_assert_held(&rdtgroup_mutex);
> 
>     list_for_each_entry(s, &resctrl_schema_all, list) {
>         list_for_each_entry(dom, &s->res->domains, list)
>             memset(dom->staged_config, 0, sizeof(dom->staged_config));
>     }
> }
> 

I understand that you are just copying what is currently done in
rdtgroup_schemata_write() but for a separate function I think something
like below would be more efficient:


	for_each_alloc_capable_rdt_resource(r) {
		list_for_each_entry(dom, &r->domains, list)
			memset(dom->staged_config, 0, sizeof(dom->staged_config));
	}

This would be more efficient since it would not clean the same memory area
twice when CDP is enabled.

Reinette