[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <140177ee-da8f-c6eb-caf6-af0775a3de0e@linux.alibaba.com>
Date: Tue, 25 Oct 2022 23:30:53 +0800
From: Shawn Wang <shawnwang@...ux.alibaba.com>
To: Reinette Chatre <reinette.chatre@...el.com>, fenghua.yu@...el.com
Cc: tglx@...utronix.de, mingo@...hat.com, bp@...en8.de,
dave.hansen@...ux.intel.com, x86@...nel.org, hpa@...or.com,
James Morse <james.morse@....com>, jamie@...iainc.com,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] x86/resctrl: Clear the stale staged config after the
configuration is completed
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():
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));
}
}
Thanks,
Shawn
Powered by blists - more mailing lists