fix dma coherent pool sizing

fix dma coherent pool sizing

[Christoph Hellwig]

Hi all

Michal pointed out that the sizing of the dma coherent pools is bonkers
as it uses the total memory to size the pool for each zone, which leads
to comically larger zones for the 16-MB ZONE_DMA on x86, which tends
leads to allocation failure warnings.  This series switches to sizing
the ZONE_DMA and ZONE_DMA32 pools based on the number of pages that
actually reside in those zones instead.

Diffstat:
 pool.c |   56 +++++++++++++++++++++++++++++++++-----------------------
 1 file changed, 33 insertions(+), 23 deletions(-)

[PATCH 1/3] dma-pool: factor out a calculate_pool_size helper

[Christoph Hellwig]

Add a helper to calculate the pool size from dma_atomic_pool_init.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Robin Murphy <robin.murphy@arm.com>
---
 kernel/dma/pool.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/kernel/dma/pool.c b/kernel/dma/pool.c
index 4d40dcce7604b..37008d4f6d7af 100644
--- a/kernel/dma/pool.c
+++ b/kernel/dma/pool.c
@@ -184,6 +184,14 @@ static __init struct gen_pool *__dma_atomic_pool_init(size_t pool_size,
 	return pool;
 }
 
+static size_t calculate_pool_size(unsigned long zone_pages)
+{
+	unsigned long pages = zone_pages / (SZ_1G / SZ_128K);
+
+	pages = min_t(unsigned long, pages, MAX_ORDER_NR_PAGES);
+	return max_t(size_t, pages << PAGE_SHIFT, SZ_128K);
+}
+
 static int __init dma_atomic_pool_init(void)
 {
 	int ret = 0;
@@ -192,11 +200,9 @@ static int __init dma_atomic_pool_init(void)
 	 * If coherent_pool was not used on the command line, default the pool
 	 * sizes to 128KB per 1GB of memory, min 128KB, max MAX_ORDER-1.
 	 */
-	if (!atomic_pool_size) {
-		unsigned long pages = totalram_pages() / (SZ_1G / SZ_128K);
-		pages = min_t(unsigned long, pages, MAX_ORDER_NR_PAGES);
-		atomic_pool_size = max_t(size_t, pages << PAGE_SHIFT, SZ_128K);
-	}
+	if (!atomic_pool_size)
+		atomic_pool_size = calculate_pool_size(totalram_pages());
+
 	INIT_WORK(&atomic_pool_work, atomic_pool_work_fn);
 
 	atomic_pool_kernel = __dma_atomic_pool_init(atomic_pool_size,
-- 
2.30.2

[PATCH 2/3] dma-pool: don't return errors from dma_atomic_pool_init

[Christoph Hellwig]

Returning errors from initcalls does not change a thing, thus don't
bother with returning -ENOMEM if one of the pool allocations failed.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Robin Murphy <robin.murphy@arm.com>
---
 kernel/dma/pool.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/kernel/dma/pool.c b/kernel/dma/pool.c
index 37008d4f6d7af..4b4c59d05e436 100644
--- a/kernel/dma/pool.c
+++ b/kernel/dma/pool.c
@@ -194,8 +194,6 @@ static size_t calculate_pool_size(unsigned long zone_pages)
 
 static int __init dma_atomic_pool_init(void)
 {
-	int ret = 0;
-
 	/*
 	 * If coherent_pool was not used on the command line, default the pool
 	 * sizes to 128KB per 1GB of memory, min 128KB, max MAX_ORDER-1.
@@ -207,23 +205,17 @@ static int __init dma_atomic_pool_init(void)
 
 	atomic_pool_kernel = __dma_atomic_pool_init(atomic_pool_size,
 						    GFP_KERNEL);
-	if (!atomic_pool_kernel)
-		ret = -ENOMEM;
 	if (has_managed_dma()) {
 		atomic_pool_dma = __dma_atomic_pool_init(atomic_pool_size,
 						GFP_KERNEL | GFP_DMA);
-		if (!atomic_pool_dma)
-			ret = -ENOMEM;
 	}
 	if (IS_ENABLED(CONFIG_ZONE_DMA32)) {
 		atomic_pool_dma32 = __dma_atomic_pool_init(atomic_pool_size,
 						GFP_KERNEL | GFP_DMA32);
-		if (!atomic_pool_dma32)
-			ret = -ENOMEM;
 	}
 
 	dma_atomic_pool_debugfs_init();
-	return ret;
+	return 0;
 }
 postcore_initcall(dma_atomic_pool_init);
 
-- 
2.30.2

[PATCH 3/3] dma-pool: limit DMA and DMA32 zone size pools

[Christoph Hellwig]

Limit the sizing of the atomic pools for allocations from
ZONE_DMA and ZONE_DMA32 based on the number of pages actually
present in those zones instead of the total memory.

Fixes: c84dc6e68a1d ("dma-pool: add additional coherent pools to map to gfp mask")
Reported-by: Michal Hocko <mhocko@suse.com>
Signed-off-by: Christoph Hellwig <hch@lst.de>
Acked-by: Michal Hocko <mhocko@suse.com>
---
 kernel/dma/pool.c | 34 +++++++++++++++++++++++-----------
 1 file changed, 23 insertions(+), 11 deletions(-)

diff --git a/kernel/dma/pool.c b/kernel/dma/pool.c
index 4b4c59d05e436..2a6c71311a175 100644
--- a/kernel/dma/pool.c
+++ b/kernel/dma/pool.c
@@ -192,28 +192,40 @@ static size_t calculate_pool_size(unsigned long zone_pages)
 	return max_t(size_t, pages << PAGE_SHIFT, SZ_128K);
 }
 
+static struct gen_pool * __init __maybe_unused zone_pool_init(int zone_idx,
+		gfp_t gfp)
+{
+	struct pglist_data *pgdat;
+	unsigned long nr_pages = 0;
+
+	for_each_online_pgdat(pgdat)
+		nr_pages += zone_managed_pages(&pgdat->node_zones[zone_idx]);
+
+	if (!nr_pages)
+		return NULL;
+
+	return __dma_atomic_pool_init(calculate_pool_size(nr_pages), gfp);
+}
+
 static int __init dma_atomic_pool_init(void)
 {
+#ifdef CONFIG_ZONE_DMA
+	atomic_pool_dma = zone_pool_init(ZONE_DMA, GFP_DMA);
+#endif
+#ifdef CONFIG_ZONE_DMA32
+	atomic_pool_dma32 = zone_pool_init(ZONE_DMA32, GFP_DMA32);
+#endif
+
 	/*
 	 * If coherent_pool was not used on the command line, default the pool
 	 * sizes to 128KB per 1GB of memory, min 128KB, max MAX_ORDER-1.
 	 */
 	if (!atomic_pool_size)
 		atomic_pool_size = calculate_pool_size(totalram_pages());
-
-	INIT_WORK(&atomic_pool_work, atomic_pool_work_fn);
-
 	atomic_pool_kernel = __dma_atomic_pool_init(atomic_pool_size,
 						    GFP_KERNEL);
-	if (has_managed_dma()) {
-		atomic_pool_dma = __dma_atomic_pool_init(atomic_pool_size,
-						GFP_KERNEL | GFP_DMA);
-	}
-	if (IS_ENABLED(CONFIG_ZONE_DMA32)) {
-		atomic_pool_dma32 = __dma_atomic_pool_init(atomic_pool_size,
-						GFP_KERNEL | GFP_DMA32);
-	}
 
+	INIT_WORK(&atomic_pool_work, atomic_pool_work_fn);
 	dma_atomic_pool_debugfs_init();
 	return 0;
 }
-- 
2.30.2

Re: [PATCH 1/3] dma-pool: factor out a calculate_pool_size helper

[Christoph Hellwig]

On Wed, Aug 17, 2022 at 01:32:42PM +0100, Robin Murphy wrote:
> On 2022-08-17 07:06, Christoph Hellwig wrote:
>> Add a helper to calculate the pool size from dma_atomic_pool_init,
>> and fix up the last max_t to use the proper type.
>
> Hmm, both atomic_pool_size and the argument to __dma_atomic_pool_init() 
> where this gets directly passed later are size_t, not to mention that the 
> function name says we're calculating a size, so I'd say size_t *is* the 
> proper type to return here.

But the type passed to calculate_pool_size isn't about the return type,
but rather the type to use for the comparing the other two arguments.

The should generally by the largest of the involved types.  Besides
that using a size_t for a number of pages is not a correct use of
size_t, but that's a separate story.

But I'll go back to what we had originally, there's no good reason
to change it in this series.

Re: [PATCH 1/3] dma-pool: factor out a calculate_pool_size helper

[Robin Murphy]

On 2022-08-17 07:06, Christoph Hellwig wrote:
> Add a helper to calculate the pool size from dma_atomic_pool_init,
> and fix up the last max_t to use the proper type.

Hmm, both atomic_pool_size and the argument to __dma_atomic_pool_init() 
where this gets directly passed later are size_t, not to mention that 
the function name says we're calculating a size, so I'd say size_t *is* 
the proper type to return here.

> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   kernel/dma/pool.c | 17 ++++++++++++-----
>   1 file changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/kernel/dma/pool.c b/kernel/dma/pool.c
> index 4d40dcce7604b..56f96678934bf 100644
> --- a/kernel/dma/pool.c
> +++ b/kernel/dma/pool.c
> @@ -184,6 +184,15 @@ static __init struct gen_pool *__dma_atomic_pool_init(size_t pool_size,
>   	return pool;
>   }
>   
> +static unsigned long calculate_pool_size(unsigned long zone_pages)
> +{
> +	unsigned long nr_pages = min_t(unsigned long,
> +				       zone_pages / (SZ_1G / SZ_128K),
> +				       MAX_ORDER_NR_PAGES);

Nit: this is arguably less readable, and objectively one line longer, 
than the original two statements.

Other that those nits though,

Reviewed-by: Robin Murphy <robin.murphy@arm.com>

> +
> +	return max_t(unsigned long, nr_pages << PAGE_SHIFT, SZ_128K);
> +}
> +
>   static int __init dma_atomic_pool_init(void)
>   {
>   	int ret = 0;
> @@ -192,11 +201,9 @@ static int __init dma_atomic_pool_init(void)
>   	 * If coherent_pool was not used on the command line, default the pool
>   	 * sizes to 128KB per 1GB of memory, min 128KB, max MAX_ORDER-1.
>   	 */
> -	if (!atomic_pool_size) {
> -		unsigned long pages = totalram_pages() / (SZ_1G / SZ_128K);
> -		pages = min_t(unsigned long, pages, MAX_ORDER_NR_PAGES);
> -		atomic_pool_size = max_t(size_t, pages << PAGE_SHIFT, SZ_128K);
> -	}
> +	if (!atomic_pool_size)
> +		atomic_pool_size = calculate_pool_size(totalram_pages());
> +
>   	INIT_WORK(&atomic_pool_work, atomic_pool_work_fn);
>   
>   	atomic_pool_kernel = __dma_atomic_pool_init(atomic_pool_size,

[PATCH 1/3] dma-pool: factor out a calculate_pool_size helper

[Christoph Hellwig]

Add a helper to calculate the pool size from dma_atomic_pool_init,
and fix up the last max_t to use the proper type.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 kernel/dma/pool.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/kernel/dma/pool.c b/kernel/dma/pool.c
index 4d40dcce7604b..56f96678934bf 100644
--- a/kernel/dma/pool.c
+++ b/kernel/dma/pool.c
@@ -184,6 +184,15 @@ static __init struct gen_pool *__dma_atomic_pool_init(size_t pool_size,
 	return pool;
 }
 
+static unsigned long calculate_pool_size(unsigned long zone_pages)
+{
+	unsigned long nr_pages = min_t(unsigned long,
+				       zone_pages / (SZ_1G / SZ_128K),
+				       MAX_ORDER_NR_PAGES);
+
+	return max_t(unsigned long, nr_pages << PAGE_SHIFT, SZ_128K);
+}
+
 static int __init dma_atomic_pool_init(void)
 {
 	int ret = 0;
@@ -192,11 +201,9 @@ static int __init dma_atomic_pool_init(void)
 	 * If coherent_pool was not used on the command line, default the pool
 	 * sizes to 128KB per 1GB of memory, min 128KB, max MAX_ORDER-1.
 	 */
-	if (!atomic_pool_size) {
-		unsigned long pages = totalram_pages() / (SZ_1G / SZ_128K);
-		pages = min_t(unsigned long, pages, MAX_ORDER_NR_PAGES);
-		atomic_pool_size = max_t(size_t, pages << PAGE_SHIFT, SZ_128K);
-	}
+	if (!atomic_pool_size)
+		atomic_pool_size = calculate_pool_size(totalram_pages());
+
 	INIT_WORK(&atomic_pool_work, atomic_pool_work_fn);
 
 	atomic_pool_kernel = __dma_atomic_pool_init(atomic_pool_size,
-- 
2.30.2