Add dmabuf read/write via io_uring

Introduce a new file callback that allows creating long-term dma mapping. All necessary information together with a dmabuf will be passed in the second argument of type struct io_dmabuf_token, which will be defined in following patches.

Signed-off-by Pavel Begunkov <[email protected]>

---
 include/linux/fs.h | 2 ++
 1 file changed, 2 insertions(+)

include/linux/fs.h +2 -0

index b5b01bb22d12..c5558aab4628 100644
@@ -1920,6 +1920,7 @@ struct dir_context {
 
 struct io_uring_cmd;
 struct offset_ctx;
+struct io_dmabuf_token;
 
 typedef unsigned int __bitwise fop_flags_t;
 

@@ -1967,6 +1968,7 @@ struct file_operations {
 	int (*uring_cmd_iopoll)(struct io_uring_cmd *, struct io_comp_batch *,
 				unsigned int poll_flags);
 	int (*mmap_prepare)(struct vm_area_desc *);
+	int (*create_dmabuf_token)(struct file *, struct io_dmabuf_token *);
 } __randomize_layout;
 
 /* Supports async buffered reads */

Quoted 4 lines · Pavel Begunkov

Introduce a new file callback that allows creating long-term dma mapping. All necessary information together with a dmabuf will be passed in the second argument of type struct io_dmabuf_token, which will be defined in following patches.

Well first of all the naming is probably not the best. Maybe rather call that dma-buf attachment or context or mappping.

Then the patch should probably define the full interface and not just add the callback here and then the structure in a follow up patch.

Quoted 6 lines · Christian König

Quoted 4 lines · Pavel Begunkov

Introduce a new file callback that allows creating long-term dma mapping. All necessary information together with a dmabuf will be passed in the second argument of type struct io_dmabuf_token, which will be defined in following patches.

Quoted 1 line

Well first of all the naming is probably not the best. Maybe rather call that dma-buf attachment or context or mappping.

"Mapping" or "attachment" would be confusing as maps are created lazily together with struct io_dmabuf_map. I can name it create_dmabuf_ctx(), but I decided to use "token" not to collide with dmabuf terminology. e.g. I wouldn't be surprised to see some dmabuf ctx in the dmabuf implementation code. Maybe "*io_ctx" would be better.

Quoted 2 lines

Then the patch should probably define the full interface and not just add the callback here and then the structure in a follow up patch.

I strongly prefer splitting patches so that they touch one tree at a time whenever possible.

tbh, I don't see much of a problem it being
not defined as it's just forwarded in first patches, but I can shuffle it around in the series so that definitions come first.

Quoted 1 line

Then the patch should probably define the full interface and not just add the callback here and then the structure in a follow up patch.

I strongly prefer splitting patches so that they touch one tree at a time whenever possible. tbh, I don't see much of a problem it being not defined as it's just forwarded in first patches, but I can shuffle it around in the series so that definitions come first.

Introduce a new iterator type for dmabuf maps. The map in an opaque object with internals and format specific to the subsystem / driver, and only it can use that subsystem / driver for issuing IO. The task of the middle layers is to pass the map / iterator further down, maybe doing basic splitting and length checking. The iterator can only be used by operations of the file the associated map was created for.

Introduce a new iterator type for dmabuf maps. The map in an opaque object with internals and format specific to the subsystem / driver, and only it can use that subsystem / driver for issuing IO. The task of the middle layers is to pass the map / iterator further down, maybe doing basic splitting and length checking. The iterator can only be used by operations of the file the associated map was created for.

Suggested-by Keith Busch <[email protected]>

Signed-off-by Pavel Begunkov <[email protected]>

---
 include/linux/uio.h | 11 +++++++++++
 lib/iov_iter.c      | 29 +++++++++++++++++++++++------
 2 files changed, 34 insertions(+), 6 deletions(-)

include/linux/uio.h +11 -0

index a9bc5b3067e3..75051aed70de 100644
@@ -12,6 +12,7 @@
 
 struct page;
 struct folio_queue;
+struct io_dmabuf_map;
 
 typedef unsigned int __bitwise iov_iter_extraction_t;
 

@@ -29,6 +30,7 @@ enum iter_type {
 	ITER_FOLIOQ,
 	ITER_XARRAY,
 	ITER_DISCARD,
+	ITER_DMABUF_MAP,
 };
 
 #define ITER_SOURCE	1	// == WRITE

@@ -71,6 +73,7 @@ struct iov_iter {
 				const struct folio_queue *folioq;
 				struct xarray *xarray;
 				void __user *ubuf;
+				struct io_dmabuf_map *dmabuf_map;
 			};
 			size_t count;
 		};

@@ -155,6 +158,11 @@ static inline bool iov_iter_is_xarray(const struct iov_iter *i)
 	return iov_iter_type(i) == ITER_XARRAY;
 }
 
+static inline bool iov_iter_is_dmabuf_map(const struct iov_iter *i)
+{
+	return iov_iter_type(i) == ITER_DMABUF_MAP;
+}
+
 static inline unsigned char iov_iter_rw(const struct iov_iter *i)
 {
 	return i->data_source ? WRITE : READ;

@@ -300,6 +308,9 @@ void iov_iter_folio_queue(struct iov_iter *i, unsigned int direction,
 			  unsigned int first_slot, unsigned int offset, size_t count);
 void iov_iter_xarray(struct iov_iter *i, unsigned int direction, struct xarray *xarray,
 		     loff_t start, size_t count);
+void iov_iter_dmabuf_map(struct iov_iter *i, unsigned int direction,
+			struct io_dmabuf_map *map,
+			loff_t off, size_t count);
 ssize_t iov_iter_get_pages2(struct iov_iter *i, struct page **pages,
 			size_t maxsize, unsigned maxpages, size_t *start);
 ssize_t iov_iter_get_pages_alloc2(struct iov_iter *i, struct page ***pages,

lib/iov_iter.c +0 -0

index 243662af1af7..e2253684b991 100644

lib/iov_iter.c +20 -4

+	    iov_iter_is_dmabuf_map(i)) {
 		BUG(); /* We should never go beyond the start of the specified
 			* range since we might then be straying into pages that
 			* aren't pinned.

@@ -775,6 +777,20 @@ void iov_iter_xarray(struct iov_iter *i, unsigned int direction,
 }
 EXPORT_SYMBOL(iov_iter_xarray);
 
+void iov_iter_dmabuf_map(struct iov_iter *i, unsigned int direction,
+			 struct io_dmabuf_map *map,
+			 loff_t off, size_t count)
+{
+	WARN_ON(direction & ~(READ | WRITE));
+	*i = (struct iov_iter){
+		.iter_type = ITER_DMABUF_MAP,
+		.data_source = direction,
+		.dmabuf_map = map,
+		.count = count,
+		.iov_offset = off,
+	};
+}
+
 /**
  * iov_iter_discard - Initialise an I/O iterator that discards data
  * @i: The iterator to initialise.

@@ -841,7 +857,7 @@ static unsigned long iov_iter_alignment_bvec(const struct iov_iter *i)
 
 unsigned long iov_iter_alignment(const struct iov_iter *i)
 {
-	if (likely(iter_is_ubuf(i))) {
+	if (likely(iter_is_ubuf(i)) || iov_iter_is_dmabuf_map(i)) {
 		size_t size = i->count;
 		if (size)
 			return ((unsigned long)i->ubuf + i->iov_offset) | size;

@@ -872,7 +888,7 @@ unsigned long iov_iter_gap_alignment(const struct iov_iter *i)
 	size_t size = i->count;
 	unsigned k;
 
-	if (iter_is_ubuf(i))
+	if (iter_is_ubuf(i) || iov_iter_is_dmabuf_map(i))
 		return 0;
 
 	if (WARN_ON(!iter_is_iovec(i)))

@@ -1469,11 +1485,12 @@ EXPORT_SYMBOL_GPL(import_ubuf);
 void iov_iter_restore(struct iov_iter *i, struct iov_iter_state *state)
 {
 	if (WARN_ON_ONCE(!iov_iter_is_bvec(i) && !iter_is_iovec(i) &&
-			 !iter_is_ubuf(i)) && !iov_iter_is_kvec(i))
+			 !iter_is_ubuf(i) && !iov_iter_is_kvec(i) &&
+			 !iov_iter_is_dmabuf_map(i)))
 		return;
 	i->iov_offset = state->iov_offset;
 	i->count = state->count;
-	if (iter_is_ubuf(i))
+	if (iter_is_ubuf(i) || iov_iter_is_dmabuf_map(i))
 		return;
 	/*
 	 * For the *vec iters, nr_segs + iov is constant - if we increment

Quoted 5 lines

lib/iov_iter.c +0 -1

@@ -575,7 +575,8 @@ void iov_iter_advance(struct iov_iter *i, size_t size)
 {
 	if (unlikely(i->count < size))
 		size = i->count;
-	if (likely(iter_is_ubuf(i)) || unlikely(iov_iter_is_xarray(i))) {

lib/iov_iter.c void iov_iter_advance(struct iov_iter *i, size_t size) · line 579

Quoted 2 lines

lib/iov_iter.c +2 -0

+	if (likely(iter_is_ubuf(i)) || unlikely(iov_iter_is_xarray(i)) ||
+	    unlikely(iov_iter_is_dmabuf_map(i))) {  

Doesn't the extra check add more code to all the non-ubuf cases? This could be fixed by either making iter_type a bitmask (with one bit set) or writing an iter_is_one_of(i, ITER_xxx, ITER_yyy) define that uses '(1 << i->iter_type) & ((1 << ITER_xxx) | ...)'

On Wed, 13 May 2026 11:05:57 +0100
David Laight <[email protected]> wrote:

...

Quoted 11 lines

lib/iov_iter.c void iov_iter_advance(struct iov_iter *i, size_t size) · line 579

Quoted 7 lines

lib/iov_iter.c +2 -1

@@ -575,7 +575,8 @@ void iov_iter_advance(struct iov_iter *i, size_t size)
  {
  	if (unlikely(i->count < size))
  		size = i->count;
-	if (likely(iter_is_ubuf(i)) || unlikely(iov_iter_is_xarray(i))) {
+	if (likely(iter_is_ubuf(i)) || unlikely(iov_iter_is_xarray(i)) ||
+	    unlikely(iov_iter_is_dmabuf_map(i))) {

Doesn't the extra check add more code to all the non-ubuf cases? This could be fixed by either making iter_type a bitmask (with one bit set)

Quoted 2 lines

or writing an iter_is_one_of(i, ITER_xxx, ITER_yyy) define that uses '(1 << i->iter_type) & ((1 << ITER_xxx) | ...)'

This seems to DTRT:

#define _ITER_IS_ONE_OF(iter, t1, t2, t3, t4, t5, t6, t7, t8, ...) \
     ((1u << (iter)->iter_type) & ((1u << ITER_##t1) | (1u << ITER_##t2) | \
         (1u << ITER_##t3) | (1u << ITER_##t4) | (1u << ITER_##t5) | \
         (1u << ITER_##t6) | (1u << ITER_##t7) | (1u << ITER_##t8)))
#define ITER_IS_ONE_OF(iter, t, ...) \
     _ITER_IS_ONE_OF(iter, t, ## __VA_ARGS__, t, t, t, t, t, t, t)

Quoted 14 lines · David Laight

On Wed, 13 May 2026 11:05:57 +0100
David Laight <[email protected]> wrote:

...

Quoted 10 lines

lib/iov_iter.c void iov_iter_advance(struct iov_iter *i, size_t size) · line 579

Quoted 7 lines

lib/iov_iter.c +2 -1

@@ -575,7 +575,8 @@ void iov_iter_advance(struct iov_iter *i, size_t size)
  {
  	if (unlikely(i->count < size))
  		size = i->count;
-	if (likely(iter_is_ubuf(i)) || unlikely(iov_iter_is_xarray(i))) {
+	if (likely(iter_is_ubuf(i)) || unlikely(iov_iter_is_xarray(i)) ||
+	    unlikely(iov_iter_is_dmabuf_map(i))) {  

Doesn't the extra check add more code to all the non-ubuf cases?

Quoted 11 lines

Quoted 2 lines

or writing an iter_is_one_of(i, ITER_xxx, ITER_yyy) define that uses '(1 << i->iter_type) & ((1 << ITER_xxx) | ...)'

This seems to DTRT:

#define _ITER_IS_ONE_OF(iter, t1, t2, t3, t4, t5, t6, t7, t8, ...) \
     ((1u << (iter)->iter_type) & ((1u << ITER_##t1) | (1u << ITER_##t2) | \
         (1u << ITER_##t3) | (1u << ITER_##t4) | (1u << ITER_##t5) | \
         (1u << ITER_##t6) | (1u << ITER_##t7) | (1u << ITER_##t8)))
#define ITER_IS_ONE_OF(iter, t, ...) \
     _ITER_IS_ONE_OF(iter, t, ## __VA_ARGS__, t, t, t, t, t, t, t)  

We definitely don't want that, using them directly would've been much cleaner.

if (get_type_mask(i) & (TYPE1 | TYPE2)) ...

To quote Cristoph: "Historically __bio_clone itself does not clone the

payload, just the bio. But we got rid of the callers that want to clone a bio but not the payload long time ago". So let's move ->bi_io_vec assignment into __bio_clone(), so we have a single point where it's set.

Quoted 1 line · Pavel Begunkov

To quote Cristoph: "Historically __bio_clone itself does not clone the

It's Christoph.

include/linux/bio.h +7 -0

+static inline void bio_advance_iter_dmabuf_map(struct bvec_iter *iter,
+					       unsigned int bytes)
+{
+	iter->bi_bvec_done += bytes;
+	iter->bi_size -= bytes;
+}
+
 static inline void bio_advance_iter(const struct bio *bio,

block/bio.c static int __bio_clone(struct bio *bio, struct bio *bio_src, gfp_t gfp) · line 860

Quoted 6 lines

block/bio.c +6 -0

+	if (!bio_flagged(bio_src, BIO_DMABUF_MAP)) {
+		bio->bi_io_vec = bio_src->bi_io_vec;
+	} else {
+		bio->dmabuf_map = bio_src->dmabuf_map;
+		bio_set_flag(bio, BIO_DMABUF_MAP);
+	}

This is backwards, please avoid pointless negations:

	if (bio_flagged(bio_src, BIO_DMABUF_MAP)) {
		bio->dmabuf_map = bio_src->dmabuf_map;
		bio_set_flag(bio, BIO_DMABUF_MAP);
	} else {
		bio->bi_io_vec = bio_src->bi_io_vec;
	}

block/blk-merge.c int bio_split_io_at(struct bio *bio, const struct queue_limits *lim, · line 360

Quoted 10 lines

block/blk-merge.c +10 -0

+	if (bio_flagged(bio, BIO_DMABUF_MAP)) {
+		nsegs = 1;
+
+		if ((bio->bi_iter.bi_bvec_done & lim->dma_alignment) ||
+		    (bio->bi_iter.bi_size & len_align_mask))
+			return -EINVAL;
+		if (bio->bi_iter.bi_size > max_bytes) {
+			bytes = max_bytes;
+			goto split;
+		}

Please add a comment explaining why nsegs is always 1 here.

block/blk.h static inline struct bio *__bio_split_to_limits(struct bio *bio, · line 429

Quoted 8 lines

block/blk.h +2 -1

@@ -424,7 +424,8 @@ static inline struct bio *__bio_split_to_limits(struct bio *bio,
  	switch (bio_op(bio)) {
  	case REQ_OP_READ:
  	case REQ_OP_WRITE:
-		if (bio_may_need_split(bio, lim))
+		if (bio_may_need_split(bio, lim) ||
+		    bio_flagged(bio, BIO_DMABUF_MAP))
  			return bio_split_rw(bio, lim, nr_segs);

The BIO_DMABUF_MAP check should go into bio_may_need_split.

Quoted 8 lines

include/linux/bio.h +7 -0

+static inline void bio_advance_iter_dmabuf_map(struct bvec_iter *iter,
+					       unsigned int bytes)
+{
+	iter->bi_bvec_done += bytes;
+	iter->bi_size -= bytes;
+}
+
  static inline void bio_advance_iter(const struct bio *bio,

include/linux/bio.h static inline bool bio_next_segment(const struct bio *bio, · line 126

Quoted 10 lines

include/linux/bio.h +3 -2

  				    struct bvec_iter *iter, unsigned int bytes)
  {
  	iter->bi_sector += bytes >> 9;
 
-	if (bio_no_advance_iter(bio))
+	if (bio_no_advance_iter(bio)) {
  		iter->bi_size -= bytes;
-	else
+	} else if (bio_flagged(bio, BIO_DMABUF_MAP)) {
+		bio_advance_iter_dmabuf_map(iter, bytes);

This is a bit of a mess. You're using bi_bvec_done for something that is not bvec_done, which makes the naming very confusing. That is even more confusing than the existing usage, which isn't great. Also we add yet another conditional to heavily inlined code. I'd suggest the following:

- add a prep patch to rename bi_bvec_done to bi_offset, as even for the existing usage it is the offset into the current bio_vec as much as it is the count of byes done, as those must be the same and it is used both ways
- add a prep patch to also increase bi_offset for bio_no_advance_iter. It is not actually use there, but incrementing it is harmless and this will avoid a new special case
- please also documet this new usage in the commet in struct bvec_iter.
- then just add the dma buf mapping to the bio_no_advance_iter condition

- figure out what to do about dm_bio_rewind_iter, which pokes into these things that really should be block layer internal

include/linux/bio.h static inline void bio_wouldblock_error(struct bio *bio) · line 409

Quoted 10 lines

  }

include/linux/bio.h +1 -1

@@ -391,7 +403,7 @@ static inline void bio_wouldblock_error(struct bio *bio)
   */
  static inline int bio_iov_vecs_to_alloc(struct iov_iter *iter, int max_segs)
  {
-	if (iov_iter_is_bvec(iter))
+	if (iov_iter_is_bvec(iter) || iov_iter_is_dmabuf_map(iter))
  		return 0;
  	return iov_iter_npages(iter, max_segs);
  }

Please update the comment for this helper.

include/linux/blk_types.h enum { · line 330

Quoted 5 lines

include/linux/blk_types.h +1 -0

@@ -322,6 +327,7 @@ enum {
  	BIO_REMAPPED,
  	BIO_ZONE_WRITE_PLUGGING, /* bio handled through zone write plugging */
  	BIO_EMULATES_ZONE_APPEND, /* bio emulates a zone append operation */
+	BIO_DMABUF_MAP, /* Using premmaped dma buffers */

Shouldn't this be a REQ_ flag as we should never mix and match bios with and without this flag in a single request?

Quoted 8 lines · Christoph Hellwig

Quoted 6 lines

+    if (!bio_flagged(bio_src, BIO_DMABUF_MAP)) {
+        bio->bi_io_vec = bio_src->bi_io_vec;
+    } else {
+        bio->dmabuf_map = bio_src->dmabuf_map;
+        bio_set_flag(bio, BIO_DMABUF_MAP);
+    }

This is backwards, please avoid pointless negations:

I can flip it, but compilers tend to prefer the true branch. E.g. this

if (cond) A; else B;
C;

can get compiled into:

jmpcc cond B
A: ...
C:
return;
B: ...
jmp C;

Quoted 24 lines

    if (bio_flagged(bio_src, BIO_DMABUF_MAP)) {
        bio->dmabuf_map = bio_src->dmabuf_map;
        bio_set_flag(bio, BIO_DMABUF_MAP);
    } else {
        bio->bi_io_vec = bio_src->bi_io_vec;
    }

Quoted 10 lines

+    if (bio_flagged(bio, BIO_DMABUF_MAP)) {
+        nsegs = 1;
+
+        if ((bio->bi_iter.bi_bvec_done & lim->dma_alignment) || +            (bio->bi_iter.bi_size & len_align_mask)) +            return -EINVAL;
+        if (bio->bi_iter.bi_size > max_bytes) {
+            bytes = max_bytes;
+            goto split;
+        }

Please add a comment explaining why nsegs is always 1 here.

Quoted 10 lines

Quoted 8 lines

block/blk.h +0 -0

@@ -424,7 +424,8 @@ static inline struct bio *__bio_split_to_limits(struct bio *bio,
       switch (bio_op(bio)) {

      case REQ_OP_READ:
      case REQ_OP_WRITE:
-        if (bio_may_need_split(bio, lim))
+        if (bio_may_need_split(bio, lim) || +            bio_flagged(bio, BIO_DMABUF_MAP)) return bio_split_rw(bio, lim, nr_segs);

The BIO_DMABUF_MAP check should go into bio_may_need_split.

Ok

Quoted 24 lines

Quoted 9 lines

include/linux/bio.h +7 -0

+static inline void bio_advance_iter_dmabuf_map(struct bvec_iter *iter,
+                           unsigned int bytes)
+{
+    iter->bi_bvec_done += bytes;
+    iter->bi_size -= bytes;
+}
+

  static inline void bio_advance_iter(const struct bio *bio, struct bvec_iter *iter, unsigned int bytes)

Quoted 8 lines

  {
      iter->bi_sector += bytes >> 9;
  -    if (bio_no_advance_iter(bio))
+    if (bio_no_advance_iter(bio)) { iter->bi_size -= bytes;
-    else
+    } else if (bio_flagged(bio, BIO_DMABUF_MAP)) { +        bio_advance_iter_dmabuf_map(iter, bytes);

This is a bit of a mess.  You're using bi_bvec_done for something that is not bvec_done, which makes the naming very confusing.  That is even more confusing than the existing usage, which isn't great.  Also we add yet another conditional to heavily inlined code.  I'd suggest the following:

Quoted 9 lines

  - add a prep patch to rename bi_bvec_done to bi_offset, as even for the existing usage it is the offset into the current bio_vec as much as it is the count of byes done, as those must be the same and it is used both ways
  - add a prep patch to also increase bi_offset for bio_no_advance_iter. It is not actually use there, but incrementing it is harmless and this will avoid a new special case
  - please also documet this new usage in the commet in struct bvec_iter.
  - then just add the dma buf mapping to the bio_no_advance_iter condition

I'll take a look

Quoted 2 lines

  - figure out what to do about dm_bio_rewind_iter, which pokes into these things that really should be block layer internal

Need to check what that is, but doesn't implement the interface and is not supposed to ever see the dmabuf iterator.

Quoted 13 lines

include/linux/bio.h static inline void bio_wouldblock_error(struct bio *bio) · line 409

Quoted 10 lines

  }

include/linux/bio.h +1 -1

@@ -391,7 +403,7 @@ static inline void bio_wouldblock_error(struct bio *bio)
    */
   static inline int bio_iov_vecs_to_alloc(struct iov_iter *iter, int max_segs)
   {
-    if (iov_iter_is_bvec(iter))
+    if (iov_iter_is_bvec(iter) || iov_iter_is_dmabuf_map(iter))
           return 0;
       return iov_iter_npages(iter, max_segs);
   }

Please update the comment for this helper.

Quoted 8 lines

Quoted 5 lines

include/linux/blk_types.h +0 -0

@@ -322,6 +327,7 @@ enum {
       BIO_REMAPPED,

      BIO_ZONE_WRITE_PLUGGING, /* bio handled through zone write plugging */ BIO_EMULATES_ZONE_APPEND, /* bio emulates a zone append operation */ +    BIO_DMABUF_MAP, /* Using premmaped dma buffers */

Shouldn't this be a REQ_ flag as we should never mix and match bios with and without this flag in a single request?

Do you mean adding both and propagating it from bio to req? submit_bio() takes a bio, so we still need to set it there before it reaches blk-mq. And there might be bio-based drivers using it in the future.

Quoted 17 lines · Pavel Begunkov

Quoted 8 lines · Christoph Hellwig

Quoted 6 lines

+    if (!bio_flagged(bio_src, BIO_DMABUF_MAP)) {
+        bio->bi_io_vec = bio_src->bi_io_vec;
+    } else {
+        bio->dmabuf_map = bio_src->dmabuf_map;
+        bio_set_flag(bio, BIO_DMABUF_MAP);
+    }

This is backwards, please avoid pointless negations:

I can flip it, but compilers tend to prefer the true branch. E.g. this

if (cond) A; else B;
C;

can get compiled into:

Quoted 6 lines

jmpcc cond B
A: ...
C:
return;
B: ...
jmp C;

When that is really a performance critical path then you can use the likely() and unlikely() macros to give the compiler the hint which one to prefer.

What could be useful is to have the true path for the more common and the false path for the less common case for documentation purposes, but in that case I would expected some code comments as well.

Quoted 1 line

When that is really a performance critical path then you can use the likely() and unlikely() macros to give the compiler the hint which one to prefer.

That might be more penalising than placing them in the right order, and it might be fine as it's new and all that, but it's not a clear cut as it's definitely not created to be a slow path.

TBH, not sure
why we're bike shedding such things, is it somewhere in the code style?

Quoted 1 line

What could be useful is to have the true path for the more common and the false path for the less common case for documentation purposes, but in that case I would expected some code comments as well.

What kind of comment are you thinking about? A "this is not a likely path" type of comment before each mention of the flag is usually not very useful.

Quoted 5 lines · Pavel Begunkov

Quoted 1 line

When that is really a performance critical path then you can use the likely() and unlikely() macros to give the compiler the hint which one to prefer.

That might be more penalising than placing them in the right order, and it might be fine as it's new and all that, but it's not a clear cut as it's definitely not created to be a slow path.

Yes. Whatever the caller is using at a given time is the fast path here, and dynamic branch prediction in modern cpus handles this really well.

Quoted 3 lines

TBH, not sure
why we're bike shedding such things, is it somewhere in the code style?

It makes reading the code annoying, so it better have a good reason. Now for a single conditional it's not much of an issue, but these things tend to pile up and then start to get really annoying. Always write your code the most straight forward way unless you have a good reason not to.

Quoted 8 lines · Christoph Hellwig

block/bio.c static int __bio_clone(struct bio *bio, struct bio *bio_src, gfp_t gfp) · line 860

Quoted 6 lines

block/bio.c +6 -0

+	if (!bio_flagged(bio_src, BIO_DMABUF_MAP)) {
+		bio->bi_io_vec = bio_src->bi_io_vec;
+	} else {
+		bio->dmabuf_map = bio_src->dmabuf_map;
+		bio_set_flag(bio, BIO_DMABUF_MAP);
+	}

This is backwards, please avoid pointless negations:

I can flip it, but compilers tend to prefer the true branch. E.g. this

Quoted 8 lines · Christoph Hellwig

block/bio.c static int __bio_clone(struct bio *bio, struct bio *bio_src, gfp_t gfp) · line 860

Quoted 6 lines

block/bio.c +6 -0

+	if (!bio_flagged(bio_src, BIO_DMABUF_MAP)) {
+		bio->bi_io_vec = bio_src->bi_io_vec;
+	} else {
+		bio->dmabuf_map = bio_src->dmabuf_map;
+		bio_set_flag(bio, BIO_DMABUF_MAP);
+	}  

This is backwards, please avoid pointless negations:

I can flip it, but compilers tend to prefer the true branch. E.g. this

if (cond) A; else B;
C;

can get compiled into:

jmpcc cond B
A: ...
C:
return;
B: ...
jmp C;

Quoted 6 lines

include/linux/blk_types.h enum { · line 330

Quoted 3 lines

BIO_ZONE_WRITE_PLUGGING, /* bio handled through zone write plugging */ BIO_EMULATES_ZONE_APPEND, /* bio emulates a zone append operation */

include/linux/blk_types.h +1 -0

+	BIO_DMABUF_MAP, /* Using premmaped dma buffers */

Shouldn't this be a REQ_ flag as we should never mix and match bios with and without this flag in a single request?

Do you mean adding both and propagating it from bio to req? submit_bio() takes a bio, so we still need to set it there before it reaches blk-mq. And there might be bio-based drivers using it in the future.

Quoted 10 lines · Pavel Begunkov

Quoted 6 lines

include/linux/blk_types.h enum { · line 330

Quoted 3 lines

BIO_ZONE_WRITE_PLUGGING, /* bio handled through zone write plugging */ BIO_EMULATES_ZONE_APPEND, /* bio emulates a zone append operation */

include/linux/blk_types.h +1 -0

+	BIO_DMABUF_MAP, /* Using premmaped dma buffers */

Shouldn't this be a REQ_ flag as we should never mix and match bios with and without this flag in a single request?

Do you mean adding both and propagating it from bio to req? submit_bio() takes a bio, so we still need to set it there before it reaches blk-mq. And there might be bio-based drivers using it in the future.

I think I forgot to reply to this, so let's do this now.

REQ_ is actually used by both bios and requests, so if you set it in bio->bi_opf it will automatically get propagated to the request, but it can also always be tested on the bio, including by bio-based drivers.

include/linux/blk_types.h struct bio { · line 240

Quoted 5 lines

include/linux/blk_types.h +5 -0

+	union {
+		struct bio_vec		*bi_io_vec;
+		/* Driver specific dma map, present only with BIO_DMABUF_MAP */
+		struct io_dmabuf_map	*dmabuf_map;
+	};

... and please add the bi_ prefix we're using for all (well except for one oddity) fields in struct bio.

Naming and placement:

This is about dma-buf based I/O. So I'd expect it to be named dma-buf-io and no io-dmabuf, and live in drivers/dma-buf and not the unrelated lib/. But I'd like to hear from the dma-buf maintainers about that.

Config option: as this unconditionally when DMA_SHARED_BUFFER is enabled, why does it need a separate config option?

Interface: io_dmabuf_token_create / ->create_dmabuf_token filling in a structure allocated by the caller feels odd.

would be to move most of io_dmabuf_token_create into a helper called by ->create_dmabuf_token so that the token is allocated in the driver data structure and returned from create_dmabuf_token.

Quoted 3 lines

This is about dma-buf based I/O. So I'd expect it to be named dma-buf-io and no io-dmabuf, and live in drivers/dma-buf and not the unrelated lib/. But I'd like to hear from the dma-buf maintainers about that.

Looking at what Ming is saying, it'd make more sense to keep some of the parts like iterator and the file op more flexible and not automatically imply dma-buf even if it's the main and for now the only medium. I.e. ublk/fuse can use a similar interface for mapping buffers to the server even without dma mappings.

I don't know how the API should look like, maybe passing memfd, and dma-buf supports mmap, but I think it's better to call the op something like "register_buffer" instead and keep all it in lib/ for the same reasons.

Quoted 10 lines · Pavel Begunkov

Quoted 3 lines

This is about dma-buf based I/O. So I'd expect it to be named dma-buf-io and no io-dmabuf, and live in drivers/dma-buf and not the unrelated lib/. But I'd like to hear from the dma-buf maintainers about that.

Looking at what Ming is saying, it'd make more sense to keep some of the parts like iterator and the file op more flexible and not automatically imply dma-buf even if it's the main and for now the only medium. I.e. ublk/fuse can use a similar interface for mapping buffers to the server even without dma mappings.

Quoted 3 lines

I don't know how the API should look like, maybe passing memfd, and dma-buf supports mmap, but I think it's better to call the op something like "register_buffer" instead and keep all it in lib/ for the same reasons.

Let's get the current version landed. If we come up with some kind of non-dma dmabuf in the future we can refactor it and move it around. I'm a little skeptic we'll be able to share code as long as dmabuf is allergic to physical addresses, though.

lib/ is most certainly the wrong place for something that absolutely is not library functionality but directly interacts with a few subsystems.

To be fair, it's not that dma-buf specific. This lib/ code only does some resv locking, fence waiting and queuing fences,

otherwise
all the attaching is done by the driver behind callbacks. Switching it to some memfd could be pretty simple. But The main thing it'd need to share is iterator handling like forwarding in the block layer, and it should be fine as it's already passed as a completely opaque object with no knowledge about pages / dma / etc. for the middle layers.

Quoted 3 lines

lib/ is most certainly the wrong place for something that absolutely is not library functionality but directly interacts with a few subsystems.

It only interacts with dma-buf, and even for dma-buf attachments are created by the driver. Block, nvme, io_uring are users, either using the helpers or implementing callbacks.

Ok. Let's assume for the argument's sake it's not dma-buf specific, if not lib/, where would you put it? I was also assuming that dma-buf being under drivers/ is rather a relic of the past rather than the desired location, hmm?

Signature · 2 lines

-- 
Pavel Begunkov

Quoted 2 lines · Pavel Begunkov

To be fair, it's not that dma-buf specific. This lib/ code only does some resv locking, fence waiting and queuing fences,

But all the dma resv/fence stuff is pretty tied into the dma-buf ecosystem. I don't think it would really apply to something not doing DMA at all.

Quoted 7 lines

otherwise
all the attaching is done by the driver behind callbacks. Switching it to some memfd could be pretty simple. But The main thing it'd need to share is iterator handling like forwarding in the block layer, and it should be fine as it's already passed as a completely opaque object with no knowledge about pages / dma / etc. for the middle layers.

But none of that really sits in the current lib/ code anyway?

Quoted 8 lines

Quoted 3 lines

lib/ is most certainly the wrong place for something that absolutely is not library functionality but directly interacts with a few subsystems.

It only interacts with dma-buf, and even for dma-buf attachments are created by the driver. Block, nvme, io_uring are users, either using the helpers or implementing callbacks.

Quoted 4 lines

Ok. Let's assume for the argument's sake it's not dma-buf specific, if not lib/, where would you put it? I was also assuming that dma-buf being under drivers/ is rather a relic of the past rather than the desired location, hmm?

drivers/dma-buf is a pretty natural place for it, I could not thing

where else you'd place dma-buffers. I'm not sure how hmm has anything to do with it.

Quoted 7 lines · Christoph Hellwig

Quoted 2 lines · Pavel Begunkov

To be fair, it's not that dma-buf specific. This lib/ code only does some resv locking, fence waiting and queuing fences,

But all the dma resv/fence stuff is pretty tied into the dma-buf ecosystem. I don't think it would really apply to something not doing DMA at all.

The point is that those can be separated to reuse the rest.

Quoted 1 line

But none of that really sits in the current lib/ code anyway?

It's about naming. E.g. passing a DMABUF_ITER that doesn't have a dma-buf would be confusing, and then it'll need renaming at all layers to support the use case.

Quoted 1 line

drivers/dma-buf is a pretty natural place for it, I could not thing

_If_ there is no dma mappings, drivers/dma-buf would definitely be an awkward spot.

+++ b/lib/io_dmabuf_token.c

(quoted diff) +0 -0

@@ -0,0 +1,272 @@

+++ b/lib/io_dmabuf_token.c

(quoted diff) +0 -0

@@ -0,0 +1,272 @@

(quoted diff) +2 -0

+static void io_dmabuf_map_release_work(struct work_struct *work)
+{

Add a trivial implementation of the create_dmabuf_token call for block devices that forwards the call to a new blk-mq callback if it's available.

Quoted 3 lines · Pavel Begunkov

Add a trivial implementation of the create_dmabuf_token call for block devices that forwards the call to a new blk-mq callback if it's available.

This should go into block_device_operations as there is nothing blk-mq specific about this. I.e. even if this patchset doesn't handle stacking drivers yet, it should be easy enough to add them in the future.

Enable BIO_DMABUF_MAP backed requests. It creates a prp list for the dmabuf when it's mapped, which is then used to initialise requests.

Enable BIO_DMABUF_MAP backed requests. It creates a prp list for the dmabuf when it's mapped, which is then used to initialise requests.

Suggested-by Keith Busch <[email protected]>

Signed-off-by Pavel Begunkov <[email protected]>

---
 drivers/nvme/host/pci.c | 282 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 282 insertions(+)

drivers/nvme/host/pci.c +254 -0

index db5fc9bf6627..d2629853a972 100644
@@ -27,6 +27,8 @@
 #include <linux/io-64-nonatomic-lo-hi.h>
 #include <linux/io-64-nonatomic-hi-lo.h>
 #include <linux/sed-opal.h>
+#include <linux/io_dmabuf_token.h>
+#include <linux/dma-resv.h>
 
 #include "trace.h"
 #include "nvme.h"

@@ -393,6 +395,17 @@ struct nvme_queue {
 	struct completion delete_done;
 };
 
+struct nvme_dmabuf_token {
+	struct dma_buf_attachment *attach;
+};
+
+struct nvme_dmabuf_map {
+	struct io_dmabuf_map base;
+	dma_addr_t *dma_list;
+	struct sg_table *sgt;
+	unsigned nr_entries;
+};
+
 /* bits for iod->flags */
 enum nvme_iod_flags {
 	/* this command has been aborted by the timeout handler */

@@ -854,6 +867,134 @@ static void nvme_free_descriptors(struct request *req)
 	}
 }
 
+static void nvme_dmabuf_map_sync(struct nvme_dev *nvme_dev, struct request *req,
+				 bool for_cpu)
+{
+	int length = blk_rq_payload_bytes(req);
+	struct device *dev = nvme_dev->dev;
+	enum dma_data_direction dma_dir;
+	struct bio *bio = req->bio;
+	struct nvme_dmabuf_map *map;
+	dma_addr_t *dma_list;
+	int offset, map_idx;
+
+	dma_dir = rq_data_dir(req) == READ ? DMA_FROM_DEVICE : DMA_TO_DEVICE;
+	map = container_of(bio->dmabuf_map, struct nvme_dmabuf_map, base);
+	dma_list = map->dma_list;
+
+	offset = bio->bi_iter.bi_bvec_done;
+	map_idx = offset / NVME_CTRL_PAGE_SIZE;
+	length += offset & (NVME_CTRL_PAGE_SIZE - 1);
+
+	while (length > 0) {
+		u64 dma_addr = dma_list[map_idx++];
+
+		if (for_cpu)
+			__dma_sync_single_for_cpu(dev, dma_addr,
+						  NVME_CTRL_PAGE_SIZE, dma_dir);
+		else
+			__dma_sync_single_for_device(dev, dma_addr,
+						     NVME_CTRL_PAGE_SIZE,
+						     dma_dir);
+		length -= NVME_CTRL_PAGE_SIZE;
+	}
+}
+
+static void nvme_rq_clean_dmabuf_map(struct nvme_dev *dev,
+				      struct request *req)
+{
+	struct nvme_iod *iod = blk_mq_rq_to_pdu(req);
+
+	nvme_dmabuf_map_sync(dev, req, true);
+
+	if (!(iod->flags & IOD_SINGLE_SEGMENT))
+		nvme_free_descriptors(req);
+}
+
+static blk_status_t nvme_rq_setup_dmabuf_map(struct request *req,
+					     struct nvme_queue *nvmeq)
+{
+	struct nvme_iod *iod = blk_mq_rq_to_pdu(req);
+	int length = blk_rq_payload_bytes(req);
+	u64 dma_addr, prp1_dma, prp2_dma;
+	struct bio *bio = req->bio;
+	struct nvme_dmabuf_map *map;
+	dma_addr_t *dma_list;
+	dma_addr_t prp_dma;
+	__le64 *prp_list;
+	int i, map_idx;
+	int offset;
+
+	nvme_dmabuf_map_sync(nvmeq->dev, req, false);
+
+	map = container_of(bio->dmabuf_map, struct nvme_dmabuf_map, base);
+	dma_list = map->dma_list;
+
+	offset = bio->bi_iter.bi_bvec_done;
+	map_idx = offset / NVME_CTRL_PAGE_SIZE;
+	offset &= (NVME_CTRL_PAGE_SIZE - 1);
+	prp1_dma = dma_list[map_idx++] + offset;
+
+	length -= (NVME_CTRL_PAGE_SIZE - offset);
+	if (length <= 0) {
+		prp2_dma = 0;
+		goto done;
+	}
+
+	if (length <= NVME_CTRL_PAGE_SIZE) {
+		prp2_dma = dma_list[map_idx];
+		goto done;
+	}
+
+	if (DIV_ROUND_UP(length, NVME_CTRL_PAGE_SIZE) <=
+	    NVME_SMALL_POOL_SIZE / sizeof(__le64))
+		iod->flags |= IOD_SMALL_DESCRIPTOR;
+
+	prp_list = dma_pool_alloc(nvme_dma_pool(nvmeq, iod), GFP_ATOMIC,
+			&prp_dma);
+	if (!prp_list)
+		return BLK_STS_RESOURCE;
+
+	iod->descriptors[iod->nr_descriptors++] = prp_list;
+	prp2_dma = prp_dma;
+	i = 0;
+	for (;;) {
+		if (i == NVME_CTRL_PAGE_SIZE >> 3) {
+			__le64 *old_prp_list = prp_list;
+
+			prp_list = dma_pool_alloc(nvmeq->descriptor_pools.large,
+					GFP_ATOMIC, &prp_dma);
+			if (!prp_list)
+				goto free_prps;
+			iod->descriptors[iod->nr_descriptors++] = prp_list;
+			prp_list[0] = old_prp_list[i - 1];
+			old_prp_list[i - 1] = cpu_to_le64(prp_dma);
+			i = 1;
+		}
+
+		dma_addr = dma_list[map_idx++];
+		prp_list[i++] = cpu_to_le64(dma_addr);
+
+		length -= NVME_CTRL_PAGE_SIZE;
+		if (length <= 0)
+			break;
+	}
+done:
+	iod->cmd.common.dptr.prp1 = cpu_to_le64(prp1_dma);
+	iod->cmd.common.dptr.prp2 = cpu_to_le64(prp2_dma);
+	return BLK_STS_OK;
+free_prps:
+	nvme_free_descriptors(req);
+	return BLK_STS_RESOURCE;
+}
+
+static inline bool nvme_rq_is_dmabuf_attached(struct request *req)
+{
+	if (!IS_ENABLED(CONFIG_DMABUF_TOKEN))
+		return false;
+	return req->bio && bio_flagged(req->bio, BIO_DMABUF_MAP);
+}
+
 static void nvme_free_prps(struct request *req, unsigned int attrs)
 {
 	struct nvme_iod *iod = blk_mq_rq_to_pdu(req);

@@ -932,6 +1073,11 @@ static void nvme_unmap_data(struct request *req)
 	struct device *dma_dev = nvmeq->dev->dev;
 	unsigned int attrs = 0;
 
+	if (nvme_rq_is_dmabuf_attached(req)) {
+		nvme_rq_clean_dmabuf_map(nvmeq->dev, req);
+		return;
+	}
+
 	if (iod->flags & IOD_SINGLE_SEGMENT) {
 		static_assert(offsetof(union nvme_data_ptr, prp1) ==
 				offsetof(union nvme_data_ptr, sgl.addr));

@@ -1222,6 +1368,9 @@ static blk_status_t nvme_map_data(struct request *req)
 	struct blk_dma_iter iter;
 	blk_status_t ret;
 
+	if (nvme_rq_is_dmabuf_attached(req))
+		return nvme_rq_setup_dmabuf_map(req, nvmeq);
+
 	/*
 	 * Try to skip the DMA iterator for single segment requests, as that
 	 * significantly improves performances for small I/O sizes.

@@ -2238,6 +2387,134 @@ static int nvme_create_queue(struct nvme_queue *nvmeq, int qid, bool polled)
 	return result;
 }
 
+#ifdef CONFIG_DMABUF_TOKEN
+static void nvme_dmabuf_invalidate_mappings(struct dma_buf_attachment *attach)
+{
+	struct io_dmabuf_token *token = attach->importer_priv;
+
+	io_dmabuf_token_invalidate_mappings(token);
+}
+
+const struct dma_buf_attach_ops nvme_dmabuf_importer_ops = {
+	.invalidate_mappings	= nvme_dmabuf_invalidate_mappings,
+	.allow_peer2peer	= true,
+};
+
+static struct io_dmabuf_map *nvme_dmabuf_token_map(struct io_dmabuf_token *token)
+{
+	struct nvme_dmabuf_token *data = token->dev_priv;
+	struct dma_buf_attachment *attach = data->attach;
+	dma_addr_t *dma_list = NULL;
+	unsigned long tmp, i = 0;
+	struct nvme_dmabuf_map *map;
+	struct scatterlist *sg;
+	struct sg_table *sgt;
+	unsigned nr_entries;
+	int ret;
+
+	dma_resv_assert_held(token->dmabuf->resv);
+
+	map = kmalloc(sizeof(*map), GFP_KERNEL);
+	if (!map)
+		return ERR_PTR(-ENOMEM);
+
+	nr_entries = token->dmabuf->size / NVME_CTRL_PAGE_SIZE;
+	dma_list = kmalloc_array(nr_entries, sizeof(dma_list[0]), GFP_KERNEL);
+	if (!dma_list) {
+		ret = -ENOMEM;
+		goto err;
+	}
+
+	sgt = dma_buf_map_attachment(attach, token->dir);
+	if (IS_ERR(sgt)) {
+		ret = PTR_ERR(sgt);
+		sgt = NULL;
+		goto err;
+	}
+
+	for_each_sgtable_dma_sg(sgt, sg, tmp) {
+		dma_addr_t dma_addr = sg_dma_address(sg);
+		unsigned long sg_len = sg_dma_len(sg);
+
+		if (sg_len % NVME_CTRL_PAGE_SIZE) {
+			ret = -EINVAL;
+			goto err;
+		}
+
+		while (sg_len) {
+			dma_list[i++] = dma_addr;
+			dma_addr += NVME_CTRL_PAGE_SIZE;
+			sg_len -= NVME_CTRL_PAGE_SIZE;
+		}
+	}
+
+	ret = io_dmabuf_init_map(token, &map->base);
+	if (ret)
+		goto err;
+	map->nr_entries = nr_entries;
+	map->dma_list = dma_list;
+	map->sgt = sgt;
+	return &map->base;
+err:
+	if (sgt)
+		dma_buf_unmap_attachment(attach, sgt, token->dir);
+	kfree(map);
+	kfree(dma_list);
+	return ERR_PTR(ret);
+}
+
+static void nvme_dmabuf_token_unmap(struct io_dmabuf_token *token,
+				 struct io_dmabuf_map *map_base)
+{
+	struct nvme_dmabuf_token *data = token->dev_priv;
+	struct nvme_dmabuf_map *map = container_of(map_base,
+						struct nvme_dmabuf_map, base);
+
+	dma_resv_assert_held(token->dmabuf->resv);
+
+	dma_buf_unmap_attachment(data->attach, map->sgt, token->dir);
+	kfree(map->dma_list);
+}
+
+static void nvme_dmabuf_token_release(struct io_dmabuf_token *token)
+{
+	struct nvme_dmabuf_token *data = token->dev_priv;
+
+	dma_buf_detach(token->dmabuf, data->attach);
+	kfree(data);
+}
+
+const struct io_dmabuf_token_dev_ops nvme_dma_token_ops = {
+	.map		= nvme_dmabuf_token_map,
+	.unmap		= nvme_dmabuf_token_unmap,
+	.release	= nvme_dmabuf_token_release,
+};
+
+static int nvme_create_dmabuf_token(struct request_queue *q,
+				 struct io_dmabuf_token *token)

Quoted 6 lines · Pavel Begunkov

Enable BIO_DMABUF_MAP backed requests. It creates a prp list for the dmabuf when it's mapped, which is then used to initialise requests.

Suggested-by Keith Busch <[email protected]>

Signed-off-by Pavel Begunkov <[email protected]>

---

Quoted 6 lines

drivers/nvme/host/pci.c +6 -0

+static int nvme_create_dmabuf_token(struct request_queue *q,
+				 struct io_dmabuf_token *token)
+{
+	struct nvme_dmabuf_token *data;
+	struct dma_buf_attachment *attach;
+	struct nvme_ns *ns = q->queuedata;

drivers/nvme/host/pci.c static int nvme_create_queue(struct nvme_queue *nvmeq, int qid, bool polled) · line 2504

Quoted 6 lines

drivers/nvme/host/pci.c +6 -0

+	struct nvme_dev *dev = to_nvme_dev(ns->ctrl);
+	struct dma_buf *dmabuf = token->dmabuf;
+
+	data = kzalloc(sizeof(data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;

Shouldn't this be kzalloc(sizeof(*data)...) ?

drivers/nvme/host/pci.c +8 -0

+static void nvme_dmabuf_map_sync(struct nvme_dev *nvme_dev, struct request *req,
+				 bool for_cpu)
+{
+	int length = blk_rq_payload_bytes(req);
+	struct device *dev = nvme_dev->dev;
+	enum dma_data_direction dma_dir;
+	struct bio *bio = req->bio;
+	struct nvme_dmabuf_map *map;

drivers/nvme/host/pci.c +4 -0

+
+	while (length > 0) {
+		u64 dma_addr = dma_list[map_idx++];
+

drivers/nvme/host/pci.c +13 -0

+static blk_status_t nvme_rq_setup_dmabuf_map(struct request *req,
+					     struct nvme_queue *nvmeq)
+{
+	struct nvme_iod *iod = blk_mq_rq_to_pdu(req);
+	int length = blk_rq_payload_bytes(req);
+	u64 dma_addr, prp1_dma, prp2_dma;
+	struct bio *bio = req->bio;
+	struct nvme_dmabuf_map *map;
+	dma_addr_t *dma_list;
+	dma_addr_t prp_dma;
+	__le64 *prp_list;
+	int i, map_idx;
+	int offset;

drivers/nvme/host/pci.c +12 -0

+	length -= (NVME_CTRL_PAGE_SIZE - offset);
+	if (length <= 0) {
+		prp2_dma = 0;
+		goto done;
+	}
+
+	if (length <= NVME_CTRL_PAGE_SIZE) {
+		prp2_dma = dma_list[map_idx];
+		goto done;
+	}
+
+	if (DIV_ROUND_UP(length, NVME_CTRL_PAGE_SIZE) <=

FYI, I really want SGL support before this get merged, but ignoring that for now:

drivers/nvme/host/pci.c struct nvme_queue { · line 406

Quoted 5 lines

drivers/nvme/host/pci.c +5 -0

+struct nvme_dmabuf_map {
+	struct io_dmabuf_map base;
+	dma_addr_t *dma_list;
+	struct sg_table *sgt;
+	unsigned nr_entries;

I'd make dma_list a variable-sized array at the end of the struture to avoid an extra allocation and pointer derefernece.

Quoted 8 lines

drivers/nvme/host/pci.c +8 -0

+static void nvme_dmabuf_map_sync(struct nvme_dev *nvme_dev, struct request *req,
+				 bool for_cpu)
+{
+	int length = blk_rq_payload_bytes(req);
+	struct device *dev = nvme_dev->dev;
+	enum dma_data_direction dma_dir;
+	struct bio *bio = req->bio;
+	struct nvme_dmabuf_map *map;

drivers/nvme/host/pci.c static void nvme_free_descriptors(struct request *req) · line 887

Quoted 10 lines

drivers/nvme/host/pci.c +10 -0

+	dma_addr_t *dma_list;
+	int offset, map_idx;
+
+	dma_dir = rq_data_dir(req) == READ ? DMA_FROM_DEVICE : DMA_TO_DEVICE;
+	map = container_of(bio->dmabuf_map, struct nvme_dmabuf_map, base);
+	dma_list = map->dma_list;
+
+	offset = bio->bi_iter.bi_bvec_done;
+	map_idx = offset / NVME_CTRL_PAGE_SIZE;
+	length += offset & (NVME_CTRL_PAGE_SIZE - 1);

Please initialize the variable at declaration time and use or add proper helpers to simplify this:

static inline struct nvme_dmabuf_map *
to_nvme_dmabuf_map(struct io_dmabuf_map *map)

{
	return container_of(map, struct nvme_dmabuf_map, base);
}

....

	enum dma_data_direction dma_dir = rq_dma_dir(req);
	struct device *dev = nvme_dev->dev;
	struct bio *bio = req->bio;
	struct nvme_dmabuf_map *map = to_nvme_dmabuf_map(bio->bi_dmabuf_map);
	dma_addr_t *dma_list = map->dma_list;
	int offset = bio->bi_iter.bi_bvec_done;
	int mmap_idx = offset / NVME_CTRL_PAGE_SIZE;
	int length = blk_rq_payload_bytes(req) +
		offset & (NVME_CTRL_PAGE_SIZE - 1);

Also a lot of these ints sound like they should be unsigned.

Quoted 10 lines

drivers/nvme/host/pci.c +10 -0

+
+	while (length > 0) {
+		u64 dma_addr = dma_list[map_idx++];
+
+		if (for_cpu)
+			__dma_sync_single_for_cpu(dev, dma_addr,
+						  NVME_CTRL_PAGE_SIZE, dma_dir);
+		else
+			__dma_sync_single_for_device(dev, dma_addr,
+						     NVME_CTRL_PAGE_SIZE,

drivers/nvme/host/pci.c static void nvme_free_descriptors(struct request *req) · line 901

Quoted 4 lines

drivers/nvme/host/pci.c +4 -0

+						     dma_dir);
+		length -= NVME_CTRL_PAGE_SIZE;
+	}
+}

Nothing should be using these __dma_sync helpers that are internal details. Using them means you call into sync code that should be skipped on most common server class systems.

Also the for_cpu argument is a bit ugly. I'd rather have separate routines as in the core dma-mapping code, even if that means a little bit of code duplication.

Quoted 13 lines

drivers/nvme/host/pci.c +13 -0

+static blk_status_t nvme_rq_setup_dmabuf_map(struct request *req,
+					     struct nvme_queue *nvmeq)
+{
+	struct nvme_iod *iod = blk_mq_rq_to_pdu(req);
+	int length = blk_rq_payload_bytes(req);
+	u64 dma_addr, prp1_dma, prp2_dma;
+	struct bio *bio = req->bio;
+	struct nvme_dmabuf_map *map;
+	dma_addr_t *dma_list;
+	dma_addr_t prp_dma;
+	__le64 *prp_list;
+	int i, map_idx;
+	int offset;

drivers/nvme/host/pci.c static void nvme_free_descriptors(struct request *req) · line 936

Quoted 10 lines

drivers/nvme/host/pci.c +10 -0

+
+	nvme_dmabuf_map_sync(nvmeq->dev, req, false);
+
+	map = container_of(bio->dmabuf_map, struct nvme_dmabuf_map, base);
+	dma_list = map->dma_list;
+
+	offset = bio->bi_iter.bi_bvec_done;
+	map_idx = offset / NVME_CTRL_PAGE_SIZE;
+	offset &= (NVME_CTRL_PAGE_SIZE - 1);
+	prp1_dma = dma_list[map_idx++] + offset;

Same comments as for the sync helper above.

Quoted 12 lines

drivers/nvme/host/pci.c +12 -0

+	length -= (NVME_CTRL_PAGE_SIZE - offset);
+	if (length <= 0) {
+		prp2_dma = 0;
+		goto done;
+	}
+
+	if (length <= NVME_CTRL_PAGE_SIZE) {
+		prp2_dma = dma_list[map_idx];
+		goto done;
+	}
+
+	if (DIV_ROUND_UP(length, NVME_CTRL_PAGE_SIZE) <=

drivers/nvme/host/pci.c static void nvme_free_descriptors(struct request *req) · line 959

Quoted 10 lines

drivers/nvme/host/pci.c +10 -0

+	    NVME_SMALL_POOL_SIZE / sizeof(__le64))
+		iod->flags |= IOD_SMALL_DESCRIPTOR;
+
+	prp_list = dma_pool_alloc(nvme_dma_pool(nvmeq, iod), GFP_ATOMIC,
+			&prp_dma);
+	if (!prp_list)
+		return BLK_STS_RESOURCE;
+
+	iod->descriptors[iod->nr_descriptors++] = prp_list;
+	prp2_dma = prp_dma;

And I really hate how this duplicates all the nasty PRP building logic, although right now I don't have a good answer to that.

drivers/nvme/host/pci.c static void nvme_free_descriptors(struct request *req) · line 996

Quoted 6 lines

drivers/nvme/host/pci.c +6 -0

+static inline bool nvme_rq_is_dmabuf_attached(struct request *req)
+{
+	if (!IS_ENABLED(CONFIG_DMABUF_TOKEN))
+		return false;
+	return req->bio && bio_flagged(req->bio, BIO_DMABUF_MAP);
+}

This is something that should go into the block layer.

Quoted 2 lines

FYI, I really want SGL support before this get merged, but ignoring that for now:

I was hoping to let Samsung guys to send a follow up they already have, but I'll ask them to have about taking it into this patch set.

Quoted 3 lines · Pavel Begunkov

Quoted 2 lines

FYI, I really want SGL support before this get merged, but ignoring that for now:

Quoted 2 lines

I was hoping to let Samsung guys to send a follow up they already have, but I'll ask them to have about taking it into this patch set.

I had done patches on top of v3 adding SGL support and PRP list reuse optimization for the dmabuf path.
Branch: https://github.com/SamsungDS/linux/commits/rw-dmabuf-v3-nvme-opt/

Also pasting the SGL patch here for quick reference:

The patch set allows to register a dmabuf to an io_uring instance for a specified file and use it with io_uring read / write requests. The infrastructure is not tied to io_uring and there could be more users in the future. A similar idea was attempted some years ago by Keith [1], from where I borrowed a good number of changes, and later was brough up by Tushar and Vishal from Intel.

It's an opt-in feature for files, and they need to implement a new file operation to use it. Only NVMe block devices are supported in this series. The user API is built on top of io_uring's "registered buffers", where a dmabuf is registered in a special way, but after it can be used as any other "registered buffer" with IORING_OP_{READ,WRITE}_FIXED requests. It's created via a new file operation and the resulted map is then passed through the I/O stack in a new iterator type. There is some additional infrastructure to bind it all, which also counts requests using a dmabuf map and managing lifetimes, which is used to implement map invalidation.

It was tested for GPU <-> NVMe transfers. Also, as it maintains a long-term dma mapping, it helps with the IOMMU cost. The numbers below are for udmabuf reads previously run by Anuj for different IOMMU modes:

Quoted 18 lines · Pavel Begunkov

The patch set allows to register a dmabuf to an io_uring instance for a specified file and use it with io_uring read / write requests. The infrastructure is not tied to io_uring and there could be more users in the future. A similar idea was attempted some years ago by Keith [1], from where I borrowed a good number of changes, and later was brough up by Tushar and Vishal from Intel.

It's an opt-in feature for files, and they need to implement a new file operation to use it. Only NVMe block devices are supported in this series. The user API is built on top of io_uring's "registered buffers", where a dmabuf is registered in a special way, but after it can be used as any other "registered buffer" with IORING_OP_{READ,WRITE}_FIXED requests. It's created via a new file operation and the resulted map is then passed through the I/O stack in a new iterator type. There is some additional infrastructure to bind it all, which also counts requests using a dmabuf map and managing lifetimes, which is used to implement map invalidation.

Quoted 4 lines

It was tested for GPU <-> NVMe transfers. Also, as it maintains a long-term dma mapping, it helps with the IOMMU cost. The numbers below are for udmabuf reads previously run by Anuj for different IOMMU modes:

Plain registered buffer is long-live too, which raises question: does this framework need to take it into account from beginning?

BTW, inspired by this approach, I adds similar feature to ublk via UBLK_IO_F_SHMEM_ZC which can maintain long-term vfio dma mapping over registered user-place aligned buffer.