Chunk Heap Support on DMA-HEAP

The upcoming patch will introduce __GFP_NORETRY semantic in alloc_contig_range which is a failfast mode of the API. Instead of adding a additional parameter for gfp, replace no_warn with gfp flag.

To keep old behaviors, it follows the rule below.

no_warn gfp_flags

false GFP_KERNEL
true GFP_KERNEL|__GFP_NOWARN
gfp & __GFP_NOWARN GFP_KERNEL | (gfp & __GFP_NOWARN)

Reviewed-by Suren Baghdasaryan <[email protected]>

Signed-off-by Minchan Kim <[email protected]>

---
 drivers/dma-buf/heaps/cma_heap.c |  2 +-
 drivers/s390/char/vmcp.c         |  2 +-
 include/linux/cma.h              |  2 +-
 kernel/dma/contiguous.c          |  3 ++-
 mm/cma.c                         | 12 ++++++------
 mm/cma_debug.c                   |  2 +-
 mm/hugetlb.c                     |  6 ++++--
 mm/secretmem.c                   |  3 ++-
 8 files changed, 18 insertions(+), 14 deletions(-)

drivers/dma-buf/heaps/cma_heap.c +1 -1

index 364fc2f3e499..0afc1907887a 100644
@@ -298,7 +298,7 @@ static int cma_heap_allocate(struct dma_heap *heap,
 	if (align > CONFIG_CMA_ALIGNMENT)
 		align = CONFIG_CMA_ALIGNMENT;
 
-	cma_pages = cma_alloc(cma_heap->cma, pagecount, align, false);
+	cma_pages = cma_alloc(cma_heap->cma, pagecount, align, GFP_KERNEL);
 	if (!cma_pages)
 		goto free_buffer;
 

drivers/s390/char/vmcp.c +1 -1

index 9e066281e2d0..78f9adf56456 100644
@@ -70,7 +70,7 @@ static void vmcp_response_alloc(struct vmcp_session *session)
 	 * anymore the system won't work anyway.
 	 */
 	if (order > 2)
-		page = cma_alloc(vmcp_cma, nr_pages, 0, false);
+		page = cma_alloc(vmcp_cma, nr_pages, 0, GFP_KERNEL);
 	if (page) {
 		session->response = (char *)page_to_phys(page);
 		session->cma_alloc = 1;

include/linux/cma.h +1 -1

index 217999c8a762..d6c02d08ddbc 100644
@@ -45,7 +45,7 @@ extern int cma_init_reserved_mem(phys_addr_t base, phys_addr_t size,
 					const char *name,
 					struct cma **res_cma);
 extern struct page *cma_alloc(struct cma *cma, size_t count, unsigned int align,
-			      bool no_warn);
+			      gfp_t gfp_mask);
 extern bool cma_release(struct cma *cma, const struct page *pages, unsigned int count);
 
 extern int cma_for_each_area(int (*it)(struct cma *cma, void *data), void *data);

kernel/dma/contiguous.c +2 -1

index 3d63d91cba5c..552ed531c018 100644
@@ -260,7 +260,8 @@ struct page *dma_alloc_from_contiguous(struct device *dev, size_t count,
 	if (align > CONFIG_CMA_ALIGNMENT)
 		align = CONFIG_CMA_ALIGNMENT;
 
-	return cma_alloc(dev_get_cma_area(dev), count, align, no_warn);
+	return cma_alloc(dev_get_cma_area(dev), count, align, GFP_KERNEL |
+			(no_warn ? __GFP_NOWARN : 0));
 }
 
 /**

mm/cma.c +6 -6

index 0ba69cd16aeb..d50627686fec 100644
@@ -419,13 +419,13 @@ static inline void cma_debug_show_areas(struct cma *cma) { }
  * @cma:   Contiguous memory region for which the allocation is performed.
  * @count: Requested number of pages.
  * @align: Requested alignment of pages (in PAGE_SIZE order).
- * @no_warn: Avoid printing message about failed allocation
+ * @gfp_mask: GFP mask to use during the cma allocation.
  *
  * This function allocates part of contiguous memory on specific
  * contiguous memory area.
  */
 struct page *cma_alloc(struct cma *cma, size_t count, unsigned int align,
-		       bool no_warn)
+		       gfp_t gfp_mask)
 {
 	unsigned long mask, offset;
 	unsigned long pfn = -1;

@@ -438,8 +438,8 @@ struct page *cma_alloc(struct cma *cma, size_t count, unsigned int align,
 	if (!cma || !cma->count || !cma->bitmap)
 		return NULL;
 
-	pr_debug("%s(cma %p, count %zu, align %d)\n", __func__, (void *)cma,
-		 count, align);
+	pr_debug("%s(cma %p, count %zu, align %d gfp_mask 0x%x)\n", __func__,
+			(void *)cma, count, align, gfp_mask);
 
 	if (!count)
 		return NULL;

@@ -471,7 +471,7 @@ struct page *cma_alloc(struct cma *cma, size_t count, unsigned int align,
 
 		pfn = cma->base_pfn + (bitmap_no << cma->order_per_bit);
 		ret = alloc_contig_range(pfn, pfn + count, MIGRATE_CMA,
-				     GFP_KERNEL | (no_warn ? __GFP_NOWARN : 0));
+						gfp_mask);
 
 		if (ret == 0) {
 			page = pfn_to_page(pfn);

@@ -500,7 +500,7 @@ struct page *cma_alloc(struct cma *cma, size_t count, unsigned int align,
 			page_kasan_tag_reset(page + i);
 	}
 
-	if (ret && !no_warn) {
+	if (ret && !(gfp_mask & __GFP_NOWARN)) {
 		pr_err("%s: alloc failed, req-size: %zu pages, ret: %d\n",
 			__func__, count, ret);
 		cma_debug_show_areas(cma);

mm/cma_debug.c +1 -1

index d5bf8aa34fdc..00170c41cf81 100644
@@ -137,7 +137,7 @@ static int cma_alloc_mem(struct cma *cma, int count)
 	if (!mem)
 		return -ENOMEM;
 
-	p = cma_alloc(cma, count, 0, false);
+	p = cma_alloc(cma, count, 0, GFP_KERNEL);
 	if (!p) {
 		kfree(mem);
 		return -ENOMEM;

mm/hugetlb.c +4 -2

index a6bad1f686c5..4209a2ed1e1b 100644
@@ -1266,7 +1266,8 @@ static struct page *alloc_gigantic_page(struct hstate *h, gfp_t gfp_mask,
 
 		if (hugetlb_cma[nid]) {
 			page = cma_alloc(hugetlb_cma[nid], nr_pages,
-					huge_page_order(h), true);
+					huge_page_order(h),
+					GFP_KERNEL | __GFP_NOWARN);
 			if (page)
 				return page;
 		}

@@ -1277,7 +1278,8 @@ static struct page *alloc_gigantic_page(struct hstate *h, gfp_t gfp_mask,
 					continue;
 
 				page = cma_alloc(hugetlb_cma[node], nr_pages,
-						huge_page_order(h), true);
+						huge_page_order(h),
+						GFP_KERNEL | __GFP_NOWARN);
 				if (page)
 					return page;
 			}

mm/secretmem.c +0 -0

index b8a32954ac68..585d55b9f9d8 100644

The upcoming patch will introduce __GFP_NORETRY semantic in alloc_contig_range which is a failfast mode of the API. Instead of adding a additional parameter for gfp, replace no_warn with gfp flag.

To keep old behaviors, it follows the rule below.

no_warn gfp_flags

false GFP_KERNEL
true GFP_KERNEL|__GFP_NOWARN
gfp & __GFP_NOWARN GFP_KERNEL | (gfp & __GFP_NOWARN)

Reviewed-by Suren Baghdasaryan <[email protected]>

Signed-off-by Minchan Kim <[email protected]>

The upcoming patch will introduce __GFP_NORETRY semantic in alloc_contig_range which is a failfast mode of the API. Instead of adding a additional parameter for gfp, replace no_warn with gfp flag.

To keep old behaviors, it follows the rule below.

no_warn gfp_flags

false GFP_KERNEL
true GFP_KERNEL|__GFP_NOWARN
gfp & __GFP_NOWARN GFP_KERNEL | (gfp & __GFP_NOWARN)

Reviewed-by Suren Baghdasaryan <[email protected]>

Signed-off-by Minchan Kim <[email protected]>

Quoted 15 lines · Minchan Kim

The upcoming patch will introduce __GFP_NORETRY semantic in alloc_contig_range which is a failfast mode of the API. Instead of adding a additional parameter for gfp, replace no_warn with gfp flag.

To keep old behaviors, it follows the rule below.

no_warn gfp_flags

false GFP_KERNEL
true GFP_KERNEL|__GFP_NOWARN
gfp & __GFP_NOWARN GFP_KERNEL | (gfp & __GFP_NOWARN)

Reviewed-by Suren Baghdasaryan <[email protected]>

Signed-off-by Minchan Kim <[email protected]>

[...]

mm/cma.c static inline void cma_debug_show_areas(struct cma *cma) { } · line 422

Quoted 10 lines

mm/cma.c +1 -1

index 0ba69cd16aeb..d50627686fec 100644
@@ -419,13 +419,13 @@ static inline void cma_debug_show_areas(struct cma *cma) { }
  * @cma:   Contiguous memory region for which the allocation is performed.
  * @count: Requested number of pages.
  * @align: Requested alignment of pages (in PAGE_SIZE order).
- * @no_warn: Avoid printing message about failed allocation
+ * @gfp_mask: GFP mask to use during the cma allocation.

Call out supported gfp flags explicitly. Have a look at kvmalloc_node for a guidance.

Quoted 17 lines · Michal Hocko

Quoted 15 lines · Minchan Kim

The upcoming patch will introduce __GFP_NORETRY semantic in alloc_contig_range which is a failfast mode of the API. Instead of adding a additional parameter for gfp, replace no_warn with gfp flag.

To keep old behaviors, it follows the rule below.

no_warn gfp_flags

false GFP_KERNEL
true GFP_KERNEL|__GFP_NOWARN
gfp & __GFP_NOWARN GFP_KERNEL | (gfp & __GFP_NOWARN)

Reviewed-by Suren Baghdasaryan <[email protected]>

Signed-off-by Minchan Kim <[email protected]>

[...]

Quoted 13 lines

mm/cma.c static inline void cma_debug_show_areas(struct cma *cma) { } · line 422

Quoted 10 lines

mm/cma.c +1 -1

index 0ba69cd16aeb..d50627686fec 100644
@@ -419,13 +419,13 @@ static inline void cma_debug_show_areas(struct cma *cma) { }
  * @cma:   Contiguous memory region for which the allocation is performed.
  * @count: Requested number of pages.
  * @align: Requested alignment of pages (in PAGE_SIZE order).
- * @no_warn: Avoid printing message about failed allocation
+ * @gfp_mask: GFP mask to use during the cma allocation.

Call out supported gfp flags explicitly. Have a look at kvmalloc_node for a guidance.

How about this?

mm/cma.c +0 -0

index d50627686fec..b94727b694d6 100644

mm/cma.c +0 -0

  */
 struct page *cma_alloc(struct cma *cma, size_t count, unsigned int align,
                       gfp_t gfp_mask)

Quoted 40 lines · Minchan Kim

Quoted 17 lines · Michal Hocko

Quoted 15 lines · Minchan Kim

The upcoming patch will introduce __GFP_NORETRY semantic in alloc_contig_range which is a failfast mode of the API. Instead of adding a additional parameter for gfp, replace no_warn with gfp flag.

To keep old behaviors, it follows the rule below.

no_warn gfp_flags

false GFP_KERNEL
true GFP_KERNEL|__GFP_NOWARN
gfp & __GFP_NOWARN GFP_KERNEL | (gfp & __GFP_NOWARN)

Reviewed-by Suren Baghdasaryan <[email protected]>

Signed-off-by Minchan Kim <[email protected]>

[...]

Quoted 13 lines

mm/cma.c static inline void cma_debug_show_areas(struct cma *cma) { } · line 422

Quoted 10 lines

mm/cma.c +1 -1

index 0ba69cd16aeb..d50627686fec 100644
@@ -419,13 +419,13 @@ static inline void cma_debug_show_areas(struct cma *cma) { }
  * @cma:   Contiguous memory region for which the allocation is performed.
  * @count: Requested number of pages.
  * @align: Requested alignment of pages (in PAGE_SIZE order).
- * @no_warn: Avoid printing message about failed allocation
+ * @gfp_mask: GFP mask to use during the cma allocation.

Call out supported gfp flags explicitly. Have a look at kvmalloc_node for a guidance.

How about this?

mm/cma.c +0 -0

index d50627686fec..b94727b694d6 100644

mm/cma.c static inline void cma_debug_show_areas(struct cma *cma) { } · line 429

Quoted 7 lines

mm/cma.c +4 -0

@@ -423,6 +423,10 @@ static inline void cma_debug_show_areas(struct cma *cma) { }
  *
  * This function allocates part of contiguous memory on specific
  * contiguous memory area.
+ *
+ * For gfp_mask, GFP_KERNEL and __GFP_NORETRY are supported. __GFP_NORETRY
+ * will avoid costly functions(e.g., waiting on page_writeback and locking)
+ * at current implementaion during the page migration.

rather than explicitly mentioning what the flag implies I think it would be more useful to state the intended usecase. See how kvmalloc_node says "__GFP_RETRY_MAYFAIL is supported, and it should be used only if kmalloc is preferable to the vmalloc fallback, due to visible performance drawbacks.
__GFP_NOWARN is also supported to suppress allocation failure messages."

This would help people not familiar with internals to see whether this flag is a good fit for them.

In this case I woul go with
"
@flags: gfp mask. Must be compatible (superset) with GFP_KERNEL. [...]
Reclaim modifiers (__GFP_RETRY_MAYFAIL, __GFP_NOFAIL) are not supported. __GFP_NORETRY is supported, and it should be used for opportunistic allocation attempts that should rather fail quickly when the caller has a fallback strategy.
"

Obviously for this patch you will go with a simple statement that Reclaim modifiers are not supported at all.

Quoted 69 lines · Michal Hocko

Quoted 40 lines · Minchan Kim

Quoted 17 lines · Michal Hocko

Quoted 15 lines · Minchan Kim

The upcoming patch will introduce __GFP_NORETRY semantic in alloc_contig_range which is a failfast mode of the API. Instead of adding a additional parameter for gfp, replace no_warn with gfp flag.

To keep old behaviors, it follows the rule below.

no_warn gfp_flags

false GFP_KERNEL
true GFP_KERNEL|__GFP_NOWARN
gfp & __GFP_NOWARN GFP_KERNEL | (gfp & __GFP_NOWARN)

Reviewed-by Suren Baghdasaryan <[email protected]>

Signed-off-by Minchan Kim <[email protected]>

[...]

Quoted 13 lines

mm/cma.c static inline void cma_debug_show_areas(struct cma *cma) { } · line 422

Quoted 10 lines

mm/cma.c +1 -1

index 0ba69cd16aeb..d50627686fec 100644
@@ -419,13 +419,13 @@ static inline void cma_debug_show_areas(struct cma *cma) { }
  * @cma:   Contiguous memory region for which the allocation is performed.
  * @count: Requested number of pages.
  * @align: Requested alignment of pages (in PAGE_SIZE order).
- * @no_warn: Avoid printing message about failed allocation
+ * @gfp_mask: GFP mask to use during the cma allocation.

Call out supported gfp flags explicitly. Have a look at kvmalloc_node for a guidance.

How about this?

mm/cma.c +0 -0

index d50627686fec..b94727b694d6 100644

mm/cma.c static inline void cma_debug_show_areas(struct cma *cma) { } · line 429

Quoted 7 lines

mm/cma.c +4 -0

@@ -423,6 +423,10 @@ static inline void cma_debug_show_areas(struct cma *cma) { }
  *
  * This function allocates part of contiguous memory on specific
  * contiguous memory area.
+ *
+ * For gfp_mask, GFP_KERNEL and __GFP_NORETRY are supported. __GFP_NORETRY
+ * will avoid costly functions(e.g., waiting on page_writeback and locking)
+ * at current implementaion during the page migration.

rather than explicitly mentioning what the flag implies I think it would be more useful to state the intended usecase. See how kvmalloc_node says "__GFP_RETRY_MAYFAIL is supported, and it should be used only if kmalloc is preferable to the vmalloc fallback, due to visible performance drawbacks.
__GFP_NOWARN is also supported to suppress allocation failure messages."

This would help people not familiar with internals to see whether this flag is a good fit for them.

In this case I woul go with
"
@flags: gfp mask. Must be compatible (superset) with GFP_KERNEL. [...]
Reclaim modifiers (__GFP_RETRY_MAYFAIL, __GFP_NOFAIL) are not supported. __GFP_NORETRY is supported, and it should be used for opportunistic allocation attempts that should rather fail quickly when the caller has a fallback strategy.
"

Quoted 2 lines

Obviously for this patch you will go with a simple statement that Reclaim modifiers are not supported at all.

After more discussion for gfp_flags in thread of next patch, let me changes a bit more based on it.

Thanks for the suggestion, Michal.

Contiguous memory allocation can be stalled due to waiting on page writeback and/or page lock which causes unpredictable delay. It's a unavoidable cost for the requestor to get *big* contiguous memory but it's expensive for *small* contiguous memory(e.g., order-4) because caller could retry the request in different range where would have easy migratable pages without stalling.

This patch introduce __GFP_NORETRY as compaction gfp_mask in alloc_contig_range so it will fail fast without blocking when it encounters pages needed waiting.

Signed-off-by Minchan Kim <[email protected]>

---
 mm/page_alloc.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

mm/page_alloc.c +6 -2

index b031a5ae0bd5..1cdc3ee0b22e 100644
@@ -8491,12 +8491,16 @@ static int __alloc_contig_migrate_range(struct compact_control *cc,
 	unsigned int nr_reclaimed;
 	unsigned long pfn = start;
 	unsigned int tries = 0;
+	unsigned int max_tries = 5;
 	int ret = 0;
 	struct migration_target_control mtc = {
 		.nid = zone_to_nid(cc->zone),
 		.gfp_mask = GFP_USER | __GFP_MOVABLE | __GFP_RETRY_MAYFAIL,
 	};
 
+	if (cc->alloc_contig && cc->mode == MIGRATE_ASYNC)
+		max_tries = 1;
+
 	migrate_prep();
 
 	while (pfn < end || !list_empty(&cc->migratepages)) {

@@ -8513,7 +8517,7 @@ static int __alloc_contig_migrate_range(struct compact_control *cc,
 				break;
 			}
 			tries = 0;
-		} else if (++tries == 5) {
+		} else if (++tries == max_tries) {
 			ret = ret < 0 ? ret : -EBUSY;
 			break;
 		}

@@ -8564,7 +8568,7 @@ int alloc_contig_range(unsigned long start, unsigned long end,
 		.nr_migratepages = 0,
 		.order = -1,
 		.zone = page_zone(pfn_to_page(start)),
-		.mode = MIGRATE_SYNC,
+		.mode = gfp_mask & __GFP_NORETRY ? MIGRATE_ASYNC : MIGRATE_SYNC,
 		.ignore_skip_hint = true,
 		.no_set_skip_hint = true,
 		.gfp_mask = current_gfp_context(gfp_mask),

Signature · 1 line

2.30.0.296.g2bfb1c46d8-goog

Quoted 8 lines · Minchan Kim

Contiguous memory allocation can be stalled due to waiting on page writeback and/or page lock which causes unpredictable delay. It's a unavoidable cost for the requestor to get *big* contiguous memory but it's expensive for *small* contiguous memory(e.g., order-4) because caller could retry the request in different range where would have easy migratable pages without stalling.

Quoted 3 lines

This patch introduce __GFP_NORETRY as compaction gfp_mask in alloc_contig_range so it will fail fast without blocking when it encounters pages needed waiting.

I am not against controling how hard this allocator tries with gfp mask but this changelog is rather void on any data and any user.

Quoted 11 lines · Minchan Kim

Contiguous memory allocation can be stalled due to waiting on page writeback and/or page lock which causes unpredictable delay. It's a unavoidable cost for the requestor to get *big* contiguous memory but it's expensive for *small* contiguous memory(e.g., order-4) because caller could retry the request in different range where would have easy migratable pages without stalling.

This patch introduce __GFP_NORETRY as compaction gfp_mask in alloc_contig_range so it will fail fast without blocking when it encounters pages needed waiting.

I am not against controling how hard this allocator tries with gfp mask but this changelog is rather void on any data and any user.

It is also rather dubious to have retries when then caller says to not retry.

Also why didn't you consider GFP_NOWAIT semantic for non blocking mode?

Quoted 47 lines

Signed-off-by Minchan Kim <[email protected]>

---
 mm/page_alloc.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

mm/page_alloc.c +6 -2

index b031a5ae0bd5..1cdc3ee0b22e 100644
@@ -8491,12 +8491,16 @@ static int __alloc_contig_migrate_range(struct compact_control *cc,
 	unsigned int nr_reclaimed;
 	unsigned long pfn = start;
 	unsigned int tries = 0;
+	unsigned int max_tries = 5;
 	int ret = 0;
 	struct migration_target_control mtc = {
 		.nid = zone_to_nid(cc->zone),
 		.gfp_mask = GFP_USER | __GFP_MOVABLE | __GFP_RETRY_MAYFAIL,
 	};
 
+	if (cc->alloc_contig && cc->mode == MIGRATE_ASYNC)
+		max_tries = 1;
+
 	migrate_prep();
 
 	while (pfn < end || !list_empty(&cc->migratepages)) {

@@ -8513,7 +8517,7 @@ static int __alloc_contig_migrate_range(struct compact_control *cc,
 				break;
 			}
 			tries = 0;
-		} else if (++tries == 5) {
+		} else if (++tries == max_tries) {
 			ret = ret < 0 ? ret : -EBUSY;
 			break;
 		}

@@ -8564,7 +8568,7 @@ int alloc_contig_range(unsigned long start, unsigned long end,
 		.nr_migratepages = 0,
 		.order = -1,
 		.zone = page_zone(pfn_to_page(start)),
-		.mode = MIGRATE_SYNC,
+		.mode = gfp_mask & __GFP_NORETRY ? MIGRATE_ASYNC : MIGRATE_SYNC,
 		.ignore_skip_hint = true,
 		.no_set_skip_hint = true,
 		.gfp_mask = current_gfp_context(gfp_mask),

Signature · 1 line

2.30.0.296.g2bfb1c46d8-goog

Signature · 2 lines

Michal Hocko
SUSE Labs

Quoted 13 lines · Michal Hocko

Quoted 11 lines · Minchan Kim

Contiguous memory allocation can be stalled due to waiting on page writeback and/or page lock which causes unpredictable delay. It's a unavoidable cost for the requestor to get *big* contiguous memory but it's expensive for *small* contiguous memory(e.g., order-4) because caller could retry the request in different range where would have easy migratable pages without stalling.

This patch introduce __GFP_NORETRY as compaction gfp_mask in alloc_contig_range so it will fail fast without blocking when it encounters pages needed waiting.

Quoted 5 lines

I am not against controling how hard this allocator tries with gfp mask but this changelog is rather void on any data and any user.

It is also rather dubious to have retries when then caller says to not retry.

Since max_tries is 1 with ++tries, it shouldn't retry.

Quoted 2 lines

Also why didn't you consider GFP_NOWAIT semantic for non blocking mode?

GFP_NOWAIT seems to be low(specific) flags rather than the one I want to express. Even though I said only page writeback/lock in the description, the goal is to avoid costly operations we might find later so such "failfast", I thought GFP_NORETRY would be good fit.

Quoted 20 lines · Minchan Kim

Quoted 18 lines · Michal Hocko

Quoted 11 lines · Minchan Kim

Contiguous memory allocation can be stalled due to waiting on page writeback and/or page lock which causes unpredictable delay. It's a unavoidable cost for the requestor to get *big* contiguous memory but it's expensive for *small* contiguous memory(e.g., order-4) because caller could retry the request in different range where would have easy migratable pages without stalling.

This patch introduce __GFP_NORETRY as compaction gfp_mask in alloc_contig_range so it will fail fast without blocking when it encounters pages needed waiting.

I am not against controling how hard this allocator tries with gfp mask but this changelog is rather void on any data and any user.

It is also rather dubious to have retries when then caller says to not retry.

Quoted 1 line

Since max_tries is 1 with ++tries, it shouldn't retry.

OK, I have missed that. This is a tricky code. ASYNC mode should be completely orthogonal to the retries count. Those are different things. Page allocator does an explicit bail out based on __GFP_NORETRY. You should be doing the same.

Quoted 8 lines

Quoted 3 lines

Also why didn't you consider GFP_NOWAIT semantic for non blocking mode?

GFP_NOWAIT seems to be low(specific) flags rather than the one I want to express. Even though I said only page writeback/lock in the description, the goal is to avoid costly operations we might find later so such "failfast", I thought GFP_NORETRY would be good fit.

I suspect you are too focused on implementation details here. Think about the indended semantic. Callers of this functionality will not think about those (I hope because if they rely on these details then the whole thing will become unmaintainable because any change would require an audit of all existing users). All you should be caring about is to control how expensive the call can be. GFP_NOWAIT is not really low level from that POV. It gives you a very lightweight non-sleeping attempt to allocate. GFP_NORETRY will give you potentially sleeping but an opportunistic-easy-to-fail attempt. And so on. See how that is absolutely free of any page writeback or any specific locking.

Signature · 2 lines

Michal Hocko
SUSE Labs

Quoted 23 lines · Michal Hocko

Quoted 21 lines · Minchan Kim

Quoted 13 lines · Michal Hocko

Quoted 11 lines · Minchan Kim

Contiguous memory allocation can be stalled due to waiting on page writeback and/or page lock which causes unpredictable delay. It's a unavoidable cost for the requestor to get *big* contiguous memory but it's expensive for *small* contiguous memory(e.g., order-4) because caller could retry the request in different range where would have easy migratable pages without stalling.

This patch introduce __GFP_NORETRY as compaction gfp_mask in alloc_contig_range so it will fail fast without blocking when it encounters pages needed waiting.

Quoted 5 lines

I am not against controling how hard this allocator tries with gfp mask but this changelog is rather void on any data and any user.

It is also rather dubious to have retries when then caller says to not retry.

Since max_tries is 1 with ++tries, it shouldn't retry.

Quoted 4 lines

OK, I have missed that. This is a tricky code. ASYNC mode should be completely orthogonal to the retries count. Those are different things. Page allocator does an explicit bail out based on __GFP_NORETRY. You should be doing the same.

A concern with __GFP_NOWAIT is regardless of flags passed to cma_alloc, internal implementation of alloc_contig_range inside will use blockable operation. See __alloc_contig_migrate_range.

If we go with __GFP_NOWAIT, we should propagate the gfp_mask inside of __alloc_contig_migrate_range to make cma_alloc consistent with alloc_pages.

(IIUC, that's what you want - make gfp_mask consistent between cma_alloc and alloc_pages) but I am worry about the direction will make complicate situation since cma invovles migration context as well as target page allocation context. Sometime, the single gfp flag could be trouble to express both contexts all at once.

Quoted 7 lines

Quoted 6 lines

Quoted 1 line

Also why didn't you consider GFP_NOWAIT semantic for non blocking mode?

GFP_NOWAIT seems to be low(specific) flags rather than the one I want to express. Even though I said only page writeback/lock in the description, the goal is to avoid costly operations we might find later so such "failfast", I thought GFP_NORETRY would be good fit.

Quoted 10 lines

I suspect you are too focused on implementation details here. Think about the indended semantic. Callers of this functionality will not think about those (I hope because if they rely on these details then the whole thing will become unmaintainable because any change would require an audit of all existing users). All you should be caring about is to control how expensive the call can be. GFP_NOWAIT is not really low level from that POV. It gives you a very lightweight non-sleeping attempt to allocate. GFP_NORETRY will give you potentially sleeping but an opportunistic-easy-to-fail attempt. And so on. See how that is absolutely free of any page writeback or any specific locking.

With above reason I mentioned, I wanted to express __GFP_NORETRY as "opportunistic-easy-to-fail attempt" to support cma_alloc as "failfast" for migration context.

Quoted 20 lines · Minchan Kim

Quoted 18 lines · Michal Hocko

Quoted 11 lines · Minchan Kim

Contiguous memory allocation can be stalled due to waiting on page writeback and/or page lock which causes unpredictable delay. It's a unavoidable cost for the requestor to get *big* contiguous memory but it's expensive for *small* contiguous memory(e.g., order-4) because caller could retry the request in different range where would have easy migratable pages without stalling.

This patch introduce __GFP_NORETRY as compaction gfp_mask in alloc_contig_range so it will fail fast without blocking when it encounters pages needed waiting.

I am not against controling how hard this allocator tries with gfp mask but this changelog is rather void on any data and any user.

It is also rather dubious to have retries when then caller says to not retry.

Quoted 1 line

Since max_tries is 1 with ++tries, it shouldn't retry.

OK, I have missed that. This is a tricky code. ASYNC mode should be completely orthogonal to the retries count. Those are different things. Page allocator does an explicit bail out based on __GFP_NORETRY. You should be doing the same.

Quoted 21 lines · Michal Hocko

Quoted 20 lines · Minchan Kim

Quoted 18 lines · Michal Hocko

Quoted 11 lines · Minchan Kim

Contiguous memory allocation can be stalled due to waiting on page writeback and/or page lock which causes unpredictable delay. It's a unavoidable cost for the requestor to get *big* contiguous memory but it's expensive for *small* contiguous memory(e.g., order-4) because caller could retry the request in different range where would have easy migratable pages without stalling.

This patch introduce __GFP_NORETRY as compaction gfp_mask in alloc_contig_range so it will fail fast without blocking when it encounters pages needed waiting.

I am not against controling how hard this allocator tries with gfp mask but this changelog is rather void on any data and any user.

It is also rather dubious to have retries when then caller says to not retry.

Quoted 6 lines

Quoted 1 line

Since max_tries is 1 with ++tries, it shouldn't retry.

OK, I have missed that. This is a tricky code. ASYNC mode should be completely orthogonal to the retries count. Those are different things. Page allocator does an explicit bail out based on __GFP_NORETRY. You should be doing the same.

Before sending next revision, let me check this part again.

I want to use __GFP_NORETRY to indicate "opportunistic-easy-to-fail attempt" and I want to use ASYNC migrate_mode to help the goal.

Do you see the problem?

Quoted 34 lines · Minchan Kim

Quoted 21 lines · Michal Hocko

Quoted 20 lines · Minchan Kim

Quoted 18 lines · Michal Hocko

Quoted 11 lines · Minchan Kim

Contiguous memory allocation can be stalled due to waiting on page writeback and/or page lock which causes unpredictable delay. It's a unavoidable cost for the requestor to get *big* contiguous memory but it's expensive for *small* contiguous memory(e.g., order-4) because caller could retry the request in different range where would have easy migratable pages without stalling.

This patch introduce __GFP_NORETRY as compaction gfp_mask in alloc_contig_range so it will fail fast without blocking when it encounters pages needed waiting.

I am not against controling how hard this allocator tries with gfp mask but this changelog is rather void on any data and any user.

It is also rather dubious to have retries when then caller says to not retry.

Quoted 6 lines

Quoted 1 line

Since max_tries is 1 with ++tries, it shouldn't retry.

OK, I have missed that. This is a tricky code. ASYNC mode should be completely orthogonal to the retries count. Those are different things. Page allocator does an explicit bail out based on __GFP_NORETRY. You should be doing the same.

Before sending next revision, let me check this part again.

I want to use __GFP_NORETRY to indicate "opportunistic-easy-to-fail attempt" and I want to use ASYNC migrate_mode to help the goal.

Quoted 1 line

Do you see the problem?

No, as I've said. This is a normal NORETRY policy. And ASYNC migration is a mere implementation detail you do not have bother your users about. This is the semantic view. From the implementation POV it should be the gfp mask to drive decisions rather than a random (ASYNC) flag to control retries as you did here.

 .../reserved-memory/dma_heap_chunk.yaml       | 56 +++++++++++++++++++
 1 file changed, 56 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/reserved-memory/dma_heap_chunk.yaml

Documentation/devicetree/bindings/reserved-memory/dma_heap_chunk.yaml new file +42 -0

new file mode 100644
index 000000000000..00db0ae6af61
@@ -0,0 +1,56 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/reserved-memory/dma_heap_chunk.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Device tree binding for chunk heap on DMA HEAP FRAMEWORK
+
+description: |
+  The DMA chunk heap is backed by the Contiguous Memory Allocator (CMA) and
+  supports bulk allocation of fixed size pages.
+
+maintainers:
+  - Hyesoo Yu <[email protected]>
+  - John Stultz <[email protected]>
+  - Minchan Kim <[email protected]>
+  - Hridya Valsaraju<[email protected]>
+
+
+properties:
+  compatible:
+    enum:
+      - dma_heap,chunk
+
+  chunk-order:
+    description: |
+            order of pages that will get allocated from the chunk DMA heap.
+    maxItems: 1
+
+  size:
+    maxItems: 1
+
+  alignment:
+    maxItems: 1
+
+required:
+  - compatible
+  - size
+  - alignment
+  - chunk-order
+
+additionalProperties: false

Quoted 52 lines · Minchan Kim

 .../reserved-memory/dma_heap_chunk.yaml       | 56 +++++++++++++++++++
 1 file changed, 56 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/reserved-memory/dma_heap_chunk.yaml

Documentation/devicetree/bindings/reserved-memory/dma_heap_chunk.yaml new file +42 -0

new file mode 100644
index 000000000000..00db0ae6af61
@@ -0,0 +1,56 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/reserved-memory/dma_heap_chunk.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Device tree binding for chunk heap on DMA HEAP FRAMEWORK
+
+description: |
+  The DMA chunk heap is backed by the Contiguous Memory Allocator (CMA) and
+  supports bulk allocation of fixed size pages.
+
+maintainers:
+  - Hyesoo Yu <[email protected]>
+  - John Stultz <[email protected]>
+  - Minchan Kim <[email protected]>
+  - Hridya Valsaraju<[email protected]>
+
+
+properties:
+  compatible:
+    enum:
+      - dma_heap,chunk
+
+  chunk-order:
+    description: |
+            order of pages that will get allocated from the chunk DMA heap.
+    maxItems: 1
+
+  size:
+    maxItems: 1
+
+  alignment:
+    maxItems: 1
+
+required:
+  - compatible
+  - size
+  - alignment
+  - chunk-order
+
+additionalProperties: false

Documentation/devicetree/bindings/reserved-memory/dma_heap_chunk.yaml line 52

Quoted 10 lines

Documentation/devicetree/bindings/reserved-memory/dma_heap_chunk.yaml new file +10 -0

+
+examples:
+  - |
+    reserved-memory {
+        #address-cells = <2>;
+        #size-cells = <1>;
+
+        chunk_memory: chunk_memory {
+            compatible = "dma_heap,chunk";
+            size = <0x3000000>;

Hey Minchan,
Looking closer here, would it make more sense to document the "reg =
<>" parameter here as well instead of just "size = <>"?

That way the address of the region could be explicitly specified (for instance, to ensure the CMA region created is 32bit addressable). And more practically, trying to satisfy the base address alignment checks in the final patch when its set dynamically may require a fair amount of luck - I couldn't manage it in my own testing on the hikey960 w/o resorting to reg= :)

It does look like the RESERVEDMEM_OF_DECLARE() logic already supports this, so it's likely just a matter of documenting it here?

thanks
-john

From: Hyesoo Yu <[email protected]>

Document devicetree binding for chunk cma heap on dma heap framework.

The DMA chunk heap supports the bulk allocation of higher order pages.

The chunk heap's allocator allocates from the CMA area. It is optimized to perform bulk allocation of higher order pages in an efficient manner. For this purpose, the heap needs an exclusive CMA area that will only be used for allocation by the heap. This is the reason why we need to use the DT to create and configure a reserved memory region for use by the chunk CMA heap driver. Since all allocation from DMA-BUF heaps happen from the user-space, there is no other appropriate device-driver that we can use to register the chunk CMA heap and configure the reserved memory region for its use.

Signed-off-by Hyesoo Yu <[email protected]>

Signed-off-by Minchan Kim <[email protected]>

Signed-off-by Hridya Valsaraju <[email protected]>

---
 .../reserved-memory/dma_heap_chunk.yaml       | 56 +++++++++++++++++++
 1 file changed, 56 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/reserved-memory/dma_heap_chunk.yaml

Documentation/devicetree/bindings/reserved-memory/dma_heap_chunk.yaml new file +13 -0

new file mode 100644
index 000000000000..00db0ae6af61
@@ -0,0 +1,56 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/reserved-memory/dma_heap_chunk.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Device tree binding for chunk heap on DMA HEAP FRAMEWORK
+
+description: |
+  The DMA chunk heap is backed by the Contiguous Memory Allocator (CMA) and
+  supports bulk allocation of fixed size pages.
+
+maintainers:

Documentation/devicetree/bindings/reserved-memory/dma_heap_chunk.yaml new file +29 -0

+    maxItems: 1
+
+  size:
+    maxItems: 1
+
+  alignment:
+    maxItems: 1
+
+required:
+  - compatible
+  - size
+  - alignment
+  - chunk-order
+
+additionalProperties: false
+
+examples:
+  - |
+    reserved-memory {
+        #address-cells = <2>;
+        #size-cells = <1>;
+
+        chunk_memory: chunk_memory {
+            compatible = "dma_heap,chunk";
+            size = <0x3000000>;
+            alignment = <0x0 0x00010000>;
+            chunk-order = <4>;
+        };
+    };

Signature · 1 line

2.30.0.296.g2bfb1c46d8-goog

drivers/dma-buf/heaps/Kconfig config DMABUF_HEAPS_CMA · line 22

Quoted 7 lines

drivers/dma-buf/heaps/Kconfig +7 -0

+config DMABUF_HEAPS_CHUNK
+	bool "DMA-BUF CHUNK Heap"
+	depends on DMABUF_HEAPS && DMA_CMA
+	help
+	  Choose this option to enable dma-buf CHUNK heap. This heap is backed
+	  by the Contiguous Memory Allocator (CMA) and allocates the buffers that
+	  are arranged into a list of fixed size chunks taken from CMA.

Overly long line, which in text flowing text is really, really annoying.

Many more later. Remember that > 80 chars are only allowed if they significantly improve readability.

drivers/dma-buf/heaps/chunk_heap.c new file +8 -0

+
+#define CHUNK_PREFIX "chunk-"
+
+static int register_chunk_heap(struct chunk_heap *chunk_heap_info)
+{
+       struct dma_heap_export_info exp_info;
+       const char *name = cma_get_name(chunk_heap_info->cma);
+       size_t len = strlen(CHUNK_PREFIX) + strlen(name) + 1;

drivers/dma-buf/heaps/chunk_heap.c new file +28 -0

+       exp_info.ops = &chunk_heap_ops;
+       exp_info.priv = chunk_heap_info;
+
+       chunk_heap_info->heap = dma_heap_add(&exp_info);
+       if (IS_ERR(chunk_heap_info->heap)) {
+               kfree(buf);
+               return PTR_ERR(chunk_heap_info->heap);
+       }
+
+       return 0;
+}
+
+static int __init chunk_heap_init(void)
+{
+       unsigned int i;
+
+       for (i = 0; i < chunk_heap_count; i++)
+               register_chunk_heap(&chunk_heaps[i]);
+
+       return 0;
+}
+module_init(chunk_heap_init);
+
+#ifdef CONFIG_OF_EARLY_FLATTREE
+
+static int __init dmabuf_chunk_heap_area_init(struct reserved_mem *rmem)
+{
+       int ret;

On Thu, Jan 21, 2021 at 9:55 AM Minchan Kim <[email protected]> wrote:

Hey Minchan,
Thanks for sending this out! I'm still working through testing with
this patch set, so I may have some more feedback tomorrow, but a few quick items I did hit below.

Quoted 13 lines

(quoted diff) +13 -0

+
+#define CHUNK_PREFIX "chunk-"
+
+static int register_chunk_heap(struct chunk_heap *chunk_heap_info)
+{
+       struct dma_heap_export_info exp_info;
+       const char *name = cma_get_name(chunk_heap_info->cma);
+       size_t len = strlen(CHUNK_PREFIX) + strlen(name) + 1;
+       char *buf = kmalloc(len, GFP_KERNEL);
+
+       if (!buf)
+               return -ENOMEM;
+

Review

Quoted 5 lines

(quoted diff) +5 -0

+       sprintf(buf, CHUNK_PREFIX"%s", cma_get_name(chunk_heap_info->cma));
+       buf[len] = '\0';
+
+       exp_info.name = buf;
+       exp_info.name = cma_get_name(chunk_heap_info->cma);

I think you intended to delete this line, as it's overwriting your prefixed name.

Quoted 35 lines

(quoted diff) +34 -0

+       exp_info.ops = &chunk_heap_ops;
+       exp_info.priv = chunk_heap_info;
+
+       chunk_heap_info->heap = dma_heap_add(&exp_info);
+       if (IS_ERR(chunk_heap_info->heap)) {
+               kfree(buf);
+               return PTR_ERR(chunk_heap_info->heap);
+       }
+
+       return 0;
+}
+
+static int __init chunk_heap_init(void)
+{
+       unsigned int i;
+
+       for (i = 0; i < chunk_heap_count; i++)
+               register_chunk_heap(&chunk_heaps[i]);
+
+       return 0;
+}
+module_init(chunk_heap_init);
+
+#ifdef CONFIG_OF_EARLY_FLATTREE
+
+static int __init dmabuf_chunk_heap_area_init(struct reserved_mem *rmem)
+{
+       int ret;
+       struct cma *cma;
+       struct chunk_heap *chunk_heap_info;
+       const __be32 *chunk_order;
+
+       phys_addr_t align = PAGE_SIZE << max(MAX_ORDER - 1, pageblock_order);
+       phys_addr_t mask = align - 1;

Review

Quoted 4 lines

(quoted diff) +4 -0

+
+       if ((rmem->base & mask) || (rmem->size & mask)) {
+               pr_err("Incorrect alignment for CMA region\n");
+               return -EINVAL;

Passing this check can be tough if you're using dynamically assigned rmem, so it might be helpful for debugging to print the base/size/mask values?

From: Hyesoo Yu <[email protected]>

This patch supports chunk heap that allocates the buffers that arranged into a list a fixed size chunks taken from CMA.

The chunk heap driver is bound directly to a reserved_memory node by following Rob Herring's suggestion in [1].

[1] https://lore.kernel.org/lkml/[email protected]/T/#m3dc63acd33fea269a584f43bb799a876f0b2b45d

Quoted 5 lines · Minchan Kim

From: Hyesoo Yu <[email protected]>

This patch supports chunk heap that allocates the buffers that arranged into a list a fixed size chunks taken from CMA.

Quoted 4 lines

The chunk heap driver is bound directly to a reserved_memory node by following Rob Herring's suggestion in [1].

[1] https://lore.kernel.org/lkml/[email protected]/T/#m3dc63acd33fea269a584f43bb799a876f0b2b45d

Who is using this allocator in the kernel?

Quoted 6 lines · Michal Hocko

Quoted 5 lines · Minchan Kim

From: Hyesoo Yu <[email protected]>

This patch supports chunk heap that allocates the buffers that arranged into a list a fixed size chunks taken from CMA.

Quoted 6 lines

Quoted 4 lines

The chunk heap driver is bound directly to a reserved_memory node by following Rob Herring's suggestion in [1].

[1] https://lore.kernel.org/lkml/[email protected]/T/#m3dc63acd33fea269a584f43bb799a876f0b2b45d

Who is using this allocator in the kernel?

Userspace uses the memory via mapping it via dmabuf.