Received-SPF: pass (google.com: domain of linux-kernel+bounces-54729-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: <6041982e-3c65-497f-b7bf-3ac444fa3623@oracle.com>
Date: Tue, 6 Feb 2024 11:01:16 +0000
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v3 1/3] iommu/iova: Tidy up iova_cache_get() failure
To: Robin Murphy <robin.murphy@arm.com>, joro@8bytes.org
Cc: will@kernel.org, pasha.tatashin@soleen.com, iommu@lists.linux.dev,
        linux-kernel@vger.kernel.org, linux-mm@kvack.org, rientjes@google.com,
        yosryahmed@google.com
References: <cover.1707144953.git.robin.murphy@arm.com>
 <ae4a3bda2d6a9b738221553c838d30473bd624e7.1707144953.git.robin.murphy@arm.com>
Content-Language: en-US
From: John Garry <john.g.garry@oracle.com>
Organization: Oracle Corporation
In-Reply-To: <ae4a3bda2d6a9b738221553c838d30473bd624e7.1707144953.git.robin.murphy@arm.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
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?d25pcTFpTUtIYmJ6QWllOW5mYnBDUis4eU1GRmJtS0t5UmE2b2VCWldHVEpk?=
 =?utf-8?B?bVU5MjhwamlyQzA4TzVzMHBIM1RHRHBFMjZwR3RPME1zQ1lCVzNFNDJGZ0Fr?=
 =?utf-8?B?ejFWTjdTRDg5bVYvQzMyT0V3UXM5NjZCclBhTjhVaUEzREFEdUY2UThvZjcr?=
 =?utf-8?B?VFZvVDVXTlhKaVY4K2U5SnZpVHVwWk1tdzVJUE81ZmQ4dVk2RlgrMDJDaERG?=
 =?utf-8?B?OFVmV2pmNWFvaVdyTlVvWjRPRWxRMWtjQURiNHF6ZXllUEQwSFRWYmRJY1J5?=
 =?utf-8?B?UVFpdlFHMnYzK1RJOXV6RFpIblRkUW1oNUlVTlhmckNzSFNJVEFMY1BUM3ZN?=
 =?utf-8?B?TDlQNllaOGdyVUFaZmVHRDZmeU9uTXlkUHBuMUtQTnEvTno1d2UxOEk1SnRK?=
 =?utf-8?B?cEhDbTlnR2MzRmNsZEJZMFBxd25MQUszR0RjamsvSFE0czFHMi8rOHBUM3R5?=
 =?utf-8?B?VlR2Vlo5bmZRaHJsSzZFYXk1cUpER1Q3MW80K01LYjNSSGg1bWRZdTQzRVpk?=
 =?utf-8?B?VTFPdVZzMjRUVWRnd05CRGNHRnhTancvUjh5ZlhBTTAwK1ZpVWQyNmh2eG56?=
 =?utf-8?B?aHNDSVczQWxmWE00M3dtamJzNGZ4WnJqUWFXSlJDMzZ6L09CUjQ3WHN1WlNQ?=
 =?utf-8?B?bjFCa3JYUzh0SDlTVGZBZXdXTDZCZ3g1NTNCdVF6RWtNQ0ZmZi9HYy9NYXhY?=
 =?utf-8?B?WDlETFUrMFo4NmQrY2paK05JWTgzeTdveG05aXVxTTFwaHh5Y1cyd1RsZC9l?=
 =?utf-8?B?d3p6ZjVNOE1ldkRwckNHcysrTkVBS1BMd0pTcXJ5SldzQ0R3WTNSZEU2VkVH?=
 =?utf-8?B?S0tkdm03TXRnc1VlazhLS3dYY3V4by9tYzNCUTJOYzZYS1p2dytWZGU3TWtU?=
 =?utf-8?B?YkJzc296SEZuUjRhWllNbVlCYVJPbUxNYXVLVTRIL2JXQ2lRZHVwYVlEYXQv?=
 =?utf-8?B?V0gyWlhMV2pubStHNnpYWWJ0enlTNTBPc1hRR1pYeU93UkRJcnk2d0t0RUll?=
 =?utf-8?B?Zy9HOHd2Zy9jODBwNVNoblgvYjc3bXRrWkpsQkpJcjR3amJZdnVhK2grL1g5?=
 =?utf-8?B?NTV1aEhkSkpoYWR0dzhXWVkrLys3M3M1ZjhsUTZLWUcvSHg2cGgwcCtKV0xL?=
 =?utf-8?B?M1dRZnB2NlNCU2RWcy9PamNWK1FZd29GODU3K05lNjdCN2JsczhGNlRLMS9Q?=
 =?utf-8?B?LzdjM3RiNDg5ZUovNkhEdmczbXVZbTZaRGtLNWNMeUlXM3NuYVFvSkhrcHU4?=
 =?utf-8?B?YWMvdFZMMFNUSGFyR1VOanRNTkpNWEpMYW1LaWl3MVRUY0hwR1lHV1BWSjRH?=
 =?utf-8?B?SVpzOU5SZS9QT3VVQXM3SDZtb0o2Yk9KKzdYQWJ6a3A2R1ZOb3pCYjFMVnZy?=
 =?utf-8?B?b1U2NmFEMVBaWGlZOSszR3JXbWZNWVpQNkF3MGR4cUVYSmVDZTVnbEMveHdI?=
 =?utf-8?B?ZEVyZENoRkp4TUliNFNxaGp5Z1NDMTNEYWlvcW5EWjZaTUhvVzNFL1pIK3Ra?=
 =?utf-8?B?NWVGK3FjUzZKbHJydWNadGZiY0VXZ1BzekdWVVhnemd3bFpodk0xb083N0gv?=
 =?utf-8?B?a3YyekcvYVE5Ui9HL1gvSjNFTnFxWnhzSVNDZVlZOXNOQzgxVkZCbzFOMUpK?=
 =?utf-8?B?Z3k1MEc3L2tHdGk4NlBxRDBPQlBsQXAxL2g5LzFhcmsvTzRma29MK2FWYlQr?=
 =?utf-8?B?SzJKQmwxOFlGOFB0ajJaSEIybjJiTGFLZ1RGNEZZSXkrMktSNjNLWlpnZjR0?=
 =?utf-8?B?MVZmMmFzY0JTZXVBazdVbmYxWTVMWG9WQ0VMcmppNXFaUnVCQmhXK3VOTUZF?=
 =?utf-8?B?dTlUQTl5am9wZDlxZ3dRSmxCZWJrRDlMWFRzODNNTmhXZEcvRGlzdnlFcEQx?=
 =?utf-8?B?ekJKM09oRXlhcDV1U1Q2UHlRTWt4b0dINHZ2Ujk0aFZwS3RpVXBwR1QwaFAv?=
 =?utf-8?B?SmxtZys2dlhJSkdSN0VENEtoRjJEYzlnaVRtQWZCSGNNRkJyTUZBZTRlVm94?=
 =?utf-8?B?VFVVR2N3VXVWREh0SnNZbkhoL3BKdkZhRmRsWXF3L0Zia001NEpFa05hYjlM?=
 =?utf-8?B?a3BjWkc1T0ZDT2RTajEzOXQ2VHFxTWZNWkFDVm84OS9nNzl6N0VLZzFmdVRv?=
 =?utf-8?Q?zJzCJt5s/+sO6TsXDwgJlCdsi?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	2jn252iugAg3GdE3nglUPkjZeVGPcNO+EfIvevF8/D9qAhmgbctF7Saw1LW06Bv/3H9EqGT1Svi77ngn3YOaPVNbnh9zaDG0tmOz1j3saXXtZeG+T8yvYZjMEHOHVU0JNfFKv5mxIyYb9z8FwzMwFbegMjXrdgNmX45lMytKPMPbl/+0jMfacmeC/wTISb0GY0xSFqpaE4lkcWQIr76W9lMrEDVBQxTmpTC0wIKB6cR+TXxY4IDgCE9sddNA3/4EhRCL57kzJVDn5R9WwR6J1Blc5Ht3JTfQdcjZB7ynTYptvIRffAUjLXeDb922SnzybqNplPcQtRyS9UhD9u47IIn/CdU9OOU0tNsRHp2hP8c0kRfokbyvaoxcrZFiOnY5EVTv1iFBY6mM/uolfvm+hCp2X6qKp6adVm5POyamr6StUhJXHEoxXPWshAuu56m31fXo73gi7ifjIxp4KHq30ZLHDLe/fYiaYQPaTCu8SRTZh/Jh5JE8Nxki+wskR4Xar5lhxIRrg3Pp07h4ezAoyQqL4r4QzXzs0FooLKlgOB67OM+xHz7mlr2GGBKQnZ4YkzXanaA+SbkAZ9QdRwQlcKUy9otKha49tjuONSc4iok=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 13fa7e13-7ef7-4a23-a074-08dc2702f3a5
X-MS-Exchange-CrossTenant-AuthSource: DM6PR10MB4313.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 06 Feb 2024 11:01:21.2745
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: svKVD7RDpItcAM/ZyJ4MQu0KJEcW6HHCrVo5VZTM9I2jibxikjqMHdAnTvpPCp0sLqPTRmXTRc51lfjdhE6KAw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR10MB4438
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.272,Aquarius:18.0.1011,Hydra:6.0.619,FMLib:17.11.176.26
 definitions=2024-02-06_04,2024-01-31_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 spamscore=0 suspectscore=0
 mlxlogscore=999 bulkscore=0 mlxscore=0 adultscore=0 malwarescore=0
 phishscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2311290000 definitions=main-2402060077
X-Proofpoint-GUID: Z7qCqXV2S7l3Mf0Yiw8U3sIHdUNtEwNg
X-Proofpoint-ORIG-GUID: Z7qCqXV2S7l3Mf0Yiw8U3sIHdUNtEwNg

On 05/02/2024 15:32, Robin Murphy wrote:
> Failure handling in iova_cache_get() is a little messy, and we'd like
> to add some more to it, so let's tidy up a bit first. By leaving the
> hotplug handler until last we can take advantage of kmem_cache_destroy()
> being NULL-safe to have a single cleanup label. We can also improve the
> error reporting, noting that kmem_cache_create() already screams if it
> fails, so that one is redundant.
> 
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>

Regardless of a couple of minor comments, below, FWIW:
Reviewed-by: John Garry <john.g.garry@oracle.com>

> ---
>   drivers/iommu/iova.c | 33 ++++++++++++++++-----------------
>   1 file changed, 16 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c
> index d30e453d0fb4..cf95001d85c0 100644
> --- a/drivers/iommu/iova.c
> +++ b/drivers/iommu/iova.c
> @@ -254,26 +254,20 @@ static void free_iova_mem(struct iova *iova)
>   
>   int iova_cache_get(void)
>   {
> +	int err = -ENOMEM;
> +
>   	mutex_lock(&iova_cache_mutex);
>   	if (!iova_cache_users) {
> -		int ret;
> +		iova_cache = kmem_cache_create("iommu_iova", sizeof(struct iova), 0,
> +					       SLAB_HWCACHE_ALIGN, NULL);

Maybe can use KMEM_CACHE(), but it would mean that the name would change.

> +		if (!iova_cache)
> +			goto out_err;
>   
> -		ret = cpuhp_setup_state_multi(CPUHP_IOMMU_IOVA_DEAD, "iommu/iova:dead", NULL,
> -					iova_cpuhp_dead);
> -		if (ret) {
> -			mutex_unlock(&iova_cache_mutex);
> -			pr_err("Couldn't register cpuhp handler\n");
> -			return ret;
> -		}
> -
> -		iova_cache = kmem_cache_create(
> -			"iommu_iova", sizeof(struct iova), 0,
> -			SLAB_HWCACHE_ALIGN, NULL);
> -		if (!iova_cache) {
> -			cpuhp_remove_multi_state(CPUHP_IOMMU_IOVA_DEAD);
> -			mutex_unlock(&iova_cache_mutex);
> -			pr_err("Couldn't create iova cache\n");
> -			return -ENOMEM;
> +		err = cpuhp_setup_state_multi(CPUHP_IOMMU_IOVA_DEAD, "iommu/iova:dead",
> +					      NULL, iova_cpuhp_dead);
> +		if (err) {
> +			pr_err("IOVA: Couldn't register cpuhp handler: %pe\n", ERR_PTR(err));

Maybe can use pr_fmt with "iova".

> +			goto out_err;
>   		}
>   	}
>   
> @@ -281,6 +275,11 @@ int iova_cache_get(void)
>   	mutex_unlock(&iova_cache_mutex);
>   
>   	return 0;
> +
> +out_err:
> +	kmem_cache_destroy(iova_cache);
> +	mutex_unlock(&iova_cache_mutex);
> +	return err;
>   }
>   EXPORT_SYMBOL_GPL(iova_cache_get);
>