[PATCH 0/6] SRP initiator related bug fixes

[PATCH 0/6] SRP initiator related bug fixes

[Bart Van Assche]

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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/6] IB/srp: Fix a memory leak

[Bart Van Assche]

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

Signed-off-by: Bart Van Assche <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
Reviewed-by: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
Reviewed-by: Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Cc: Sebastian Parschauer <sebastian.riemer-EIkl63zCoXaH+58JC4qpiA@public.gmane.org>
Cc: stable <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
---
 drivers/infiniband/ulp/srp/ib_srp.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index 9909022..a074dad 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/6] IB/srp: Fix possible send queue overflow

[Bart Van Assche]

From: Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

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.

Signed-off-by: Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
CC: Stable <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
---
 drivers/infiniband/ulp/srp/ib_srp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index a074dad..c7a95d2 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/6] IB/srp: Fix possible send queue overflow

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/6] IB/srp: Initialize dma_length in srp_map_idb

[Bart Van Assche]

From: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>

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

Fixes: commit f7f7aab1a5c0 ("IB/srp: Convert to new registration API")
Signed-off-by: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
---
 drivers/infiniband/ulp/srp/ib_srp.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index c7a95d2..72fac20 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/6] IB/srp: Initialize dma_length in srp_map_idb

[Sagi Grimberg]

Reviewed-by: Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4/6] IB/srp: Fix indirect data buffer rkey endianness

[Bart Van Assche]

Detected by sparse.

Fixes: commit 330179f2fa93 ("IB/srp: Register the indirect data buffer descriptor")
Signed-off-by: Bart Van Assche <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
Cc: stable <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org> # v4.3+
Cc: Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Cc: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
Cc: Sebastian Parschauer <sebastian.riemer-EIkl63zCoXaH+58JC4qpiA@public.gmane.org>
---
 drivers/infiniband/ulp/srp/ib_srp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index 72fac20..fac1423 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/6] IB/srp: Fix indirect data buffer rkey endianness

[Sagi Grimberg]

On 01/12/2015 20:18, Bart Van Assche wrote:
> Detected by sparse.
>
> Fixes: commit 330179f2fa93 ("IB/srp: Register the indirect data buffer descriptor")
> Signed-off-by: Bart Van Assche <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
> Cc: stable <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org> # v4.3+
> Cc: Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> Cc: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
> Cc: Sebastian Parschauer <sebastian.riemer-EIkl63zCoXaH+58JC4qpiA@public.gmane.org>
> ---
>   drivers/infiniband/ulp/srp/ib_srp.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
> index 72fac20..fac1423 100644
> --- a/drivers/infiniband/ulp/srp/ib_srp.c
> +++ b/drivers/infiniband/ulp/srp/ib_srp.c
> @@ -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);
>   	}

Wouldn't it make more sense to define idb_rkey to be u32 and change
endianness when assigning it to the indirect desc?
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/6] IB/srp: Fix indirect data buffer rkey endianness

[Bart Van Assche]

On 12/01/2015 10:37 AM, Sagi Grimberg wrote:
> On 01/12/2015 20:18, Bart Van Assche wrote:
>> Detected by sparse.
>>
>> Fixes: commit 330179f2fa93 ("IB/srp: Register the indirect data buffer
>> descriptor")
>> Signed-off-by: Bart Van Assche <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
>> Cc: stable <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org> # v4.3+
>> Cc: Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
>> Cc: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
>> Cc: Sebastian Parschauer <sebastian.riemer-EIkl63zCoXaH+58JC4qpiA@public.gmane.org>
>> ---
>>   drivers/infiniband/ulp/srp/ib_srp.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/infiniband/ulp/srp/ib_srp.c
>> b/drivers/infiniband/ulp/srp/ib_srp.c
>> index 72fac20..fac1423 100644
>> --- a/drivers/infiniband/ulp/srp/ib_srp.c
>> +++ b/drivers/infiniband/ulp/srp/ib_srp.c
>> @@ -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);
>>       }
>
> Wouldn't it make more sense to define idb_rkey to be u32 and change
> endianness when assigning it to the indirect desc?

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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/6] IB/srp: Fix indirect data buffer rkey endianness

[Sagi Grimberg]

>>> diff --git a/drivers/infiniband/ulp/srp/ib_srp.c
>>> b/drivers/infiniband/ulp/srp/ib_srp.c
>>> index 72fac20..fac1423 100644
>>> --- a/drivers/infiniband/ulp/srp/ib_srp.c
>>> +++ b/drivers/infiniband/ulp/srp/ib_srp.c
>>> @@ -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);
>>>       }
>>
>> Wouldn't it make more sense to define idb_rkey to be u32 and change
>> endianness when assigning it to the indirect desc?
>
> 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.

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

Re: [PATCH 4/6] IB/srp: Fix indirect data buffer rkey endianness

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 5/6] IB core: Fix ib_sg_to_pages()

[Bart Van Assche]

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.

Fixes: commit 4c67e2bfc8b7 ("IB/core: Introduce new fast registration API")
Reported-by: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
Signed-off-by: Bart Van Assche <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
Cc: stable <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org> # v4.4+
Cc: Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Cc: Sebastian Parschauer <sebastian.riemer-EIkl63zCoXaH+58JC4qpiA@public.gmane.org>
---
 drivers/infiniband/core/verbs.c | 31 +++++++------------------------
 1 file changed, 7 insertions(+), 24 deletions(-)

diff --git a/drivers/infiniband/core/verbs.c b/drivers/infiniband/core/verbs.c
index 043a60e..95836c6 100644
--- a/drivers/infiniband/core/verbs.c
+++ b/drivers/infiniband/core/verbs.c
@@ -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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/6] IB core: Fix ib_sg_to_pages()

[Sagi Grimberg]

Hi Bart,

> 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.

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?

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

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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/6] IB core: Fix ib_sg_to_pages()

[Bart Van Assche]

On 12/01/2015 10:32 AM, Sagi Grimberg wrote:
>> 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.
>
> 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?

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.

>> Ensure that this function returns a negative error code instead of
>> zero if the first set_page() call fails.
>
> 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.

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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/6] IB core: Fix ib_sg_to_pages()

[Sagi Grimberg]

On 01/12/2015 21:10, Bart Van Assche wrote:
> On 12/01/2015 10:32 AM, Sagi Grimberg wrote:
>>> 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.
>>
>> 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?
>
> 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.

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?
diff --git a/drivers/infiniband/core/verbs.c 
b/drivers/infiniband/core/verbs.c
index 043a60e..23457fe 100644
--- a/drivers/infiniband/core/verbs.c
+++ b/drivers/infiniband/core/verbs.c
@@ -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.

> 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.

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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/6] IB core: Fix ib_sg_to_pages()

[Bart Van Assche]

On 12/02/2015 01:31 AM, Sagi Grimberg wrote:
> On 01/12/2015 21:10, Bart Van Assche wrote:
>> On 12/01/2015 10:32 AM, Sagi Grimberg wrote:
>> 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.
>
> 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.

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.

Fixes: commit 4c67e2bfc8b7 ("IB/core: Introduce new fast registration API")
Reported-by: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
---
 drivers/infiniband/core/verbs.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/infiniband/core/verbs.c b/drivers/infiniband/core/verbs.c
index 043a60e..1972c50 100644
--- a/drivers/infiniband/core/verbs.c
+++ b/drivers/infiniband/core/verbs.c
@@ -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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/6] IB core: Fix ib_sg_to_pages()

[Sagi Grimberg]

> 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.

I'm fine with that.

>
> How about the patch below ?

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

Re: [PATCH 5/6] IB core: Fix ib_sg_to_pages()

[Christoph Hellwig]

> How about the patch below ?

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

> -		if (i && page_addr != dma_addr) {
> +		if (i && (page_addr != dma_addr || last_page_off != 0)) {
>  			if (last_end_dma_addr != dma_addr) {

Wo about we one or two sentences for each of the conditions here?

>  				/* gap */
> -				goto done;
> -
> +				break;
>  			} else if (last_page_off + dma_len <= mr->page_size) {
>  				/* chunk this fragment with the last */
>  				mr->length += dma_len;

It would be great to avoid the else clauses if we already to a
break/continue/goto to make the code flow more clear, e.g.


			/*
			 * 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;


--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/6] IB core: Fix ib_sg_to_pages()

[Bart Van Assche]

On 12/03/2015 01:18 AM, Christoph Hellwig wrote:
> The patch looks good to me, but while we touch this area, how about
> throwing in a few cosmetic fixes as well?
 
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.

Fixes: commit 4c67e2bfc8b7 ("IB/core: Introduce new fast registration API")
Reported-by: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
---
 drivers/infiniband/core/verbs.c | 43 +++++++++++++++++++++--------------------
 1 file changed, 22 insertions(+), 21 deletions(-)

diff --git a/drivers/infiniband/core/verbs.c b/drivers/infiniband/core/verbs.c
index 043a60e..545906d 100644
--- a/drivers/infiniband/core/verbs.c
+++ b/drivers/infiniband/core/verbs.c
@@ -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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/6] IB core: Fix ib_sg_to_pages()

[Sagi Grimberg]

Looks fine,

Reviewed-by: Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/6] IB core: Fix ib_sg_to_pages()

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 6/6] IB/srp: Fix srp_map_sg_fr()

[Bart Van Assche]

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().

Fixes: commit f7f7aab1a5c0 ("IB/srp: Convert to new registration API")
Reported-by: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
Signed-off-by: Bart Van Assche <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
Cc: stable <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org> # v4.4+
Cc: Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Cc: Sebastian Parschauer <sebastian.riemer-EIkl63zCoXaH+58JC4qpiA@public.gmane.org>
---
 drivers/infiniband/ulp/srp/ib_srp.c | 19 ++++++++-----------
 drivers/infiniband/ulp/srp/ib_srp.h |  5 +----
 2 files changed, 9 insertions(+), 15 deletions(-)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index fac1423..3db9a65 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -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) {
diff --git a/drivers/infiniband/ulp/srp/ib_srp.h b/drivers/infiniband/ulp/srp/ib_srp.h
index 87a2a91..f6af531 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.h
+++ b/drivers/infiniband/ulp/srp/ib_srp.h
@@ -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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 6/6] IB/srp: Fix srp_map_sg_fr()

[Sagi Grimberg]

> 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().

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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 6/6] IB/srp: Fix srp_map_sg_fr()

[Bart Van Assche]

On 12/01/2015 10:35 AM, Sagi Grimberg wrote:
>> 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().
>
> 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?

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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 6/6] IB/srp: Fix srp_map_sg_fr()

[Sagi Grimberg]

On 01/12/2015 20:39, Bart Van Assche wrote:
> On 12/01/2015 10:35 AM, Sagi Grimberg wrote:
>>> 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().
>>
>> 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?
>
> 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>

 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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 6/6] IB/srp: Fix srp_map_sg_fr()

[Christoph Hellwig]

On Wed, Dec 02, 2015 at 01:59:38PM +0200, Sagi Grimberg wrote:
> >where nents is the number of entries in the sglist.
> ></quote>
> 
> 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>

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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 6/6] IB/srp: Fix srp_map_sg_fr()

[Sagi Grimberg]

> 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.

Oh I see, I didn't know that.

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

Reviewed-by: Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

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

Re: [PATCH 6/6] IB/srp: Fix srp_map_sg_fr()

[Sagi Grimberg]

Replying to my own email,


>> 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.

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

>> I'm not sure anyone still does that, but the first spot to check would
>> be the Parisc IOMMU drivers.

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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 6/6] IB/srp: Fix srp_map_sg_fr()

[Christoph Hellwig]

On Thu, Dec 03, 2015 at 10:46:10AM +0200, Sagi Grimberg wrote:
> >>   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.
> 
> So what would be in the last entry {dma_addr, dma_len}? zeros?
> 
> >>I'm not sure anyone still does that, but the first spot to check would
> >>be the Parisc IOMMU drivers.
> 
> 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?

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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/6] SRP initiator related bug fixes

[Bart Van Assche]

On 12/01/2015 10:16 AM, Bart Van Assche wrote:
> [ ... ]

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(+)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index 3db9a65..23e3c25 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -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");
diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
index 9a68a19..5f9cba7 100644
--- a/include/rdma/ib_verbs.h
+++ b/include/rdma/ib_verbs.h
@@ -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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/6] SRP initiator related bug fixes

[Christoph Hellwig]

Hi Bart,

On Fri, Dec 04, 2015 at 03:08:15PM -0800, Bart Van Assche wrote:
> 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 ?

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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/6] SRP initiator related bug fixes

[Bart Van Assche]

On 12/05/2015 03:17 AM, Christoph Hellwig wrote:
> Hi Bart,
>
> On Fri, Dec 04, 2015 at 03:08:15PM -0800, Bart Van Assche wrote:
>> 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 ?
>
> dma_map*/dma_unmap* have to do implicit syncs, 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..).

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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/6] SRP initiator related bug fixes

[Bart Van Assche]

On 12/01/2015 10:16 AM, Bart Van Assche wrote:
> 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

(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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/6] SRP initiator related bug fixes

[Doug Ledford]

[-- Attachment #1: Type: text/plain, Size: 1441 bytes --]

On 12/07/2015 02:26 PM, Bart Van Assche wrote:
> On 12/01/2015 10:16 AM, Bart Van Assche wrote:
>> 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
> 
> (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 ?

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 <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
              GPG KeyID: 0E572FDD



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]

Re: [PATCH 0/6] SRP initiator related bug fixes

[Bart Van Assche]

On 12/07/2015 01:07 PM, Doug Ledford wrote:
> On 12/07/2015 02:26 PM, Bart Van Assche wrote:
>> On 12/01/2015 10:16 AM, Bart Van Assche wrote:
>>> 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
>>
>> (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 ?
>
> 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?

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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html