Received-SPF: pass (google.com: domain of linux-kernel+bounces-84093-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <be17b788-dda1-4671-a037-6d79d383fdc4@intel.com>
Date: Tue, 27 Feb 2024 14:05:05 -0800
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v3 1/2] x86/resctrl: Pass domain to target CPU
Content-Language: en-US
To: Tony Luck <tony.luck@intel.com>, Borislav Petkov <bp@alien8.de>, "James
 Morse" <james.morse@arm.com>
CC: Thomas Gleixner <tglx@linutronix.de>, <x86@kernel.org>,
	<linux-kernel@vger.kernel.org>, "Yu, Fenghua" <fenghua.yu@intel.com>, "Ingo
 Molnar" <mingo@redhat.com>, 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>
References: <ZdZPht8hY4J9uIOz@agluck-desk3>
 <20240222185039.72201-1-tony.luck@intel.com>
 <20240222185039.72201-2-tony.luck@intel.com>
From: Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <20240222185039.72201-2-tony.luck@intel.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?Vkp5STBJc2tlN1orOG9KWjZaSzBCN0hwendRc2dqcVp0b3ltZkVBTVlFa3Bx?=
 =?utf-8?B?a3FRRXJ1YlRkVVpJb3BnckFXOTlhOU1uQUJRck1iS3dSdDFKdm5DZVdrVUp4?=
 =?utf-8?B?VnNXNkR1VkppZmphUzZWWVloOGpEeGxkQUgrY29PWFNraHE3RWJMQXd1aTNP?=
 =?utf-8?B?SGdmU3ZBK1B1UlRiYnVJd0dHQ2hGeFptN2wyeU9vVWlYY3hxYmMwOVZIN25T?=
 =?utf-8?B?VmRRZG5xU1lmV2N0akw0K0JLcjl5bzg0WCsveHo0S01JVytKTWtMZldrcFdI?=
 =?utf-8?B?NC9GNXluS1E0UENNei9ieFBaeFpXUTRIaTJpdDV1d2xiTTgySjgvRE9QWlRI?=
 =?utf-8?B?SzE3WXlqcEcrRUpRRDNrMWNOc0k5ZVo2WUNDL2tTVjVtcVltZ1g1TFR1OVN4?=
 =?utf-8?B?WThFZHVaRXlIL1YxaHNaa0lvZWhhVzhWMkJxc3Zhdnc2VmxrNXNPQ1dYWGV1?=
 =?utf-8?B?VnF1Y1UwWmdXcy9ZcmlqK3VNZVVsVWVUdXNpaWJuRzlubjFMay92Mm4wcmFj?=
 =?utf-8?B?ZmYxc0NBVjZHY0RBS3ZwODRsTHlENk5rS3MrN1FqVktXWnpZZS9RVlRxdTdz?=
 =?utf-8?B?Ly8vL1B2TnlNaWwwQmpQUlFDSXpBNG9Dbmk4NXlBeWZPM0ViUWtQdmZrOEhj?=
 =?utf-8?B?L3ZzcWMwZ3lRUmhPOElHd2M0VktPaU8wQ2Y1Q2ZtMDdNWWRmaXlZTkxkYnNv?=
 =?utf-8?B?WkJEUGtIQXBFVzhBUWUzNDY5MTcvTEhNUGpPOGR6SlRnZFE1aXVyMXRHNnRj?=
 =?utf-8?B?S1NDbHF6WmRtWDFZWCt2Szh1NUJMZU1tdmJzek9TcW1KQncxV0tydGt1ZkpP?=
 =?utf-8?B?Z1RRY2hJMUw3K3A4b1NMaTB2Y25VYS90SmlSb0JaVElyYWtLb2tkYWh4YnBR?=
 =?utf-8?B?cHhhbVMxeG1INkRuWXFJVVRqVmw2YjJYcmd1dmVkUy9uNzZYSDJ4S3daeG0w?=
 =?utf-8?B?bkJ3aE5RMGUzVTJGS2h2MnpucnY4Q2c4c3UyZWJNbXUva3lrV0tFRW43cCtY?=
 =?utf-8?B?S2NRME5XL3VjZ3k0cWVEbHZjYW5tK3BrL3plM2o0UEt6N0pSL2FQREVtNyty?=
 =?utf-8?B?REFldFZnaDJJN0VzWklwMml5SkpRWnRXUnAvcUZPaDB3MWR1eEZzMkZWYXpi?=
 =?utf-8?B?VjNZTDVCaTBnNGxnWUY1dTRPOVNhRjZSQjBaeWsrNDlGK2pXRDBMSHoraGtZ?=
 =?utf-8?B?aGx6VTZiSUU5RDBRRG04ZU1oWTlBYVFpN2lrNS9BSE1qSU9CRWY4ZnJsUWRx?=
 =?utf-8?B?ektlNlE3RXJmZjlPdThtMnRJSFNwUHNnWWVaZ0pINC9QYzFUZmhQdmdQZHFS?=
 =?utf-8?B?VWFJblpXQnRtbEk2SDZBSjMxNytsY1VqTnB6RFRrZWk4cE90eFk1ZGRraGtJ?=
 =?utf-8?B?cUVaTmhtRDk2ZjA1T2dFV0RST0lGOXh3VWNCMGhEcEdqeWRGN1lGY3M5MmtI?=
 =?utf-8?B?UUQwaE1WS2tXM2pSQjBuU08zUmhkZllIUzU5Z05OOTBOMnRRaUlJTWVZc1d4?=
 =?utf-8?B?cVJKZDgxdXo0K1RpLzRjUjRNOGY4cy82WTlzSmxRc2VJcDJRRlM0ZnFmZnJl?=
 =?utf-8?B?R0dSTzFBTjVPTk8zQWxMaStsR1FaQm5GbXl0MWZDRVhrbDQzb1BGbTJxTjZO?=
 =?utf-8?B?M29zWFFwZEVGL29kUnJFaHBOdlJTcStZUWc2VmRXZStvQ0FpSnpCL2NOUmdx?=
 =?utf-8?B?ZmJzbUQ3MGFaVisyS1E2MXB1d1NjdkZoV2RKVW01VzhZV3lwcWpmTFV2aGN2?=
 =?utf-8?B?ZlVraTgrcUdyVDJtbUFCWFNGOWdTbGRPWWpIMkhTRk9Ta3h1WkhDMkpXVTVL?=
 =?utf-8?B?UGNrdHd6TGNYODBjS0hWd24zS2FnMm1ZdUxDenRIUnZ3d2lnTE9FaXJBd2U0?=
 =?utf-8?B?bFhUaklsOHl0bm5GMG9DUWUyZ1orck55ejNtdTNFaFpmOHYwMW1SY3lNNHJY?=
 =?utf-8?B?ckFQR2huWENhNVJXdVpBT2daVzZpOUtXVTd1d2NObU5uQ0pIVjRwOFhGZkFW?=
 =?utf-8?B?cmNXVFNyQnpsS2cwV2FEOXNkVzBMc0kyamR4WlJFT0RMSUVvWlVwZmNYTVUz?=
 =?utf-8?B?Y241a1dXRWZjMmErMVNuci9zUmVEZlZpNGkyNEtnaTZqeFlRODFTTmZob2d5?=
 =?utf-8?B?bWRRVzlhOXAxTThxYkZ4U2V5UXJLWnYwcDNreTYyS1EzZnZRVUhuQkYwc3VR?=
 =?utf-8?B?dFE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 2e0a28f9-1dcf-4315-98c7-08dc37e0291e
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 27 Feb 2024 22:05:08.4833
 (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: qX1yKJ3CHeGTGtadJG5XAVVug0x/RHf3Lu3t8xjQGCPmuJ5bu4B8WXiMvPWCz6aZqzx5OSUYl5BCnBFQOGMFsEM50/v0hznsspwpDtsCyl8=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH3PR11MB8589
X-OriginatorOrg: intel.com

Hi Tony,

On 2/22/2024 10:50 AM, Tony Luck wrote:
> reset_all_ctrls() and resctrl_arch_update_domains() use on_each_cpu_mask()
> to call rdt_ctrl_update() on potentially one CPU from each domain.
> 
> But this means rdt_ctrl_update() needs to figure out which domain to apply
> changes to. Doing so requires a search of all domains in a resource,
> which can only be done safely if cpus_lock is held. Both callers do
> hold this lock, but there isn't a way for a function called on another
> CPU via IPI to verify this.
> 
> Commit c0d848fcb09d ("x86/resctrl: Remove lockdep annotation that triggers
> false positive") removed the incorrect assertions.
> 
> Adding the target domain to the msr_param structure, and calling
> for each domain separately using smp_call_function_single() means
> that rdt_ctrl_update() doesn't need to search for the domain.  Thus
> get_domain_from_cpu() can safely assert that the cpus_lock is held since
> the remaining callers do not use IPI.

Please stick to the imperative tone. Something like (please feel free to
improve):

	Add the target domain to the msr_param structure and call
	rdt_ctrl_update() for each domain separately using
	smp_call_function_single(). This means that rdt_ctrl_update()
	doesn't need to search for the domain and get_domain_from_cpu()
	can safely assert that the cpus_lock is held since
	the remaining callers do not use IPI.


..

> @@ -463,6 +457,8 @@ static int domain_setup_ctrlval(struct rdt_resource *r, struct rdt_domain *d)
>  	hw_dom->ctrl_val = dc;
>  	setup_default_ctrlval(r, dc);
>  
> +	m.res = r;

This belongs in the next patch.

> +	m.dom = d;
>  	m.low = 0;
>  	m.high = hw_res->num_closid;
>  	hw_res->msr_update(d, &m, r);

The rest looks good to me and I think it is a good improvement.
Thank you very much.

Reinette