Received-SPF: pass (google.com: domain of linux-kernel+bounces-54766-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <879751d4-486c-4d5e-b9b3-60e8822ff3fb@arm.com>
Date: Tue, 6 Feb 2024 11:25:03 +0000
Precedence: bulk
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 <john.g.garry@oracle.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>
 <6041982e-3c65-497f-b7bf-3ac444fa3623@oracle.com>
From: Robin Murphy <robin.murphy@arm.com>
In-Reply-To: <6041982e-3c65-497f-b7bf-3ac444fa3623@oracle.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit

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 <robin.murphy@arm.com>
> 
> Regardless of a couple of minor comments, below, FWIW:
> Reviewed-by: John Garry <john.g.garry@oracle.com>

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);
>