Received-SPF: pass (google.com: domain of linux-kernel+bounces-152113-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <7e424ab4-e769-4eb5-b3d4-8ff7e7a76012@intel.com>
Date: Fri, 19 Apr 2024 20:58:20 -0700
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v1 05/31] x86/resctrl: Remove rdtgroup from
 update_cpu_closid_rmid()
To: Dave Martin <Dave.Martin@arm.com>
CC: James Morse <james.morse@arm.com>, <x86@kernel.org>,
	<linux-kernel@vger.kernel.org>, Fenghua Yu <fenghua.yu@intel.com>, "Thomas
 Gleixner" <tglx@linutronix.de>, Ingo Molnar <mingo@redhat.com>, "Borislav
 Petkov" <bp@alien8.de>, H Peter Anvin <hpa@zytor.com>, Babu Moger
	<Babu.Moger@amd.com>, <shameerali.kolothum.thodi@huawei.com>, "D Scott
 Phillips OS" <scott@os.amperecomputing.com>, <carl@os.amperecomputing.com>,
	<lcherian@marvell.com>, <bobo.shaobowang@huawei.com>,
	<tan.shaopeng@fujitsu.com>, <baolin.wang@linux.alibaba.com>, Jamie Iles
	<quic_jiles@quicinc.com>, Xin Hao <xhao@linux.alibaba.com>,
	<peternewman@google.com>, <dfustini@baylibre.com>, <amitsinght@marvell.com>,
	David Hildenbrand <david@redhat.com>, Rex Nie <rex.nie@jaguarmicro.com>
References: <20240321165106.31602-1-james.morse@arm.com>
 <20240321165106.31602-6-james.morse@arm.com>
 <6bbff669-cbe5-4284-b64a-4825a541b35f@intel.com>
 <ZhldWJVck7VmU3G3@e133380.arm.com>
 <0698418a-aede-40d6-90ce-dbf6e8796916@intel.com>
 <Zh6kchkH9Dy+ywXc@e133380.arm.com>
 <069c0e2e-81e1-4343-8a9f-21f1cb74bde6@intel.com>
 <ZiE6X3nKu1ACFQDc@e133380.arm.com>
Content-Language: en-US
From: Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <ZiE6X3nKu1ACFQDc@e133380.arm.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?OGRESnBpbWNhUXpTL0Z1OEdtSjRJaTIwdWpQVVQ4MmRDQUZvcFR1VE1iQi92?=
 =?utf-8?B?RWVSYkRXdUtBTjRSNDFRUDU1VGM5aCtwT3hBSFh2RzYzRlhrdFFLVlZVR2ll?=
 =?utf-8?B?MDZ4eER4RTIrbk5kMW1qYVJlb2ZIMEpsY2RpZzV2b3ZNZDlCWDEyOWdQVUx1?=
 =?utf-8?B?M2tVZkxCZXY3anZSQzRVZ1BLQkZ4aGluNUdyV0NOYUo5dmdQbCtvR1FIZWRv?=
 =?utf-8?B?VmVUVmZGUVVsTElYZTRtakVLSmdKWWlIUWFpTVczakJ3YkNZbFBpTTZta2NO?=
 =?utf-8?B?ZWNWMXVHWWNKdndsRURpNzg3NkwwdW4vWEd3dGd0ZUlKU21uT2lKbzdKalBz?=
 =?utf-8?B?cytsYWQ1MHdVNGRsdGM0UE8xOURINnFLYlUwMkNEeGJBWGgyNVh5M3Bka2xX?=
 =?utf-8?B?eUxwZ01aOWc2aCtNdFpIZEs4cTcxYkgwVjlBMVV5Tm9ubWRQcnhCNVoxcFdS?=
 =?utf-8?B?ZWx1QzFiUUhqSVhLWHJCbjN6YjhWWDhTbmozeHEySFFET0lXd2dkL205Smdq?=
 =?utf-8?B?dDVzTGxTMzhNTisrZndKS3ZuYlVaT2dYcGpLanRJWTdsQTZYbWlsWS9odVN2?=
 =?utf-8?B?QWxvZFFpSGUyYXl1cC9RcHRYYmF6RTRuYzFJTVpreEI0VGxNSHBUVVB6eHVz?=
 =?utf-8?B?YzZiQ1V0azU1S0F6TUU1b1dpdDFJWW9TbjVEc0xBL0VHNVdUcDBMWnBRSE5k?=
 =?utf-8?B?YXBpcWNLeXM0OFd2NHVrV1hLQnZEN0xiSWFCT3BKKzhRR1dmYm13bDNjRHBp?=
 =?utf-8?B?T3V0R0E3MHNocFB3ZUcxckZzN0lXdk8xZDdKZFlWY3VITThOVlZ1a3FCbzRW?=
 =?utf-8?B?cDFKMnZrcnRQaFlEa2lTdmxUNVZwcmNPZnp1bE5pMng4SWJMelltR245cU5C?=
 =?utf-8?B?NGd5QW9ram5NRkF6TWphbExtdFJnZXdiUnY2Nmp4MGM0SnhEU29Vb2NBaS9Q?=
 =?utf-8?B?Mnd5czBLRXUwYzJ2cDlOUlpBS1JzRzRCeG96R090Sy90OFNnd2tpZkUwemtz?=
 =?utf-8?B?YXFIb3dVbGx4andEaDFsK2RXWmFESzNXbUwrTXVPcy9PTnZ4VFdkeXJpVUNU?=
 =?utf-8?B?RVpIdnNqcWV1VG5TVjMxOS9DaXBOOFVRSlZBYmlFR1NkSDhZNkl3UGFGcUJ0?=
 =?utf-8?B?MlNZNjA1Q0lLL1d0K2RTSmZyVENxa3pXODVsUitwTnRWMDlxam00cXhsekxW?=
 =?utf-8?B?NzAzbFhNc3p3YlNEV1FRNDRHNHRPeFlSc2ZkY3pMNnNuY09pMURUVGh5TGJ1?=
 =?utf-8?B?c1U1WE85b3JLVmpINS84STVieDhTMjZtZlg3YlJFOWV0SC9kdXB4SlRpcWxv?=
 =?utf-8?B?eDN3eHZPRHFPenU3OGRZTzJ2QWpFU04zdHQxQVNnNkpRRlkzRGp4RDR0am1m?=
 =?utf-8?B?L2FvRjB3UWw2bkVncTA5eE5MbnQ3THRPNU54cnc5aExBMjlvTFBXVXJWUmgx?=
 =?utf-8?B?Q0lQVjAvZXpha3M5YVdjQk54NFV0SFJ2VDljNE1Hbi9SamVmQ3pSTmxSMGhk?=
 =?utf-8?B?R01mSFlFVEFjWjFIdjVTbUF5M3lXR1o5bEZySDRkd09ENTVtMUdDTVMvUWVm?=
 =?utf-8?B?VnhrM01HSk1XZWNtRFFZZGpNeGlmOU1rOW8vanBYZFlzUmpNVGhGM3ZtcC9J?=
 =?utf-8?B?RTM5eGlSajJPMnY4Vi9qNDAxUlpmMFdnMjQrWTl0eXM0cmo5WDhKMjR3cXN4?=
 =?utf-8?B?dnJQcVhJK1JwK1h2dnJVb2wvQlZ6VnhHaytjK3hrZzBzamxRQ0k1b0FzTkFH?=
 =?utf-8?B?QTRLWWlaWTZNOENnaGZSRWhTS3Y2SzRWQXBkQmNqN2pHUWhHd2VPTjR4azQy?=
 =?utf-8?B?WW5iUThtT1N1c1RhTDlOTS9NNW9YbW53ekJBclVkT2oyQlBUaXlObFNNRU4z?=
 =?utf-8?B?elNPTWFZQzg3ZUtTWnFEYkVNV0VyUExnaXpkcnQwLzBzVXVydUk1cWJ6bFNZ?=
 =?utf-8?B?dVozY21KSDEwOGYxNVRpZ0lqRnRtTjlsTjcyOUlBVHR2MEIzRVhjOENYdGVa?=
 =?utf-8?B?WWlUZFV6RkMwMEZOS1pBam1PVzdrRWxWclhnZGs2dFVNOVF3bExhZDZPMkt5?=
 =?utf-8?B?dVAzYXZNTTVsNUdEN285MFlBd2hBSmtlZGZpOEt1cmxOdUtJVm1EUHRWY3pI?=
 =?utf-8?B?bWNMOVN0Tm83ZEY2RStNQnRUckhaemwwN1hUQ00vN2FiNmx5aUlSK2pDd0tZ?=
 =?utf-8?B?VEE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 6b5c7e56-5775-425c-a9f7-08dc60ee1f85
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 20 Apr 2024 03:58:22.8366
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: r/bAwWOrUyHNWTvqxF+37j5o/SfT0ohRjxJugdcbjdJfzvlqwttnJGzBO6fTv8YBv8REjJOSn9F8ySPNaKgREIhftKYG4TuUG0vYzpChENU=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA1PR11MB6267
X-OriginatorOrg: intel.com

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