Received-SPF: pass (google.com: domain of linux-kernel+bounces-91392-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <12766261-26b2-4aa4-a735-c2380c4963fd@intel.com>
Date: Mon, 4 Mar 2024 15:07:31 -0800
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v4 1/2] x86/resctrl: Pass domain to target CPU
Content-Language: en-US
To: Tony Luck <tony.luck@intel.com>, Fenghua Yu <fenghua.yu@intel.com>, "Peter
 Newman" <peternewman@google.com>, Jonathan Corbet <corbet@lwn.net>, "Shuah
 Khan" <skhan@linuxfoundation.org>, <x86@kernel.org>
CC: Shaopeng Tan <tan.shaopeng@fujitsu.com>, James Morse
	<james.morse@arm.com>, Jamie Iles <quic_jiles@quicinc.com>, Babu Moger
	<babu.moger@amd.com>, Randy Dunlap <rdunlap@infradead.org>, Drew Fustini
	<dfustini@baylibre.com>, <linux-kernel@vger.kernel.org>,
	<linux-doc@vger.kernel.org>, <patches@lists.linux.dev>
References: <20240228112215.8044-tony.luck@intel.com>
 <20240228193717.8170-1-tony.luck@intel.com>
From: Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <20240228193717.8170-1-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?dTcxYStrT0ZmUUJDdHVRZUIwbXlQRnd5NXpTY05CLzBuaDkwSzVoSlNEcFFZ?=
 =?utf-8?B?QmVBWW5YSVg5amo4ZTd3QU12WlU2bTRWZnVuNzhRTmhoTDdVSGhlOUtERkRv?=
 =?utf-8?B?K1RuODVCdm1LN3g5LzhhcFNQb0p5TEV1N2hMS1gyUnAxTmlkMnNWUkR3L2hK?=
 =?utf-8?B?MFJYWFJkS2tHMzZPL0I5SHRidFNwdGdtNjdzRnk5NTZ0QW9Wa1Jaa3Rxc1Zp?=
 =?utf-8?B?b2VadlVnUVJrRzZuandYbjYySUF5d3g0QjhSNkYvVFRyNUdET245NmNrWTFF?=
 =?utf-8?B?MVduLytjQ2xHREh1a1YvUnpOdEFsK0Q3cjM2K2FWSENldWwxeWowVllOQ1JC?=
 =?utf-8?B?MHEwWnRHYjB5aUZaNGlTQWkzemJTRHZFblptRXdna2FydEJ5M09UNVc5OEVL?=
 =?utf-8?B?d2JNdnMrSE40emU3Zy9rUGhXQjB6M2RiUUQ4c0JpN2sxbW42WU9tSmJNZWNu?=
 =?utf-8?B?My9BOTEyS29FK1NhMHowWk5GRml1Ui9JSk4wUzhLQ3pCY25PYU8zVXI4WTVE?=
 =?utf-8?B?dUd1V0o1azhEVDFKSGtjR1phWjBoNFpHeXlGdkRPbnZOSmdNVjFOR0U4U1F5?=
 =?utf-8?B?aVA2YnJrMG1KNFV4Z1V0aTNRbDJ1M3pleit5aFhKWEJ4dlBnZU5qQkt2eFI2?=
 =?utf-8?B?T3JmOTI1Rk5uNUlKRHRQR1ZIOXZ2VmRlRUc0SzlyVllQdzBIamRDMVhzYlkv?=
 =?utf-8?B?d3o0OEVvZUcwNXVFdisrMWQwN0NOTEtxM3ZCS0x6V2pWMkRDaWFhaEhQZWor?=
 =?utf-8?B?d3VYSENIVE9QUHBhYjdYVko0M3lrN0l0WDJ2My9IMFhrcjlSVnRVQVp3ZWxV?=
 =?utf-8?B?Slc2d2dTTkxnQU1jUWxXUzU2V3FLM01xa1VxOTVpKzBTOUFhSmNaVFR5RDJX?=
 =?utf-8?B?Ui9ITlkra2FEM1kwZmN3WFpFWG12ZjdLNXMwWlNhUForVy9mR2dOK0FqZkVR?=
 =?utf-8?B?OG9tQSsxU0NzQlZ5aVNSUWlYMElnM3k5K2dvVUd6WDV4Y2wwVHk0cVUvaXB2?=
 =?utf-8?B?YUxHUXBhWENuU3NSMFVCMlVGVVlIaFZEZ21KR21qdFJUc1B2bE5lRVlaQnRz?=
 =?utf-8?B?cHdTaVRQeXZRWjNxOVpncUkvREFDNWdxL0V2S3VjZTJGcmZkUERySHdsblBl?=
 =?utf-8?B?WTVWaE9IMHN5SHk3V2Q0UDZiRlc5THJEcDBHWUVlOWJvVy9wYXE2N04yWldi?=
 =?utf-8?B?R25VYSs5eW5BekdkZ1U3bzlONFhqaWZiZ0ROWTQ1YWUwUkh4S2VlamVXZmd3?=
 =?utf-8?B?Y0lyeW0wcnBXcDd5aXpMd3pubTNBcTEzQWphKzI4QUNWdVlBR0xnSEIxaUR5?=
 =?utf-8?B?UkVEZjFEZHYzdlNkSkRkNFQrRnhQK0x1SE05NGdKNnR6QlFUVUxzcnB0M2Jh?=
 =?utf-8?B?L0pGR2l2azJFcm1WWkU4YkFhR1ZabkVNNFk2eDIwcW9DZGJDUGZFdDVaV1JZ?=
 =?utf-8?B?azdhbmgwMDlWQ0Z4aDhzd21jMGpBcmlRZktsSUpMMnViTUhNNjltekZoalZM?=
 =?utf-8?B?aFpraVg3YXRrVkFIOGYycTN6aFFHMnBJeDNPUFYrV1kxaWhobnRLdUxBT1dy?=
 =?utf-8?B?Q1lKdjJBQnZyK3kvcmZzdkN3ZjZTOXUycTE2WXg2clE4ck1YSlRPWi9YUTdy?=
 =?utf-8?B?d0l0QnNwRFpsQ0Y2TSt1a0Z0R1JOV0Ywd3dJN0JVakd2NW0vMjc1MVY1L21v?=
 =?utf-8?B?R2pQM2VhdzV2dGs2MzVxRG9JVDJ3engrVlpIcVFheGxOMUFVOWJzakx2UVlk?=
 =?utf-8?B?bHR2M1BCdlpPMGVJVXRaeWVnZGthT0Y2VHh3dkRFNmtCLzFwTC8zRWE1eU5Z?=
 =?utf-8?B?STMxWHgyZnI5K090VUlPd29CRjE2NzRUVkpsWDRvZHAzcnBLd05SWFhmZzFm?=
 =?utf-8?B?YUVJRklqQUQyS0p2dk8xZmJFT2hPVHpWRWdBQWwzTXo5WHFwZGFWaHVjbEVH?=
 =?utf-8?B?Q216R1ZRdEp2R3ZrSk9tNUNCTmtnQngvampBck5tUDlDdGZxTXdqd3ZaRnNo?=
 =?utf-8?B?eVlaRUtKWC9Tb3I2NzlINk93K3hqU0lMZTdndllMcUlBUlFmWi9FS21CaFM2?=
 =?utf-8?B?N0QwWGs2OFY3b0M3Tkh4bXRWa21meGtZbEJDYjRNbm9oTmZydU43bzVvY1dy?=
 =?utf-8?B?d3JpOWY5QjJkdFk1N3pqQlVLeU1SbHkzUUp0WFdoQVErMU83NnE1dElqb29U?=
 =?utf-8?B?R2c9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 666109f9-aa9b-4bc2-b7a6-08dc3c9fdfe3
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 04 Mar 2024 23:07:33.5170
 (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: EMdketBqz+63707jCMksbqp7lKJv7GHYZXb/qC5skmRfj4U4lL2i/kF4X5tdHx3OhrsIwM25RQfCWnwc+cn2feXaE4oMj50+FWrpLZcW5NQ=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA1PR11MB6074
X-OriginatorOrg: intel.com

Hi Tony,

On 2/28/2024 11:36 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.
> 
> 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.
> 
> Signed-off-by: Tony Luck <tony.luck@intel.com>
> ---

..

> @@ -315,27 +300,27 @@ int resctrl_arch_update_domains(struct rdt_resource *r, u32 closid)
>  	struct rdt_hw_domain *hw_dom;
>  	struct msr_param msr_param;
>  	enum resctrl_conf_type t;
> -	cpumask_var_t cpu_mask;
>  	struct rdt_domain *d;
> +	int cpu;
>  	u32 idx;
>  
>  	/* Walking r->domains, ensure it can't race with cpuhp */
>  	lockdep_assert_cpus_held();
>  
> -	if (!zalloc_cpumask_var(&cpu_mask, GFP_KERNEL))
> -		return -ENOMEM;
> -
> -	msr_param.res = NULL;
>  	list_for_each_entry(d, &r->domains, list) {
>  		hw_dom = resctrl_to_arch_dom(d);
> +		msr_param.res = NULL;
> +		msr_param.dom = d;
>  		for (t = 0; t < CDP_NUM_TYPES; t++) {
>  			cfg = &hw_dom->d_resctrl.staged_config[t];
>  			if (!cfg->have_new_ctrl)
>  				continue;
>  
>  			idx = get_config_index(closid, t);
> -			if (!apply_config(hw_dom, cfg, idx, cpu_mask))
> +			if (cfg->new_ctrl == hw_dom->ctrl_val[idx])
>  				continue;
> +			hw_dom->ctrl_val[idx] = cfg->new_ctrl;
> +			cpu = cpumask_any(&d->cpu_mask);
>  

cpu only needs to be assigned once. How about initializing cpu to
nr_cpu_ids at the same time msr_param.res and msr_param.dom is
initialized and only assign it when msr_param.res is assigned?
I think that will be more robust.

If you agree and do this then please feel free to add:
Reviewed-by: Reinette Chatre <reinette.chatre@intel.com>

Reinette