From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 69701C4828D for ; Tue, 6 Feb 2024 11:25:10 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id F1E796B007E; Tue, 6 Feb 2024 06:25:09 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id ECF096B0081; Tue, 6 Feb 2024 06:25:09 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id D96FF6B0083; Tue, 6 Feb 2024 06:25:09 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0013.hostedemail.com [216.40.44.13]) by kanga.kvack.org (Postfix) with ESMTP id C91B46B007E for ; Tue, 6 Feb 2024 06:25:09 -0500 (EST) Received: from smtpin15.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay01.hostedemail.com (Postfix) with ESMTP id 9DEAA1C0A2F for ; Tue, 6 Feb 2024 11:25:09 +0000 (UTC) X-FDA: 81761147538.15.D6BF3D1 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by imf16.hostedemail.com (Postfix) with ESMTP id 7868318000B for ; Tue, 6 Feb 2024 11:25:07 +0000 (UTC) Authentication-Results: imf16.hostedemail.com; dkim=none; dmarc=pass (policy=none) header.from=arm.com; spf=pass (imf16.hostedemail.com: domain of robin.murphy@arm.com designates 217.140.110.172 as permitted sender) smtp.mailfrom=robin.murphy@arm.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1707218708; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=tRK63h9Z5GGvvcVUDvg/7+LLJBt23wX9onR5VI9VSFk=; b=EaDzb2UVaUHd0EuLSHQ6V3qClAo9patWQsYNJBqaYc3vm4/ADUyYDJXxM6CMvBiRBYB3f4 KCrVS0/PKNoLUGJMQM6fkwbzwgCIiOX9v/IDdn3b5ROxPMdtwKSUeEiJ/NzIcpcxd4lnii zUDQ5PXofTp0QRrx5OlLevfTyDwyck4= ARC-Authentication-Results: i=1; imf16.hostedemail.com; dkim=none; dmarc=pass (policy=none) header.from=arm.com; spf=pass (imf16.hostedemail.com: domain of robin.murphy@arm.com designates 217.140.110.172 as permitted sender) smtp.mailfrom=robin.murphy@arm.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1707218708; a=rsa-sha256; cv=none; b=xXvEU0F+C55f99WBOkX7pJVjP33oungqXnHV0/rdPcaY25da463rfY5yU09rjz5O3CkUU1 KJuTUsnxPV3fxiW6avmGtx+8lNRFevFWmpeuKWevj0I2KmPawRo05kdUeJqu886VJQ1T13 73m2L34HwKFwPqKSH4s4opHQChLiyxU= Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id CB9CD1FB; Tue, 6 Feb 2024 03:25:48 -0800 (PST) Received: from [10.1.196.40] (e121345-lin.cambridge.arm.com [10.1.196.40]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 02F873F762; Tue, 6 Feb 2024 03:25:04 -0800 (PST) Message-ID: <879751d4-486c-4d5e-b9b3-60e8822ff3fb@arm.com> Date: Tue, 6 Feb 2024 11:25:03 +0000 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v3 1/3] iommu/iova: Tidy up iova_cache_get() failure Content-Language: en-GB To: John Garry , 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: <6041982e-3c65-497f-b7bf-3ac444fa3623@oracle.com> From: Robin Murphy In-Reply-To: <6041982e-3c65-497f-b7bf-3ac444fa3623@oracle.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Rspamd-Queue-Id: 7868318000B X-Rspam-User: X-Rspamd-Server: rspam02 X-Stat-Signature: 4u9ie7a6kqoxja4x3ysdc47rbtw5azdp X-HE-Tag: 1707218707-196274 X-HE-Meta: U2FsdGVkX1+fpTQEGmMe42nAjaQMeKVJ7tB8JFzZ+MBpc8QB7dCZFwQKB/XRHoxB1aW7/DS6On6M58nYTyIFi/gr4N5hcychhaYAsSvd147rN/2uoRtoQk7sLsYmnbkN50uXJchMLWJiYS3wJdJ1qbpHM+txgXtzTaMtfBKm8b3nPNB/2YbxA6ZS6m7OSDsCqW57xFagAmY7AhqjinZ7Dj/JuYDSVtAw/p7y9DFO8QdLeUhEkmH9z/MWrvH+4Q5CdAoo2iDkMtJt/6jFfDHvAjllqRPKB9HC/J73j1QnXIoi3VlzzkefR2V+z4ivz+GbFlHLuHEI5NIulZfTL6nP4q+8+aivjZ6RLweCaqz7pULbSJA5W2anOUe3GIP8Mgu/elsoQkmjumMf0BddIRgddZhZeRLsKKRsqk83A2DTc17hSuVt1KyQa1JWrgwfQF3FqWq8rj5nUye64UwKN+wo/X7Ept0oFRw/DP4pV9YwPby9qvqQ1eSt0K5E9xO9WcI+4uMS/iPqzNxaV0W3L8tfqrFpHskxHpNmMDNtRmU1lMo9YY3M6Die2SgSGWtngNBQ6kqwCFCJefkNNIxCla2H7dKDiIyvopQynJD1E3Ag5Ku/wJkdYKgPG+22xDCOo8gak7B/bOPpPvT8F3Bad6mSeeSQu6OYGTJE9Wa6g5Ok5Wc3voUIuwHFP6hgeW/p/yvarZlh0GZhyu0Z5mRI2Z9uo2w9qHTgYi4kXCDsZjcp2idZbykektA+tvIufvFo4gRskZRxBX0kTwmT7717MnrUU9O+iBRzlSZKFBVB1iW+TR+H1Jy4XkGVewGhnpsC86juBpERz+7MRQlzL8yIBgNcMbsW/54cV+/um3uRVeR1+hoat8P2ZWj0V0aJR4uPybtmIdjkxJJ8IgEOaw38NyVA7zZROzfXT9xY9c2hZxj4ha14GqOC7iGXoJYMcCMTfb2Ac3teEhuxnvxJdgntRdi uvtOh7F5 S1VRrh/CY62vI9rFtmLUf9YGxzt6WZSeL5Aj0Dr2JCm/P64Oj9Pow9hcUKymLWx2l7bo16Kv4RcNzHJwh24ozF1DnR1oPy7XWQDQ2fE6K0iZ7IQwmvd+5vjfZOQ1O/mKlUWzD5Pbyw2mGcZqACvuJeOEhieu5FGSnKp0nVWMfzrWNZqGvvCYj5TAK+52lM1G7IJ8uqwKRkT9SfUU6J0jmmkQtrk1dt3c3yVNqkD1YRMtYT/kYu2Vl+zCgD70+fIGxKw2ceNdrZ2SCUn7Vz2BlrfOtHzffXfwHbAPjcAW5Vr7cp1s= X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: List-Subscribe: List-Unsubscribe: On 06/02/2024 11:01 am, John Garry wrote: > 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 > > Regardless of a couple of minor comments, below, FWIW: > Reviewed-by: John Garry Thanks! >> --- >>   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. Oh, I never came across that before. On reflection, I am inclined to think that the "iommu_" names are usefully informative to userspace, and it's not worth churning the structure names themselves just to save a couple of lines here. >> +        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". That one I did consider, but since we now have just this one print and no imminent reason to add more, I figured I'd just stick it inline, and we can factor out a pr_fmt in future if we ever have cause to. Cheers, Robin. > >> +            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); >