Messages in this thread | | | Date | Fri, 19 Apr 2024 20:58:20 -0700 | Subject | Re: [PATCH v1 05/31] x86/resctrl: Remove rdtgroup from update_cpu_closid_rmid() | From | Reinette Chatre <> |
| |
Hi Dave,
On 4/18/2024 8:21 AM, Dave Martin wrote: > On Wed, Apr 17, 2024 at 10:12:35PM -0700, Reinette Chatre wrote: >> 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: ..
> <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?
The MSRs do not all have the same overhead. MSR_IA32_PQR_ASSOC is intended to be updated quickly but it is not free and I'd prefer to keep avoiding unnecessary updates where possible.
> > 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@e133380.arm.com/ > > 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.
resctrl_arch_sync_cpu_closid_rmid() sounds good to me.
Reinette
| |