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

[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@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.

resctrl_arch_sync_cpu_closid_rmid() sounds good to me.

Reinette