Messages in this thread | | | Date | Tue, 25 Oct 2022 23:30:53 +0800 | From | Shawn Wang <> | 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
| |