Messages in this thread | | | Date | Tue, 16 Apr 2024 17:16:50 +0100 | From | Dave Martin <> | Subject | Re: [PATCH v1 05/31] x86/resctrl: Remove rdtgroup from update_cpu_closid_rmid() |
| |
On Mon, Apr 15, 2024 at 10:47:55AM -0700, Reinette Chatre wrote: > Hi Dave, > > On 4/12/2024 9:12 AM, Dave Martin wrote: > > On Mon, Apr 08, 2024 at 08:16:08PM -0700, Reinette Chatre wrote: > >> Hi James, > >> > >> On 3/21/2024 9:50 AM, James Morse wrote: > >>> update_cpu_closid_rmid() takes a struct rdtgroup as an argument, which > >>> it uses to update the local CPUs default pqr values. This is a problem > >>> once the resctrl parts move out to /fs/, as the arch code cannot > >>> poke around inside struct rdtgroup. > >>> > >>> Rename update_cpu_closid_rmid() as resctrl_arch_sync_cpus_defaults() > >>> to be used as the target of an IPI, and pass the effective CLOSID > >>> and RMID in a new struct. > >>> > >>> Signed-off-by: James Morse <james.morse@arm.com> > >>> --- > >>> arch/x86/kernel/cpu/resctrl/rdtgroup.c | 19 +++++++++++++++---- > >>> include/linux/resctrl.h | 11 +++++++++++ > >>> 2 files changed, 26 insertions(+), 4 deletions(-) > >>> > >>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c > >>> index 5d2c1ce5b6b1..18f097fce51e 100644 > >>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c > >>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c > >>> @@ -341,13 +341,13 @@ static int rdtgroup_cpus_show(struct kernfs_open_file *of, > >>> * from update_closid_rmid() is protected against __switch_to() because > >>> * preemption is disabled. > >>> */ > >>> -static void update_cpu_closid_rmid(void *info) > >>> +void resctrl_arch_sync_cpu_defaults(void *info) > >>> { > >>> - struct rdtgroup *r = info; > >>> + struct resctrl_cpu_sync *r = info; > >>> > >>> if (r) { > >>> this_cpu_write(pqr_state.default_closid, r->closid); > >>> - this_cpu_write(pqr_state.default_rmid, r->mon.rmid); > >>> + this_cpu_write(pqr_state.default_rmid, r->rmid); > >>> } > >>> > >>> /* > >>> @@ -362,11 +362,22 @@ static void update_cpu_closid_rmid(void *info) > >>> * Update the PGR_ASSOC MSR on all cpus in @cpu_mask, > >>> * > >>> * Per task closids/rmids must have been set up before calling this function. > >>> + * @r may be NULL. > >>> */ > >>> static void > >>> update_closid_rmid(const struct cpumask *cpu_mask, struct rdtgroup *r) > >>> { > >>> - on_each_cpu_mask(cpu_mask, update_cpu_closid_rmid, r, 1); > >>> + struct resctrl_cpu_sync defaults; > >>> + struct resctrl_cpu_sync *defaults_p = NULL; > >> > >> Please maintain reverse fir order. > > > > Or, more tersely as follows? > > > > struct resctrl_cpu_sync defaults, *defaults_p = NULL; > > Sure.
[*]
> > > > "Reverse fir order" seems to be documented as a preference rather than a > > rule. > > This does not seem to be a place that warrants an exception to this > preference. Note how this function is not consistent with any other > in the file.
Ack (just bikeshedding here TBH).
> > > The declarations can be swapped, but defaults_p is in some sense a weak > > pointer to defaults, so it feels a bit strange to declare them backwards. > > > > Alternatively, could we rename defaults_p to p? Given the size of this > > function I don't think that impacts clarity.
[...]
> > > > I'll wait for your opinion on this. > > > > > > Do you imply that this would maintain the order in this patch? It does > not look to me that it would but I may be looking wrong.
I'm not sure without looking again, but since this discussion is not a good use of your time I'll just go ahead and implement the change at [*] above, while restoring referse FIR order, if that is good for you.
> > sidenote: the "on_each_cpu_mask()" in update_closid_rmid() can be on > one line.
I guess that might have been split to stick to the 80-char limit.
Due the the small size of this function, shall I just rename defaults_p to p? Alternatively, there are already a few non-printk lines over 80 chars, so maybe we can tolerate one more here?
> > .. > > >>> + * struct resctrl_cpu_sync, or NULL. > >>> + */ > >> > >> Updating the CPU's defaults is not the primary goal of this function and because > >> of that I do not think this should be the focus with the main goal (updating > >> RMID and CLOSID on CPU) ignored. Specifically, this function only updates > >> the defaults if *info is set but it _always_ ensures CPU is running with > >> appropriate CLOSID/RMID (which may or may not be from a CPU default). > >> > >> I think resctrl_arch_sync_cpu_closid_rmid() may be more appropriate > >> and the comment needs to elaborate what the function does. > >> > >>> +void resctrl_arch_sync_cpu_defaults(void *info); > > > > That seems reasonable, and follows the original naming and what the > > code does: > > > > What about: > > > > /** > > * resctrl_arch_sync_cpu_defaults() - Refresh the CPU's CLOSID and RMID. > > * Call via IPI. > > Did you intend to change function name?
Er, yes, I meant to use your suggestion here, so: resctrl_arch_sync_cpu_closid_rmid().
Also, Babu Moger's suggestion to rename struct resctrl_cpu_sync to resctrl_cpu_defaults seems good, since that accurately describes what is specified in the struct (and what is *not* specified if NULL is passed).
> > How about "Refresh the CPU's ..." -> "Refresh this CPU's ..." I think it > makes it more obvious how this function is called.
Agreed.
> > > * @info: If non-NULL, a pointer to a struct resctrl_cpu_sync specifying > > * the new CLOSID and RMID for tasks in the default resctrl ctrl > > * and mon group when running on this CPU. If NULL, the default > > * CLOSID and RMID are not changed. > > "If NULL, this CPU is not re-assigned to a different group." ?
Agreed.
> > * > > * This is how reassignment of CPUs and/or tasks to different resctrl groups > > * is propagated when requested by the resctrl fs core code. > > Could you please use imperative tone here? For example, "Propagates reassignment > of CPUs and/or tasks to different resctrl groups."
Yes, that's better (and shorter).
> > > * > > * This function should typically record the per-cpu defaults specified by > > "should" sounds like there may be cases when this is not done? Maybe just > "Records the per-CPU defaults specified ..."
I didn't want to pre-judge what implementation-specific cruft the arch code needs here, so I was intentionally vague. But the arch would need to put the CPU defaults into effect somehow or other, so yes, I think your text is better here.
I'll make a note of those changes.
[...]
Cheers ---Dave
| |