Re: [PATCH v2 07/18] x86/resctrl: Move CLOSID/RMID matching and setting to use helpers

[James Morse]

Hi Fenghua,

On 17/01/2023 19:10, Yu, Fenghua wrote:
>> When switching tasks, the CLOSID and RMID that the new task should use are
>> stored in struct task_struct. For x86 the CLOSID known by resctrl, the value in
>> task_struct, and the value written to the CPU register are all the same thing.
>>
>> MPAM's CPU interface has two different PARTID's one for data accesses the
>> other for instruction fetch. Storing resctrl's CLOSID value in struct task_struct
>> implies the arch code knows whether resctrl is using CDP.
>>
>> Move the matching and setting of the struct task_struct properties to use
>> helpers. This allows arm64 to store the hardware format of the register, instead
>> of having to convert it each time.
>>
>> __rdtgroup_move_task()s use of READ_ONCE()/WRITE_ONCE() ensures torn
>> values aren't seen as another CPU may schedule the task being moved while the
>> value is being changed. MPAM has an additional corner-case here as the PMG
>> bits extend the PARTID space. If the scheduler sees a new-CLOSID but old-RMID,
>> the task will dirty an RMID that the limbo code is not watching causing an
>> inaccurate count. x86's RMID are independent values, so the limbo code will still
>> be watching the old-RMID in this circumstance.
>> To avoid this, arm64 needs both the CLOSID/RMID WRITE_ONCE()d together.
>> Both values must be provided together.
>>
>> Because MPAM's RMID values are not unique, the CLOSID must be provided
>> when matching the RMID.

>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> index e1f879e13823..ced7400decae 100644
>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> @@ -84,7 +84,7 @@ void rdt_last_cmd_printf(const char *fmt, ...)
>>   *
>>   * Using a global CLOSID across all resources has some advantages and
>>   * some drawbacks:
>> - * + We can simply set "current->closid" to assign a task to a resource
>> + * + We can simply set current's closid to assign a task to a resource
>>   *   group.
> 
> Seems this change doesn't gain anything. Maybe this change can be removed?

After this patch the CLOSID might not be in current at all, this comment would be the only
thing that suggests it is. I'd prefer not to suggest anyone access 'current->closid'
directly in resctrl, as such code wouldn't compile on arm64.

This is a 'saves bugs in the future' change.


>>   * + Context switch code can avoid extra memory references deciding which
>>   *   CLOSID to load into the PQR_ASSOC MSR

(please trim your replies!)


Thanks,

James