[PATCH v1 0/3] udmabuf: Add support for page migration out of movable zone or CMA

[PATCH v1 0/3] udmabuf: Add support for page migration out of movable zone or CMA

[Vivek Kasireddy]

This patch series adds support for migrating pages associated with
a udmabuf out of the movable zone or CMA to avoid breaking features
such as memory hotunplug.

The first patch exports check_and_migrate_movable_pages() function
out of GUP so that the udmabuf driver can leverage it for page
migration that is done as part of the second patch. The last patch
adds two new udmabuf selftests to verify data coherency after
page migration.

Cc: David Hildenbrand <david@redhat.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Mike Kravetz <mike.kravetz@oracle.com>
Cc: Hugh Dickins <hughd@google.com>
Cc: Peter Xu <peterx@redhat.com>
Cc: Jason Gunthorpe <jgg@nvidia.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Dongwon Kim <dongwon.kim@intel.com>
Cc: Junxiao Chang <junxiao.chang@intel.com>

Vivek Kasireddy (3):
  mm/gup: Export check_and_migrate_movable_pages()
  udmabuf: Add support for page migration out of movable zone or CMA
  selftests/dma-buf/udmabuf: Add tests to verify data after page
    migration

 drivers/dma-buf/udmabuf.c                     | 106 +++++++++++-
 include/linux/mm.h                            |   2 +
 mm/gup.c                                      |   9 +-
 .../selftests/drivers/dma-buf/udmabuf.c       | 151 +++++++++++++++++-
 4 files changed, 254 insertions(+), 14 deletions(-)

-- 
2.39.2

[PATCH v1 1/3] mm/gup: Export check_and_migrate_movable_pages()

[Vivek Kasireddy]

For drivers that would like to migrate pages out of the movable
zone (or CMA) in order to pin them (longterm) for DMA, using
check_and_migrate_movable_pages() directly provides a convenient
option instead of duplicating similar checks (e.g, checking
the folios for zone, hugetlb, etc) and calling migrate_pages()
directly.

Ideally, a driver is expected to call pin_user_pages(FOLL_LONGTERM)
to migrate and pin the pages for longterm DMA but there are
situations where the GUP APIs cannot be used directly for
various reasons (e.g, when the VMA or start addr cannot be
easily determined but the relevant pages are available).

Cc: David Hildenbrand <david@redhat.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Mike Kravetz <mike.kravetz@oracle.com>
Cc: Hugh Dickins <hughd@google.com>
Cc: Peter Xu <peterx@redhat.com>
Cc: Jason Gunthorpe <jgg@nvidia.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Dongwon Kim <dongwon.kim@intel.com>
Cc: Junxiao Chang <junxiao.chang@intel.com>
Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
---
 include/linux/mm.h | 2 ++
 mm/gup.c           | 9 +++++----
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index 406ab9ea818f..81871ffd3ff9 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -1547,6 +1547,8 @@ void unpin_user_pages_dirty_lock(struct page **pages, unsigned long npages,
 void unpin_user_page_range_dirty_lock(struct page *page, unsigned long npages,
 				      bool make_dirty);
 void unpin_user_pages(struct page **pages, unsigned long npages);
+long check_and_migrate_movable_pages(unsigned long nr_pages,
+				     struct page **pages);
 
 static inline bool is_cow_mapping(vm_flags_t flags)
 {
diff --git a/mm/gup.c b/mm/gup.c
index 76d222ccc3ff..18beda89fcf3 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -2141,8 +2141,8 @@ static int migrate_longterm_unpinnable_pages(
  * If everything is OK and all pages in the range are allowed to be pinned, then
  * this routine leaves all pages pinned and returns zero for success.
  */
-static long check_and_migrate_movable_pages(unsigned long nr_pages,
-					    struct page **pages)
+long check_and_migrate_movable_pages(unsigned long nr_pages,
+				     struct page **pages)
 {
 	unsigned long collected;
 	LIST_HEAD(movable_page_list);
@@ -2156,12 +2156,13 @@ static long check_and_migrate_movable_pages(unsigned long nr_pages,
 						pages);
 }
 #else
-static long check_and_migrate_movable_pages(unsigned long nr_pages,
-					    struct page **pages)
+long check_and_migrate_movable_pages(unsigned long nr_pages,
+				     struct page **pages)
 {
 	return 0;
 }
 #endif /* CONFIG_MIGRATION */
+EXPORT_SYMBOL(check_and_migrate_movable_pages);
 
 /*
  * __gup_longterm_locked() is a wrapper for __get_user_pages_locked which
-- 
2.39.2

[PATCH v1 2/3] udmabuf: Add support for page migration out of movable zone or CMA

[Vivek Kasireddy]

Since udmabuf could potentially pin pages that may reside in the
movable zone or CMA and thereby break features such as memory
hotunplug, it makes sense to migrate the pages out of these
areas. In order to accomplish this, we note the mapping and the
index of each page and then call check_and_migrate_movable_pages().

As check_and_migrate_movable_pages() unpins all the pages (and
also replaces the migrated pages in the mapping) upon successful
migration, we need to retrieve all the pages from their associated
mapping using the index we noted down earlier and re-pin them again.

Cc: David Hildenbrand <david@redhat.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Mike Kravetz <mike.kravetz@oracle.com>
Cc: Hugh Dickins <hughd@google.com>
Cc: Peter Xu <peterx@redhat.com>
Cc: Jason Gunthorpe <jgg@nvidia.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Dongwon Kim <dongwon.kim@intel.com>
Cc: Junxiao Chang <junxiao.chang@intel.com>
Suggested-by: David Hildenbrand <david@redhat.com>
Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
---
 drivers/dma-buf/udmabuf.c | 106 +++++++++++++++++++++++++++++++++++---
 1 file changed, 100 insertions(+), 6 deletions(-)

diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c
index 1a41c4a069ea..63912c73d122 100644
--- a/drivers/dma-buf/udmabuf.c
+++ b/drivers/dma-buf/udmabuf.c
@@ -30,6 +30,12 @@ struct udmabuf {
 	struct sg_table *sg;
 	struct miscdevice *device;
 	pgoff_t *subpgoff;
+	struct udmabuf_backing_info *backing;
+};
+
+struct udmabuf_backing_info {
+	struct address_space *mapping;
+	pgoff_t mapidx;
 };
 
 static vm_fault_t udmabuf_vm_fault(struct vm_fault *vmf)
@@ -156,8 +162,10 @@ static void release_udmabuf(struct dma_buf *buf)
 		put_sg_table(dev, ubuf->sg, DMA_BIDIRECTIONAL);
 
 	for (pg = 0; pg < ubuf->pagecount; pg++)
-		put_page(ubuf->pages[pg]);
+		unpin_user_page(ubuf->pages[pg]);
+
 	kfree(ubuf->subpgoff);
+	kfree(ubuf->backing);
 	kfree(ubuf->pages);
 	kfree(ubuf);
 }
@@ -211,6 +219,76 @@ static const struct dma_buf_ops udmabuf_ops = {
 #define SEALS_WANTED (F_SEAL_SHRINK)
 #define SEALS_DENIED (F_SEAL_WRITE)
 
+static int udmabuf_pin_pages(struct udmabuf *ubuf)
+{
+	struct address_space *mapping;
+	struct folio *folio;
+	struct page *page;
+	pgoff_t pg, mapidx;
+	int ret;
+
+	for (pg = 0; pg < ubuf->pagecount; pg++) {
+		mapping = ubuf->backing[pg].mapping;
+		mapidx = ubuf->backing[pg].mapidx;
+
+		if (!ubuf->pages[pg]) {
+			page = find_get_page_flags(mapping, mapidx,
+						   FGP_ACCESSED);
+			if (!page) {
+				if (!shmem_mapping(mapping)) {
+					ret = -EINVAL;
+					goto err;
+				}
+
+				page = shmem_read_mapping_page(mapping,
+							       mapidx);
+				if (IS_ERR(page)) {
+					ret = PTR_ERR(page);
+					goto err;
+				}
+			}
+			ubuf->pages[pg] = page;
+		}
+
+		folio = page_folio(ubuf->pages[pg]);
+		if (folio_test_large(folio))
+			atomic_add(1, &folio->_pincount);
+		else
+			folio_ref_add(folio, GUP_PIN_COUNTING_BIAS);
+
+		/* Since we are doing the equivalent of FOLL_PIN above, we can
+		 * go ahead and release our (udmabuf) reference on the pages.
+		 * Otherwise, migrate_pages() will fail as it doesn't like the
+		 * extra reference.
+		 */
+		put_page(ubuf->pages[pg]);
+	}
+	return 0;
+
+err:
+	while (pg > 0 && ubuf->pages[--pg]) {
+		unpin_user_page(ubuf->pages[pg]);
+		ubuf->pages[pg] = NULL;
+	}
+	return ret;
+}
+
+static long udmabuf_migrate_pages(struct udmabuf *ubuf)
+{
+	long ret;
+
+	do {
+		ret = udmabuf_pin_pages(ubuf);
+		if (ret < 0)
+			break;
+
+		ret = check_and_migrate_movable_pages(ubuf->pagecount,
+						      ubuf->pages);
+	} while (ret == -EAGAIN);
+
+	return ret;
+}
+
 static long udmabuf_create(struct miscdevice *device,
 			   struct udmabuf_create_list *head,
 			   struct udmabuf_create_item *list)
@@ -224,7 +302,8 @@ static long udmabuf_create(struct miscdevice *device,
 	struct page *page, *hpage = NULL;
 	pgoff_t mapidx, chunkoff, maxchunks;
 	struct hstate *hpstate;
-	int seals, ret = -EINVAL;
+	long ret = -EINVAL;
+	int seals;
 	u32 i, flags;
 
 	ubuf = kzalloc(sizeof(*ubuf), GFP_KERNEL);
@@ -252,6 +331,13 @@ static long udmabuf_create(struct miscdevice *device,
 		goto err;
 	}
 
+	ubuf->backing = kmalloc_array(ubuf->pagecount, sizeof(*ubuf->backing),
+				      GFP_KERNEL);
+	if (!ubuf->backing) {
+		ret = -ENOMEM;
+		goto err;
+	}
+
 	pgbuf = 0;
 	for (i = 0; i < head->count; i++) {
 		ret = -EBADFD;
@@ -298,7 +384,8 @@ static long udmabuf_create(struct miscdevice *device,
 				}
 				get_page(hpage);
 				ubuf->pages[pgbuf] = hpage;
-				ubuf->subpgoff[pgbuf++] = chunkoff << PAGE_SHIFT;
+				ubuf->subpgoff[pgbuf] = chunkoff << PAGE_SHIFT;
+				ubuf->backing[pgbuf].mapidx = mapidx;
 				if (++chunkoff == maxchunks) {
 					put_page(hpage);
 					hpage = NULL;
@@ -312,8 +399,10 @@ static long udmabuf_create(struct miscdevice *device,
 					ret = PTR_ERR(page);
 					goto err;
 				}
-				ubuf->pages[pgbuf++] = page;
+				ubuf->pages[pgbuf] = page;
+				ubuf->backing[pgbuf].mapidx = mapidx;
 			}
+			ubuf->backing[pgbuf++].mapping = mapping;
 		}
 		fput(memfd);
 		memfd = NULL;
@@ -323,6 +412,10 @@ static long udmabuf_create(struct miscdevice *device,
 		}
 	}
 
+	ret = udmabuf_migrate_pages(ubuf);
+	if (ret < 0)
+		goto err;
+
 	exp_info.ops  = &udmabuf_ops;
 	exp_info.size = ubuf->pagecount << PAGE_SHIFT;
 	exp_info.priv = ubuf;
@@ -341,11 +434,12 @@ static long udmabuf_create(struct miscdevice *device,
 	return dma_buf_fd(buf, flags);
 
 err:
-	while (pgbuf > 0)
-		put_page(ubuf->pages[--pgbuf]);
+	while (pgbuf > 0 && ubuf->pages[--pgbuf])
+		put_page(ubuf->pages[pgbuf]);
 	if (memfd)
 		fput(memfd);
 	kfree(ubuf->subpgoff);
+	kfree(ubuf->backing);
 	kfree(ubuf->pages);
 	kfree(ubuf);
 	return ret;
-- 
2.39.2

[PATCH v1 3/3] selftests/dma-buf/udmabuf: Add tests to verify data after page migration

[Vivek Kasireddy]

Since the memfd pages associated with a udmabuf may be migrated
as part of udmabuf create, we need to verify the data coherency
after successful migration. The new tests added in this patch try
to do just that using 4k sized pages and also 2 MB sized huge
pages for the memfd.

Successful completion of the tests would mean that there is no
disconnect between the memfd pages and the ones associated with
a udmabuf. And, these tests can also be augmented in the future
to test newer udmabuf features (such as handling memfd hole punch).

Cc: Shuah Khan <shuah@kernel.org>
Cc: David Hildenbrand <david@redhat.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Mike Kravetz <mike.kravetz@oracle.com>
Cc: Hugh Dickins <hughd@google.com>
Cc: Peter Xu <peterx@redhat.com>
Cc: Jason Gunthorpe <jgg@nvidia.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Dongwon Kim <dongwon.kim@intel.com>
Cc: Junxiao Chang <junxiao.chang@intel.com>
Based-on-patch-by: Mike Kravetz <mike.kravetz@oracle.com>
Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
---
 .../selftests/drivers/dma-buf/udmabuf.c       | 151 +++++++++++++++++-
 1 file changed, 147 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/drivers/dma-buf/udmabuf.c b/tools/testing/selftests/drivers/dma-buf/udmabuf.c
index c812080e304e..d76c813fe652 100644
--- a/tools/testing/selftests/drivers/dma-buf/udmabuf.c
+++ b/tools/testing/selftests/drivers/dma-buf/udmabuf.c
@@ -9,26 +9,132 @@
 #include <errno.h>
 #include <fcntl.h>
 #include <malloc.h>
+#include <stdbool.h>
 
 #include <sys/ioctl.h>
 #include <sys/syscall.h>
+#include <sys/mman.h>
 #include <linux/memfd.h>
 #include <linux/udmabuf.h>
 
 #define TEST_PREFIX	"drivers/dma-buf/udmabuf"
 #define NUM_PAGES       4
+#define NUM_ENTRIES     4
+#define MEMFD_SIZE      1024 /* in pages */
 
-static int memfd_create(const char *name, unsigned int flags)
+static unsigned int page_size;
+
+static int create_memfd_with_seals(off64_t size, bool hpage)
+{
+	int memfd, ret;
+	unsigned int flags = MFD_ALLOW_SEALING;
+
+	if (hpage)
+		flags |= MFD_HUGETLB;
+
+	memfd = memfd_create("udmabuf-test", flags);
+	if (memfd < 0) {
+		printf("%s: [skip,no-memfd]\n", TEST_PREFIX);
+		exit(77);
+	}
+
+	ret = fcntl(memfd, F_ADD_SEALS, F_SEAL_SHRINK);
+	if (ret < 0) {
+		printf("%s: [skip,fcntl-add-seals]\n", TEST_PREFIX);
+		exit(77);
+	}
+
+	ret = ftruncate(memfd, size);
+	if (ret == -1) {
+		printf("%s: [FAIL,memfd-truncate]\n", TEST_PREFIX);
+		exit(1);
+	}
+
+	return memfd;
+}
+
+static int create_udmabuf_list(int devfd, int memfd, off64_t memfd_size)
+{
+	struct udmabuf_create_list *list;
+	int ubuf_fd, i;
+
+	list = malloc(sizeof(struct udmabuf_create_list) +
+		      sizeof(struct udmabuf_create_item) * NUM_ENTRIES);
+	if (!list) {
+		printf("%s: [FAIL, udmabuf-malloc]\n", TEST_PREFIX);
+		exit(1);
+	}
+
+	for (i = 0; i < NUM_ENTRIES; i++) {
+		list->list[i].memfd  = memfd;
+		list->list[i].offset = i * (memfd_size / NUM_ENTRIES);
+		list->list[i].size   = getpagesize() * NUM_PAGES;
+	}
+
+	list->count = NUM_ENTRIES;
+	list->flags = UDMABUF_FLAGS_CLOEXEC;
+	ubuf_fd = ioctl(devfd, UDMABUF_CREATE_LIST, list);
+	free(list);
+	if (ubuf_fd < 0) {
+		printf("%s: [FAIL, udmabuf-create]\n", TEST_PREFIX);
+		exit(1);
+	}
+
+	return ubuf_fd;
+}
+
+static void write_to_memfd(void *addr, off64_t size, char chr)
+{
+	int i;
+
+	for (i = 0; i < size / page_size; i++) {
+		*((char *)addr + (i * page_size)) = chr;
+	}
+}
+
+static void *mmap_fd(int fd, off64_t size)
 {
-	return syscall(__NR_memfd_create, name, flags);
+	void *addr;
+
+	addr = mmap(NULL, size, PROT_READ|PROT_WRITE, MAP_SHARED, fd, 0);
+	if (addr == MAP_FAILED) {
+		printf("%s: ubuf_fd mmap fail\n", TEST_PREFIX);
+		exit(1);
+	}
+
+	return addr;
+}
+
+static int compare_chunks(void *addr1, void *addr2, off64_t memfd_size)
+{
+	off64_t off;
+	int i = 0, j, k = 0, ret = 0;
+	char char1, char2;
+
+	while (i < NUM_ENTRIES) {
+		off = i * (memfd_size / NUM_ENTRIES);
+		for (j = 0; j < NUM_PAGES; j++, k++) {
+			char1 = *((char *)addr1 + off + (j * getpagesize()));
+			char2 = *((char *)addr2 + (k * getpagesize()));
+			if (char1 != char2) {
+				ret = -1;
+				goto err;
+			}
+		}
+		i++;
+	}
+err:
+	munmap(addr1, memfd_size);
+	munmap(addr2, NUM_ENTRIES * NUM_PAGES * getpagesize());
+	return ret;
 }
 
 int main(int argc, char *argv[])
 {
 	struct udmabuf_create create;
 	int devfd, memfd, buf, ret;
-	off_t size;
-	void *mem;
+	off64_t size;
+	void *addr1, *addr2;
 
 	devfd = open("/dev/udmabuf", O_RDWR);
 	if (devfd < 0) {
@@ -90,6 +196,9 @@ int main(int argc, char *argv[])
 	}
 
 	/* should work */
+	page_size = getpagesize();
+	addr1 = mmap_fd(memfd, size);
+	write_to_memfd(addr1, size, 'a');
 	create.memfd  = memfd;
 	create.offset = 0;
 	create.size   = size;
@@ -98,6 +207,40 @@ int main(int argc, char *argv[])
 		printf("%s: [FAIL,test-4]\n", TEST_PREFIX);
 		exit(1);
 	}
+	munmap(addr1, size);
+	close(buf);
+	close(memfd);
+
+	/* should work (migration of 4k size pages)*/
+	size = MEMFD_SIZE * page_size;
+	memfd = create_memfd_with_seals(size, false);
+	addr1 = mmap_fd(memfd, size);
+	write_to_memfd(addr1, size, 'a');
+	buf = create_udmabuf_list(devfd, memfd, size);
+	addr2 = mmap_fd(buf, NUM_PAGES * NUM_ENTRIES * getpagesize());
+	write_to_memfd(addr1, size, 'b');
+	ret = compare_chunks(addr1, addr2, size);
+	if (ret < 0) {
+		printf("%s: [FAIL,test-5]\n", TEST_PREFIX);
+		exit(1);
+	}
+	close(buf);
+	close(memfd);
+
+	/* should work (migration of 2MB size huge pages)*/
+	page_size = getpagesize() * 512; /* 2 MB */
+	size = MEMFD_SIZE * page_size;
+	memfd = create_memfd_with_seals(size, true);
+	addr1 = mmap_fd(memfd, size);
+	write_to_memfd(addr1, size, 'a');
+	buf = create_udmabuf_list(devfd, memfd, size);
+	addr2 = mmap_fd(buf, NUM_PAGES * NUM_ENTRIES * getpagesize());
+	write_to_memfd(addr1, size, 'b');
+	ret = compare_chunks(addr1, addr2, size);
+	if (ret < 0) {
+		printf("%s: [FAIL,test-6]\n", TEST_PREFIX);
+		exit(1);
+	}
 
 	fprintf(stderr, "%s: ok\n", TEST_PREFIX);
 	close(buf);
-- 
2.39.2

Re: [PATCH v1 0/3] udmabuf: Add support for page migration out of movable zone or CMA

[Jason Gunthorpe]

On Wed, Aug 16, 2023 at 11:49:31PM -0700, Vivek Kasireddy wrote:
> This patch series adds support for migrating pages associated with
> a udmabuf out of the movable zone or CMA to avoid breaking features
> such as memory hotunplug.
> 
> The first patch exports check_and_migrate_movable_pages() function
> out of GUP so that the udmabuf driver can leverage it for page
> migration that is done as part of the second patch. The last patch
> adds two new udmabuf selftests to verify data coherency after
> page migration.

Please don't do this. If you want to do what GUP does then call
GUP. udmabuf is not so special that it needs to open code its own
weird version of it.

Jason

RE: [PATCH v1 0/3] udmabuf: Add support for page migration out of movable zone or CMA

[Kasireddy, Vivek]

Hi Jason,

> > This patch series adds support for migrating pages associated with
> > a udmabuf out of the movable zone or CMA to avoid breaking features
> > such as memory hotunplug.
> >
> > The first patch exports check_and_migrate_movable_pages() function
> > out of GUP so that the udmabuf driver can leverage it for page
> > migration that is done as part of the second patch. The last patch
> > adds two new udmabuf selftests to verify data coherency after
> > page migration.
> 
> Please don't do this. If you want to do what GUP does then call
> GUP. udmabuf is not so special that it needs to open code its own
> weird version of it.
We can't call GUP directly as explained in the first patch of this series:
"For drivers that would like to migrate pages out of the movable
zone (or CMA) in order to pin them (longterm) for DMA, using
check_and_migrate_movable_pages() directly provides a convenient
option instead of duplicating similar checks (e.g, checking
the folios for zone, hugetlb, etc) and calling migrate_pages()
directly.

Ideally, a driver is expected to call pin_user_pages(FOLL_LONGTERM)
to migrate and pin the pages for longterm DMA but there are
situations where the GUP APIs cannot be used directly for
various reasons (e.g, when the VMA or start addr cannot be
easily determined but the relevant pages are available)."

Given the current (model and) UAPI (udmabuf_create), the userspace
only shares (memfd, offset, size) values that we use to find the 
relevant pages and pin them (by doing get_page()). Since the goal
is to also migrate these pages, I think we have the following options:
- Leverage check_and_migrate_movable_pages(); but this function
  needs to be exported from GUP.

- Iterate over all the pages (in udmabuf) to check for folio_is_longterm_pinnable()
  and call migrate_pages() eventually. This requires changes only to
  the udmabuf driver but we'd be duplicating much of the functionality
  provided by check_and_migrate_movable_pages().

- Call pin_user_pages_fast(FOLL_LONGTERM) from udmabuf driver. In
  order to do this, we have to first unpin all pages and iterate over all
  the VMAs of the VMM to identify the Guest RAM VMA and then use
  page_address_in_vma() to find the start addr of the ranges and then
  call GUP. Although this approach is feasible, it feels a bit convoluted.

- Add a new udmabuf UAPI to have userspace share (start addr, len) values
  so that the udmabuf driver can directly call GUP APIs. But this means all
  the current users of udmabuf such as Qemu, CrosVM, etc, need to be
  updated to use the new UAPI. 

- Add a new API to the backing store/allocator to longterm-pin the page.
  For example, something along the lines of shmem_pin_mapping_page_longterm()
  for shmem as suggested by Daniel. A similar one needs to be added for
  hugetlbfs as well.

Among these options, the first one seems very reasonable to me.

Thanks,
Vivek

> 
> Jason

Re: [PATCH v1 0/3] udmabuf: Add support for page migration out of movable zone or CMA

[Jason Gunthorpe]

On Tue, Aug 22, 2023 at 05:36:56AM +0000, Kasireddy, Vivek wrote:
> Hi Jason,
> 
> > > This patch series adds support for migrating pages associated with
> > > a udmabuf out of the movable zone or CMA to avoid breaking features
> > > such as memory hotunplug.
> > >
> > > The first patch exports check_and_migrate_movable_pages() function
> > > out of GUP so that the udmabuf driver can leverage it for page
> > > migration that is done as part of the second patch. The last patch
> > > adds two new udmabuf selftests to verify data coherency after
> > > page migration.
> > 
> > Please don't do this. If you want to do what GUP does then call
> > GUP. udmabuf is not so special that it needs to open code its own
> > weird version of it.
> We can't call GUP directly as explained in the first patch of this series:
> "For drivers that would like to migrate pages out of the movable
> zone (or CMA) in order to pin them (longterm) for DMA, using
> check_and_migrate_movable_pages() directly provides a convenient
> option instead of duplicating similar checks (e.g, checking
> the folios for zone, hugetlb, etc) and calling migrate_pages()
> directly.
> 
> Ideally, a driver is expected to call pin_user_pages(FOLL_LONGTERM)
> to migrate and pin the pages for longterm DMA but there are
> situations where the GUP APIs cannot be used directly for
> various reasons (e.g, when the VMA or start addr cannot be
> easily determined but the relevant pages are available)."
> 
> Given the current (model and) UAPI (udmabuf_create), the userspace
> only shares (memfd, offset, size) values that we use to find the
> relevant pages and pin them (by doing get_page()). Since the goal
> is to also migrate these pages, I think we have the following options:

This seems like a bad choice of uAPI - we don't have any kernel
support for pinning from a memfd. If you want this then you have to
build this as generic code, not open code it into udmabuf.

> - Leverage check_and_migrate_movable_pages(); but this function
>   needs to be exported from GUP.

GUP has many behaviors, we keep adding more, these functions should
not leak out of the mm core code into drivers.
 
> - Iterate over all the pages (in udmabuf) to check for folio_is_longterm_pinnable()
>   and call migrate_pages() eventually. This requires changes only to
>   the udmabuf driver but we'd be duplicating much of the functionality
>   provided by check_and_migrate_movable_pages().

Definately not

> - Call pin_user_pages_fast(FOLL_LONGTERM) from udmabuf driver. In
>   order to do this, we have to first unpin all pages and iterate over all
>   the VMAs of the VMM to identify the Guest RAM VMA and then use
>   page_address_in_vma() to find the start addr of the ranges and then
>   call GUP. Although this approach is feasible, it feels a bit convoluted.

Userspace should have told the kernel where the memfd is mapped.
 
> - Add a new udmabuf UAPI to have userspace share (start addr, len) values
>   so that the udmabuf driver can directly call GUP APIs. But this means all
>   the current users of udmabuf such as Qemu, CrosVM, etc, need to be
>   updated to use the new UAPI. 

There you go
 
> - Add a new API to the backing store/allocator to longterm-pin the page.
>   For example, something along the lines of shmem_pin_mapping_page_longterm()
>   for shmem as suggested by Daniel. A similar one needs to be added for
>   hugetlbfs as well.

This may also be reasonable.

Jason

Re: [PATCH v1 0/3] udmabuf: Add support for page migration out of movable zone or CMA

[David Hildenbrand]

>> - Add a new API to the backing store/allocator to longterm-pin the page.
>>    For example, something along the lines of shmem_pin_mapping_page_longterm()
>>    for shmem as suggested by Daniel. A similar one needs to be added for
>>    hugetlbfs as well.
> 
> This may also be reasonable.

Sounds reasonable to keep the old API (that we unfortunately have) working.

-- 
Cheers,

David / dhildenb

RE: [PATCH v1 0/3] udmabuf: Add support for page migration out of movable zone or CMA

[Kasireddy, Vivek]

Hi David,

> 
> >> - Add a new API to the backing store/allocator to longterm-pin the page.
> >>    For example, something along the lines of
> shmem_pin_mapping_page_longterm()
> >>    for shmem as suggested by Daniel. A similar one needs to be added for
> >>    hugetlbfs as well.
> >
> > This may also be reasonable.
> 
> Sounds reasonable to keep the old API (that we unfortunately have) working.
I agree; I'd like to avoid adding new APIs unless absolutely necessary. Given this,
and considering the options I have mentioned earlier, what would be your
recommendation for how page migration needs to be done in udmabuf driver?

Thanks,
Vivek

> 
> --
> Cheers,
> 
> David / dhildenb

Re: [PATCH v1 0/3] udmabuf: Add support for page migration out of movable zone or CMA

[David Hildenbrand]

On 24.08.23 08:31, Kasireddy, Vivek wrote:
> Hi David,
> 
>>
>>>> - Add a new API to the backing store/allocator to longterm-pin the page.
>>>>     For example, something along the lines of
>> shmem_pin_mapping_page_longterm()
>>>>     for shmem as suggested by Daniel. A similar one needs to be added for
>>>>     hugetlbfs as well.
>>>
>>> This may also be reasonable.
>>
>> Sounds reasonable to keep the old API (that we unfortunately have) working.
> I agree; I'd like to avoid adding new APIs unless absolutely necessary. Given this,
> and considering the options I have mentioned earlier, what would be your
> recommendation for how page migration needs to be done in udmabuf driver?

I guess using proper APIs for shmem and hugetlb. So, turning roughly 
what you have in patch#1 for now into common code, and only calling into 
that from udmabug.

-- 
Cheers,

David / dhildenb

Re: [PATCH v1 0/3] udmabuf: Add support for page migration out of movable zone or CMA

[Jason Gunthorpe]

On Thu, Aug 24, 2023 at 08:30:17PM +0200, David Hildenbrand wrote:
> On 24.08.23 08:31, Kasireddy, Vivek wrote:
> > Hi David,
> > 
> > > 
> > > > > - Add a new API to the backing store/allocator to longterm-pin the page.
> > > > >     For example, something along the lines of
> > > shmem_pin_mapping_page_longterm()
> > > > >     for shmem as suggested by Daniel. A similar one needs to be added for
> > > > >     hugetlbfs as well.
> > > > 
> > > > This may also be reasonable.
> > > 
> > > Sounds reasonable to keep the old API (that we unfortunately have) working.
> > I agree; I'd like to avoid adding new APIs unless absolutely necessary. Given this,
> > and considering the options I have mentioned earlier, what would be your
> > recommendation for how page migration needs to be done in udmabuf driver?
> 
> I guess using proper APIs for shmem and hugetlb. So, turning roughly what
> you have in patch#1 for now into common code, and only calling into that
> from udmabug.

This is a lot of work for an obscure uapi :\

Jason

Re: [PATCH v1 0/3] udmabuf: Add support for page migration out of movable zone or CMA

[David Hildenbrand]

On 24.08.23 20:30, Jason Gunthorpe wrote:
> On Thu, Aug 24, 2023 at 08:30:17PM +0200, David Hildenbrand wrote:
>> On 24.08.23 08:31, Kasireddy, Vivek wrote:
>>> Hi David,
>>>
>>>>
>>>>>> - Add a new API to the backing store/allocator to longterm-pin the page.
>>>>>>      For example, something along the lines of
>>>> shmem_pin_mapping_page_longterm()
>>>>>>      for shmem as suggested by Daniel. A similar one needs to be added for
>>>>>>      hugetlbfs as well.
>>>>>
>>>>> This may also be reasonable.
>>>>
>>>> Sounds reasonable to keep the old API (that we unfortunately have) working.
>>> I agree; I'd like to avoid adding new APIs unless absolutely necessary. Given this,
>>> and considering the options I have mentioned earlier, what would be your
>>> recommendation for how page migration needs to be done in udmabuf driver?
>>
>> I guess using proper APIs for shmem and hugetlb. So, turning roughly what
>> you have in patch#1 for now into common code, and only calling into that
>> from udmabug.
> 
> This is a lot of work for an obscure uapi :\

Well, what can we otherwise to to *not* break existing users? I'm not 
happy about this either.

Of course, we can come up with a new uapi, but we have to handle the old 
uapi somehow.

Sure, we can simply always fail when we detect ZONE_MOVABLE or 
MIGRATE_CMA. Maybe that keeps at least some use cases working.

-- 
Cheers,

David / dhildenb

Re: [PATCH v1 0/3] udmabuf: Add support for page migration out of movable zone or CMA

[Jason Gunthorpe]

On Thu, Aug 24, 2023 at 08:33:09PM +0200, David Hildenbrand wrote:

> Sure, we can simply always fail when we detect ZONE_MOVABLE or MIGRATE_CMA.
> Maybe that keeps at least some use cases working.

That seems fairly reasonable

Jason

RE: [PATCH v1 0/3] udmabuf: Add support for page migration out of movable zone or CMA

[Kasireddy, Vivek]

Hi Jason, David,

> 
> > Sure, we can simply always fail when we detect ZONE_MOVABLE or
> MIGRATE_CMA.
> > Maybe that keeps at least some use cases working.
> 
> That seems fairly reasonable
AFAICS, failing udmabuf_create() if we detect one or more pages are in
ZONE_MOVABLE or MIGRATE_CMA would not be a recoverable failure --
as it would result in the failure of Guest GUI (or compositor).

I think it makes sense to have a generic version of 
And, since check_and_migrate_movable_pages() is GUP-specific, would
it be ok to create a generic version of that (in mm/migrate.c) which can be
used by udmabuf and/or other drivers in the future?

Thanks,
Vivek

> 
> Jason

RE: [PATCH v1 0/3] udmabuf: Add support for page migration out of movable zone or CMA

[Kasireddy, Vivek]

Hi Jason, David,

> > > Sure, we can simply always fail when we detect ZONE_MOVABLE or
> > MIGRATE_CMA.
> > > Maybe that keeps at least some use cases working.
> >
> > That seems fairly reasonable
> AFAICS, failing udmabuf_create() if we detect one or more pages are in
> ZONE_MOVABLE or MIGRATE_CMA would not be a recoverable failure --
> as it would result in the failure of Guest GUI (or compositor).
> 
> I think it makes sense to have a generic version of
> And, since check_and_migrate_movable_pages() is GUP-specific, would
> it be ok to create a generic version of that (in mm/migrate.c) which can be
> used by udmabuf and/or other drivers in the future?
Sorry, I accidentally sent this earlier email before finishing it. 
What I meant to say is since the same situation (inadvertently pinning pages
in movable) may probably arise in the future with another driver, I think it makes
sense to have a generic (non-GUP) version of check_and_migrate_movable_pages()
available in migration.h that drivers can use to ensure that they don't break
memory hotunplug accidentally.

Thanks,
Vivek

> 
> Thanks,
> Vivek
> 
> >
> > Jason
> 

Re: [PATCH v1 0/3] udmabuf: Add support for page migration out of movable zone or CMA

[Jason Gunthorpe]

On Sun, Aug 27, 2023 at 07:05:59PM +0000, Kasireddy, Vivek wrote:
> Hi Jason, David,
> 
> > > > Sure, we can simply always fail when we detect ZONE_MOVABLE or
> > > MIGRATE_CMA.
> > > > Maybe that keeps at least some use cases working.
> > >
> > > That seems fairly reasonable
> > AFAICS, failing udmabuf_create() if we detect one or more pages are in
> > ZONE_MOVABLE or MIGRATE_CMA would not be a recoverable failure --
> > as it would result in the failure of Guest GUI (or compositor).

Yes, you can't use whatever this driver is while enabling MOVABLE or
CMA in your kernel boot.

> > I think it makes sense to have a generic version of
> > And, since check_and_migrate_movable_pages() is GUP-specific, would
> > it be ok to create a generic version of that (in mm/migrate.c) which can be
> > used by udmabuf and/or other drivers in the future?
> Sorry, I accidentally sent this earlier email before finishing it.
> What I meant to say is since the same situation (inadvertently pinning pages
> in movable) may probably arise in the future with another driver, 

Why?

It was a big mistake to design a uAPI around taking in a FD and
extracting pages from it, we don't have kernel infrastructure for
that, and code liek that does not belong outside the MM at all.

> I think it makes sense to have a generic (non-GUP) version of
> check_and_migrate_movable_pages() available in migration.h that
> drivers can use to ensure that they don't break memory hotunplug
> accidentally.

Definately not.

Either use the VMA and pin_user_pages(), or implement
pin_user_pages_fd() in core code.

Do not open code something wonky in drivers.

Jason

Re: [PATCH v1 0/3] udmabuf: Add support for page migration out of movable zone or CMA

[David Hildenbrand]

>> I think it makes sense to have a generic (non-GUP) version of
>> check_and_migrate_movable_pages() available in migration.h that
>> drivers can use to ensure that they don't break memory hotunplug
>> accidentally.
> 
> Definately not.
> 
> Either use the VMA and pin_user_pages(), or implement
> pin_user_pages_fd() in core code.
> 
> Do not open code something wonky in drivers.

Agreed. pin_user_pages_fd() might become relevant in the context of 
vfio/mdev + KVM gmem -- don't mmap guest memory but instead provide it 
via a special memfd to the kernel.

So there might be value in having such a core infrastructure.

-- 
Cheers,

David / dhildenb

RE: [PATCH v1 0/3] udmabuf: Add support for page migration out of movable zone or CMA

[Kasireddy, Vivek]

Hi David,

> >> I think it makes sense to have a generic (non-GUP) version of
> >> check_and_migrate_movable_pages() available in migration.h that
> >> drivers can use to ensure that they don't break memory hotunplug
> >> accidentally.
> >
> > Definately not.
> >
> > Either use the VMA and pin_user_pages(), or implement
> > pin_user_pages_fd() in core code.
> >
> > Do not open code something wonky in drivers.
> 
> Agreed. pin_user_pages_fd() might become relevant in the context of
> vfio/mdev + KVM gmem -- don't mmap guest memory but instead provide it
> via a special memfd to the kernel.
> 
> So there might be value in having such a core infrastructure.
Ok, I'll work on adding pin_user_pages_fd() soon.

Thanks,
Vivek
> 
> --
> Cheers,
> 
> David / dhildenb