Messages in this thread | | | Date | Wed, 17 Apr 2024 22:12:35 -0700 | Subject | Re: [PATCH v1 05/31] x86/resctrl: Remove rdtgroup from update_cpu_closid_rmid() | From | Reinette Chatre <> |
| |
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.
> >> >> .. >> >>>>> + * 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/
> 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
| |