[PATCH v2 0/4] udmbuf bug fix and some improvements

[PATCH v2 0/4] udmbuf bug fix and some improvements

[Huan Yang]

This patchset attempts to fix some errors in udmabuf and remove the
upin_list structure.

Some of this fix just gather the patches which I upload before.

Patch1,2,4 has passed the udmabuf self-test suite's tests.
Suggested by Kasireddy, Vivek <vivek.kasireddy@intel.com>
Test item 6 maybe requires running the command:
  echo 1024 > /sys/kernel/mm/hugepages/hugepages-2048kB/nr_hugepages

Patch3 changed vmap, which can't simple test by testsuit.
But Patch4 restore vmap to the implementation before adding folio.

Patch1
===
Try to remove page fault mmap and direct map it.
Due to current udmabuf has already obtained and pinned the folio
upon completion of the creation.This means that the physical memory has
already been acquired, rather than being accessed dynamically. The
current page fault method only saves some page table memory.

As a result, the page fault mechanism has lost its purpose as a demanding
page. Due to the fact that page fault requires trapping into kernel mode
and filling in when accessing the corresponding virtual address in mmap,
this means that user mode access to virtual addresses needs to trap into
kernel mode.

Therefore, when creating a large size udmabuf, this represents a
considerable overhead.

Patch2
===
This is the same to patch:
https://lore.kernel.org/all/20240725021349.580574-1-link@vivo.com/

Patch3
===
The current implementation of udmabuf's vmap has issues.

It does not correctly set each page of the folio to the page structure,
so that when vmap is called, all pages are the head page of the folio.

This implementation is same as this patch:
https://lore.kernel.org/all/20240731090233.1343559-1-link@vivo.com/

Patch4
===
Attempt to remove unpin_list and other related data structures.

In order to adapt to Folio, we established the unpin_list data structure
to unpin all folios and maintain the page mapping relationship.

However, this data structure requires 24 bytes for each page and has low
traversal performance for the list. And maintaining the offset structure
also consumes a portion of memory.

This patch attempts to remove these data structures and modify the
semantics of some existing data structures.

udmabuf:
  add folios -> folio array, which only contain's the folio, org contains
duplicate.
  add foliocount -> folios array number
  add pages -> page array, which contains pages which in folios, offset,
               size determined by the offset of the memfd when udmabuf
               created.

This patch also remove single folios' create on each create item, use it
be the ubuf->folios arrays' pointer, slide to fill the corresponding
folio under the item into the array.

Changelog
===
  v2 -> v1:
    Patch1, 3 Rectify the improper use of the sg table.
    suggested-by Christian König <christian.koenig@amd.com>
    
    Patch2 add acked-by Christian K�nig <christian.koenig@amd.com> which
    marked in v1

    Patch4
    Modify the data structure to restore the use of pages and
    correct the misunderstanding of loop conditions such as "pgcnt".
    make sure pass self test.

    remove v1's patch4

v1
  https://lore.kernel.org/all/20240801104512.4056860-1-link@vivo.com/

Huan Yang (4):
  udmabuf: cancel mmap page fault, direct map it
  udmabuf: change folios array from kmalloc to kvmalloc
  fix vmap_udmabuf error page set
  udmabuf: remove folio unpin list

 drivers/dma-buf/udmabuf.c | 207 ++++++++++++++------------------------
 1 file changed, 78 insertions(+), 129 deletions(-)


base-commit: 048d8cb65cde9fe7534eb4440bcfddcf406bb49c
-- 
2.45.2

[PATCH v2 1/4] udmabuf: cancel mmap page fault, direct map it

[Huan Yang]

The current udmabuf mmap uses a page fault mechanism to populate the vma.

However, the current udmabuf has already obtained and pinned the folio
upon completion of the creation.This means that the physical memory has
already been acquired, rather than being accessed dynamically. The
current page fault method only saves some page table memory.

As a result, the page fault mechanism has lost its purpose as a demanding
page. Due to the fact that page fault requires trapping into kernel mode
and filling in when accessing the corresponding virtual address in mmap,
this means that user mode access to virtual addresses needs to trap into
kernel mode.

Therefore, when creating a large size udmabuf, this represents a
considerable overhead.

The current patch removes the page fault method of mmap and
instead fills it directly when mmap is triggered.

Signed-off-by: Huan Yang <link@vivo.com>
---
 drivers/dma-buf/udmabuf.c | 39 ++++++++++++++++-----------------------
 1 file changed, 16 insertions(+), 23 deletions(-)

diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c
index 047c3cd2ceff..475268d4ebb1 100644
--- a/drivers/dma-buf/udmabuf.c
+++ b/drivers/dma-buf/udmabuf.c
@@ -38,36 +38,29 @@ struct udmabuf_folio {
 	struct list_head list;
 };
 
-static vm_fault_t udmabuf_vm_fault(struct vm_fault *vmf)
-{
-	struct vm_area_struct *vma = vmf->vma;
-	struct udmabuf *ubuf = vma->vm_private_data;
-	pgoff_t pgoff = vmf->pgoff;
-	unsigned long pfn;
-
-	if (pgoff >= ubuf->pagecount)
-		return VM_FAULT_SIGBUS;
-
-	pfn = folio_pfn(ubuf->folios[pgoff]);
-	pfn += ubuf->offsets[pgoff] >> PAGE_SHIFT;
-
-	return vmf_insert_pfn(vma, vmf->address, pfn);
-}
-
-static const struct vm_operations_struct udmabuf_vm_ops = {
-	.fault = udmabuf_vm_fault,
-};
-
 static int mmap_udmabuf(struct dma_buf *buf, struct vm_area_struct *vma)
 {
 	struct udmabuf *ubuf = buf->priv;
+	unsigned long addr;
+	unsigned long end;
+	unsigned long pgoff;
+	int ret;
 
 	if ((vma->vm_flags & (VM_SHARED | VM_MAYSHARE)) == 0)
 		return -EINVAL;
 
-	vma->vm_ops = &udmabuf_vm_ops;
-	vma->vm_private_data = ubuf;
-	vm_flags_set(vma, VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP);
+	for (pgoff = vma->vm_pgoff, end = vma->vm_end, addr = vma->vm_start;
+	     addr < end; pgoff++, addr += PAGE_SIZE) {
+		struct page *page =
+			folio_page(ubuf->folios[pgoff],
+				   ubuf->offsets[pgoff] >> PAGE_SHIFT);
+
+		ret = remap_pfn_range(vma, addr, page_to_pfn(page), PAGE_SIZE,
+				      vma->vm_page_prot);
+		if (ret)
+			return ret;
+	}
+
 	return 0;
 }
 
-- 
2.45.2

[PATCH v2 2/4] udmabuf: change folios array from kmalloc to kvmalloc

[Huan Yang]

When PAGE_SIZE 4096, MAX_PAGE_ORDER 10, 64bit machine,
page_alloc only support 4MB.
If above this, trigger this warn and return NULL.

udmabuf can change size limit, if change it to 3072(3GB), and then alloc
3GB udmabuf, will fail create.

[ 4080.876581] ------------[ cut here ]------------
[ 4080.876843] WARNING: CPU: 3 PID: 2015 at mm/page_alloc.c:4556 __alloc_pages+0x2c8/0x350
[ 4080.878839] RIP: 0010:__alloc_pages+0x2c8/0x350
[ 4080.879470] Call Trace:
[ 4080.879473]  <TASK>
[ 4080.879473]  ? __alloc_pages+0x2c8/0x350
[ 4080.879475]  ? __warn.cold+0x8e/0xe8
[ 4080.880647]  ? __alloc_pages+0x2c8/0x350
[ 4080.880909]  ? report_bug+0xff/0x140
[ 4080.881175]  ? handle_bug+0x3c/0x80
[ 4080.881556]  ? exc_invalid_op+0x17/0x70
[ 4080.881559]  ? asm_exc_invalid_op+0x1a/0x20
[ 4080.882077]  ? udmabuf_create+0x131/0x400

Because MAX_PAGE_ORDER, kmalloc can max alloc 4096 * (1 << 10), 4MB
memory, each array entry is pointer(8byte), so can save 524288 pages(2GB).

Further more, costly order(order 3) may not be guaranteed that it can be
applied for, due to fragmentation.

This patch change udmabuf array use kvmalloc_array, this can fallback
alloc into vmalloc, which can guarantee allocation for any size and does
not affect the performance of kmalloc allocations.

Signed-off-by: Huan Yang <link@vivo.com>
Acked-by: Christian König <christian.koenig@amd.com>
---
 drivers/dma-buf/udmabuf.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c
index 475268d4ebb1..af2391cea0bf 100644
--- a/drivers/dma-buf/udmabuf.c
+++ b/drivers/dma-buf/udmabuf.c
@@ -73,7 +73,7 @@ static int vmap_udmabuf(struct dma_buf *buf, struct iosys_map *map)
 
 	dma_resv_assert_held(buf->resv);
 
-	pages = kmalloc_array(ubuf->pagecount, sizeof(*pages), GFP_KERNEL);
+	pages = kvmalloc_array(ubuf->pagecount, sizeof(*pages), GFP_KERNEL);
 	if (!pages)
 		return -ENOMEM;
 
@@ -81,7 +81,7 @@ static int vmap_udmabuf(struct dma_buf *buf, struct iosys_map *map)
 		pages[pg] = &ubuf->folios[pg]->page;
 
 	vaddr = vm_map_ram(pages, ubuf->pagecount, -1);
-	kfree(pages);
+	kvfree(pages);
 	if (!vaddr)
 		return -EINVAL;
 
@@ -189,8 +189,8 @@ static void release_udmabuf(struct dma_buf *buf)
 		put_sg_table(dev, ubuf->sg, DMA_BIDIRECTIONAL);
 
 	unpin_all_folios(&ubuf->unpin_list);
-	kfree(ubuf->offsets);
-	kfree(ubuf->folios);
+	kvfree(ubuf->offsets);
+	kvfree(ubuf->folios);
 	kfree(ubuf);
 }
 
@@ -315,14 +315,14 @@ static long udmabuf_create(struct miscdevice *device,
 	if (!ubuf->pagecount)
 		goto err;
 
-	ubuf->folios = kmalloc_array(ubuf->pagecount, sizeof(*ubuf->folios),
-				    GFP_KERNEL);
+	ubuf->folios = kvmalloc_array(ubuf->pagecount, sizeof(*ubuf->folios),
+				      GFP_KERNEL);
 	if (!ubuf->folios) {
 		ret = -ENOMEM;
 		goto err;
 	}
-	ubuf->offsets = kcalloc(ubuf->pagecount, sizeof(*ubuf->offsets),
-				GFP_KERNEL);
+	ubuf->offsets =
+		kvcalloc(ubuf->pagecount, sizeof(*ubuf->offsets), GFP_KERNEL);
 	if (!ubuf->offsets) {
 		ret = -ENOMEM;
 		goto err;
@@ -336,7 +336,7 @@ static long udmabuf_create(struct miscdevice *device,
 			goto err;
 
 		pgcnt = list[i].size >> PAGE_SHIFT;
-		folios = kmalloc_array(pgcnt, sizeof(*folios), GFP_KERNEL);
+		folios = kvmalloc_array(pgcnt, sizeof(*folios), GFP_KERNEL);
 		if (!folios) {
 			ret = -ENOMEM;
 			goto err;
@@ -346,7 +346,7 @@ static long udmabuf_create(struct miscdevice *device,
 		ret = memfd_pin_folios(memfd, list[i].offset, end,
 				       folios, pgcnt, &pgoff);
 		if (ret <= 0) {
-			kfree(folios);
+			kvfree(folios);
 			if (!ret)
 				ret = -EINVAL;
 			goto err;
@@ -375,7 +375,7 @@ static long udmabuf_create(struct miscdevice *device,
 			}
 		}
 
-		kfree(folios);
+		kvfree(folios);
 		fput(memfd);
 		memfd = NULL;
 	}
@@ -391,8 +391,8 @@ static long udmabuf_create(struct miscdevice *device,
 	if (memfd)
 		fput(memfd);
 	unpin_all_folios(&ubuf->unpin_list);
-	kfree(ubuf->offsets);
-	kfree(ubuf->folios);
+	kvfree(ubuf->offsets);
+	kvfree(ubuf->folios);
 	kfree(ubuf);
 	return ret;
 }
-- 
2.45.2

[PATCH v2 3/4] fix vmap_udmabuf error page set

[Huan Yang]

Currently vmap_udmabuf set page's array by each folio.
But, ubuf->folios is only contain's the folio's head page.

That mean we repeatedly mapped the folio head page to the vmalloc area.

This patch fix it, set each folio's page correct, so that pages array
contains right page, and then map into vmalloc area

Signed-off-by: Huan Yang <link@vivo.com>
---
 drivers/dma-buf/udmabuf.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c
index af2391cea0bf..9737f063b6b3 100644
--- a/drivers/dma-buf/udmabuf.c
+++ b/drivers/dma-buf/udmabuf.c
@@ -78,7 +78,8 @@ static int vmap_udmabuf(struct dma_buf *buf, struct iosys_map *map)
 		return -ENOMEM;
 
 	for (pg = 0; pg < ubuf->pagecount; pg++)
-		pages[pg] = &ubuf->folios[pg]->page;
+		pages[pg] = folio_page(ubuf->folios[pg],
+				       ubuf->offsets[pg] >> PAGE_SHIFT);
 
 	vaddr = vm_map_ram(pages, ubuf->pagecount, -1);
 	kvfree(pages);
-- 
2.45.2

[PATCH v2 4/4] udmabuf: remove folio unpin list

[Huan Yang]

Currently, udmabuf handles folio by creating an unpin list to record
each folio obtained from the list and unpinning them when released. To
maintain this approach, many data structures have been established.

However, maintaining this type of data structure requires a significant
amount of memory and traversing the list is a substantial overhead,
which is not friendly to the CPU cache, TLB, and so on.

Therefore, this patch removes the relationship between the folio and its
offset in the linear address mapping.

As an alternative, udmabuf both maintain the folio array and page array,
folio array use to unpin, and the page array is used as before to handle
the requirements for the page.

So, udmabuf's folios only save the folio struct, foliocount point
the size of array. pages save page in folios, number offset given by
create list, pagecount point the size of array.

Even if we restore the pages structure, its memory usage should be
smaller than the combined memory usage of offsets(8 bytes in 64bit machine)
and udmabuf_folio structures(24 bytes in 64bit machine).

By doing this, we can accept the overhead of the udmabuf_folio structure
and the performance loss of traversing the list during unpinning.

Signed-off-by: Huan Yang <link@vivo.com>
---
 drivers/dma-buf/udmabuf.c | 167 ++++++++++++++------------------------
 1 file changed, 61 insertions(+), 106 deletions(-)

diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c
index 9737f063b6b3..442ed99d8b33 100644
--- a/drivers/dma-buf/udmabuf.c
+++ b/drivers/dma-buf/udmabuf.c
@@ -25,17 +25,24 @@ module_param(size_limit_mb, int, 0644);
 MODULE_PARM_DESC(size_limit_mb, "Max size of a dmabuf, in megabytes. Default is 64.");
 
 struct udmabuf {
+	/**
+	 * Each page used by udmabuf in the folio. When obtaining a page from a
+	 * folio, it does not necessarily begin from the head page. This is
+	 * determined by the offset of the memfd when udmabuf created.
+	 */
 	pgoff_t pagecount;
+	struct page **pages;
+
+	/**
+	 * Each folio in memfd, when a udmabuf is created, it is pinned to
+	 * ensure that the folio is not moved or reclaimed.
+	 * folio array used to unpin all when releasing.
+	 */
+	pgoff_t foliocount;
 	struct folio **folios;
+
 	struct sg_table *sg;
 	struct miscdevice *device;
-	pgoff_t *offsets;
-	struct list_head unpin_list;
-};
-
-struct udmabuf_folio {
-	struct folio *folio;
-	struct list_head list;
 };
 
 static int mmap_udmabuf(struct dma_buf *buf, struct vm_area_struct *vma)
@@ -51,9 +58,7 @@ static int mmap_udmabuf(struct dma_buf *buf, struct vm_area_struct *vma)
 
 	for (pgoff = vma->vm_pgoff, end = vma->vm_end, addr = vma->vm_start;
 	     addr < end; pgoff++, addr += PAGE_SIZE) {
-		struct page *page =
-			folio_page(ubuf->folios[pgoff],
-				   ubuf->offsets[pgoff] >> PAGE_SHIFT);
+		struct page *page = ubuf->pages[pgoff];
 
 		ret = remap_pfn_range(vma, addr, page_to_pfn(page), PAGE_SIZE,
 				      vma->vm_page_prot);
@@ -67,22 +72,11 @@ static int mmap_udmabuf(struct dma_buf *buf, struct vm_area_struct *vma)
 static int vmap_udmabuf(struct dma_buf *buf, struct iosys_map *map)
 {
 	struct udmabuf *ubuf = buf->priv;
-	struct page **pages;
 	void *vaddr;
-	pgoff_t pg;
 
 	dma_resv_assert_held(buf->resv);
 
-	pages = kvmalloc_array(ubuf->pagecount, sizeof(*pages), GFP_KERNEL);
-	if (!pages)
-		return -ENOMEM;
-
-	for (pg = 0; pg < ubuf->pagecount; pg++)
-		pages[pg] = folio_page(ubuf->folios[pg],
-				       ubuf->offsets[pg] >> PAGE_SHIFT);
-
-	vaddr = vm_map_ram(pages, ubuf->pagecount, -1);
-	kvfree(pages);
+	vaddr = vm_map_ram(ubuf->pages, ubuf->pagecount, -1);
 	if (!vaddr)
 		return -EINVAL;
 
@@ -104,30 +98,25 @@ static struct sg_table *get_sg_table(struct device *dev, struct dma_buf *buf,
 {
 	struct udmabuf *ubuf = buf->priv;
 	struct sg_table *sg;
-	struct scatterlist *sgl;
-	unsigned int i = 0;
 	int ret;
 
 	sg = kzalloc(sizeof(*sg), GFP_KERNEL);
 	if (!sg)
 		return ERR_PTR(-ENOMEM);
 
-	ret = sg_alloc_table(sg, ubuf->pagecount, GFP_KERNEL);
+	ret = sg_alloc_table_from_pages(sg, ubuf->pages, ubuf->pagecount,
+					0, ubuf->pagecount << PAGE_SHIFT,
+					GFP_KERNEL);
 	if (ret < 0)
-		goto err_alloc;
-
-	for_each_sg(sg->sgl, sgl, ubuf->pagecount, i)
-		sg_set_folio(sgl, ubuf->folios[i], PAGE_SIZE,
-			     ubuf->offsets[i]);
+		goto err;
 
 	ret = dma_map_sgtable(dev, sg, direction, 0);
 	if (ret < 0)
-		goto err_map;
+		goto err;
 	return sg;
 
-err_map:
+err:
 	sg_free_table(sg);
-err_alloc:
 	kfree(sg);
 	return ERR_PTR(ret);
 }
@@ -153,34 +142,6 @@ static void unmap_udmabuf(struct dma_buf_attachment *at,
 	return put_sg_table(at->dev, sg, direction);
 }
 
-static void unpin_all_folios(struct list_head *unpin_list)
-{
-	struct udmabuf_folio *ubuf_folio;
-
-	while (!list_empty(unpin_list)) {
-		ubuf_folio = list_first_entry(unpin_list,
-					      struct udmabuf_folio, list);
-		unpin_folio(ubuf_folio->folio);
-
-		list_del(&ubuf_folio->list);
-		kfree(ubuf_folio);
-	}
-}
-
-static int add_to_unpin_list(struct list_head *unpin_list,
-			     struct folio *folio)
-{
-	struct udmabuf_folio *ubuf_folio;
-
-	ubuf_folio = kzalloc(sizeof(*ubuf_folio), GFP_KERNEL);
-	if (!ubuf_folio)
-		return -ENOMEM;
-
-	ubuf_folio->folio = folio;
-	list_add_tail(&ubuf_folio->list, unpin_list);
-	return 0;
-}
-
 static void release_udmabuf(struct dma_buf *buf)
 {
 	struct udmabuf *ubuf = buf->priv;
@@ -189,9 +150,9 @@ static void release_udmabuf(struct dma_buf *buf)
 	if (ubuf->sg)
 		put_sg_table(dev, ubuf->sg, DMA_BIDIRECTIONAL);
 
-	unpin_all_folios(&ubuf->unpin_list);
-	kvfree(ubuf->offsets);
+	unpin_folios(ubuf->folios, ubuf->foliocount);
 	kvfree(ubuf->folios);
+	kvfree(ubuf->pages);
 	kfree(ubuf);
 }
 
@@ -289,19 +250,18 @@ static long udmabuf_create(struct miscdevice *device,
 			   struct udmabuf_create_list *head,
 			   struct udmabuf_create_item *list)
 {
-	pgoff_t pgoff, pgcnt, pglimit, pgbuf = 0;
-	long nr_folios, ret = -EINVAL;
+	pgoff_t pgoff, pgcnt, pglimit, nr_pages;
+	long nr_folios = 0, ret = -EINVAL;
 	struct file *memfd = NULL;
 	struct folio **folios;
 	struct udmabuf *ubuf;
-	u32 i, j, k, flags;
+	u32 i, flags;
 	loff_t end;
 
 	ubuf = kzalloc(sizeof(*ubuf), GFP_KERNEL);
 	if (!ubuf)
 		return -ENOMEM;
 
-	INIT_LIST_HEAD(&ubuf->unpin_list);
 	pglimit = (size_limit_mb * 1024 * 1024) >> PAGE_SHIFT;
 	for (i = 0; i < head->count; i++) {
 		if (!IS_ALIGNED(list[i].offset, PAGE_SIZE))
@@ -322,64 +282,58 @@ static long udmabuf_create(struct miscdevice *device,
 		ret = -ENOMEM;
 		goto err;
 	}
-	ubuf->offsets =
-		kvcalloc(ubuf->pagecount, sizeof(*ubuf->offsets), GFP_KERNEL);
-	if (!ubuf->offsets) {
+	folios = ubuf->folios;
+
+	ubuf->pages = kvmalloc_array(ubuf->pagecount, sizeof(*ubuf->pages),
+				     GFP_KERNEL);
+	if (!ubuf->pages) {
 		ret = -ENOMEM;
 		goto err;
 	}
 
-	pgbuf = 0;
-	for (i = 0; i < head->count; i++) {
+	for (i = 0, nr_pages = 0; i < head->count; i++) {
+		u32 j, pg;
+
 		memfd = fget(list[i].memfd);
 		ret = check_memfd_seals(memfd);
 		if (ret < 0)
 			goto err;
 
 		pgcnt = list[i].size >> PAGE_SHIFT;
-		folios = kvmalloc_array(pgcnt, sizeof(*folios), GFP_KERNEL);
-		if (!folios) {
-			ret = -ENOMEM;
-			goto err;
-		}
 
 		end = list[i].offset + (pgcnt << PAGE_SHIFT) - 1;
-		ret = memfd_pin_folios(memfd, list[i].offset, end,
-				       folios, pgcnt, &pgoff);
+		ret = memfd_pin_folios(memfd, list[i].offset, end, folios,
+				       pgcnt, &pgoff);
 		if (ret <= 0) {
-			kvfree(folios);
-			if (!ret)
-				ret = -EINVAL;
+			ret = ret ?: -EINVAL;
 			goto err;
 		}
 
-		nr_folios = ret;
-		pgoff >>= PAGE_SHIFT;
-		for (j = 0, k = 0; j < pgcnt; j++) {
-			ubuf->folios[pgbuf] = folios[k];
-			ubuf->offsets[pgbuf] = pgoff << PAGE_SHIFT;
-
-			if (j == 0 || ubuf->folios[pgbuf-1] != folios[k]) {
-				ret = add_to_unpin_list(&ubuf->unpin_list,
-							folios[k]);
-				if (ret < 0) {
-					kfree(folios);
-					goto err;
-				}
-			}
-
-			pgbuf++;
-			if (++pgoff == folio_nr_pages(folios[k])) {
-				pgoff = 0;
-				if (++k == nr_folios)
-					break;
+		/**
+		 * Iter the pinned folios and record them for later unpin
+		 * when releasing.
+		 * memfd may start from any offset, so we need check it
+		 * carefully at first.
+		 */
+		for (j = 0, pgoff >>= PAGE_SHIFT, pg = 0; j < ret;
+		     ++j, pgoff = 0) {
+			pgoff_t k;
+			struct folio *folio = folios[j];
+
+			for (k = pgoff; k < folio_nr_pages(folio); ++k) {
+				ubuf->pages[nr_pages++] = folio_page(folio, k);
+
+				if (++pg >= pgcnt)
+					goto end;
 			}
 		}
-
-		kvfree(folios);
+end:
+		folios += ret;
+		nr_folios += ret;
 		fput(memfd);
 		memfd = NULL;
 	}
+	ubuf->foliocount = nr_folios;
 
 	flags = head->flags & UDMABUF_FLAGS_CLOEXEC ? O_CLOEXEC : 0;
 	ret = export_udmabuf(ubuf, device, flags);
@@ -391,8 +345,9 @@ static long udmabuf_create(struct miscdevice *device,
 err:
 	if (memfd)
 		fput(memfd);
-	unpin_all_folios(&ubuf->unpin_list);
-	kvfree(ubuf->offsets);
+	if (nr_folios)
+		unpin_folios(ubuf->folios, nr_folios);
+	kvfree(ubuf->pages);
 	kvfree(ubuf->folios);
 	kfree(ubuf);
 	return ret;
-- 
2.45.2

RE: [PATCH v2 1/4] udmabuf: cancel mmap page fault, direct map it

[Kasireddy, Vivek]

Hi Huan,

> 
> The current udmabuf mmap uses a page fault mechanism to populate the
> vma.
> 
> However, the current udmabuf has already obtained and pinned the folio
> upon completion of the creation.This means that the physical memory has
> already been acquired, rather than being accessed dynamically. The
> current page fault method only saves some page table memory.
> 
> As a result, the page fault mechanism has lost its purpose as a demanding
> page. Due to the fact that page fault requires trapping into kernel mode
> and filling in when accessing the corresponding virtual address in mmap,
> this means that user mode access to virtual addresses needs to trap into
> kernel mode.
> 
> Therefore, when creating a large size udmabuf, this represents a
> considerable overhead.
> 
> The current patch removes the page fault method of mmap and
> instead fills it directly when mmap is triggered.
I think it makes sense to populate the vma when the first fault is triggered
instead of doing it during mmap. This is because the userspace may call
mmap but does not actually use the data. Qemu works this way depending on
whether opengl is available in the environment or not.

> 
> Signed-off-by: Huan Yang <link@vivo.com>
> ---
>  drivers/dma-buf/udmabuf.c | 39 ++++++++++++++++-----------------------
>  1 file changed, 16 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c
> index 047c3cd2ceff..475268d4ebb1 100644
> --- a/drivers/dma-buf/udmabuf.c
> +++ b/drivers/dma-buf/udmabuf.c
> @@ -38,36 +38,29 @@ struct udmabuf_folio {
>  	struct list_head list;
>  };
> 
> -static vm_fault_t udmabuf_vm_fault(struct vm_fault *vmf)
> -{
> -	struct vm_area_struct *vma = vmf->vma;
> -	struct udmabuf *ubuf = vma->vm_private_data;
> -	pgoff_t pgoff = vmf->pgoff;
> -	unsigned long pfn;
> -
> -	if (pgoff >= ubuf->pagecount)
> -		return VM_FAULT_SIGBUS;
> -
> -	pfn = folio_pfn(ubuf->folios[pgoff]);
> -	pfn += ubuf->offsets[pgoff] >> PAGE_SHIFT;
> -
> -	return vmf_insert_pfn(vma, vmf->address, pfn);
> -}
> -
> -static const struct vm_operations_struct udmabuf_vm_ops = {
> -	.fault = udmabuf_vm_fault,
> -};
> -
>  static int mmap_udmabuf(struct dma_buf *buf, struct vm_area_struct
> *vma)
>  {
>  	struct udmabuf *ubuf = buf->priv;
> +	unsigned long addr;
> +	unsigned long end;
> +	unsigned long pgoff;
> +	int ret;
> 
>  	if ((vma->vm_flags & (VM_SHARED | VM_MAYSHARE)) == 0)
>  		return -EINVAL;
> 
> -	vma->vm_ops = &udmabuf_vm_ops;
> -	vma->vm_private_data = ubuf;
> -	vm_flags_set(vma, VM_PFNMAP | VM_DONTEXPAND |
> VM_DONTDUMP);
> +	for (pgoff = vma->vm_pgoff, end = vma->vm_end, addr = vma-
> >vm_start;
> +	     addr < end; pgoff++, addr += PAGE_SIZE) {
> +		struct page *page =
> +			folio_page(ubuf->folios[pgoff],
> +				   ubuf->offsets[pgoff] >> PAGE_SHIFT);
Please don't use struct page pointers, given the recent conversion to use
only folios in udmabuf driver. I think what you are trying to do above can
be done using only folios.

> +
> +		ret = remap_pfn_range(vma, addr, page_to_pfn(page),
> PAGE_SIZE,
> +				      vma->vm_page_prot);
Could you please retain the use of vmf_insert_pfn() here, given the simplicity,
among other reasons?

Thanks,
Vivek

> +		if (ret)
> +			return ret;
> +	}
> +
>  	return 0;
>  }
> 
> --
> 2.45.2

RE: [PATCH v2 2/4] udmabuf: change folios array from kmalloc to kvmalloc

[Kasireddy, Vivek]

Hi Huan,

> 
> When PAGE_SIZE 4096, MAX_PAGE_ORDER 10, 64bit machine,
> page_alloc only support 4MB.
> If above this, trigger this warn and return NULL.
> 
> udmabuf can change size limit, if change it to 3072(3GB), and then alloc
> 3GB udmabuf, will fail create.
> 
> [ 4080.876581] ------------[ cut here ]------------
> [ 4080.876843] WARNING: CPU: 3 PID: 2015 at mm/page_alloc.c:4556
> __alloc_pages+0x2c8/0x350
> [ 4080.878839] RIP: 0010:__alloc_pages+0x2c8/0x350
> [ 4080.879470] Call Trace:
> [ 4080.879473]  <TASK>
> [ 4080.879473]  ? __alloc_pages+0x2c8/0x350
> [ 4080.879475]  ? __warn.cold+0x8e/0xe8
> [ 4080.880647]  ? __alloc_pages+0x2c8/0x350
> [ 4080.880909]  ? report_bug+0xff/0x140
> [ 4080.881175]  ? handle_bug+0x3c/0x80
> [ 4080.881556]  ? exc_invalid_op+0x17/0x70
> [ 4080.881559]  ? asm_exc_invalid_op+0x1a/0x20
> [ 4080.882077]  ? udmabuf_create+0x131/0x400
> 
> Because MAX_PAGE_ORDER, kmalloc can max alloc 4096 * (1 << 10), 4MB
> memory, each array entry is pointer(8byte), so can save 524288 pages(2GB).
> 
> Further more, costly order(order 3) may not be guaranteed that it can be
> applied for, due to fragmentation.
> 
> This patch change udmabuf array use kvmalloc_array, this can fallback
> alloc into vmalloc, which can guarantee allocation for any size and does
> not affect the performance of kmalloc allocations.
> 
> Signed-off-by: Huan Yang <link@vivo.com>
> Acked-by: Christian König <christian.koenig@amd.com>
> ---
>  drivers/dma-buf/udmabuf.c | 26 +++++++++++++-------------
>  1 file changed, 13 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c
> index 475268d4ebb1..af2391cea0bf 100644
> --- a/drivers/dma-buf/udmabuf.c
> +++ b/drivers/dma-buf/udmabuf.c
> @@ -73,7 +73,7 @@ static int vmap_udmabuf(struct dma_buf *buf, struct
> iosys_map *map)
> 
>  	dma_resv_assert_held(buf->resv);
> 
> -	pages = kmalloc_array(ubuf->pagecount, sizeof(*pages),
> GFP_KERNEL);
> +	pages = kvmalloc_array(ubuf->pagecount, sizeof(*pages),
> GFP_KERNEL);
>  	if (!pages)
>  		return -ENOMEM;
> 
> @@ -81,7 +81,7 @@ static int vmap_udmabuf(struct dma_buf *buf, struct
> iosys_map *map)
>  		pages[pg] = &ubuf->folios[pg]->page;
> 
>  	vaddr = vm_map_ram(pages, ubuf->pagecount, -1);
> -	kfree(pages);
> +	kvfree(pages);
>  	if (!vaddr)
>  		return -EINVAL;
> 
> @@ -189,8 +189,8 @@ static void release_udmabuf(struct dma_buf *buf)
>  		put_sg_table(dev, ubuf->sg, DMA_BIDIRECTIONAL);
> 
>  	unpin_all_folios(&ubuf->unpin_list);
> -	kfree(ubuf->offsets);
> -	kfree(ubuf->folios);
> +	kvfree(ubuf->offsets);
> +	kvfree(ubuf->folios);
>  	kfree(ubuf);
>  }
> 
> @@ -315,14 +315,14 @@ static long udmabuf_create(struct miscdevice
> *device,
>  	if (!ubuf->pagecount)
>  		goto err;
> 
> -	ubuf->folios = kmalloc_array(ubuf->pagecount, sizeof(*ubuf->folios),
> -				    GFP_KERNEL);
> +	ubuf->folios = kvmalloc_array(ubuf->pagecount, sizeof(*ubuf-
> >folios),
> +				      GFP_KERNEL);
>  	if (!ubuf->folios) {
>  		ret = -ENOMEM;
>  		goto err;
>  	}
> -	ubuf->offsets = kcalloc(ubuf->pagecount, sizeof(*ubuf->offsets),
> -				GFP_KERNEL);
> +	ubuf->offsets =
> +		kvcalloc(ubuf->pagecount, sizeof(*ubuf->offsets),
No strong opinion, but I'd prefer to keep the kvcalloc on the same line.
Regardless,

Acked-by: Vivek Kasireddy <vivek.kasireddy@intel.com>

> GFP_KERNEL);
>  	if (!ubuf->offsets) {
>  		ret = -ENOMEM;
>  		goto err;
> @@ -336,7 +336,7 @@ static long udmabuf_create(struct miscdevice
> *device,
>  			goto err;
> 
>  		pgcnt = list[i].size >> PAGE_SHIFT;
> -		folios = kmalloc_array(pgcnt, sizeof(*folios), GFP_KERNEL);
> +		folios = kvmalloc_array(pgcnt, sizeof(*folios), GFP_KERNEL);
>  		if (!folios) {
>  			ret = -ENOMEM;
>  			goto err;
> @@ -346,7 +346,7 @@ static long udmabuf_create(struct miscdevice
> *device,
>  		ret = memfd_pin_folios(memfd, list[i].offset, end,
>  				       folios, pgcnt, &pgoff);
>  		if (ret <= 0) {
> -			kfree(folios);
> +			kvfree(folios);
>  			if (!ret)
>  				ret = -EINVAL;
>  			goto err;
> @@ -375,7 +375,7 @@ static long udmabuf_create(struct miscdevice
> *device,
>  			}
>  		}
> 
> -		kfree(folios);
> +		kvfree(folios);
>  		fput(memfd);
>  		memfd = NULL;
>  	}
> @@ -391,8 +391,8 @@ static long udmabuf_create(struct miscdevice
> *device,
>  	if (memfd)
>  		fput(memfd);
>  	unpin_all_folios(&ubuf->unpin_list);
> -	kfree(ubuf->offsets);
> -	kfree(ubuf->folios);
> +	kvfree(ubuf->offsets);
> +	kvfree(ubuf->folios);
>  	kfree(ubuf);
>  	return ret;
>  }
> --
> 2.45.2

RE: [PATCH v2 3/4] fix vmap_udmabuf error page set

[Kasireddy, Vivek]

Hi Huan,

> 
> Currently vmap_udmabuf set page's array by each folio.
> But, ubuf->folios is only contain's the folio's head page.
> 
> That mean we repeatedly mapped the folio head page to the vmalloc area.
> 
> This patch fix it, set each folio's page correct, so that pages array
> contains right page, and then map into vmalloc area
> 
> Signed-off-by: Huan Yang <link@vivo.com>
> ---
>  drivers/dma-buf/udmabuf.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c
> index af2391cea0bf..9737f063b6b3 100644
> --- a/drivers/dma-buf/udmabuf.c
> +++ b/drivers/dma-buf/udmabuf.c
> @@ -78,7 +78,8 @@ static int vmap_udmabuf(struct dma_buf *buf, struct
> iosys_map *map)
>  		return -ENOMEM;
> 
>  	for (pg = 0; pg < ubuf->pagecount; pg++)
> -		pages[pg] = &ubuf->folios[pg]->page;
> +		pages[pg] = folio_page(ubuf->folios[pg],
> +				       ubuf->offsets[pg] >> PAGE_SHIFT);
I believe the correct way to address this issue is to introduce a folio variant
of vm_map_ram() and use that instead, along with the offsets info. 

However, for the time being, I think we can reject vmap of hugetlb folios
by checking for non-zero offset values.

Thanks,
Vivek

> 
>  	vaddr = vm_map_ram(pages, ubuf->pagecount, -1);
>  	kvfree(pages);
> --
> 2.45.2

RE: [PATCH v2 4/4] udmabuf: remove folio unpin list

[Kasireddy, Vivek]

Hi Huan,

> 
> Currently, udmabuf handles folio by creating an unpin list to record
> each folio obtained from the list and unpinning them when released. To
> maintain this approach, many data structures have been established.
> 
> However, maintaining this type of data structure requires a significant
> amount of memory and traversing the list is a substantial overhead,
Have you tried to quantify this overhead?

> which is not friendly to the CPU cache, TLB, and so on.
> 
> Therefore, this patch removes the relationship between the folio and its
> offset in the linear address mapping.
> 
> As an alternative, udmabuf both maintain the folio array and page array,
> folio array use to unpin, and the page array is used as before to handle
> the requirements for the page.
Using pages is a step backwards, given the trend towards embracing folios.
Moreover, the feedback from the former hugetlb maintainer (Mike Kravetz)
was to not use subpages (or tail pages) of a hugetlb folio directly in udmabuf
driver as it would cause problems, particularly when hugetlb vmemmap
optimization (HVO) is enabled. AFAIU, if HVO is enabled by default, a tail page's
struct page pointer may not be available (as it may very well be freed to
save memory). Given all of this, it made sense to convert the udmabuf driver
to only use the head pages of a folio along with the offsets of tail pages.

> 
> So, udmabuf's folios only save the folio struct, foliocount point
> the size of array. pages save page in folios, number offset given by
> create list, pagecount point the size of array.
> 
> Even if we restore the pages structure, its memory usage should be
> smaller than the combined memory usage of offsets(8 bytes in 64bit
> machine)
> and udmabuf_folio structures(24 bytes in 64bit machine).
> 
> By doing this, we can accept the overhead of the udmabuf_folio structure
> and the performance loss of traversing the list during unpinning.
Does your use-case involve frequent pinning/unpinning operations? Note
that this would be considered "shortterm" pin, which is different from the
the way the folios are currently pinned in udmabuf driver, which is considered
"longterm" pin.

However, one optimization I can think of, for memfds backed by shmem, is
to not use unpin_list completely. This way you can probably avoid creating
udmabuf_folio objects and having to traverse the list. But this would require
differentiating udmabufs backed by shmem vs hugetlb folios, which is not
great in my opinion and may not work if THP is enabled.

Thanks,
Vivek

> 
> Signed-off-by: Huan Yang <link@vivo.com>
> ---
>  drivers/dma-buf/udmabuf.c | 167 ++++++++++++++------------------------
>  1 file changed, 61 insertions(+), 106 deletions(-)
> 
> diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c
> index 9737f063b6b3..442ed99d8b33 100644
> --- a/drivers/dma-buf/udmabuf.c
> +++ b/drivers/dma-buf/udmabuf.c
> @@ -25,17 +25,24 @@ module_param(size_limit_mb, int, 0644);
>  MODULE_PARM_DESC(size_limit_mb, "Max size of a dmabuf, in megabytes.
> Default is 64.");
> 
>  struct udmabuf {
> +	/**
> +	 * Each page used by udmabuf in the folio. When obtaining a page
> from a
> +	 * folio, it does not necessarily begin from the head page. This is
> +	 * determined by the offset of the memfd when udmabuf created.
> +	 */
>  	pgoff_t pagecount;
> +	struct page **pages;
> +
> +	/**
> +	 * Each folio in memfd, when a udmabuf is created, it is pinned to
> +	 * ensure that the folio is not moved or reclaimed.
> +	 * folio array used to unpin all when releasing.
> +	 */
> +	pgoff_t foliocount;
>  	struct folio **folios;
> +
>  	struct sg_table *sg;
>  	struct miscdevice *device;
> -	pgoff_t *offsets;
> -	struct list_head unpin_list;
> -};
> -
> -struct udmabuf_folio {
> -	struct folio *folio;
> -	struct list_head list;
>  };
> 
>  static int mmap_udmabuf(struct dma_buf *buf, struct vm_area_struct
> *vma)
> @@ -51,9 +58,7 @@ static int mmap_udmabuf(struct dma_buf *buf, struct
> vm_area_struct *vma)
> 
>  	for (pgoff = vma->vm_pgoff, end = vma->vm_end, addr = vma-
> >vm_start;
>  	     addr < end; pgoff++, addr += PAGE_SIZE) {
> -		struct page *page =
> -			folio_page(ubuf->folios[pgoff],
> -				   ubuf->offsets[pgoff] >> PAGE_SHIFT);
> +		struct page *page = ubuf->pages[pgoff];
> 
>  		ret = remap_pfn_range(vma, addr, page_to_pfn(page),
> PAGE_SIZE,
>  				      vma->vm_page_prot);
> @@ -67,22 +72,11 @@ static int mmap_udmabuf(struct dma_buf *buf,
> struct vm_area_struct *vma)
>  static int vmap_udmabuf(struct dma_buf *buf, struct iosys_map *map)
>  {
>  	struct udmabuf *ubuf = buf->priv;
> -	struct page **pages;
>  	void *vaddr;
> -	pgoff_t pg;
> 
>  	dma_resv_assert_held(buf->resv);
> 
> -	pages = kvmalloc_array(ubuf->pagecount, sizeof(*pages),
> GFP_KERNEL);
> -	if (!pages)
> -		return -ENOMEM;
> -
> -	for (pg = 0; pg < ubuf->pagecount; pg++)
> -		pages[pg] = folio_page(ubuf->folios[pg],
> -				       ubuf->offsets[pg] >> PAGE_SHIFT);
> -
> -	vaddr = vm_map_ram(pages, ubuf->pagecount, -1);
> -	kvfree(pages);
> +	vaddr = vm_map_ram(ubuf->pages, ubuf->pagecount, -1);
>  	if (!vaddr)
>  		return -EINVAL;
> 
> @@ -104,30 +98,25 @@ static struct sg_table *get_sg_table(struct device
> *dev, struct dma_buf *buf,
>  {
>  	struct udmabuf *ubuf = buf->priv;
>  	struct sg_table *sg;
> -	struct scatterlist *sgl;
> -	unsigned int i = 0;
>  	int ret;
> 
>  	sg = kzalloc(sizeof(*sg), GFP_KERNEL);
>  	if (!sg)
>  		return ERR_PTR(-ENOMEM);
> 
> -	ret = sg_alloc_table(sg, ubuf->pagecount, GFP_KERNEL);
> +	ret = sg_alloc_table_from_pages(sg, ubuf->pages, ubuf->pagecount,
> +					0, ubuf->pagecount << PAGE_SHIFT,
> +					GFP_KERNEL);
>  	if (ret < 0)
> -		goto err_alloc;
> -
> -	for_each_sg(sg->sgl, sgl, ubuf->pagecount, i)
> -		sg_set_folio(sgl, ubuf->folios[i], PAGE_SIZE,
> -			     ubuf->offsets[i]);
> +		goto err;
> 
>  	ret = dma_map_sgtable(dev, sg, direction, 0);
>  	if (ret < 0)
> -		goto err_map;
> +		goto err;
>  	return sg;
> 
> -err_map:
> +err:
>  	sg_free_table(sg);
> -err_alloc:
>  	kfree(sg);
>  	return ERR_PTR(ret);
>  }
> @@ -153,34 +142,6 @@ static void unmap_udmabuf(struct
> dma_buf_attachment *at,
>  	return put_sg_table(at->dev, sg, direction);
>  }
> 
> -static void unpin_all_folios(struct list_head *unpin_list)
> -{
> -	struct udmabuf_folio *ubuf_folio;
> -
> -	while (!list_empty(unpin_list)) {
> -		ubuf_folio = list_first_entry(unpin_list,
> -					      struct udmabuf_folio, list);
> -		unpin_folio(ubuf_folio->folio);
> -
> -		list_del(&ubuf_folio->list);
> -		kfree(ubuf_folio);
> -	}
> -}
> -
> -static int add_to_unpin_list(struct list_head *unpin_list,
> -			     struct folio *folio)
> -{
> -	struct udmabuf_folio *ubuf_folio;
> -
> -	ubuf_folio = kzalloc(sizeof(*ubuf_folio), GFP_KERNEL);
> -	if (!ubuf_folio)
> -		return -ENOMEM;
> -
> -	ubuf_folio->folio = folio;
> -	list_add_tail(&ubuf_folio->list, unpin_list);
> -	return 0;
> -}
> -
>  static void release_udmabuf(struct dma_buf *buf)
>  {
>  	struct udmabuf *ubuf = buf->priv;
> @@ -189,9 +150,9 @@ static void release_udmabuf(struct dma_buf *buf)
>  	if (ubuf->sg)
>  		put_sg_table(dev, ubuf->sg, DMA_BIDIRECTIONAL);
> 
> -	unpin_all_folios(&ubuf->unpin_list);
> -	kvfree(ubuf->offsets);
> +	unpin_folios(ubuf->folios, ubuf->foliocount);
>  	kvfree(ubuf->folios);
> +	kvfree(ubuf->pages);
>  	kfree(ubuf);
>  }
> 
> @@ -289,19 +250,18 @@ static long udmabuf_create(struct miscdevice
> *device,
>  			   struct udmabuf_create_list *head,
>  			   struct udmabuf_create_item *list)
>  {
> -	pgoff_t pgoff, pgcnt, pglimit, pgbuf = 0;
> -	long nr_folios, ret = -EINVAL;
> +	pgoff_t pgoff, pgcnt, pglimit, nr_pages;
> +	long nr_folios = 0, ret = -EINVAL;
>  	struct file *memfd = NULL;
>  	struct folio **folios;
>  	struct udmabuf *ubuf;
> -	u32 i, j, k, flags;
> +	u32 i, flags;
>  	loff_t end;
> 
>  	ubuf = kzalloc(sizeof(*ubuf), GFP_KERNEL);
>  	if (!ubuf)
>  		return -ENOMEM;
> 
> -	INIT_LIST_HEAD(&ubuf->unpin_list);
>  	pglimit = (size_limit_mb * 1024 * 1024) >> PAGE_SHIFT;
>  	for (i = 0; i < head->count; i++) {
>  		if (!IS_ALIGNED(list[i].offset, PAGE_SIZE))
> @@ -322,64 +282,58 @@ static long udmabuf_create(struct miscdevice
> *device,
>  		ret = -ENOMEM;
>  		goto err;
>  	}
> -	ubuf->offsets =
> -		kvcalloc(ubuf->pagecount, sizeof(*ubuf->offsets),
> GFP_KERNEL);
> -	if (!ubuf->offsets) {
> +	folios = ubuf->folios;
> +
> +	ubuf->pages = kvmalloc_array(ubuf->pagecount, sizeof(*ubuf-
> >pages),
> +				     GFP_KERNEL);
> +	if (!ubuf->pages) {
>  		ret = -ENOMEM;
>  		goto err;
>  	}
> 
> -	pgbuf = 0;
> -	for (i = 0; i < head->count; i++) {
> +	for (i = 0, nr_pages = 0; i < head->count; i++) {
> +		u32 j, pg;
> +
>  		memfd = fget(list[i].memfd);
>  		ret = check_memfd_seals(memfd);
>  		if (ret < 0)
>  			goto err;
> 
>  		pgcnt = list[i].size >> PAGE_SHIFT;
> -		folios = kvmalloc_array(pgcnt, sizeof(*folios), GFP_KERNEL);
> -		if (!folios) {
> -			ret = -ENOMEM;
> -			goto err;
> -		}
> 
>  		end = list[i].offset + (pgcnt << PAGE_SHIFT) - 1;
> -		ret = memfd_pin_folios(memfd, list[i].offset, end,
> -				       folios, pgcnt, &pgoff);
> +		ret = memfd_pin_folios(memfd, list[i].offset, end, folios,
> +				       pgcnt, &pgoff);
>  		if (ret <= 0) {
> -			kvfree(folios);
> -			if (!ret)
> -				ret = -EINVAL;
> +			ret = ret ?: -EINVAL;
>  			goto err;
>  		}
> 
> -		nr_folios = ret;
> -		pgoff >>= PAGE_SHIFT;
> -		for (j = 0, k = 0; j < pgcnt; j++) {
> -			ubuf->folios[pgbuf] = folios[k];
> -			ubuf->offsets[pgbuf] = pgoff << PAGE_SHIFT;
> -
> -			if (j == 0 || ubuf->folios[pgbuf-1] != folios[k]) {
> -				ret = add_to_unpin_list(&ubuf->unpin_list,
> -							folios[k]);
> -				if (ret < 0) {
> -					kfree(folios);
> -					goto err;
> -				}
> -			}
> -
> -			pgbuf++;
> -			if (++pgoff == folio_nr_pages(folios[k])) {
> -				pgoff = 0;
> -				if (++k == nr_folios)
> -					break;
> +		/**
> +		 * Iter the pinned folios and record them for later unpin
> +		 * when releasing.
> +		 * memfd may start from any offset, so we need check it
> +		 * carefully at first.
> +		 */
> +		for (j = 0, pgoff >>= PAGE_SHIFT, pg = 0; j < ret;
> +		     ++j, pgoff = 0) {
> +			pgoff_t k;
> +			struct folio *folio = folios[j];
> +
> +			for (k = pgoff; k < folio_nr_pages(folio); ++k) {
> +				ubuf->pages[nr_pages++] = folio_page(folio,
> k);
> +
> +				if (++pg >= pgcnt)
> +					goto end;
>  			}
>  		}
> -
> -		kvfree(folios);
> +end:
> +		folios += ret;
> +		nr_folios += ret;
>  		fput(memfd);
>  		memfd = NULL;
>  	}
> +	ubuf->foliocount = nr_folios;
> 
>  	flags = head->flags & UDMABUF_FLAGS_CLOEXEC ? O_CLOEXEC : 0;
>  	ret = export_udmabuf(ubuf, device, flags);
> @@ -391,8 +345,9 @@ static long udmabuf_create(struct miscdevice
> *device,
>  err:
>  	if (memfd)
>  		fput(memfd);
> -	unpin_all_folios(&ubuf->unpin_list);
> -	kvfree(ubuf->offsets);
> +	if (nr_folios)
> +		unpin_folios(ubuf->folios, nr_folios);
> +	kvfree(ubuf->pages);
>  	kvfree(ubuf->folios);
>  	kfree(ubuf);
>  	return ret;
> --
> 2.45.2

Re: [PATCH v2 1/4] udmabuf: cancel mmap page fault, direct map it

[Huan Yang]

在 2024/8/10 9:28, Kasireddy, Vivek 写道:
> [Some people who received this message don't often get email from vivek.kasireddy@intel.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> Hi Huan,
>
>> The current udmabuf mmap uses a page fault mechanism to populate the
>> vma.
>>
>> However, the current udmabuf has already obtained and pinned the folio
>> upon completion of the creation.This means that the physical memory has
>> already been acquired, rather than being accessed dynamically. The
>> current page fault method only saves some page table memory.
>>
>> As a result, the page fault mechanism has lost its purpose as a demanding
>> page. Due to the fact that page fault requires trapping into kernel mode
>> and filling in when accessing the corresponding virtual address in mmap,
>> this means that user mode access to virtual addresses needs to trap into
>> kernel mode.
>>
>> Therefore, when creating a large size udmabuf, this represents a
>> considerable overhead.
>>
>> The current patch removes the page fault method of mmap and
>> instead fills it directly when mmap is triggered.
> I think it makes sense to populate the vma when the first fault is triggered
> instead of doing it during mmap. This is because the userspace may call
> mmap but does not actually use the data. Qemu works this way depending on
Yes, the idea of this is also related to the concept of page fault.

However, the folio has already been pinned during creation. I think 
using the page fault

again is theoretically sound, but it may not save memory, only increase 
context switch overhead.


> whether opengl is available in the environment or not.
>
>> Signed-off-by: Huan Yang <link@vivo.com>
>> ---
>>   drivers/dma-buf/udmabuf.c | 39 ++++++++++++++++-----------------------
>>   1 file changed, 16 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c
>> index 047c3cd2ceff..475268d4ebb1 100644
>> --- a/drivers/dma-buf/udmabuf.c
>> +++ b/drivers/dma-buf/udmabuf.c
>> @@ -38,36 +38,29 @@ struct udmabuf_folio {
>>        struct list_head list;
>>   };
>>
>> -static vm_fault_t udmabuf_vm_fault(struct vm_fault *vmf)
>> -{
>> -     struct vm_area_struct *vma = vmf->vma;
>> -     struct udmabuf *ubuf = vma->vm_private_data;
>> -     pgoff_t pgoff = vmf->pgoff;
>> -     unsigned long pfn;
>> -
>> -     if (pgoff >= ubuf->pagecount)
>> -             return VM_FAULT_SIGBUS;
>> -
>> -     pfn = folio_pfn(ubuf->folios[pgoff]);
>> -     pfn += ubuf->offsets[pgoff] >> PAGE_SHIFT;
>> -
>> -     return vmf_insert_pfn(vma, vmf->address, pfn);
>> -}
>> -
>> -static const struct vm_operations_struct udmabuf_vm_ops = {
>> -     .fault = udmabuf_vm_fault,
>> -};
>> -
>>   static int mmap_udmabuf(struct dma_buf *buf, struct vm_area_struct
>> *vma)
>>   {
>>        struct udmabuf *ubuf = buf->priv;
>> +     unsigned long addr;
>> +     unsigned long end;
>> +     unsigned long pgoff;
>> +     int ret;
>>
>>        if ((vma->vm_flags & (VM_SHARED | VM_MAYSHARE)) == 0)
>>                return -EINVAL;
>>
>> -     vma->vm_ops = &udmabuf_vm_ops;
>> -     vma->vm_private_data = ubuf;
>> -     vm_flags_set(vma, VM_PFNMAP | VM_DONTEXPAND |
>> VM_DONTDUMP);
>> +     for (pgoff = vma->vm_pgoff, end = vma->vm_end, addr = vma-
>>> vm_start;
>> +          addr < end; pgoff++, addr += PAGE_SIZE) {
>> +             struct page *page =
>> +                     folio_page(ubuf->folios[pgoff],
>> +                                ubuf->offsets[pgoff] >> PAGE_SHIFT);
> Please don't use struct page pointers, given the recent conversion to use
> only folios in udmabuf driver. I think what you are trying to do above can
> be done using only folios.
Yes, just use pfn. Consider of HVO, must use this.
>
>> +
>> +             ret = remap_pfn_range(vma, addr, page_to_pfn(page),
>> PAGE_SIZE,
>> +                                   vma->vm_page_prot);
> Could you please retain the use of vmf_insert_pfn() here, given the simplicity,
> among other reasons?
I will make the correction.

Thanks.
>
> Thanks,
> Vivek
>
>> +             if (ret)
>> +                     return ret;
>> +     }
>> +
>>        return 0;
>>   }
>>
>> --
>> 2.45.2

Re: [PATCH v2 2/4] udmabuf: change folios array from kmalloc to kvmalloc

[Huan Yang]

在 2024/8/10 9:29, Kasireddy, Vivek 写道:
> [Some people who received this message don't often get email from vivek.kasireddy@intel.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> Hi Huan,
>
>> When PAGE_SIZE 4096, MAX_PAGE_ORDER 10, 64bit machine,
>> page_alloc only support 4MB.
>> If above this, trigger this warn and return NULL.
>>
>> udmabuf can change size limit, if change it to 3072(3GB), and then alloc
>> 3GB udmabuf, will fail create.
>>
>> [ 4080.876581] ------------[ cut here ]------------
>> [ 4080.876843] WARNING: CPU: 3 PID: 2015 at mm/page_alloc.c:4556
>> __alloc_pages+0x2c8/0x350
>> [ 4080.878839] RIP: 0010:__alloc_pages+0x2c8/0x350
>> [ 4080.879470] Call Trace:
>> [ 4080.879473]  <TASK>
>> [ 4080.879473]  ? __alloc_pages+0x2c8/0x350
>> [ 4080.879475]  ? __warn.cold+0x8e/0xe8
>> [ 4080.880647]  ? __alloc_pages+0x2c8/0x350
>> [ 4080.880909]  ? report_bug+0xff/0x140
>> [ 4080.881175]  ? handle_bug+0x3c/0x80
>> [ 4080.881556]  ? exc_invalid_op+0x17/0x70
>> [ 4080.881559]  ? asm_exc_invalid_op+0x1a/0x20
>> [ 4080.882077]  ? udmabuf_create+0x131/0x400
>>
>> Because MAX_PAGE_ORDER, kmalloc can max alloc 4096 * (1 << 10), 4MB
>> memory, each array entry is pointer(8byte), so can save 524288 pages(2GB).
>>
>> Further more, costly order(order 3) may not be guaranteed that it can be
>> applied for, due to fragmentation.
>>
>> This patch change udmabuf array use kvmalloc_array, this can fallback
>> alloc into vmalloc, which can guarantee allocation for any size and does
>> not affect the performance of kmalloc allocations.
>>
>> Signed-off-by: Huan Yang <link@vivo.com>
>> Acked-by: Christian König <christian.koenig@amd.com>
>> ---
>>   drivers/dma-buf/udmabuf.c | 26 +++++++++++++-------------
>>   1 file changed, 13 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c
>> index 475268d4ebb1..af2391cea0bf 100644
>> --- a/drivers/dma-buf/udmabuf.c
>> +++ b/drivers/dma-buf/udmabuf.c
>> @@ -73,7 +73,7 @@ static int vmap_udmabuf(struct dma_buf *buf, struct
>> iosys_map *map)
>>
>>        dma_resv_assert_held(buf->resv);
>>
>> -     pages = kmalloc_array(ubuf->pagecount, sizeof(*pages),
>> GFP_KERNEL);
>> +     pages = kvmalloc_array(ubuf->pagecount, sizeof(*pages),
>> GFP_KERNEL);
>>        if (!pages)
>>                return -ENOMEM;
>>
>> @@ -81,7 +81,7 @@ static int vmap_udmabuf(struct dma_buf *buf, struct
>> iosys_map *map)
>>                pages[pg] = &ubuf->folios[pg]->page;
>>
>>        vaddr = vm_map_ram(pages, ubuf->pagecount, -1);
>> -     kfree(pages);
>> +     kvfree(pages);
>>        if (!vaddr)
>>                return -EINVAL;
>>
>> @@ -189,8 +189,8 @@ static void release_udmabuf(struct dma_buf *buf)
>>                put_sg_table(dev, ubuf->sg, DMA_BIDIRECTIONAL);
>>
>>        unpin_all_folios(&ubuf->unpin_list);
>> -     kfree(ubuf->offsets);
>> -     kfree(ubuf->folios);
>> +     kvfree(ubuf->offsets);
>> +     kvfree(ubuf->folios);
>>        kfree(ubuf);
>>   }
>>
>> @@ -315,14 +315,14 @@ static long udmabuf_create(struct miscdevice
>> *device,
>>        if (!ubuf->pagecount)
>>                goto err;
>>
>> -     ubuf->folios = kmalloc_array(ubuf->pagecount, sizeof(*ubuf->folios),
>> -                                 GFP_KERNEL);
>> +     ubuf->folios = kvmalloc_array(ubuf->pagecount, sizeof(*ubuf-
>>> folios),
>> +                                   GFP_KERNEL);
>>        if (!ubuf->folios) {
>>                ret = -ENOMEM;
>>                goto err;
>>        }
>> -     ubuf->offsets = kcalloc(ubuf->pagecount, sizeof(*ubuf->offsets),
>> -                             GFP_KERNEL);
>> +     ubuf->offsets =
>> +             kvcalloc(ubuf->pagecount, sizeof(*ubuf->offsets),
> No strong opinion, but I'd prefer to keep the kvcalloc on the same line.
> Regardless,

This style is auto formatted by my clang-format with .clang-format set 
in kernel.

But, I chang into online:

     ubuf->offsets = kvcalloc(ubuf->pagecount, sizeof(*ubuf->offsets),
                  GFP_KERNEL);

checkpatch  also did not report any errors.

So, I can send the next version of the patch when needed.

Thanks.

>
> Acked-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
>
>> GFP_KERNEL);
>>        if (!ubuf->offsets) {
>>                ret = -ENOMEM;
>>                goto err;
>> @@ -336,7 +336,7 @@ static long udmabuf_create(struct miscdevice
>> *device,
>>                        goto err;
>>
>>                pgcnt = list[i].size >> PAGE_SHIFT;
>> -             folios = kmalloc_array(pgcnt, sizeof(*folios), GFP_KERNEL);
>> +             folios = kvmalloc_array(pgcnt, sizeof(*folios), GFP_KERNEL);
>>                if (!folios) {
>>                        ret = -ENOMEM;
>>                        goto err;
>> @@ -346,7 +346,7 @@ static long udmabuf_create(struct miscdevice
>> *device,
>>                ret = memfd_pin_folios(memfd, list[i].offset, end,
>>                                       folios, pgcnt, &pgoff);
>>                if (ret <= 0) {
>> -                     kfree(folios);
>> +                     kvfree(folios);
>>                        if (!ret)
>>                                ret = -EINVAL;
>>                        goto err;
>> @@ -375,7 +375,7 @@ static long udmabuf_create(struct miscdevice
>> *device,
>>                        }
>>                }
>>
>> -             kfree(folios);
>> +             kvfree(folios);
>>                fput(memfd);
>>                memfd = NULL;
>>        }
>> @@ -391,8 +391,8 @@ static long udmabuf_create(struct miscdevice
>> *device,
>>        if (memfd)
>>                fput(memfd);
>>        unpin_all_folios(&ubuf->unpin_list);
>> -     kfree(ubuf->offsets);
>> -     kfree(ubuf->folios);
>> +     kvfree(ubuf->offsets);
>> +     kvfree(ubuf->folios);
>>        kfree(ubuf);
>>        return ret;
>>   }
>> --
>> 2.45.2

Re: [PATCH v2 3/4] fix vmap_udmabuf error page set

[Huan Yang]

在 2024/8/10 10:39, Kasireddy, Vivek 写道:
> [Some people who received this message don't often get email from vivek.kasireddy@intel.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> Hi Huan,
>
>> Currently vmap_udmabuf set page's array by each folio.
>> But, ubuf->folios is only contain's the folio's head page.
>>
>> That mean we repeatedly mapped the folio head page to the vmalloc area.
>>
>> This patch fix it, set each folio's page correct, so that pages array
>> contains right page, and then map into vmalloc area
>>
>> Signed-off-by: Huan Yang <link@vivo.com>
>> ---
>>   drivers/dma-buf/udmabuf.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c
>> index af2391cea0bf..9737f063b6b3 100644
>> --- a/drivers/dma-buf/udmabuf.c
>> +++ b/drivers/dma-buf/udmabuf.c
>> @@ -78,7 +78,8 @@ static int vmap_udmabuf(struct dma_buf *buf, struct
>> iosys_map *map)
>>                return -ENOMEM;
>>
>>        for (pg = 0; pg < ubuf->pagecount; pg++)
>> -             pages[pg] = &ubuf->folios[pg]->page;
>> +             pages[pg] = folio_page(ubuf->folios[pg],
>> +                                    ubuf->offsets[pg] >> PAGE_SHIFT);
> I believe the correct way to address this issue is to introduce a folio variant
> of vm_map_ram() and use that instead, along with the offsets info.
>
> However, for the time being, I think we can reject vmap of hugetlb folios
> by checking for non-zero offset values.

Do you mean, we do not want to vmap hugetlb folios?  So by check this is 
reasonable.

BTW, I found that recently shmem has started to support mTHP. (Even if 
need enable a switch)

If this, not only hugetlb contains large folio, so ignore offset may not 
too good?(I am not entirely sure whether mTHP can be used with shmem for 
memfd.)

Thanks.

>
> Thanks,
> Vivek
>
>>        vaddr = vm_map_ram(pages, ubuf->pagecount, -1);
>>        kvfree(pages);
>> --
>> 2.45.2

Re: [PATCH v2 4/4] udmabuf: remove folio unpin list

[Huan Yang]

在 2024/8/10 10:52, Kasireddy, Vivek 写道:
> [Some people who received this message don't often get email from vivek.kasireddy@intel.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> Hi Huan,
>
>> Currently, udmabuf handles folio by creating an unpin list to record
>> each folio obtained from the list and unpinning them when released. To
>> maintain this approach, many data structures have been established.
>>
>> However, maintaining this type of data structure requires a significant
>> amount of memory and traversing the list is a substantial overhead,
> Have you tried to quantify this overhead?

Sorry, no, my viewpoint is just a thought experiment.

udmabuf_folios is 24bytes, if each folio just 4KB, the additional memory 
occupied will be very high, far exceeding the size of the page pointer.

"list_head" is a linked list, and accessing it is not CPU 
cache-friendly, making prefetching difficult.

On the other hand, "page array" is a cache-friendly data structure.

>
>> which is not friendly to the CPU cache, TLB, and so on.
>>
>> Therefore, this patch removes the relationship between the folio and its
>> offset in the linear address mapping.
>>
>> As an alternative, udmabuf both maintain the folio array and page array,
>> folio array use to unpin, and the page array is used as before to handle
>> the requirements for the page.
> Using pages is a step backwards, given the trend towards embracing folios.
Agree.
> Moreover, the feedback from the former hugetlb maintainer (Mike Kravetz)
> was to not use subpages (or tail pages) of a hugetlb folio directly in udmabuf
> driver as it would cause problems, particularly when hugetlb vmemmap
> optimization (HVO) is enabled. AFAIU, if HVO is enabled by default, a tail page's
> struct page pointer may not be available (as it may very well be freed to
> save memory). Given all of this, it made sense to convert the udmabuf driver
> to only use the head pages of a folio along with the offsets of tail pages.
I haven't considered that HVO would have an impact on this.

If so, does it mean that we cannot guarantee the correct reference to 
all Pages under the folio?
I see vmap and sgt based on page, I HVO already freed all sub page's 
struct, These uses will all become unreasonable.

BTW, if page struct can't relyable, can we just save pfn array? Hmm, but 
many of they just rely on page.

>
>> So, udmabuf's folios only save the folio struct, foliocount point
>> the size of array. pages save page in folios, number offset given by
>> create list, pagecount point the size of array.
>>
>> Even if we restore the pages structure, its memory usage should be
>> smaller than the combined memory usage of offsets(8 bytes in 64bit
>> machine)
>> and udmabuf_folio structures(24 bytes in 64bit machine).
>>
>> By doing this, we can accept the overhead of the udmabuf_folio structure
>> and the performance loss of traversing the list during unpinning.
> Does your use-case involve frequent pinning/unpinning operations? Note

My only use case at the moment is to read large files (around 3GB) using 
udmabuf in direct I/O mode.

This means that if HUGETLB is not used, the udmabuf_folio structure will 
be particularly large.(((3 << 30) >> 12) * 24) 18MB

And the list when pin(add), unpin(del), may cost a bit.

> that this would be considered "shortterm" pin, which is different from the
> the way the folios are currently pinned in udmabuf driver, which is considered
Could you please describe it in detail? I didn't understand.

> "longterm" pin.
>
> However, one optimization I can think of, for memfds backed by shmem, is
> to not use unpin_list completely. This way you can probably avoid creating
> udmabuf_folio objects and having to traverse the list. But this would require
> differentiating udmabufs backed by shmem vs hugetlb folios, which is not
> great in my opinion and may not work if THP is enabled.

Considering the existence of HVO, I also feel the need to find further 
optimization methods.

Thanks.

>
> Thanks,
> Vivek
>
>> Signed-off-by: Huan Yang <link@vivo.com>
>> ---
>>   drivers/dma-buf/udmabuf.c | 167 ++++++++++++++------------------------
>>   1 file changed, 61 insertions(+), 106 deletions(-)
>>
>> diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c
>> index 9737f063b6b3..442ed99d8b33 100644
>> --- a/drivers/dma-buf/udmabuf.c
>> +++ b/drivers/dma-buf/udmabuf.c
>> @@ -25,17 +25,24 @@ module_param(size_limit_mb, int, 0644);
>>   MODULE_PARM_DESC(size_limit_mb, "Max size of a dmabuf, in megabytes.
>> Default is 64.");
>>
>>   struct udmabuf {
>> +     /**
>> +      * Each page used by udmabuf in the folio. When obtaining a page
>> from a
>> +      * folio, it does not necessarily begin from the head page. This is
>> +      * determined by the offset of the memfd when udmabuf created.
>> +      */
>>        pgoff_t pagecount;
>> +     struct page **pages;
>> +
>> +     /**
>> +      * Each folio in memfd, when a udmabuf is created, it is pinned to
>> +      * ensure that the folio is not moved or reclaimed.
>> +      * folio array used to unpin all when releasing.
>> +      */
>> +     pgoff_t foliocount;
>>        struct folio **folios;
>> +
>>        struct sg_table *sg;
>>        struct miscdevice *device;
>> -     pgoff_t *offsets;
>> -     struct list_head unpin_list;
>> -};
>> -
>> -struct udmabuf_folio {
>> -     struct folio *folio;
>> -     struct list_head list;
>>   };
>>
>>   static int mmap_udmabuf(struct dma_buf *buf, struct vm_area_struct
>> *vma)
>> @@ -51,9 +58,7 @@ static int mmap_udmabuf(struct dma_buf *buf, struct
>> vm_area_struct *vma)
>>
>>        for (pgoff = vma->vm_pgoff, end = vma->vm_end, addr = vma-
>>> vm_start;
>>             addr < end; pgoff++, addr += PAGE_SIZE) {
>> -             struct page *page =
>> -                     folio_page(ubuf->folios[pgoff],
>> -                                ubuf->offsets[pgoff] >> PAGE_SHIFT);
>> +             struct page *page = ubuf->pages[pgoff];
>>
>>                ret = remap_pfn_range(vma, addr, page_to_pfn(page),
>> PAGE_SIZE,
>>                                      vma->vm_page_prot);
>> @@ -67,22 +72,11 @@ static int mmap_udmabuf(struct dma_buf *buf,
>> struct vm_area_struct *vma)
>>   static int vmap_udmabuf(struct dma_buf *buf, struct iosys_map *map)
>>   {
>>        struct udmabuf *ubuf = buf->priv;
>> -     struct page **pages;
>>        void *vaddr;
>> -     pgoff_t pg;
>>
>>        dma_resv_assert_held(buf->resv);
>>
>> -     pages = kvmalloc_array(ubuf->pagecount, sizeof(*pages),
>> GFP_KERNEL);
>> -     if (!pages)
>> -             return -ENOMEM;
>> -
>> -     for (pg = 0; pg < ubuf->pagecount; pg++)
>> -             pages[pg] = folio_page(ubuf->folios[pg],
>> -                                    ubuf->offsets[pg] >> PAGE_SHIFT);
>> -
>> -     vaddr = vm_map_ram(pages, ubuf->pagecount, -1);
>> -     kvfree(pages);
>> +     vaddr = vm_map_ram(ubuf->pages, ubuf->pagecount, -1);
>>        if (!vaddr)
>>                return -EINVAL;
>>
>> @@ -104,30 +98,25 @@ static struct sg_table *get_sg_table(struct device
>> *dev, struct dma_buf *buf,
>>   {
>>        struct udmabuf *ubuf = buf->priv;
>>        struct sg_table *sg;
>> -     struct scatterlist *sgl;
>> -     unsigned int i = 0;
>>        int ret;
>>
>>        sg = kzalloc(sizeof(*sg), GFP_KERNEL);
>>        if (!sg)
>>                return ERR_PTR(-ENOMEM);
>>
>> -     ret = sg_alloc_table(sg, ubuf->pagecount, GFP_KERNEL);
>> +     ret = sg_alloc_table_from_pages(sg, ubuf->pages, ubuf->pagecount,
>> +                                     0, ubuf->pagecount << PAGE_SHIFT,
>> +                                     GFP_KERNEL);
>>        if (ret < 0)
>> -             goto err_alloc;
>> -
>> -     for_each_sg(sg->sgl, sgl, ubuf->pagecount, i)
>> -             sg_set_folio(sgl, ubuf->folios[i], PAGE_SIZE,
>> -                          ubuf->offsets[i]);
>> +             goto err;
>>
>>        ret = dma_map_sgtable(dev, sg, direction, 0);
>>        if (ret < 0)
>> -             goto err_map;
>> +             goto err;
>>        return sg;
>>
>> -err_map:
>> +err:
>>        sg_free_table(sg);
>> -err_alloc:
>>        kfree(sg);
>>        return ERR_PTR(ret);
>>   }
>> @@ -153,34 +142,6 @@ static void unmap_udmabuf(struct
>> dma_buf_attachment *at,
>>        return put_sg_table(at->dev, sg, direction);
>>   }
>>
>> -static void unpin_all_folios(struct list_head *unpin_list)
>> -{
>> -     struct udmabuf_folio *ubuf_folio;
>> -
>> -     while (!list_empty(unpin_list)) {
>> -             ubuf_folio = list_first_entry(unpin_list,
>> -                                           struct udmabuf_folio, list);
>> -             unpin_folio(ubuf_folio->folio);
>> -
>> -             list_del(&ubuf_folio->list);
>> -             kfree(ubuf_folio);
>> -     }
>> -}
>> -
>> -static int add_to_unpin_list(struct list_head *unpin_list,
>> -                          struct folio *folio)
>> -{
>> -     struct udmabuf_folio *ubuf_folio;
>> -
>> -     ubuf_folio = kzalloc(sizeof(*ubuf_folio), GFP_KERNEL);
>> -     if (!ubuf_folio)
>> -             return -ENOMEM;
>> -
>> -     ubuf_folio->folio = folio;
>> -     list_add_tail(&ubuf_folio->list, unpin_list);
>> -     return 0;
>> -}
>> -
>>   static void release_udmabuf(struct dma_buf *buf)
>>   {
>>        struct udmabuf *ubuf = buf->priv;
>> @@ -189,9 +150,9 @@ static void release_udmabuf(struct dma_buf *buf)
>>        if (ubuf->sg)
>>                put_sg_table(dev, ubuf->sg, DMA_BIDIRECTIONAL);
>>
>> -     unpin_all_folios(&ubuf->unpin_list);
>> -     kvfree(ubuf->offsets);
>> +     unpin_folios(ubuf->folios, ubuf->foliocount);
>>        kvfree(ubuf->folios);
>> +     kvfree(ubuf->pages);
>>        kfree(ubuf);
>>   }
>>
>> @@ -289,19 +250,18 @@ static long udmabuf_create(struct miscdevice
>> *device,
>>                           struct udmabuf_create_list *head,
>>                           struct udmabuf_create_item *list)
>>   {
>> -     pgoff_t pgoff, pgcnt, pglimit, pgbuf = 0;
>> -     long nr_folios, ret = -EINVAL;
>> +     pgoff_t pgoff, pgcnt, pglimit, nr_pages;
>> +     long nr_folios = 0, ret = -EINVAL;
>>        struct file *memfd = NULL;
>>        struct folio **folios;
>>        struct udmabuf *ubuf;
>> -     u32 i, j, k, flags;
>> +     u32 i, flags;
>>        loff_t end;
>>
>>        ubuf = kzalloc(sizeof(*ubuf), GFP_KERNEL);
>>        if (!ubuf)
>>                return -ENOMEM;
>>
>> -     INIT_LIST_HEAD(&ubuf->unpin_list);
>>        pglimit = (size_limit_mb * 1024 * 1024) >> PAGE_SHIFT;
>>        for (i = 0; i < head->count; i++) {
>>                if (!IS_ALIGNED(list[i].offset, PAGE_SIZE))
>> @@ -322,64 +282,58 @@ static long udmabuf_create(struct miscdevice
>> *device,
>>                ret = -ENOMEM;
>>                goto err;
>>        }
>> -     ubuf->offsets =
>> -             kvcalloc(ubuf->pagecount, sizeof(*ubuf->offsets),
>> GFP_KERNEL);
>> -     if (!ubuf->offsets) {
>> +     folios = ubuf->folios;
>> +
>> +     ubuf->pages = kvmalloc_array(ubuf->pagecount, sizeof(*ubuf-
>>> pages),
>> +                                  GFP_KERNEL);
>> +     if (!ubuf->pages) {
>>                ret = -ENOMEM;
>>                goto err;
>>        }
>>
>> -     pgbuf = 0;
>> -     for (i = 0; i < head->count; i++) {
>> +     for (i = 0, nr_pages = 0; i < head->count; i++) {
>> +             u32 j, pg;
>> +
>>                memfd = fget(list[i].memfd);
>>                ret = check_memfd_seals(memfd);
>>                if (ret < 0)
>>                        goto err;
>>
>>                pgcnt = list[i].size >> PAGE_SHIFT;
>> -             folios = kvmalloc_array(pgcnt, sizeof(*folios), GFP_KERNEL);
>> -             if (!folios) {
>> -                     ret = -ENOMEM;
>> -                     goto err;
>> -             }
>>
>>                end = list[i].offset + (pgcnt << PAGE_SHIFT) - 1;
>> -             ret = memfd_pin_folios(memfd, list[i].offset, end,
>> -                                    folios, pgcnt, &pgoff);
>> +             ret = memfd_pin_folios(memfd, list[i].offset, end, folios,
>> +                                    pgcnt, &pgoff);
>>                if (ret <= 0) {
>> -                     kvfree(folios);
>> -                     if (!ret)
>> -                             ret = -EINVAL;
>> +                     ret = ret ?: -EINVAL;
>>                        goto err;
>>                }
>>
>> -             nr_folios = ret;
>> -             pgoff >>= PAGE_SHIFT;
>> -             for (j = 0, k = 0; j < pgcnt; j++) {
>> -                     ubuf->folios[pgbuf] = folios[k];
>> -                     ubuf->offsets[pgbuf] = pgoff << PAGE_SHIFT;
>> -
>> -                     if (j == 0 || ubuf->folios[pgbuf-1] != folios[k]) {
>> -                             ret = add_to_unpin_list(&ubuf->unpin_list,
>> -                                                     folios[k]);
>> -                             if (ret < 0) {
>> -                                     kfree(folios);
>> -                                     goto err;
>> -                             }
>> -                     }
>> -
>> -                     pgbuf++;
>> -                     if (++pgoff == folio_nr_pages(folios[k])) {
>> -                             pgoff = 0;
>> -                             if (++k == nr_folios)
>> -                                     break;
>> +             /**
>> +              * Iter the pinned folios and record them for later unpin
>> +              * when releasing.
>> +              * memfd may start from any offset, so we need check it
>> +              * carefully at first.
>> +              */
>> +             for (j = 0, pgoff >>= PAGE_SHIFT, pg = 0; j < ret;
>> +                  ++j, pgoff = 0) {
>> +                     pgoff_t k;
>> +                     struct folio *folio = folios[j];
>> +
>> +                     for (k = pgoff; k < folio_nr_pages(folio); ++k) {
>> +                             ubuf->pages[nr_pages++] = folio_page(folio,
>> k);
>> +
>> +                             if (++pg >= pgcnt)
>> +                                     goto end;
>>                        }
>>                }
>> -
>> -             kvfree(folios);
>> +end:
>> +             folios += ret;
>> +             nr_folios += ret;
>>                fput(memfd);
>>                memfd = NULL;
>>        }
>> +     ubuf->foliocount = nr_folios;
>>
>>        flags = head->flags & UDMABUF_FLAGS_CLOEXEC ? O_CLOEXEC : 0;
>>        ret = export_udmabuf(ubuf, device, flags);
>> @@ -391,8 +345,9 @@ static long udmabuf_create(struct miscdevice
>> *device,
>>   err:
>>        if (memfd)
>>                fput(memfd);
>> -     unpin_all_folios(&ubuf->unpin_list);
>> -     kvfree(ubuf->offsets);
>> +     if (nr_folios)
>> +             unpin_folios(ubuf->folios, nr_folios);
>> +     kvfree(ubuf->pages);
>>        kvfree(ubuf->folios);
>>        kfree(ubuf);
>>        return ret;
>> --
>> 2.45.2

Re: [PATCH v2 1/4] udmabuf: cancel mmap page fault, direct map it

[Christian König]

Am 12.08.24 um 04:49 schrieb Huan Yang:
>
> 在 2024/8/10 9:28, Kasireddy, Vivek 写道:
>> [Some people who received this message don't often get email from 
>> vivek.kasireddy@intel.com. Learn why this is important at 
>> https://aka.ms/LearnAboutSenderIdentification ]
>>
>> Hi Huan,
>>
>>> The current udmabuf mmap uses a page fault mechanism to populate the
>>> vma.
>>>
>>> However, the current udmabuf has already obtained and pinned the folio
>>> upon completion of the creation.This means that the physical memory has
>>> already been acquired, rather than being accessed dynamically. The
>>> current page fault method only saves some page table memory.
>>>
>>> As a result, the page fault mechanism has lost its purpose as a 
>>> demanding
>>> page. Due to the fact that page fault requires trapping into kernel 
>>> mode
>>> and filling in when accessing the corresponding virtual address in 
>>> mmap,
>>> this means that user mode access to virtual addresses needs to trap 
>>> into
>>> kernel mode.
>>>
>>> Therefore, when creating a large size udmabuf, this represents a
>>> considerable overhead.
>>>
>>> The current patch removes the page fault method of mmap and
>>> instead fills it directly when mmap is triggered.
>> I think it makes sense to populate the vma when the first fault is 
>> triggered
>> instead of doing it during mmap. This is because the userspace may call
>> mmap but does not actually use the data. Qemu works this way 
>> depending on
> Yes, the idea of this is also related to the concept of page fault.
>
> However, the folio has already been pinned during creation. I think 
> using the page fault
>
> again is theoretically sound, but it may not save memory, only 
> increase context switch overhead.

This is not about saving memory but rather correctness and desired handling.

A mmap() operation is for creating the VMA and *not* filling the page 
tables. That might work but is not really a desired approach.

Regards,
Christian.

>
>
>> whether opengl is available in the environment or not.
>>
>>> Signed-off-by: Huan Yang <link@vivo.com>
>>> ---
>>>   drivers/dma-buf/udmabuf.c | 39 
>>> ++++++++++++++++-----------------------
>>>   1 file changed, 16 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c
>>> index 047c3cd2ceff..475268d4ebb1 100644
>>> --- a/drivers/dma-buf/udmabuf.c
>>> +++ b/drivers/dma-buf/udmabuf.c
>>> @@ -38,36 +38,29 @@ struct udmabuf_folio {
>>>        struct list_head list;
>>>   };
>>>
>>> -static vm_fault_t udmabuf_vm_fault(struct vm_fault *vmf)
>>> -{
>>> -     struct vm_area_struct *vma = vmf->vma;
>>> -     struct udmabuf *ubuf = vma->vm_private_data;
>>> -     pgoff_t pgoff = vmf->pgoff;
>>> -     unsigned long pfn;
>>> -
>>> -     if (pgoff >= ubuf->pagecount)
>>> -             return VM_FAULT_SIGBUS;
>>> -
>>> -     pfn = folio_pfn(ubuf->folios[pgoff]);
>>> -     pfn += ubuf->offsets[pgoff] >> PAGE_SHIFT;
>>> -
>>> -     return vmf_insert_pfn(vma, vmf->address, pfn);
>>> -}
>>> -
>>> -static const struct vm_operations_struct udmabuf_vm_ops = {
>>> -     .fault = udmabuf_vm_fault,
>>> -};
>>> -
>>>   static int mmap_udmabuf(struct dma_buf *buf, struct vm_area_struct
>>> *vma)
>>>   {
>>>        struct udmabuf *ubuf = buf->priv;
>>> +     unsigned long addr;
>>> +     unsigned long end;
>>> +     unsigned long pgoff;
>>> +     int ret;
>>>
>>>        if ((vma->vm_flags & (VM_SHARED | VM_MAYSHARE)) == 0)
>>>                return -EINVAL;
>>>
>>> -     vma->vm_ops = &udmabuf_vm_ops;
>>> -     vma->vm_private_data = ubuf;
>>> -     vm_flags_set(vma, VM_PFNMAP | VM_DONTEXPAND |
>>> VM_DONTDUMP);
>>> +     for (pgoff = vma->vm_pgoff, end = vma->vm_end, addr = vma-
>>>> vm_start;
>>> +          addr < end; pgoff++, addr += PAGE_SIZE) {
>>> +             struct page *page =
>>> +                     folio_page(ubuf->folios[pgoff],
>>> +                                ubuf->offsets[pgoff] >> PAGE_SHIFT);
>> Please don't use struct page pointers, given the recent conversion to 
>> use
>> only folios in udmabuf driver. I think what you are trying to do 
>> above can
>> be done using only folios.
> Yes, just use pfn. Consider of HVO, must use this.
>>
>>> +
>>> +             ret = remap_pfn_range(vma, addr, page_to_pfn(page),
>>> PAGE_SIZE,
>>> +                                   vma->vm_page_prot);
>> Could you please retain the use of vmf_insert_pfn() here, given the 
>> simplicity,
>> among other reasons?
> I will make the correction.
>
> Thanks.
>>
>> Thanks,
>> Vivek
>>
>>> +             if (ret)
>>> +                     return ret;
>>> +     }
>>> +
>>>        return 0;
>>>   }
>>>
>>> -- 
>>> 2.45.2