Re: [PATCH v3 1/3] iommu/iova: Tidy up iova_cache_get() failure

[Robin Murphy <robin.murphy@arm.com>]

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