[PATCH v2 RFC/RFT 0/5] Save single pages from CMA area

[PATCH v2 RFC/RFT 0/5] Save single pages from CMA area

[Nicolin Chen]

This series of patches try to save single pages from CMA area bypassing
all CMA single page alloctions and allocating normal pages instead, as
all addresses within one single page are contiguous.

We had once applied the PATCH-5 but reverted it as actually not all the
callers handled the fallback allocations. Per Robin's suggestion, let's
stuff alloc_pages()/free_page() fallbacks to those callers before having
PATCH-5.

Changlog
v1->v2:
 * PATCH-2: Initialized page pointer to NULL

Nicolin Chen (5):
  ARM: dma-mapping: Add fallback normal page allocations
  dma-remap: Run alloc_pages() on failure
  iommu: amd_iommu: Add fallback normal page allocations
  arm64: dma-mapping: Add fallback normal page allocations
  dma-contiguous: Do not allocate a single page from CMA area

 arch/arm/mm/dma-mapping.c   | 13 ++++++++++---
 arch/arm64/mm/dma-mapping.c | 19 ++++++++++++-------
 drivers/iommu/amd_iommu.c   |  3 +++
 kernel/dma/contiguous.c     | 22 +++++++++++++++++++---
 kernel/dma/remap.c          |  4 ++--
 5 files changed, 46 insertions(+), 15 deletions(-)

-- 
2.17.1

[PATCH v2 RFC/RFT 1/5] ARM: dma-mapping: Add fallback normal page allocations

[Nicolin Chen]

The CMA allocation will skip allocations of single pages to save CMA
resource. This requires its callers to rebound those page allocations
from normal area. So this patch adds fallback routines.

Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com>
---
 arch/arm/mm/dma-mapping.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index 8a90f298af96..febaf637a25b 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -589,6 +589,8 @@ static void *__alloc_from_contiguous(struct device *dev, size_t size,
 	void *ptr = NULL;
 
 	page = dma_alloc_from_contiguous(dev, count, order, gfp & __GFP_NOWARN);
+	if (!page)
+		page = alloc_pages(gfp, order);
 	if (!page)
 		return NULL;
 
@@ -600,7 +602,8 @@ static void *__alloc_from_contiguous(struct device *dev, size_t size,
 	if (PageHighMem(page)) {
 		ptr = __dma_alloc_remap(page, size, GFP_KERNEL, prot, caller);
 		if (!ptr) {
-			dma_release_from_contiguous(dev, page, count);
+			if (!dma_release_from_contiguous(dev, page, count))
+				__free_pages(page, get_order(size));
 			return NULL;
 		}
 	} else {
@@ -622,7 +625,8 @@ static void __free_from_contiguous(struct device *dev, struct page *page,
 		else
 			__dma_remap(page, size, PAGE_KERNEL);
 	}
-	dma_release_from_contiguous(dev, page, size >> PAGE_SHIFT);
+	if (!dma_release_from_contiguous(dev, page, size >> PAGE_SHIFT))
+		__free_pages(page, get_order(size));
 }
 
 static inline pgprot_t __get_dma_pgprot(unsigned long attrs, pgprot_t prot)
@@ -1295,6 +1299,8 @@ static struct page **__iommu_alloc_buffer(struct device *dev, size_t size,
 
 		page = dma_alloc_from_contiguous(dev, count, order,
 						 gfp & __GFP_NOWARN);
+		if (!page)
+			page = alloc_pages(gfp, order);
 		if (!page)
 			goto error;
 
@@ -1369,7 +1375,8 @@ static int __iommu_free_buffer(struct device *dev, struct page **pages,
 	int i;
 
 	if (attrs & DMA_ATTR_FORCE_CONTIGUOUS) {
-		dma_release_from_contiguous(dev, pages[0], count);
+		if (!dma_release_from_contiguous(dev, pages[0], count))
+			__free_pages(page[0], get_order(size));
 	} else {
 		for (i = 0; i < count; i++)
 			if (pages[i])
-- 
2.17.1

Re: [PATCH v2 RFC/RFT 1/5] ARM: dma-mapping: Add fallback normal page allocations

[Christoph Hellwig]

On Tue, Mar 26, 2019 at 04:01:27PM -0700, Nicolin Chen wrote:
>  	page = dma_alloc_from_contiguous(dev, count, order, gfp & __GFP_NOWARN);
> +	if (!page)
> +		page = alloc_pages(gfp, order);

We have this fallback in most callers already.  And with me adding
it to the dma-iommu code in one series, and you to arm here I think
we really need to take a step back and think of a better way
to handle this, and the general mess that dma_alloc_from_contiguous.

So what about:

 (1) change the dma_alloc_from_contiguous prototype to be:

struct page *dma_alloc_contiguous(struct device *dev, size_t size, gfp_t gfp);

     that is: calculate order and count internally, pass the full gfp_t
     and mask it internally, and drop the pointless from in the name.
     I'd also use the oppurtunity to forbid a NULL dev argument and
     opencode those uses.
 
 (2) handle the alloc_pages fallback internally.  Note that we should
     use alloc_pages_node as we do in dma-direct.

> +			if (!dma_release_from_contiguous(dev, page, count))
> +				__free_pages(page, get_order(size));

Same for dma_release_from_contiguous - drop the _from, pass the
actual size, and handle the free_pages fallback.

Re: [PATCH v2 RFC/RFT 1/5] ARM: dma-mapping: Add fallback normal page allocations

[Christoph Hellwig]

On Wed, Apr 24, 2019 at 05:06:38PM +0200, Christoph Hellwig wrote:
>      I'd also use the oppurtunity to forbid a NULL dev argument and
>      opencode those uses.

Actually, looking at your last patch again the NULL argument might
still fit in ok, so maybe we should keep it.

Re: [PATCH v2 RFC/RFT 1/5] ARM: dma-mapping: Add fallback normal page allocations

[Nicolin Chen]

Hi Christoph,

On Wed, Apr 24, 2019 at 05:06:38PM +0200, Christoph Hellwig wrote:
> On Tue, Mar 26, 2019 at 04:01:27PM -0700, Nicolin Chen wrote:
> >  	page = dma_alloc_from_contiguous(dev, count, order, gfp & __GFP_NOWARN);
> > +	if (!page)
> > +		page = alloc_pages(gfp, order);
> 
> We have this fallback in most callers already.  And with me adding
> it to the dma-iommu code in one series, and you to arm here I think
> we really need to take a step back and think of a better way
> to handle this, and the general mess that dma_alloc_from_contiguous.
> 
> So what about:

Thanks for the suggestion!

>  (1) change the dma_alloc_from_contiguous prototype to be:
> 
> struct page *dma_alloc_contiguous(struct device *dev, size_t size, gfp_t gfp);
> 
>      that is: calculate order and count internally, pass the full gfp_t
>      and mask it internally, and drop the pointless from in the name.
>      I'd also use the oppurtunity to forbid a NULL dev argument and
>      opencode those uses.
>  
>  (2) handle the alloc_pages fallback internally.  Note that we should
>      use alloc_pages_node as we do in dma-direct.

I feel it's similar to my previous set, which did most of these
internally except the renaming part. But Catalin had a concern
that some platforms might have limits on CMA range [1]. Will it
be still okay to do the fallback internally?

[1: https://www.spinics.net/lists/arm-kernel/msg714295.html ]

Re: [PATCH v2 RFC/RFT 1/5] ARM: dma-mapping: Add fallback normal page allocations

[Christoph Hellwig]

On Wed, Apr 24, 2019 at 11:33:11AM -0700, Nicolin Chen wrote:
> I feel it's similar to my previous set, which did most of these
> internally except the renaming part. But Catalin had a concern
> that some platforms might have limits on CMA range [1]. Will it
> be still okay to do the fallback internally?
> 
> [1: https://www.spinics.net/lists/arm-kernel/msg714295.html ]

Catalins statement is correct, but I don't see how it applies to
your patch.  Your patch just ensures that the fallback we have
in most callers is uniformly applied everywhere.  The non-iommu
callers will still need to select a specific zone and/or retry
just the page allocator with other flags if the CMA (or fallback)
page doesn't match what they need.  dma-direct does this correctly
and I think the arm32 allocator does as well, although it is a bit
hard to follow sometimes.

Re: [PATCH v2 RFC/RFT 1/5] ARM: dma-mapping: Add fallback normal page allocations

[Nicolin Chen]

On Wed, Apr 24, 2019 at 09:26:52PM +0200, Christoph Hellwig wrote:
> On Wed, Apr 24, 2019 at 11:33:11AM -0700, Nicolin Chen wrote:
> > I feel it's similar to my previous set, which did most of these
> > internally except the renaming part. But Catalin had a concern
> > that some platforms might have limits on CMA range [1]. Will it
> > be still okay to do the fallback internally?
> > 
> > [1: https://www.spinics.net/lists/arm-kernel/msg714295.html ]
> 
> Catalins statement is correct, but I don't see how it applies to
> your patch.  Your patch just ensures that the fallback we have
> in most callers is uniformly applied everywhere.  The non-iommu
> callers will still need to select a specific zone and/or retry
> just the page allocator with other flags if the CMA (or fallback)
> page doesn't match what they need.  dma-direct does this correctly
> and I think the arm32 allocator does as well, although it is a bit
> hard to follow sometimes.

Okay. I will revise and submit the patches then. I think we
can still discuss on this topic once we have the changes.

Thanks

Re: [PATCH v2 RFC/RFT 1/5] ARM: dma-mapping: Add fallback normal page allocations

[Catalin Marinas]

(catching up on email)

On Wed, Apr 24, 2019 at 09:26:52PM +0200, Christoph Hellwig wrote:
> On Wed, Apr 24, 2019 at 11:33:11AM -0700, Nicolin Chen wrote:
> > I feel it's similar to my previous set, which did most of these
> > internally except the renaming part. But Catalin had a concern
> > that some platforms might have limits on CMA range [1]. Will it
> > be still okay to do the fallback internally?
> > 
> > [1: https://www.spinics.net/lists/arm-kernel/msg714295.html ]
> 
> Catalins statement is correct, but I don't see how it applies to
> your patch.  Your patch just ensures that the fallback we have
> in most callers is uniformly applied everywhere.  The non-iommu
> callers will still need to select a specific zone and/or retry
> just the page allocator with other flags if the CMA (or fallback)
> page doesn't match what they need.  dma-direct does this correctly
> and I think the arm32 allocator does as well, although it is a bit
> hard to follow sometimes.

My reading of the arm32 __dma_alloc() is that if the conditions are
right for the CMA allocator (allows blocking) and there is a default CMA
area or a per-device one, the call ends up in cma_alloc() without any
fallback if such allocation fails. Whether this is on purpose, I'm not
entirely sure. There are a couple of arm32 SoCs which call
dma_declare_contiguous() or dma_contiguous_reserve_area() and a few DT
files describing a specific CMA range (e.g. arch/arm/boot/dts/sun5i.dtsi
with a comment that address must be kept in the lower 256MB).

If ZONE_DMA is set up correctly so that cma_alloc() is (or can be made)
interchangeable with alloc_pages(GFP_DMA) from a device DMA capability
perspective , I think it should be fine to have such fallback.

-- 
Catalin

Re: [PATCH v2 RFC/RFT 1/5] ARM: dma-mapping: Add fallback normal page allocations

[Christoph Hellwig]

On Tue, Apr 30, 2019 at 04:24:21PM +0100, Catalin Marinas wrote:
> My reading of the arm32 __dma_alloc() is that if the conditions are
> right for the CMA allocator (allows blocking) and there is a default CMA
> area or a per-device one, the call ends up in cma_alloc() without any
> fallback if such allocation fails. Whether this is on purpose, I'm not
> entirely sure. There are a couple of arm32 SoCs which call
> dma_declare_contiguous() or dma_contiguous_reserve_area() and a few DT
> files describing a specific CMA range (e.g. arch/arm/boot/dts/sun5i.dtsi
> with a comment that address must be kept in the lower 256MB).
> 
> If ZONE_DMA is set up correctly so that cma_alloc() is (or can be made)
> interchangeable with alloc_pages(GFP_DMA) from a device DMA capability
> perspective , I think it should be fine to have such fallback.

Indeed.  I missed arm32 being different from everyone else, but we
already addresses that in another thread.  Sorry for misleading
everyone.

Re: [PATCH v2 RFC/RFT 1/5] ARM: dma-mapping: Add fallback normal page allocations

[Nicolin Chen]

On Wed, Apr 24, 2019 at 05:06:38PM +0200, Christoph Hellwig wrote:
> > +			if (!dma_release_from_contiguous(dev, page, count))
> > +				__free_pages(page, get_order(size));
> 
> Same for dma_release_from_contiguous - drop the _from, pass the
> actual size, and handle the free_pages fallback.

What do you think of dma_free_contiguous() instead? I feel "free"
is a bit more commonly used (in dma-mapping.h) and it's shorter.

Thanks

Re: [PATCH v2 RFC/RFT 1/5] ARM: dma-mapping: Add fallback normal page allocations

[Christoph Hellwig]

On Fri, Apr 26, 2019 at 01:21:12PM -0700, Nicolin Chen wrote:
> What do you think of dma_free_contiguous() instead? I feel "free"
> is a bit more commonly used (in dma-mapping.h) and it's shorter.

Yeah, that sounds good.

[PATCH v2 RFC/RFT 2/5] dma-remap: Run alloc_pages() on failure

[Nicolin Chen]

The CMA allocation will skip allocations of single pages to save CMA
resource. This requires its callers to rebound those page allocations
from normal area.

So this patch moves the alloc_pages() call to the fallback routines.

Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com>
---
Changlog
v1->v2:
 * PATCH-2: Initialized page pointer to NULL

 kernel/dma/remap.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/dma/remap.c b/kernel/dma/remap.c
index 2b750f13bc8f..c2076c6d6c17 100644
--- a/kernel/dma/remap.c
+++ b/kernel/dma/remap.c
@@ -109,14 +109,14 @@ int __init dma_atomic_pool_init(gfp_t gfp, pgprot_t prot)
 {
 	unsigned int pool_size_order = get_order(atomic_pool_size);
 	unsigned long nr_pages = atomic_pool_size >> PAGE_SHIFT;
-	struct page *page;
+	struct page *page = NULL;
 	void *addr;
 	int ret;
 
 	if (dev_get_cma_area(NULL))
 		page = dma_alloc_from_contiguous(NULL, nr_pages,
 						 pool_size_order, false);
-	else
+	if (!page)
 		page = alloc_pages(gfp, pool_size_order);
 	if (!page)
 		goto out;
-- 
2.17.1

[PATCH v2 RFC/RFT 3/5] iommu: amd_iommu: Add fallback normal page allocations

[Nicolin Chen]

The CMA allocation will skip allocations of single pages to save CMA
resource. This requires its callers to rebound those page allocations
from normal area. So this patch adds fallback routines.

Note: amd_iommu driver uses dma_alloc_from_contiguous() as a fallback
      allocation and uses alloc_pages() as its first round allocation.
      This's in reverse order than other callers. So the alloc_pages()
      added by this change becomes a second fallback, though it likely
      won't succeed since the alloc_pages() has failed once.

Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com>
---
 drivers/iommu/amd_iommu.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index 21cb088d6687..2aa4818f5249 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -2701,6 +2701,9 @@ static void *alloc_coherent(struct device *dev, size_t size,
 
 		page = dma_alloc_from_contiguous(dev, size >> PAGE_SHIFT,
 					get_order(size), flag & __GFP_NOWARN);
+		if (!page)
+			page = alloc_pages(flag | __GFP_NOWARN,
+					   get_order(size));
 		if (!page)
 			return NULL;
 	}
-- 
2.17.1

[PATCH v2 RFC/RFT 4/5] arm64: dma-mapping: Add fallback normal page allocations

[Nicolin Chen]

The cma allocation will skip allocations of single pages to save CMA
resource. This requires its callers to rebound those page allocations
from normal area. So this patch adds fallback routines.

Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com>
---
 arch/arm64/mm/dma-mapping.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
index 78c0a72f822c..be2302533334 100644
--- a/arch/arm64/mm/dma-mapping.c
+++ b/arch/arm64/mm/dma-mapping.c
@@ -156,17 +156,20 @@ static void *__iommu_alloc_attrs(struct device *dev, size_t size,
 		}
 	} else if (attrs & DMA_ATTR_FORCE_CONTIGUOUS) {
 		pgprot_t prot = arch_dma_mmap_pgprot(dev, PAGE_KERNEL, attrs);
+		unsigned long count = PAGE_ALIGN(size) >> PAGE_SHIFT;
 		struct page *page;
 
-		page = dma_alloc_from_contiguous(dev, size >> PAGE_SHIFT,
-					get_order(size), gfp & __GFP_NOWARN);
+		page = dma_alloc_from_contiguous(dev, count, get_order(size),
+						 gfp & __GFP_NOWARN);
+		if (!page)
+			page = alloc_pages(gfp, get_order(size));
 		if (!page)
 			return NULL;
 
 		*handle = iommu_dma_map_page(dev, page, 0, iosize, ioprot);
 		if (*handle == DMA_MAPPING_ERROR) {
-			dma_release_from_contiguous(dev, page,
-						    size >> PAGE_SHIFT);
+			if (!dma_release_from_contiguous(dev, page, count))
+				__free_pages(page, get_order(size));
 			return NULL;
 		}
 		addr = dma_common_contiguous_remap(page, size, VM_USERMAP,
@@ -178,8 +181,8 @@ static void *__iommu_alloc_attrs(struct device *dev, size_t size,
 			memset(addr, 0, size);
 		} else {
 			iommu_dma_unmap_page(dev, *handle, iosize, 0, attrs);
-			dma_release_from_contiguous(dev, page,
-						    size >> PAGE_SHIFT);
+			if (!dma_release_from_contiguous(dev, page, count))
+				__free_pages(page, get_order(size));
 		}
 	} else {
 		pgprot_t prot = arch_dma_mmap_pgprot(dev, PAGE_KERNEL, attrs);
@@ -201,6 +204,7 @@ static void *__iommu_alloc_attrs(struct device *dev, size_t size,
 static void __iommu_free_attrs(struct device *dev, size_t size, void *cpu_addr,
 			       dma_addr_t handle, unsigned long attrs)
 {
+	unsigned long count = PAGE_ALIGN(size) >> PAGE_SHIFT;
 	size_t iosize = size;
 
 	size = PAGE_ALIGN(size);
@@ -222,7 +226,8 @@ static void __iommu_free_attrs(struct device *dev, size_t size, void *cpu_addr,
 		struct page *page = vmalloc_to_page(cpu_addr);
 
 		iommu_dma_unmap_page(dev, handle, iosize, 0, attrs);
-		dma_release_from_contiguous(dev, page, size >> PAGE_SHIFT);
+		if (!dma_release_from_contiguous(dev, page, count))
+			__free_pages(page, get_order(size));
 		dma_common_free_remap(cpu_addr, size, VM_USERMAP);
 	} else if (is_vmalloc_addr(cpu_addr)){
 		struct vm_struct *area = find_vm_area(cpu_addr);
-- 
2.17.1

[PATCH v2 RFC/RFT 5/5] dma-contiguous: Do not allocate a single page from CMA area

[Nicolin Chen]

The addresses within a single page are always contiguous, so it's
not so necessary to always allocate one single page from CMA area.
Since the CMA area has a limited predefined size of space, it may
run out of space in heavy use cases, where there might be quite a
lot CMA pages being allocated for single pages.

However, there is also a concern that a device might care where a
page comes from -- it might expect the page from CMA area and act
differently if the page doesn't.

This patch tries to skip of one-page size allocations and returns
NULL so as to let callers allocate normal pages unless the device
has its own CMA area. This would save resources from the CMA area
for more CMA allocations. And it'd also reduce CMA fragmentations
resulted from trivial allocations.

Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com>
---
 kernel/dma/contiguous.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/kernel/dma/contiguous.c b/kernel/dma/contiguous.c
index b2a87905846d..09074bd04793 100644
--- a/kernel/dma/contiguous.c
+++ b/kernel/dma/contiguous.c
@@ -186,16 +186,32 @@ int __init dma_contiguous_reserve_area(phys_addr_t size, phys_addr_t base,
  *
  * This function allocates memory buffer for specified device. It uses
  * device specific contiguous memory area if available or the default
- * global one. Requires architecture specific dev_get_cma_area() helper
- * function.
+ * global one.
+ *
+ * However, it skips one-page size of allocations from the global area.
+ * As the addresses within one page are always contiguous, so there is
+ * no need to waste CMA pages for that kind; it also helps reduce the
+ * fragmentations in the CMA area. So a caller should be the rebounder
+ * in such case to allocate a normal page upon NULL return value.
+ *
+ * Requires architecture specific dev_get_cma_area() helper function.
  */
 struct page *dma_alloc_from_contiguous(struct device *dev, size_t count,
 				       unsigned int align, bool no_warn)
 {
+	struct cma *cma;
+
 	if (align > CONFIG_CMA_ALIGNMENT)
 		align = CONFIG_CMA_ALIGNMENT;
 
-	return cma_alloc(dev_get_cma_area(dev), count, align, no_warn);
+	if (dev && dev->cma_area)
+		cma = dev->cma_area;
+	else if (count > 1)
+		cma = dma_contiguous_default_area;
+	else
+		return NULL;
+
+	return cma_alloc(cma, count, align, no_warn);
 }
 
 /**
-- 
2.17.1

Re: [PATCH v2 RFC/RFT 0/5] Save single pages from CMA area

[Christoph Hellwig]

On Tue, Mar 26, 2019 at 04:01:26PM -0700, Nicolin Chen wrote:
> This series of patches try to save single pages from CMA area bypassing
> all CMA single page alloctions and allocating normal pages instead, as
> all addresses within one single page are contiguous.
> 
> We had once applied the PATCH-5 but reverted it as actually not all the
> callers handled the fallback allocations. Per Robin's suggestion, let's
> stuff alloc_pages()/free_page() fallbacks to those callers before having
> PATCH-5.

Given the problems this has caused so far I'd like to see a good
explanation of why this optimization is so important that all the churn
is even worth it..

Re: [PATCH v2 RFC/RFT 0/5] Save single pages from CMA area

[Nicolin Chen]

On Wed, Mar 27, 2019 at 09:08:21AM +0100, Christoph Hellwig wrote:
> On Tue, Mar 26, 2019 at 04:01:26PM -0700, Nicolin Chen wrote:
> > This series of patches try to save single pages from CMA area bypassing
> > all CMA single page alloctions and allocating normal pages instead, as
> > all addresses within one single page are contiguous.
> > 
> > We had once applied the PATCH-5 but reverted it as actually not all the
> > callers handled the fallback allocations. Per Robin's suggestion, let's
> > stuff alloc_pages()/free_page() fallbacks to those callers before having
> > PATCH-5.
> 
> Given the problems this has caused so far I'd like to see a good
> explanation of why this optimization is so important that all the churn
> is even worth it..

With certain downstream user cases, we had run into a CMA exhaustion
situation, and this was one of the changes that eased the problem. I
have all the reasoning in the PATCH-5 commit message. And what Robin
tested can also justify for it:
https://lore.kernel.org/patchwork/patch/1004934/#1190139

Thanks