Messages in this thread | | | Date | Tue, 16 Apr 2024 15:33:55 -0500 | Subject | Re: [PATCH v1 01/31] x86/resctrl: Fix allocation of cleanest CLOSID on platforms with no monitors | From | "Moger, Babu" <> |
| |
Hi Dave,
On 4/16/24 11:13, Dave Martin wrote: > On Mon, Apr 15, 2024 at 03:27:42PM -0500, Moger, Babu wrote: >> >> >> On 3/21/24 11:50, James Morse wrote: >>> commit 6eac36bb9eb0 ("x86/resctrl: Allocate the cleanest CLOSID by >>> searching closid_num_dirty_rmid") added a Kconfig option that causes >> >> This is not true. The Kconfig option is never added. It is added later in >> the series. There is also comment >> on this https://lore.kernel.org/lkml/ZhecyLQsGZ9Iv8wU@gmail.com/ >> >> >> Shouldn't the Kconfig option added first before doing this change? >> >>> resctrl to search for the CLOSID with the fewest dirty cache lines when >>> creating a new control group. This depends on the values read from the >>> llc_occupancy counters. > > See David's comments and previous discussion on this patch. > > You're right to point out that the description of the original commit > does seem a bit garbled though: CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID is > not present in Kconfig here, but already referenced by other code. > > We seem to have a consensus that it's OK to have a dangling IS_ENABLED() > so long as the option is added formally to Kconfig later, but it looks > like the commit message here should be reworded. > > Does the following make sense? > > --8<-- > > commit 6eac36bb9eb0 ("x86/resctrl: Allocate the cleanest CLOSID by > searching closid_num_dirty_rmid") added logic that causes resctrl to > search for the CLOSID with the fewest dirty cache lines when creating a > new control group, if requested by the arch code. This depends on the > values read from the llc_occupancy counters. The logic is applicable to > architectures where the CLOSID effectively forms part of the monitoring > identifier and so do not allow complete freedom to choose an unused > monitoring identifier for a given CLOSID. > > -->8--
That looks good. Thanks
> > > Although it would probably have been better if the Kconfig option had > been added upstream, this patch does not create that that situation and > the series (taken as a whole) resolves it. > > So I am not sure that anything would be solved or improved by changing > the body of this patch, but if people still have concerns then I guess > we can look at it.
> > >>> >>> This support missed that some platforms may not have these counters. >>> This causes a NULL pointer dereference when creating a new control >>> group as the array was not allocated by dom_data_init(). >>> >>> As this feature isn't necessary on platforms that don't have cache >>> occupancy monitors, add this to the check that occurs when a new >>> control group is allocated. >>> >>> The existing code is not selected by any upstream platform, it makes >>> no sense to backport this patch to stable. >>> >>> Signed-off-by: James Morse <james.morse@arm.com> >>> --- >>> arch/x86/kernel/cpu/resctrl/rdtgroup.c | 3 ++- >>> 1 file changed, 2 insertions(+), 1 deletion(-) >>> >>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c >>> index 011e17efb1a6..1767c1affa60 100644 >>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c >>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c >>> @@ -149,7 +149,8 @@ static int closid_alloc(void) >>> >>> lockdep_assert_held(&rdtgroup_mutex); >>> >>> - if (IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID)) { >>> + if (IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID) && >>> + is_llc_occupancy_enabled()) { >>> cleanest_closid = resctrl_find_cleanest_closid(); >>> if (cleanest_closid < 0) >>> return cleanest_closid; >> >> -- >> Thanks >> Babu Moger >> > > [...] > > Cheers > ---Dave
-- Thanks Babu Moger
| |