SRP initiator related bug fixes

This patch series contains six bug fixes either for the SRP initiator itself or for IB core functionality used by the SRP initiator. The order of these patches matches the order in which the corresponding bugs have been introduced. The patches in this series are:

0001-IB-srp-Fix-a-memory-leak.patch 0002-IB-srp-Fix-possible-send-queue-overflow.patch 0003-IB-srp-Initialize-dma_length-in-srp_map_idb.patch 0004-IB-srp-Fix-indirect-data-buffer-rkey-endianness.patch 0005-IB-core-Fix-ib_sg_to_pages.patch 0006-IB-srp-Fix-srp_map_sg_fr.patch
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

If srp_connect_ch() returns a positive value then that is considered by its caller as a connection failure but this does not result in a scsi_host_put() call and additionally causes the srp_create_target() function to return a positive value while it should return a negative value. Avoid all this confusion and additionally fix a memory leak by ensuring that srp_connect_ch() always returns a value that is <= 0. This patch avoids that a rejected login triggers the following memory leak:

unreferenced object 0xffff88021b24a220 (size 8):
comm "srp_daemon", pid 56421, jiffies 4295006762 (age 4240.750s) hex dump (first 8 bytes):

    68 6f 73 74 35 38 00 a5                          host58..
  backtrace:
    [<ffffffff8151014a>] kmemleak_alloc+0x7a/0xc0
    [<ffffffff81165c1e>] __kmalloc_track_caller+0xfe/0x160
    [<ffffffff81260d2b>] kvasprintf+0x5b/0x90
    [<ffffffff81260e2d>] kvasprintf_const+0x8d/0xb0
    [<ffffffff81254b0c>] kobject_set_name_vargs+0x3c/0xa0
    [<ffffffff81337e3c>] dev_set_name+0x3c/0x40
    [<ffffffff81355757>] scsi_host_alloc+0x327/0x4b0
    [<ffffffffa03edc8e>] srp_create_target+0x4e/0x8a0 [ib_srp]
    [<ffffffff8133778b>] dev_attr_store+0x1b/0x20
    [<ffffffff811f27fa>] sysfs_kf_write+0x4a/0x60
    [<ffffffff811f1e8e>] kernfs_fop_write+0x14e/0x180
    [<ffffffff81176eef>] __vfs_write+0x2f/0xf0
    [<ffffffff811771e4>] vfs_write+0xa4/0x100
    [<ffffffff81177c64>] SyS_write+0x54/0xc0
    [<ffffffff8151b257>] entry_SYSCALL_64_fastpath+0x12/0x6f

---
 drivers/infiniband/ulp/srp/ib_srp.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

index 9909022..a074dad 100644
@@ -994,16 +994,16 @@ static int srp_connect_ch(struct srp_rdma_ch *ch, bool multich)
 
 	ret = srp_lookup_path(ch);
 	if (ret)
-		return ret;
+		goto out;
 
 	while (1) {
 		init_completion(&ch->done);
 		ret = srp_send_req(ch, multich);
 		if (ret)
-			return ret;
+			goto out;
 		ret = wait_for_completion_interruptible(&ch->done);
 		if (ret < 0)
-			return ret;
+			goto out;
 
 		/*
 		 * The CM event handling code will set status to

@@ -1011,15 +1011,16 @@ static int srp_connect_ch(struct srp_rdma_ch *ch, bool multich)
 		 * back, or SRP_DLID_REDIRECT if we get a lid/qp
 		 * redirect REJ back.
 		 */
-		switch (ch->status) {
+		ret = ch->status;
+		switch (ret) {
 		case 0:
 			ch->connected = true;
-			return 0;
+			goto out;
 
 		case SRP_PORT_REDIRECT:
 			ret = srp_lookup_path(ch);
 			if (ret)
-				return ret;
+				goto out;
 			break;
 
 		case SRP_DLID_REDIRECT:

@@ -1028,13 +1029,16 @@ static int srp_connect_ch(struct srp_rdma_ch *ch, bool multich)
 		case SRP_STALE_CONN:
 			shost_printk(KERN_ERR, target->scsi_host, PFX
 				     "giving up on stale connection\n");
-			ch->status = -ECONNRESET;
-			return ch->status;
+			ret = -ECONNRESET;
+			goto out;
 
 		default:
-			return ch->status;
+			goto out;
 		}
 	}
+
+out:
+	return ret <= 0 ? ret : -ENODEV;
 }
 
 static int srp_inv_rkey(struct srp_rdma_ch *ch, u32 rkey)

2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

From: Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/[email protected]>

When using work request based memory registration (fast_reg) we must reserve SQ entries for registration and invalidation in addition to send operations. Each IO consumes 3 SQ entries (registration, send, invalidation) so we need to allocate 3x larger send-queue instead of 2x.

---
 drivers/infiniband/ulp/srp/ib_srp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index a074dad..c7a95d2 100644
@@ -488,7 +488,7 @@ static int srp_create_ch_ib(struct srp_rdma_ch *ch)
 	struct ib_qp *qp;
 	struct ib_fmr_pool *fmr_pool = NULL;
 	struct srp_fr_pool *fr_pool = NULL;
-	const int m = 1 + dev->use_fast_reg;
+	const int m = dev->use_fast_reg ? 3 : 1;
 	struct ib_cq_init_attr cq_attr = {};
 	int ret;
 

2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

From: Christoph Hellwig <[email protected]>

Without this sg_dma_len will return 0 on architectures tha have the dma_length field.

---
 drivers/infiniband/ulp/srp/ib_srp.c | 3 +++
 1 file changed, 3 insertions(+)

index c7a95d2..72fac20 100644
@@ -1524,6 +1524,9 @@ static int srp_map_idb(struct srp_rdma_ch *ch, struct srp_request *req,
 		state.sg_nents = 1;
 		sg_set_buf(idb_sg, req->indirect_desc, idb_len);
 		idb_sg->dma_address = req->indirect_dma_addr; /* hack! */
+#ifdef CONFIG_NEED_SG_DMA_LENGTH
+		idb_sg->dma_length = idb_sg->length;	      /* hack^2 */
+#endif
 		ret = srp_map_finish_fr(&state, ch);
 		if (ret < 0)
 			return ret;

2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Detected by sparse.

---
 drivers/infiniband/ulp/srp/ib_srp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 72fac20..fac1423 100644
@@ -1662,7 +1662,7 @@ static int srp_map_data(struct scsi_cmnd *scmnd, struct srp_rdma_ch *ch,
 			return ret;
 		req->nmdesc++;
 	} else {
-		idb_rkey = target->global_mr->rkey;
+		idb_rkey = cpu_to_be32(target->global_mr->rkey);
 	}
 
 	indirect_hdr->table_desc.va = cpu_to_be64(req->indirect_dma_addr);

2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Fix the code for detecting gaps and disable the code for chunking. A gap occurs not only if the second or later scatterlist element is not aligned but also if any scatterlist element other than the last does not end at a page boundary. Disable the code for chunking. Ensure that this function returns a negative error code instead of zero if the first set_page() call fails.

---
 drivers/infiniband/core/verbs.c | 31 +++++++------------------------
 1 file changed, 7 insertions(+), 24 deletions(-)

index 043a60e..95836c6 100644
@@ -1530,10 +1530,9 @@ int ib_sg_to_pages(struct ib_mr *mr,
 		   int (*set_page)(struct ib_mr *, u64))
 {
 	struct scatterlist *sg;
-	u64 last_end_dma_addr = 0, last_page_addr = 0;
 	unsigned int last_page_off = 0;
 	u64 page_mask = ~((u64)mr->page_size - 1);
-	int i;
+	int i, ret;
 
 	mr->iova = sg_dma_address(&sgl[0]);
 	mr->length = 0;

@@ -1544,37 +1543,21 @@ int ib_sg_to_pages(struct ib_mr *mr,
 		u64 end_dma_addr = dma_addr + dma_len;
 		u64 page_addr = dma_addr & page_mask;
 
-		if (i && page_addr != dma_addr) {
-			if (last_end_dma_addr != dma_addr) {
-				/* gap */
-				goto done;
-
-			} else if (last_page_off + dma_len <= mr->page_size) {
-				/* chunk this fragment with the last */
-				mr->length += dma_len;
-				last_end_dma_addr += dma_len;
-				last_page_off += dma_len;
-				continue;
-			} else {
-				/* map starting from the next page */
-				page_addr = last_page_addr + mr->page_size;
-				dma_len -= mr->page_size - last_page_off;
-			}
-		}
+		/* gap */
+		if (i && (last_page_off || page_addr != dma_addr))
+			break;
 
 		do {
-			if (unlikely(set_page(mr, page_addr)))
-				goto done;
+			ret = set_page(mr, page_addr);
+			if (unlikely(ret < 0))
+				return i ? i : ret;
 			page_addr += mr->page_size;
 		} while (page_addr < end_dma_addr);
 
 		mr->length += dma_len;
-		last_end_dma_addr = end_dma_addr;
-		last_page_addr = end_dma_addr & page_mask;
 		last_page_off = end_dma_addr & ~page_mask;
 	}
 
-done:
 	return i;
 }
 EXPORT_SYMBOL(ib_sg_to_pages);

2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

After dma_map_sg() has been called the return value of that function must be used as the number of elements in the scatterlist instead of scsi_sg_count().

---
 drivers/infiniband/ulp/srp/ib_srp.c | 19 ++++++++-----------
 drivers/infiniband/ulp/srp/ib_srp.h |  5 +----
 2 files changed, 9 insertions(+), 15 deletions(-)

index fac1423..3db9a65 100644
@@ -1313,7 +1313,7 @@ reset_state:
 }
 
 static int srp_map_finish_fr(struct srp_map_state *state,
-			     struct srp_rdma_ch *ch)
+			     struct srp_rdma_ch *ch, int sg_nents)
 {
 	struct srp_target_port *target = ch->target;
 	struct srp_device *dev = target->srp_host->srp_dev;

@@ -1328,10 +1328,10 @@ static int srp_map_finish_fr(struct srp_map_state *state,
 
 	WARN_ON_ONCE(!dev->use_fast_reg);
 
-	if (state->sg_nents == 0)
+	if (sg_nents == 0)
 		return 0;
 
-	if (state->sg_nents == 1 && target->global_mr) {
+	if (sg_nents == 1 && target->global_mr) {
 		srp_map_desc(state, sg_dma_address(state->sg),
 			     sg_dma_len(state->sg),
 			     target->global_mr->rkey);

@@ -1345,8 +1345,7 @@ static int srp_map_finish_fr(struct srp_map_state *state,
 	rkey = ib_inc_rkey(desc->mr->rkey);
 	ib_update_fast_reg_key(desc->mr, rkey);
 
-	n = ib_map_mr_sg(desc->mr, state->sg, state->sg_nents,
-			 dev->mr_page_size);
+	n = ib_map_mr_sg(desc->mr, state->sg, sg_nents, dev->mr_page_size);
 	if (unlikely(n < 0))
 		return n;
 

@@ -1452,16 +1451,15 @@ static int srp_map_sg_fr(struct srp_map_state *state, struct srp_rdma_ch *ch,
 	state->fr.next = req->fr_list;
 	state->fr.end = req->fr_list + ch->target->cmd_sg_cnt;
 	state->sg = scat;
-	state->sg_nents = scsi_sg_count(req->scmnd);
 
-	while (state->sg_nents) {
+	while (count) {
 		int i, n;
 
-		n = srp_map_finish_fr(state, ch);
+		n = srp_map_finish_fr(state, ch, count);
 		if (unlikely(n < 0))
 			return n;
 
-		state->sg_nents -= n;
+		count -= n;
 		for (i = 0; i < n; i++)
 			state->sg = sg_next(state->sg);
 	}

@@ -1521,13 +1519,12 @@ static int srp_map_idb(struct srp_rdma_ch *ch, struct srp_request *req,
 
 	if (dev->use_fast_reg) {
 		state.sg = idb_sg;
-		state.sg_nents = 1;
 		sg_set_buf(idb_sg, req->indirect_desc, idb_len);
 		idb_sg->dma_address = req->indirect_dma_addr; /* hack! */
 #ifdef CONFIG_NEED_SG_DMA_LENGTH
 		idb_sg->dma_length = idb_sg->length;	      /* hack^2 */
 #endif
-		ret = srp_map_finish_fr(&state, ch);
+		ret = srp_map_finish_fr(&state, ch, 1);
 		if (ret < 0)
 			return ret;
 	} else if (dev->use_fmr) {

index 87a2a91..f6af531 100644
@@ -300,10 +300,7 @@ struct srp_map_state {
 	dma_addr_t		base_dma_addr;
 	u32			dma_len;
 	u32			total_len;
-	union {
-		unsigned int	npages;
-		int		sg_nents;
-	};
+	unsigned int		npages;
 	unsigned int		nmdesc;
 	unsigned int		ndesc;
 };

2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hi Bart,

So can you explain what went wrong with the code? If ib_sg_to_pages() supports chunking, then sg elements are allowed not to end at a page boundary if it is physically contig to the next sg and then the next is chunked. Care to explain how did ib_sg_to_pages mess up?

Umm, my recollection was to make ib_map_mr_sg return the largest prefix mapped. I don't mind a negative error in this case, but isn't zero an implicit error (given you didn't want to map 0 sg elements)?

If we do agree on this we need to change ib_map_mr_sg documentation accordingly.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Umm, but ib_map_mr_sg iterates on the sg list. Say you have sg_nents=3 and after mapping you got dma_nents=2 (2 entries are contig) and you pass that, ib_sg_to_pages will only iterate on 2 elements won't it? am I missing something?
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Hello Sagi,

From https://www.kernel.org/doc/Documentation/DMA-API-HOWTO.txt:

<quote>
With scatterlists, you map a region gathered from several regions by:

	int i, count = dma_map_sg(dev, sglist, nents, direction);
	struct scatterlist *sg;

	for_each_sg(sglist, sg, count, i) {
		hw_address[i] = sg_dma_address(sg);
		hw_len[i] = sg_dma_len(sg);
	}

where nents is the number of entries in the sglist. </quote>

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Hello Sagi,

That's possible, but that would cause the endianness of the indirect data buffer rkey to be changed three times - a first time in srp_map_desc(), a second time in srp_map_idb() and a third time in srp_map_data(). Hence the choice to fix the IDB rkey via the above patch.

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

With the upstream implementation, if the previous element ends at a page boundary (last_end_dma_addr == dma_addr) but the current element does not start at a page boundary (page_addr != dma_addr) then the current element is mapped. I think this is wrong because this condition indicates a gap and hence that ib_sg_to_pages() should stop iterating in this case.

How ib_sg_to_pages() reports to its caller that mapping the first scatterlist element failed is not important to me. I included that change in this patch because I noticed that in the upstream SRP initiator does not consider ib_map_mr_sg() returning zero as an error. I think either ib_sg_to_pages() or ib_map_mr_sg() should be modified such that "no pages have been mapped" is handled as a mapping failure.

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Why is (last_end_dma_addr == dma_addr) implying that the previous element ends at a page boundary? it tests if the current is contig to the prev (dma_addr is not necessarily the page address).

But I think I see whats wrong. If the prev sg didn't end aligned to page we won't get into the above test at all and continue mapping.

Does this make the problem go away?

index 043a60e..23457fe 100644
@@ -1544,7 +1544,7 @@ int ib_sg_to_pages(struct ib_mr *mr,
                u64 end_dma_addr = dma_addr + dma_len;
                u64 page_addr = dma_addr & page_mask;
 
-               if (i && page_addr != dma_addr) {
+               if (i && (page_addr != dma_addr || last_page_off)) {
                        if (last_end_dma_addr != dma_addr) {
                                /* gap */
                                goto done;

--

Note that I don't mind making the chunking code go away if no one wants it (I think Steve specifically asked for it). I'm just trying to understand the exact mistake here.

I don't either, but the patch introduces inconsistency in a case where the caller passed sg_nents=0 (which will return 0 and not -errno).

Would it make better sense to have srp treat 0 return as error? note that all other ULPs treat return_val != sg_nents as error.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Fine with me.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

From Documentation/DMA_API.txt

<quote>
int
dma_map_sg(struct device *dev, struct scatterlist *sg,
int nents, enum dma_data_direction direction)

Returns: the number of DMA address segments mapped (this may be shorter than <nents> passed in if some elements of the scatter/gather list are physically or virtually adjacent and an IOMMU maps them with a single entry).
</quote>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Looks good,

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Looks good,

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

dma_map_sg returns the actual number of entries to iterate. At least historically some IOMMU implementations would do strange tricks like:

If entries 2 and 3 could be merged dma_len for 2 would span 2 and 3, and then entry 3 would actually have the dma addr and len for entry 4.

I'm not sure anyone still does that, but the first spot to check would be the Parisc IOMMU drivers.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Oh I see, I didn't know that.

Thanks for clarifying this for me. AFAICT this patch is fine.

iser, nfs needs fixing too.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Hello Sagi,

Hmm ... why would it be unacceptable to return 0 if sg_nents == 0 ?

Regarding which component to modify if mapping the first page fails:
for almost every kernel function I know a negative return value means failure and a return value >= 0 means success. Hence my proposal to change the return value of the ib_map_mr_sg() function if mapping the first page fails.

How about the patch below ?

Bart.

[PATCH] IB core: Fix ib_sg_to_pages()

Fix the code for detecting gaps. A gap occurs not only if the second or later scatterlist element is not aligned but also if any scatterlist element other than the last does not end at a page boundary. Ensure that this function returns a negative error code instead of zero if the first set_page() call fails.

---
 drivers/infiniband/core/verbs.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

index 043a60e..1972c50 100644
@@ -1516,7 +1516,7 @@ EXPORT_SYMBOL(ib_map_mr_sg);
  * @sg_nents:      number of entries in sg
  * @set_page:      driver page assignment function pointer
  *
- * Core service helper for drivers to covert the largest
+ * Core service helper for drivers to convert the largest
  * prefix of given sg list to a page vector. The sg list
  * prefix converted is the prefix that meet the requirements
  * of ib_map_mr_sg.

@@ -1533,7 +1533,7 @@ int ib_sg_to_pages(struct ib_mr *mr,
 	u64 last_end_dma_addr = 0, last_page_addr = 0;
 	unsigned int last_page_off = 0;
 	u64 page_mask = ~((u64)mr->page_size - 1);
-	int i;
+	int i, ret;
 
 	mr->iova = sg_dma_address(&sgl[0]);
 	mr->length = 0;

@@ -1544,11 +1544,10 @@ int ib_sg_to_pages(struct ib_mr *mr,
 		u64 end_dma_addr = dma_addr + dma_len;
 		u64 page_addr = dma_addr & page_mask;
 
-		if (i && page_addr != dma_addr) {
+		if (i && (page_addr != dma_addr || last_page_off != 0)) {
 			if (last_end_dma_addr != dma_addr) {
 				/* gap */
-				goto done;
-
+				break;
 			} else if (last_page_off + dma_len <= mr->page_size) {
 				/* chunk this fragment with the last */
 				mr->length += dma_len;

@@ -1563,8 +1562,9 @@ int ib_sg_to_pages(struct ib_mr *mr,
 		}
 
 		do {
-			if (unlikely(set_page(mr, page_addr)))
-				goto done;
+			ret = set_page(mr, page_addr);
+			if (unlikely(ret < 0))
+				return i ? : ret;
 			page_addr += mr->page_size;
 		} while (page_addr < end_dma_addr);
 

@@ -1574,7 +1574,6 @@ int ib_sg_to_pages(struct ib_mr *mr,
 		last_page_off = end_dma_addr & ~page_mask;
 	}
 
-done:
 	return i;
 }
 EXPORT_SYMBOL(ib_sg_to_pages);

2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Replying to my own email,

So what would be in the last entry {dma_addr, dma_len}? zeros?

So how does that sit with the fact that dma_unmap requires the same sg_nents as in dma_map and not the actual value of dma entries? --
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

I'm fine with that.

Looks fine.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Take a look at drivers/parisc/iommu-helpers.h:iommu_coalesce_chunks() and drivers/parisc/sba_iommu.c:sba_unmap_sg() for example.

The first fills out the sglist, and zeroes all unused entries past what it fills in. The unmap side than simply exits the loop if the entries are zeroed.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

The patch looks good to me, but while we touch this area, how about throwing in a few cosmetic fixes as well?

			/*
			 * Gap to the previous segment, we'll need to return
			 * and use another FR to map the reminder.
			 */
			if (last_end_dma_addr != dma_addr)
				break;

			/*
			 * See if this segment is contiguous to the
			 * previous one and just merge it in that case.
			 */
			if (last_page_off + dma_len <= mr->page_size) {
				last_end_dma_addr += dma_len;
				last_page_off += dma_len;
				mr->length += dma_len;
				continue;
			}

			/*
			 * New page-aligned segment to map:
			 */
			page_addr = last_page_addr + mr->page_size;
			dma_len -= mr->page_size - last_page_off;

How about the patch below ? In that version of the ib_sg_to_pages() fix these concerns have been addressed and additionally to more bugs have been fixed.

------------

[PATCH] IB core: Fix ib_sg_to_pages()

Fix the code for detecting gaps. A gap occurs not only if the second or later scatterlist element is not aligned but also if any scatterlist element other than the last does not end at a page boundary.

In the code for coalescing contiguous elements, ensure that mr->length is correct and that last_page_addr is up-to-date.

Ensure that this function returns a negative
error code instead of zero if the first set_page() call fails.

---
 drivers/infiniband/core/verbs.c | 43 +++++++++++++++++++++--------------------
 1 file changed, 22 insertions(+), 21 deletions(-)

index 043a60e..545906d 100644
@@ -1516,7 +1516,7 @@ EXPORT_SYMBOL(ib_map_mr_sg);
  * @sg_nents:      number of entries in sg
  * @set_page:      driver page assignment function pointer
  *
- * Core service helper for drivers to covert the largest
+ * Core service helper for drivers to convert the largest
  * prefix of given sg list to a page vector. The sg list
  * prefix converted is the prefix that meet the requirements
  * of ib_map_mr_sg.

@@ -1533,7 +1533,7 @@ int ib_sg_to_pages(struct ib_mr *mr,
 	u64 last_end_dma_addr = 0, last_page_addr = 0;
 	unsigned int last_page_off = 0;
 	u64 page_mask = ~((u64)mr->page_size - 1);
-	int i;
+	int i, ret;
 
 	mr->iova = sg_dma_address(&sgl[0]);
 	mr->length = 0;

@@ -1544,27 +1544,29 @@ int ib_sg_to_pages(struct ib_mr *mr,
 		u64 end_dma_addr = dma_addr + dma_len;
 		u64 page_addr = dma_addr & page_mask;
 
-		if (i && page_addr != dma_addr) {
-			if (last_end_dma_addr != dma_addr) {
-				/* gap */
-				goto done;
-
-			} else if (last_page_off + dma_len <= mr->page_size) {
-				/* chunk this fragment with the last */
-				mr->length += dma_len;
-				last_end_dma_addr += dma_len;
-				last_page_off += dma_len;
-				continue;
-			} else {
-				/* map starting from the next page */
-				page_addr = last_page_addr + mr->page_size;
-				dma_len -= mr->page_size - last_page_off;
-			}
+		/*
+		 * For the second and later elements, check whether either the
+		 * end of element i-1 or the start of element i is not aligned
+		 * on a page boundary.
+		 */
+		if (i && (last_page_off != 0 || page_addr != dma_addr)) {
+			/* Stop mapping if there is a gap. */
+			if (last_end_dma_addr != dma_addr)
+				break;
+
+			/*
+			 * Coalesce this element with the last. If it is small
+			 * enough just update mr->length. Otherwise start
+			 * mapping from the next page.
+			 */
+			goto next_page;
 		}
 
 		do {
-			if (unlikely(set_page(mr, page_addr)))
-				goto done;
+			ret = set_page(mr, page_addr);
+			if (unlikely(ret < 0))
+				return i ? : ret;
+next_page:
 			page_addr += mr->page_size;
 		} while (page_addr < end_dma_addr);
 

@@ -1574,7 +1576,6 @@ int ib_sg_to_pages(struct ib_mr *mr,
 		last_page_off = end_dma_addr & ~page_mask;
 	}
 
-done:
 	return i;
 }
 EXPORT_SYMBOL(ib_sg_to_pages);

2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hello,

While preparing this patch series I noticed that none of the SCSI RDMA initiator drivers syncs RDMA buffers before performing RDMA. Does anyone know why something like the code below is not present in these drivers and why no dma_sync_sg operations are present in struct ib_dma_mapping_ops ?

Thanks,

Bart.

[PATCH] IB/srp: Sync scatterlists before and after DMA

---
 drivers/infiniband/ulp/srp/ib_srp.c | 10 ++++++++++
 include/rdma/ib_verbs.h             | 16 ++++++++++++++++
 2 files changed, 26 insertions(+)

index 3db9a65..23e3c25 100644
@@ -1780,6 +1780,7 @@ static int srp_post_recv(struct srp_rdma_ch *ch, struct srp_iu *iu)
 static void srp_process_rsp(struct srp_rdma_ch *ch, struct srp_rsp *rsp)
 {
 	struct srp_target_port *target = ch->target;
+	struct ib_device *dev = target->srp_host->srp_dev->dev;
 	struct srp_request *req;
 	struct scsi_cmnd *scmnd;
 	unsigned long flags;

@@ -1828,6 +1829,11 @@ static void srp_process_rsp(struct srp_rdma_ch *ch, struct srp_rsp *rsp)
 		else if (unlikely(rsp->flags & SRP_RSP_FLAG_DOOVER))
 			scsi_set_resid(scmnd, -be32_to_cpu(rsp->data_out_res_cnt));
 
+		if (scmnd->sc_data_direction != DMA_NONE)
+			ib_dma_sync_sg_for_cpu(dev, scsi_sglist(scmnd),
+					       scsi_sg_count(scmnd),
+					       scmnd->sc_data_direction);
+
 		srp_free_req(ch, req, scmnd,
 			     be32_to_cpu(rsp->req_lim_delta));
 

@@ -2112,6 +2118,10 @@ static int srp_queuecommand(struct Scsi_Host *shost, struct scsi_cmnd *scmnd)
 
 	ib_dma_sync_single_for_device(dev, iu->dma, target->max_iu_len,
 				      DMA_TO_DEVICE);
+	if (scmnd->sc_data_direction != DMA_NONE)
+		ib_dma_sync_sg_for_device(dev, scsi_sglist(scmnd),
+					  scsi_sg_count(scmnd),
+					  scmnd->sc_data_direction);
 
 	if (srp_post_send(ch, iu, len)) {
 		shost_printk(KERN_ERR, target->scsi_host, PFX "Send failed\n");

index 9a68a19..5f9cba7 100644
@@ -2796,6 +2796,22 @@ static inline void ib_dma_sync_single_for_device(struct ib_device *dev,
 		dma_sync_single_for_device(dev->dma_device, addr, size, dir);
 }
 
+/* Prepare DMA region to be accessed by CPU */
+static inline void ib_dma_sync_sg_for_cpu(struct ib_device *dev,
+					  struct scatterlist *sg, int nelems,
+					  enum dma_data_direction dir)
+{
+	dma_sync_sg_for_cpu(dev->dma_device, sg, nelems, dir);
+}
+
+/* Prepare DMA region to be accessed by HCA */
+static inline void ib_dma_sync_sg_for_device(struct ib_device *dev,
+					     struct scatterlist *sg, int nelems,
+					     enum dma_data_direction dir)
+{
+	dma_sync_sg_for_device(dev->dma_device, sg, nelems, dir);
+}
+
 /**
  * ib_dma_alloc_coherent - Allocate memory and map it for DMA
  * @dev: The device for which the DMA address is requested

2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hi Bart,

dma_map*/dma_unmap* have to do implicit syns, so no explicit call is required. That's probably the reason why we don't need them in the weird RDMA shadows of these functions (for which I still don't understand the reason, while we're at it..).
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Looks fine,

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Looks good,

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Hello Christoph,

struct ib_dma_mapping_ops was added through commit "IB: Add DMA mapping functions to allow device drivers to interpose" (https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=9b513090a3c5e4964f9ac09016c1586988abb3d5). I'm not sure that these functions are still needed by the qib or ipath drivers since the .dma_address and .dma_len members have been removed some time ago from that structure (see also http://thread.gmane.org/gmane.linux.drivers.rdma/19730). It would be useful to have data available that shows the performance difference with and without struct ib_dma_mapping_ops for the qib and ipath drivers but I do not know whether such data is available publicly.

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

(replying to my own e-mail)

Hello Doug,

A consensus has been achieved about this patch series. Since one of the patches in this series fixes a data corruption bug that has been introduced during the latest (v4.4) merge window I'd like to see these patches upstream soon. Please recommend how I should proceed. When I repost this patch series, should I send it to you, to Linus or perhaps to someone else ?

Thanks,

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

I was just getting ready to pull this set in. The only one you actually changed, and therefore needs either a resubmit or I have to be careful to get the follow-on version in Patchworks is 5/6, yes?

Doug Ledford <[email protected]>
              GPG KeyID: 0E572FDD

Hello Doug,

You are correct, only for patch 5 the patch itself has been modified. The only change for the other patches is that Reviewed-by tags have been posted on the list.

Thanks,

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html