RDMA/siw: Enable SGL's with mixed memory types

Commit Message

Bernard Metzler Aug. 22, 2019, 12:16 p.m. UTC

This patch enables the transmission of work requests with SGL's
of mixed types, e.g. kernel buffers and PBL regions referenced
by same work request. This enables iSER as a kernel client.

Reported-by: Krishnamraju Eraparaju <[e-mail]>
Tested-by: Krishnamraju Eraparaju <[e-mail]>
Signed-off-by: Bernard Metzler <[e-mail]>
---
 drivers/infiniband/sw/siw/siw_qp_tx.c | 37 +++++++++++----------------
 1 file changed, 15 insertions(+), 22 deletions(-)

Comments

Doug Ledford Aug. 22, 2019, 2:44 p.m. UTC | #1

On Thu, 2019-08-22 at 14:16 +0200, Bernard Metzler wrote:
> This patch enables the transmission of work requests with SGL's
> of mixed types, e.g. kernel buffers and PBL regions referenced
> by same work request. This enables iSER as a kernel client.
> 
> Reported-by: Krishnamraju Eraparaju <[e-mail]>
> Tested-by: Krishnamraju Eraparaju <[e-mail]>
> Signed-off-by: Bernard Metzler <[e-mail]>

Hi Bernard,

Commit subject and message are much better this time.  However, it's rc5
already, and we *just* merged siw this merge cycle, so I'd rather have
this in the next -rc pull and not in for-next so that siw "just works"
across the board on initial release.  Your language in a commit message
makes all the difference in the world in terms of whether or not a
commit should go to for-rc.  In this case, you used "Enable SGL's...". 
Enable is new feature language.  For the rc cycles, you need Fix
language.  Something like this:

RDMA/siw: Fix SGE element mapping issues

Most upper layer kernel modules submit WQEs where the SG list entries
are all of a single type.  iSER in particular, however, will send us
WQEs with mixed SG types: sge[0] = kernel buffer, sge[1] = PBL region. 
Check and set is_kva on each SG entry individually instead of assuming
the first SGE type carries through to the last.  This fixes iSER over
siw.

Same patch, but the difference in wording makes a world of difference in
terms of whether or not Linus will give you the evil eye for sending it
in an -rc cycle.  And really, you didn't care about enabling SGLs with
mixed memory types.  It's not like that's some sort of sought after
feature.  It was what was needed to fix siw.  So just remember that in
the future.  Fix language for fixes, enable language for features.  The
difference does matter ;-)

Please resubmit with a fixed commit message and a Fixes: tag.

Bernard Metzler Aug. 22, 2019, 3:19 p.m. UTC | #2

---
Bernard Metzler, PhD
Tech. Leader High Performance I/O, Principal Research Staff
IBM Zurich Research Laboratory
Saeumerstrasse 4
CH-8803 Rueschlikon, Switzerland
[phone]

-----"Doug Ledford" <[e-mail]> wrote: -----

>To: "Bernard Metzler" <[e-mail]>,
>[e-mail]
>From: "Doug Ledford" <[e-mail]>
>Date: 08/22/2019 04:44PM
>Cc: [e-mail]
>Subject: [EXTERNAL] Re: [PATCH] RDMA/siw: Enable SGL's with mixed
>memory types
>
>On Thu, 2019-08-22 at 14:16 +0200, Bernard Metzler wrote:
>> This patch enables the transmission of work requests with SGL's
>> of mixed types, e.g. kernel buffers and PBL regions referenced
>> by same work request. This enables iSER as a kernel client.
>> 
>> Reported-by: Krishnamraju Eraparaju <[e-mail]>
>> Tested-by: Krishnamraju Eraparaju <[e-mail]>
>> Signed-off-by: Bernard Metzler <[e-mail]>
>
>Hi Bernard,
>
>Commit subject and message are much better this time.  However, it's
>rc5
>already, and we *just* merged siw this merge cycle, so I'd rather
>have
>this in the next -rc pull and not in for-next so that siw "just
>works"
>across the board on initial release.  Your language in a commit
>message
>makes all the difference in the world in terms of whether or not a
>commit should go to for-rc.  In this case, you used "Enable
>SGL's...". 
>Enable is new feature language.  For the rc cycles, you need Fix
>language.  Something like this:
>
>RDMA/siw: Fix SGE element mapping issues
>
>Most upper layer kernel modules submit WQEs where the SG list entries
>are all of a single type.  iSER in particular, however, will send us
>WQEs with mixed SG types: sge[0] = kernel buffer, sge[1] = PBL
>region. 
>Check and set is_kva on each SG entry individually instead of
>assuming
>the first SGE type carries through to the last.  This fixes iSER over
>siw.
>
>Same patch, but the difference in wording makes a world of difference
>in
>terms of whether or not Linus will give you the evil eye for sending
>it
>in an -rc cycle.  And really, you didn't care about enabling SGLs
>with
>mixed memory types.  It's not like that's some sort of sought after
>feature.  It was what was needed to fix siw.  So just remember that
>in
>the future.  Fix language for fixes, enable language for features.

Makes sense, I'll try hard to adhere to that in the future.


>The
>difference does matter ;-)
>
>Please resubmit with a fixed commit message and a Fixes: tag.
>
>-- 
>Doug Ledford <[e-mail]>
>    GPG KeyID: B826A3330E572FDD
>    Fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD
>
[attachment "signature.asc" removed by Bernard Metzler/Zurich/IBM]

Patch

diff --git a/drivers/infiniband/sw/siw/siw_qp_tx.c b/drivers/infiniband/sw/siw/siw_qp_tx.c
index 43020d2040fc..42c63622c7bd 100644
--- a/drivers/infiniband/sw/siw/siw_qp_tx.c
+++ b/drivers/infiniband/sw/siw/siw_qp_tx.c
@@ -398,15 +398,13 @@  static int siw_0copy_tx(struct socket *s, struct page **page,
 
 #define MAX_TRAILER (MPA_CRC_SIZE + 4)
 
-static void siw_unmap_pages(struct page **pages, int hdr_len, int num_maps)
+static void siw_unmap_pages(struct page **pp, unsigned long kmap_mask)
 {
-	if (hdr_len) {
-		++pages;
-		--num_maps;
-	}
-	while (num_maps-- > 0) {
-		kunmap(*pages);
-		pages++;
+	while (kmap_mask) {
+		if (kmap_mask & BIT(0))
+			kunmap(*pp);
+		pp++;
+		kmap_mask >>= 1;
 	}
 }
 
@@ -437,6 +435,7 @@  static int siw_tx_hdt(struct siw_iwarp_tx *c_tx, struct socket *s)
 	unsigned int data_len = c_tx->bytes_unsent, hdr_len = 0, trl_len = 0,
 		     sge_off = c_tx->sge_off, sge_idx = c_tx->sge_idx,
 		     pbl_idx = c_tx->pbl_idx;
+	unsigned long kmap_mask = 0L;
 
 	if (c_tx->state == SIW_SEND_HDR) {
 		if (c_tx->use_sendpage) {
@@ -463,8 +462,7 @@  static int siw_tx_hdt(struct siw_iwarp_tx *c_tx, struct socket *s)
 
 		if (!(tx_flags(wqe) & SIW_WQE_INLINE)) {
 			mem = wqe->mem[sge_idx];
-			if (!mem->mem_obj)
-				is_kva = 1;
+			is_kva = mem->mem_obj == NULL ? 1 : 0;
 		} else {
 			is_kva = 1;
 		}
@@ -500,12 +498,7 @@  static int siw_tx_hdt(struct siw_iwarp_tx *c_tx, struct socket *s)
 					p = siw_get_upage(mem->umem,
 							  sge->laddr + sge_off);
 				if (unlikely(!p)) {
-					if (hdr_len)
-						seg--;
-					if (!c_tx->use_sendpage && seg) {
-						siw_unmap_pages(page_array,
-								hdr_len, seg);
-					}
+					siw_unmap_pages(page_array, kmap_mask);
 					wqe->processed -= c_tx->bytes_unsent;
 					rv = -EFAULT;
 					goto done_crc;
@@ -515,6 +508,10 @@  static int siw_tx_hdt(struct siw_iwarp_tx *c_tx, struct socket *s)
 				if (!c_tx->use_sendpage) {
 					iov[seg].iov_base = kmap(p) + fp_off;
 					iov[seg].iov_len = plen;
+
+					/* Remember for later kunmap() */
+					kmap_mask |= BIT(seg);
+
 					if (do_crc)
 						crypto_shash_update(
 							c_tx->mpa_crc_hd,
@@ -543,10 +540,7 @@  static int siw_tx_hdt(struct siw_iwarp_tx *c_tx, struct socket *s)
 
 			if (++seg > (int)MAX_ARRAY) {
 				siw_dbg_qp(tx_qp(c_tx), "to many fragments\n");
-				if (!is_kva && !c_tx->use_sendpage) {
-					siw_unmap_pages(page_array, hdr_len,
-							seg - 1);
-				}
+				siw_unmap_pages(page_array, kmap_mask);
 				wqe->processed -= c_tx->bytes_unsent;
 				rv = -EMSGSIZE;
 				goto done_crc;
@@ -597,8 +591,7 @@  static int siw_tx_hdt(struct siw_iwarp_tx *c_tx, struct socket *s)
 	} else {
 		rv = kernel_sendmsg(s, &msg, iov, seg + 1,
 				    hdr_len + data_len + trl_len);
-		if (!is_kva)
-			siw_unmap_pages(page_array, hdr_len, seg);
+		siw_unmap_pages(page_array, kmap_mask);
 	}
 	if (rv < (int)hdr_len) {
 		/* Not even complete hdr pushed or negative rv */