Re: [PATCH v1 05/31] x86/resctrl: Remove rdtgroup from update_cpu_closid_rmid()

[Dave Martin]

On Wed, Apr 17, 2024 at 10:12:35PM -0700, Reinette Chatre wrote:
> Hi Dave,
> 
> On 4/16/2024 9:16 AM, Dave Martin wrote:
> > On Mon, Apr 15, 2024 at 10:47:55AM -0700, Reinette Chatre wrote:
> >> On 4/12/2024 9:12 AM, Dave Martin wrote:
> >>> On Mon, Apr 08, 2024 at 08:16:08PM -0700, Reinette Chatre wrote:
> >>>> On 3/21/2024 9:50 AM, James Morse wrote:
> ..
> 
> >> 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?
> 
> 80 chars are not enforced so strictly that it impacts readability. You
> may refer to how update_task_closid_rmid() looks for more confidence in/
> motivation for placing this on one line.

Agreed.

(I did in fact rename default_p to p in my fixup, which shortens the
affected line as a side-effect anyway.)


<aside>

Although probably out of scope for this series, I wonder whether these
two paths can be combined?

update_task_closid_rmid() selects the cross_call target by task, where
update_closid_rmid() selects the cross_call target(s) by cpu.  But the
backend work that the arch code needs to do seems basically the same:
possibly update the the CPU default group membership, the reconfigure
the MSRs for the running task to ensure that they aren't stale (with a
possible optimisation not to bother if we sure that the MSRs are not
stale for the task actually running, or if we know they wouldn't be
changed by the write).

Even the check to see whether the right task is running seems somewhat
redundant: we already paid the cost of taking the IPI, and we have to
cope with spurious, idempotent updates to the MSRs anyway since this is
all racy.

Is there a high overhead to writing the MSRs on x86?

For arm64, the relevant system register only affects EL0 (i.e.,
userspace) execution, so we defer synchronisation of a whole bunch of
stuff until the return to userspace.

</aside>


> 
> > 
> >>
> >> ..
> >>
> >>>>> + * 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().
> > 
> 
> I'm a bit confused here when comparing with your response in [1] mentioning
> a change to another name. 
> 
> [1] https://lore.kernel.org/lkml/Zh6kgs1%2fbji1P1Hl@xxxxxxxxxxxxxxx/

My bad (sorry Babu!).

I read that suggestion carelessly and assumed it was aligned with
Reinette's.

The most important thing seems to be to transfer the "defaults" from the
name of the function to the name of the struct, since the struct is
about defaults (and only about defaults), while the function is about
defaults and the running task.

To avoid extra busy-work, I'll stick with
resctrl_arch_sync_cpu_closid_rmid() for now, but I don't mind changing
it if people prefer.


> > 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).
> 
> Sounds good, yes.
> 
> Reinette
> 

Cheers
---Dave