[PATCH v3 0/3] iommu/iova: use named kmem_cache for iova magazines

[PATCH v3 0/3] iommu/iova: use named kmem_cache for iova magazines

[Robin Murphy]

Hi all,

I decided that Pasha's patch[1] was a good excuse to tackle a bit more
cleanup of the existing IOVA code, so it can fit in more neatly. Thus
to save time and effort I've taken the liberty of putting together this
mini-series as the version I'd like to merge.

Cheers,
Robin.

[1] https://lore.kernel.org/linux-iommu/20240202192820.536408-1-pasha.tatashin@soleen.com/


Pasha Tatashin (1):
  iommu/iova: use named kmem_cache for iova magazines

Robin Murphy (2):
  iommu/iova: Tidy up iova_cache_get() failure
  iommu/iova: Reorganise some code

 drivers/iommu/iova.c | 143 +++++++++++++++++++++++--------------------
 1 file changed, 76 insertions(+), 67 deletions(-)

-- 
2.39.2.101.g768bb238c484.dirty

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

[Robin Murphy]

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

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);
 
-- 
2.39.2.101.g768bb238c484.dirty

[PATCH v3 2/3] iommu/iova: Reorganise some code

[Robin Murphy]

The iova_cache_{get,put}() calls really represent top-level lifecycle
management for the whole IOVA library, so it's long been rather
confusing to have them buried right in the middle of the allocator
implementation details. Move them to a more expected position at the end
of the file, where it will then also be easier to expand them. With
this, we can also move the rcache hotplug handler (plus another stray
function) into the rcache portion of the file.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/iommu/iova.c | 128 +++++++++++++++++++++----------------------
 1 file changed, 64 insertions(+), 64 deletions(-)

diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c
index cf95001d85c0..b5de865ee50b 100644
--- a/drivers/iommu/iova.c
+++ b/drivers/iommu/iova.c
@@ -24,24 +24,8 @@ static bool iova_rcache_insert(struct iova_domain *iovad,
 static unsigned long iova_rcache_get(struct iova_domain *iovad,
 				     unsigned long size,
 				     unsigned long limit_pfn);
-static void free_cpu_cached_iovas(unsigned int cpu, struct iova_domain *iovad);
 static void free_iova_rcaches(struct iova_domain *iovad);
-
-unsigned long iova_rcache_range(void)
-{
-	return PAGE_SIZE << (IOVA_RANGE_CACHE_MAX_SIZE - 1);
-}
-
-static int iova_cpuhp_dead(unsigned int cpu, struct hlist_node *node)
-{
-	struct iova_domain *iovad;
-
-	iovad = hlist_entry_safe(node, struct iova_domain, cpuhp_dead);
-
-	free_cpu_cached_iovas(cpu, iovad);
-	return 0;
-}
-
+static void free_cpu_cached_iovas(unsigned int cpu, struct iova_domain *iovad);
 static void free_global_cached_iovas(struct iova_domain *iovad);
 
 static struct iova *to_iova(struct rb_node *node)
@@ -252,53 +236,6 @@ static void free_iova_mem(struct iova *iova)
 		kmem_cache_free(iova_cache, iova);
 }
 
-int iova_cache_get(void)
-{
-	int err = -ENOMEM;
-
-	mutex_lock(&iova_cache_mutex);
-	if (!iova_cache_users) {
-		iova_cache = kmem_cache_create("iommu_iova", sizeof(struct iova), 0,
-					       SLAB_HWCACHE_ALIGN, NULL);
-		if (!iova_cache)
-			goto out_err;
-
-		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;
-		}
-	}
-
-	iova_cache_users++;
-	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);
-
-void iova_cache_put(void)
-{
-	mutex_lock(&iova_cache_mutex);
-	if (WARN_ON(!iova_cache_users)) {
-		mutex_unlock(&iova_cache_mutex);
-		return;
-	}
-	iova_cache_users--;
-	if (!iova_cache_users) {
-		cpuhp_remove_multi_state(CPUHP_IOMMU_IOVA_DEAD);
-		kmem_cache_destroy(iova_cache);
-	}
-	mutex_unlock(&iova_cache_mutex);
-}
-EXPORT_SYMBOL_GPL(iova_cache_put);
-
 /**
  * alloc_iova - allocates an iova
  * @iovad: - iova domain in question
@@ -653,6 +590,11 @@ struct iova_rcache {
 	struct delayed_work work;
 };
 
+unsigned long iova_rcache_range(void)
+{
+	return PAGE_SIZE << (IOVA_RANGE_CACHE_MAX_SIZE - 1);
+}
+
 static struct iova_magazine *iova_magazine_alloc(gfp_t flags)
 {
 	struct iova_magazine *mag;
@@ -989,5 +931,63 @@ static void free_global_cached_iovas(struct iova_domain *iovad)
 		spin_unlock_irqrestore(&rcache->lock, flags);
 	}
 }
+
+static int iova_cpuhp_dead(unsigned int cpu, struct hlist_node *node)
+{
+	struct iova_domain *iovad;
+
+	iovad = hlist_entry_safe(node, struct iova_domain, cpuhp_dead);
+
+	free_cpu_cached_iovas(cpu, iovad);
+	return 0;
+}
+
+int iova_cache_get(void)
+{
+	int err = -ENOMEM;
+
+	mutex_lock(&iova_cache_mutex);
+	if (!iova_cache_users) {
+		iova_cache = kmem_cache_create("iommu_iova", sizeof(struct iova), 0,
+					       SLAB_HWCACHE_ALIGN, NULL);
+		if (!iova_cache)
+			goto out_err;
+
+		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;
+		}
+	}
+
+	iova_cache_users++;
+	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);
+
+void iova_cache_put(void)
+{
+	mutex_lock(&iova_cache_mutex);
+	if (WARN_ON(!iova_cache_users)) {
+		mutex_unlock(&iova_cache_mutex);
+		return;
+	}
+	iova_cache_users--;
+	if (!iova_cache_users) {
+		cpuhp_remove_multi_state(CPUHP_IOMMU_IOVA_DEAD);
+		kmem_cache_destroy(iova_cache);
+	}
+	mutex_unlock(&iova_cache_mutex);
+}
+EXPORT_SYMBOL_GPL(iova_cache_put);
+
 MODULE_AUTHOR("Anil S Keshavamurthy <anil.s.keshavamurthy@intel.com>");
 MODULE_LICENSE("GPL");
-- 
2.39.2.101.g768bb238c484.dirty

[PATCH v3 3/3] iommu/iova: use named kmem_cache for iova magazines

[Robin Murphy]

From: Pasha Tatashin <pasha.tatashin@soleen.com>

The magazine buffers can take gigabytes of kmem memory, dominating all
other allocations. For observability purpose create named slab cache so
the iova magazine memory overhead can be clearly observed.

With this change:

> slabtop -o | head
 Active / Total Objects (% used)    : 869731 / 952904 (91.3%)
 Active / Total Slabs (% used)      : 103411 / 103974 (99.5%)
 Active / Total Caches (% used)     : 135 / 211 (64.0%)
 Active / Total Size (% used)       : 395389.68K / 411430.20K (96.1%)
 Minimum / Average / Maximum Object : 0.02K / 0.43K / 8.00K

OBJS ACTIVE  USE OBJ SIZE  SLABS OBJ/SLAB CACHE SIZE NAME
244412 244239 99%    1.00K  61103       4    244412K iommu_iova_magazine
 91636  88343 96%    0.03K    739     124      2956K kmalloc-32
 75744  74844 98%    0.12K   2367      32      9468K kernfs_node_cache

On this machine it is now clear that magazine use 242M of kmem memory.

Acked-by: David Rientjes <rientjes@google.com>
Signed-off-by: Pasha Tatashin <pasha.tatashin@soleen.com>
[ rm: adjust to rework of iova_cache_{get,put} ]
Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/iommu/iova.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c
index b5de865ee50b..d59d0ea2fd21 100644
--- a/drivers/iommu/iova.c
+++ b/drivers/iommu/iova.c
@@ -590,6 +590,8 @@ struct iova_rcache {
 	struct delayed_work work;
 };
 
+static struct kmem_cache *iova_magazine_cache;
+
 unsigned long iova_rcache_range(void)
 {
 	return PAGE_SIZE << (IOVA_RANGE_CACHE_MAX_SIZE - 1);
@@ -599,7 +601,7 @@ static struct iova_magazine *iova_magazine_alloc(gfp_t flags)
 {
 	struct iova_magazine *mag;
 
-	mag = kmalloc(sizeof(*mag), flags);
+	mag = kmem_cache_alloc(iova_magazine_cache, flags);
 	if (mag)
 		mag->size = 0;
 
@@ -608,7 +610,7 @@ static struct iova_magazine *iova_magazine_alloc(gfp_t flags)
 
 static void iova_magazine_free(struct iova_magazine *mag)
 {
-	kfree(mag);
+	kmem_cache_free(iova_magazine_cache, mag);
 }
 
 static void
@@ -953,6 +955,12 @@ int iova_cache_get(void)
 		if (!iova_cache)
 			goto out_err;
 
+		iova_magazine_cache = kmem_cache_create("iommu_iova_magazine",
+							sizeof(struct iova_magazine),
+							0, SLAB_HWCACHE_ALIGN, NULL);
+		if (!iova_magazine_cache)
+			goto out_err;
+
 		err = cpuhp_setup_state_multi(CPUHP_IOMMU_IOVA_DEAD, "iommu/iova:dead",
 					      NULL, iova_cpuhp_dead);
 		if (err) {
@@ -968,6 +976,7 @@ int iova_cache_get(void)
 
 out_err:
 	kmem_cache_destroy(iova_cache);
+	kmem_cache_destroy(iova_magazine_cache);
 	mutex_unlock(&iova_cache_mutex);
 	return err;
 }
@@ -984,6 +993,7 @@ void iova_cache_put(void)
 	if (!iova_cache_users) {
 		cpuhp_remove_multi_state(CPUHP_IOMMU_IOVA_DEAD);
 		kmem_cache_destroy(iova_cache);
+		kmem_cache_destroy(iova_magazine_cache);
 	}
 	mutex_unlock(&iova_cache_mutex);
 }
-- 
2.39.2.101.g768bb238c484.dirty

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

[David Rientjes]

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>

Re: [PATCH v3 2/3] iommu/iova: Reorganise some code

[David Rientjes]

On Mon, 5 Feb 2024, Robin Murphy wrote:

> The iova_cache_{get,put}() calls really represent top-level lifecycle
> management for the whole IOVA library, so it's long been rather
> confusing to have them buried right in the middle of the allocator
> implementation details. Move them to a more expected position at the end
> of the file, where it will then also be easier to expand them. With
> this, we can also move the rcache hotplug handler (plus another stray
> function) into the rcache portion of the file.
> 
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>

Acked-by: David Rientjes <rientjes@google.com>

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

[Pasha Tatashin]

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

Re: [PATCH v3 2/3] iommu/iova: Reorganise some code

[Pasha Tatashin]

On Mon, Feb 5, 2024 at 10:32 AM Robin Murphy <robin.murphy@arm.com> wrote:
>
> The iova_cache_{get,put}() calls really represent top-level lifecycle
> management for the whole IOVA library, so it's long been rather
> confusing to have them buried right in the middle of the allocator
> implementation details. Move them to a more expected position at the end
> of the file, where it will then also be easier to expand them. With
> this, we can also move the rcache hotplug handler (plus another stray
> function) into the rcache portion of the file.
>
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>

Reviewed-by: Pasha Tatashin <pasha.tatashin@soleen.com>

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

[John Garry]

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

Re: [PATCH v3 2/3] iommu/iova: Reorganise some code

[John Garry]

On 05/02/2024 15:32, Robin Murphy wrote:
> The iova_cache_{get,put}() calls really represent top-level lifecycle
> management for the whole IOVA library, so it's long been rather
> confusing to have them buried right in the middle of the allocator
> implementation details. Move them to a more expected position at the end
> of the file, where it will then also be easier to expand them. With
> this, we can also move the rcache hotplug handler (plus another stray
> function) into the rcache portion of the file.
> 
> Signed-off-by: Robin Murphy<robin.murphy@arm.com>
> ---
FWIW:
Reviewed-by: John Garry <john.g.garry@oracle.com>

Re: [PATCH v3 3/3] iommu/iova: use named kmem_cache for iova magazines

[John Garry]

On 05/02/2024 15:32, Robin Murphy wrote:
> From: Pasha Tatashin <pasha.tatashin@soleen.com>
> 
> The magazine buffers can take gigabytes of kmem memory, dominating all
> other allocations. For observability purpose create named slab cache so
> the iova magazine memory overhead can be clearly observed.
> 
> With this change:
> 
>> slabtop -o | head
>   Active / Total Objects (% used)    : 869731 / 952904 (91.3%)
>   Active / Total Slabs (% used)      : 103411 / 103974 (99.5%)
>   Active / Total Caches (% used)     : 135 / 211 (64.0%)
>   Active / Total Size (% used)       : 395389.68K / 411430.20K (96.1%)
>   Minimum / Average / Maximum Object : 0.02K / 0.43K / 8.00K
> 
> OBJS ACTIVE  USE OBJ SIZE  SLABS OBJ/SLAB CACHE SIZE NAME
> 244412 244239 99%    1.00K  61103       4    244412K iommu_iova_magazine
>   91636  88343 96%    0.03K    739     124      2956K kmalloc-32
>   75744  74844 98%    0.12K   2367      32      9468K kernfs_node_cache
> 
> On this machine it is now clear that magazine use 242M of kmem memory.

Those caches could do with a trimming ...

> 
> Acked-by: David Rientjes <rientjes@google.com>
> Signed-off-by: Pasha Tatashin <pasha.tatashin@soleen.com>
> [ rm: adjust to rework of iova_cache_{get,put} ]
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---

FWIW:
Reviewed-by: John Garry <john.g.garry@oracle.com>

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

[Robin Murphy]

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

Re: [PATCH v3 3/3] iommu/iova: use named kmem_cache for iova magazines

[Robin Murphy]

On 06/02/2024 11:24 am, John Garry wrote:
> On 05/02/2024 15:32, Robin Murphy wrote:
>> From: Pasha Tatashin <pasha.tatashin@soleen.com>
>>
>> The magazine buffers can take gigabytes of kmem memory, dominating all
>> other allocations. For observability purpose create named slab cache so
>> the iova magazine memory overhead can be clearly observed.
>>
>> With this change:
>>
>>> slabtop -o | head
>>   Active / Total Objects (% used)    : 869731 / 952904 (91.3%)
>>   Active / Total Slabs (% used)      : 103411 / 103974 (99.5%)
>>   Active / Total Caches (% used)     : 135 / 211 (64.0%)
>>   Active / Total Size (% used)       : 395389.68K / 411430.20K (96.1%)
>>   Minimum / Average / Maximum Object : 0.02K / 0.43K / 8.00K
>>
>> OBJS ACTIVE  USE OBJ SIZE  SLABS OBJ/SLAB CACHE SIZE NAME
>> 244412 244239 99%    1.00K  61103       4    244412K iommu_iova_magazine
>>   91636  88343 96%    0.03K    739     124      2956K kmalloc-32
>>   75744  74844 98%    0.12K   2367      32      9468K kernfs_node_cache
>>
>> On this machine it is now clear that magazine use 242M of kmem memory.
> 
> Those caches could do with a trimming ...

See the discussion on v1 for more details:

https://lore.kernel.org/linux-iommu/20240201193014.2785570-1-tatashin@google.com/

but it seems this really is the idle baseline for lots of CPUs * lots of 
domains - if all those devices get going in anger it's likely that 
combined iova + iova_magazine usage truly will blow up into gigabytes.

Cheers,
Robin.

> 
>>
>> Acked-by: David Rientjes <rientjes@google.com>
>> Signed-off-by: Pasha Tatashin <pasha.tatashin@soleen.com>
>> [ rm: adjust to rework of iova_cache_{get,put} ]
>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>> ---
> 
> FWIW:
> Reviewed-by: John Garry <john.g.garry@oracle.com>

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

[Jerry Snitselaar]

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>

Re: [PATCH v3 2/3] iommu/iova: Reorganise some code

[Jerry Snitselaar]

On Mon, Feb 05, 2024 at 03:32:40PM +0000, Robin Murphy wrote:
> The iova_cache_{get,put}() calls really represent top-level lifecycle
> management for the whole IOVA library, so it's long been rather
> confusing to have them buried right in the middle of the allocator
> implementation details. Move them to a more expected position at the end
> of the file, where it will then also be easier to expand them. With
> this, we can also move the rcache hotplug handler (plus another stray
> function) into the rcache portion of the file.
> 
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
>  drivers/iommu/iova.c | 128 +++++++++++++++++++++----------------------
>  1 file changed, 64 insertions(+), 64 deletions(-)
> 

Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>

Re: [PATCH v3 3/3] iommu/iova: use named kmem_cache for iova magazines

[Jerry Snitselaar]

On Mon, Feb 05, 2024 at 03:32:41PM +0000, Robin Murphy wrote:
> From: Pasha Tatashin <pasha.tatashin@soleen.com>
> 
> The magazine buffers can take gigabytes of kmem memory, dominating all
> other allocations. For observability purpose create named slab cache so
> the iova magazine memory overhead can be clearly observed.
> 
> With this change:
> 
> > slabtop -o | head
>  Active / Total Objects (% used)    : 869731 / 952904 (91.3%)
>  Active / Total Slabs (% used)      : 103411 / 103974 (99.5%)
>  Active / Total Caches (% used)     : 135 / 211 (64.0%)
>  Active / Total Size (% used)       : 395389.68K / 411430.20K (96.1%)
>  Minimum / Average / Maximum Object : 0.02K / 0.43K / 8.00K
> 
> OBJS ACTIVE  USE OBJ SIZE  SLABS OBJ/SLAB CACHE SIZE NAME
> 244412 244239 99%    1.00K  61103       4    244412K iommu_iova_magazine
>  91636  88343 96%    0.03K    739     124      2956K kmalloc-32
>  75744  74844 98%    0.12K   2367      32      9468K kernfs_node_cache
> 
> On this machine it is now clear that magazine use 242M of kmem memory.
> 
> Acked-by: David Rientjes <rientjes@google.com>
> Signed-off-by: Pasha Tatashin <pasha.tatashin@soleen.com>
> [ rm: adjust to rework of iova_cache_{get,put} ]
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---

Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>

Re: [PATCH v3 0/3] iommu/iova: use named kmem_cache for iova magazines

[Joerg Roedel]

On Mon, Feb 05, 2024 at 03:32:38PM +0000, Robin Murphy wrote:
> Pasha Tatashin (1):
>   iommu/iova: use named kmem_cache for iova magazines
> 
> Robin Murphy (2):
>   iommu/iova: Tidy up iova_cache_get() failure
>   iommu/iova: Reorganise some code
> 
>  drivers/iommu/iova.c | 143 +++++++++++++++++++++++--------------------
>  1 file changed, 76 insertions(+), 67 deletions(-)

Applied, thanks Robin.