| Message ID | ae4a3bda2d6a9b738221553c838d30473bd624e7.1707144953.git.robin.murphy@arm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | iommu/iova: use named kmem_cache for iova magazines | expand |
On Mon, 5 Feb 2024, 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> Much easier to follow :) Acked-by: David Rientjes <rientjes@google.com>
On Mon, Feb 5, 2024 at 10:32 AM Robin Murphy <robin.murphy@arm.com> 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> Reviewed-by: Pasha Tatashin <pasha.tatashin@soleen.com> Thank you, Pasha
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); >
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); >
On Mon, Feb 05, 2024 at 03:32:39PM +0000, 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> > --- > drivers/iommu/iova.c | 33 ++++++++++++++++----------------- > 1 file changed, 16 insertions(+), 17 deletions(-) > Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>
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); + 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)); + 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);
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> --- drivers/iommu/iova.c | 33 ++++++++++++++++----------------- 1 file changed, 16 insertions(+), 17 deletions(-)