Messages in this thread | | | Date | Mon, 20 Mar 2023 17:12:19 +0000 | Subject | Re: [PATCH v2 06/18] x86/resctrl: Allow the allocator to check if a CLOSID can allocate clean RMID | From | James Morse <> |
| |
Hi Reinette,
On 10/03/2023 19:59, Reinette Chatre wrote: > On 3/3/2023 10:36 AM, James Morse wrote: >> On 02/02/2023 23:46, Reinette Chatre wrote: >>> On 1/13/2023 9:54 AM, James Morse wrote: > > ... > >>>> +bool resctrl_closid_is_dirty(u32 closid) >>>> +{ >>>> + struct rmid_entry *entry; >>>> + int i; >>>> + >>>> + lockdep_assert_held(&rdtgroup_mutex); >>>> + >>>> + if (!IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID)) >>>> + return false; >> >>> Why is a config option chosen? Is this not something that can be set in the >>> architecture specific code using a global in the form matching existing related >>> items like "arch_has..." or "arch_needs..."? >> >> It doesn't vary by platform, so making it a runtime variable would mean x86 has to carry >> this extra code around, even though it will never use it. Done like this, the compiler can >> dead-code eliminate the below checks and embed the constant return value in all the callers.
> This is fair. I missed any other mention of this option in this series so I > assume this will be a config that will be "select"ed automatically without > users needing to think about whether it is needed?
Yes, MPAM platforms would unconditionally enable it, as it reflects how MPAM works. [0] Users would never need to know it exists.
>>>> + for (i = 0; i < resctrl_arch_system_num_rmid_idx(); i++) { >>>> + entry = &rmid_ptrs[i]; >>>> + if (entry->closid != closid) >>>> + continue; >>>> + >>>> + if (entry->busy) >>>> + return true; >>>> + } >>>> + >>>> + return false; >>>> +} >>> >>> If I understand this correctly resctrl_closid_is_dirty() will return true if >>> _any_ RMID/PMG associated with a CLOSID is in use. That is, a CLOSID may be >>> able to support 100s of PMG but if only one of them is busy then the CLOSID >>> will be considered unusable ("dirty"). It sounds to me that there could be scenarios >>> when CLOSID could be considered unavailable while there are indeed sufficient >>> resources? >> >> You are right this could happen. I guess the better approach would be to prefer the >> cleanest CLOSID that has a clean PMG=0. User-space may not be able to allocate all the >> monitor groups immediately, but that would be preferable to failing the control group >> creation. >> >> But as this code doesn't get built until the MPAM driver is merged, I'd like to keep it to >> an absolute minimum. This would be more than is needed for MPAM to have close to resctrl >> feature-parity, so I'd prefer to do this as an improvement once the MPAM driver is upstream. >> >> (also in this category is better use of MPAM's monitors and labelling traffic from the iommu) >> >> >>> The function comment states "Determine if clean RMID can be allocate for this >>> CLOSID." - if I understand correctly it behaves more like "Determine if all >>> RMID associated with CLOSID are available". >> >> Yes, I'll fix that. > > I understand your reasoning for the solution chosen. Would you be ok to expand on > the function comment more to document the intentions that you summarize above (eg. "This > is the absolute minimum solution that will be/should be/could be improved ...")?
Sure thing,
Thanks,
James
[0] https://git.kernel.org/pub/scm/linux/kernel/git/morse/linux.git/tree/drivers/platform/mpam/Kconfig?h=mpam/snapshot/v6.2&id=ef6b11256ba2cceaff846c96183e8eb6019642d7
| |