Messages in this thread | | | Date | Wed, 26 Oct 2022 19:03:36 +0800 | Subject | Re: [PATCH v2] x86/resctrl: Clear the stale staged config after the configuration is completed | From | Shawn Wang <> |
| |
Hi Reinette,
On 10/26/2022 3:34 AM, Reinette Chatre wrote: > 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. >
Thank you, I didn't notice this function before. I will add it in a new version.
Shawn
| |