[PATCH 0/4] Remove use of kmap()

[PATCH 0/4] Remove use of kmap()

[ira.weiny]

From: Ira Weiny <ira.weiny@intel.com>

kmap() is being deprecated and will break uses of device dax after PKS
protection is introduced.[1]

These kmap() usages don't need to be global and work fine as thread local
mappings.

Replace these kmap() calls with kmap_local_page() which is more appropriate.

The only final use of kmap() in the RDMA subsystem is in the qib driver which
is pretty old at this point.  The use is pretty convoluted and I doubt systems
using that driver are using persistent memory.  So it is left as is.  If this
is a problem I can dig into converting it as well.

[1] https://lore.kernel.org/lkml/20201009195033.3208459-59-ira.weiny@intel.com/

Ira Weiny (4):
  RDMA/hfi1: Remove use of kmap()
  RDMA/i40iw: Remove use of kmap()
  RDMA/siw: Remove kmap()
  RDMA/siw: Convert siw_tx_hdt() to kmap_local_page()

 drivers/infiniband/hw/hfi1/sdma.c      |  4 +--
 drivers/infiniband/hw/i40iw/i40iw_cm.c | 10 +++---
 drivers/infiniband/sw/siw/siw_qp_tx.c  | 47 +++++++++++++++-----------
 3 files changed, 34 insertions(+), 27 deletions(-)

-- 
2.28.0.rc0.12.gb6a658bd00c9

[PATCH 1/4] RDMA/hfi1: Remove use of kmap()

[ira.weiny]

From: Ira Weiny <ira.weiny@intel.com>

kmap() is being deprecated and will break uses of device dax after PKS
protection is introduced.[1]

The kmap() used in sdma does not need to be global.  Use the new
kmap_local_page() which works with PKS and may provide better
performance for this thread local mapping anyway.

[1] https://lore.kernel.org/lkml/20201009195033.3208459-59-ira.weiny@intel.com/

Signed-off-by: Ira Weiny <ira.weiny@intel.com>
---
 drivers/infiniband/hw/hfi1/sdma.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/infiniband/hw/hfi1/sdma.c b/drivers/infiniband/hw/hfi1/sdma.c
index 46b5290b2839..af43dcbb0928 100644
--- a/drivers/infiniband/hw/hfi1/sdma.c
+++ b/drivers/infiniband/hw/hfi1/sdma.c
@@ -3130,7 +3130,7 @@ int ext_coal_sdma_tx_descs(struct hfi1_devdata *dd, struct sdma_txreq *tx,
 		}
 
 		if (type == SDMA_MAP_PAGE) {
-			kvaddr = kmap(page);
+			kvaddr = kmap_local_page(page);
 			kvaddr += offset;
 		} else if (WARN_ON(!kvaddr)) {
 			__sdma_txclean(dd, tx);
@@ -3140,7 +3140,7 @@ int ext_coal_sdma_tx_descs(struct hfi1_devdata *dd, struct sdma_txreq *tx,
 		memcpy(tx->coalesce_buf + tx->coalesce_idx, kvaddr, len);
 		tx->coalesce_idx += len;
 		if (type == SDMA_MAP_PAGE)
-			kunmap(page);
+			kunmap_local(kvaddr);
 
 		/* If there is more data, return */
 		if (tx->tlen - tx->coalesce_idx)
-- 
2.28.0.rc0.12.gb6a658bd00c9

Re: [PATCH 1/4] RDMA/hfi1: Remove use of kmap()

[Jason Gunthorpe]

On Mon, Jun 21, 2021 at 11:14:19PM -0700, ira.weiny@intel.com wrote:
> From: Ira Weiny <ira.weiny@intel.com>
> 
> kmap() is being deprecated and will break uses of device dax after PKS
> protection is introduced.[1]
> 
> The kmap() used in sdma does not need to be global.  Use the new
> kmap_local_page() which works with PKS and may provide better
> performance for this thread local mapping anyway.
> 
> [1] https://lore.kernel.org/lkml/20201009195033.3208459-59-ira.weiny@intel.com/
> 
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> ---
>  drivers/infiniband/hw/hfi1/sdma.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Applied to for-next, thanks

Jason

[PATCH 2/4] RDMA/i40iw: Remove use of kmap()

[ira.weiny]

From: Ira Weiny <ira.weiny@intel.com>

kmap() is being deprecated and will break uses of device dax after PKS
protection is introduced.[1]

The kmap() used in the i40iw CM driver is thread local.  Therefore
kmap_local_page() sufficient to use and may provide performance benefits
as well.  kmap_local_page() will work with device dax and pgmap
protected pages.

Use kmap_local_page() instead of kmap().

[1] https://lore.kernel.org/lkml/20201009195033.3208459-59-ira.weiny@intel.com/

Signed-off-by: Ira Weiny <ira.weiny@intel.com>
---
 drivers/infiniband/hw/i40iw/i40iw_cm.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/infiniband/hw/i40iw/i40iw_cm.c b/drivers/infiniband/hw/i40iw/i40iw_cm.c
index ac65c8237b2e..13e40ebd5322 100644
--- a/drivers/infiniband/hw/i40iw/i40iw_cm.c
+++ b/drivers/infiniband/hw/i40iw/i40iw_cm.c
@@ -3725,7 +3725,7 @@ int i40iw_accept(struct iw_cm_id *cm_id, struct iw_cm_conn_param *conn_param)
 		ibmr->device = iwpd->ibpd.device;
 		iwqp->lsmm_mr = ibmr;
 		if (iwqp->page)
-			iwqp->sc_qp.qp_uk.sq_base = kmap(iwqp->page);
+			iwqp->sc_qp.qp_uk.sq_base = kmap_local_page(iwqp->page);
 		dev->iw_priv_qp_ops->qp_send_lsmm(&iwqp->sc_qp,
 							iwqp->ietf_mem.va,
 							(accept.size + conn_param->private_data_len),
@@ -3733,12 +3733,12 @@ int i40iw_accept(struct iw_cm_id *cm_id, struct iw_cm_conn_param *conn_param)
 
 	} else {
 		if (iwqp->page)
-			iwqp->sc_qp.qp_uk.sq_base = kmap(iwqp->page);
+			iwqp->sc_qp.qp_uk.sq_base = kmap_local_page(iwqp->page);
 		dev->iw_priv_qp_ops->qp_send_lsmm(&iwqp->sc_qp, NULL, 0, 0);
 	}
 
 	if (iwqp->page)
-		kunmap(iwqp->page);
+		kunmap_local(iwqp->sc_qp.qp_uk.sq_base);
 
 	iwqp->cm_id = cm_id;
 	cm_node->cm_id = cm_id;
@@ -4106,10 +4106,10 @@ static void i40iw_cm_event_connected(struct i40iw_cm_event *event)
 	i40iw_cm_init_tsa_conn(iwqp, cm_node);
 	read0 = (cm_node->send_rdma0_op == SEND_RDMA_READ_ZERO);
 	if (iwqp->page)
-		iwqp->sc_qp.qp_uk.sq_base = kmap(iwqp->page);
+		iwqp->sc_qp.qp_uk.sq_base = kmap_local_page(iwqp->page);
 	dev->iw_priv_qp_ops->qp_send_rtt(&iwqp->sc_qp, read0);
 	if (iwqp->page)
-		kunmap(iwqp->page);
+		kunmap_local(iwqp->sc_qp.qp_uk.sq_base);
 
 	memset(&attr, 0, sizeof(attr));
 	attr.qp_state = IB_QPS_RTS;
-- 
2.28.0.rc0.12.gb6a658bd00c9

Re: [PATCH 2/4] RDMA/i40iw: Remove use of kmap()

[Jason Gunthorpe]

On Mon, Jun 21, 2021 at 11:14:20PM -0700, ira.weiny@intel.com wrote:
> From: Ira Weiny <ira.weiny@intel.com>
> 
> kmap() is being deprecated and will break uses of device dax after PKS
> protection is introduced.[1]
> 
> The kmap() used in the i40iw CM driver is thread local.  Therefore
> kmap_local_page() sufficient to use and may provide performance benefits
> as well.  kmap_local_page() will work with device dax and pgmap
> protected pages.
> 
> Use kmap_local_page() instead of kmap().
> 
> [1] https://lore.kernel.org/lkml/20201009195033.3208459-59-ira.weiny@intel.com/
> 
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> ---
>  drivers/infiniband/hw/i40iw/i40iw_cm.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)

This needs to be resent against irdma instead

Jason

[PATCH V2] RDMA/irdma: Remove use of kmap()

[ira.weiny]

From: Ira Weiny <ira.weiny@intel.com>

kmap() is being deprecated and will break uses of device dax after PKS
protection is introduced.[1]

The kmap() used in the irdma CM driver is thread local.  Therefore
kmap_local_page() is sufficient to use and may provide performance benefits
as well.  kmap_local_page() will work with device dax and pgmap
protected pages.

Use kmap_local_page() instead of kmap().

[1] https://lore.kernel.org/lkml/20201009195033.3208459-59-ira.weiny@intel.com/

Signed-off-by: Ira Weiny <ira.weiny@intel.com>

---
Changes for V2:
	Move to the new irdma driver for 5.14
---
 drivers/infiniband/hw/irdma/cm.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/infiniband/hw/irdma/cm.c b/drivers/infiniband/hw/irdma/cm.c
index 3d2bdb033a54..6b62299abfbb 100644
--- a/drivers/infiniband/hw/irdma/cm.c
+++ b/drivers/infiniband/hw/irdma/cm.c
@@ -3675,14 +3675,14 @@ int irdma_accept(struct iw_cm_id *cm_id, struct iw_cm_conn_param *conn_param)
 	ibmr->device = iwpd->ibpd.device;
 	iwqp->lsmm_mr = ibmr;
 	if (iwqp->page)
-		iwqp->sc_qp.qp_uk.sq_base = kmap(iwqp->page);
+		iwqp->sc_qp.qp_uk.sq_base = kmap_local_page(iwqp->page);
 
 	cm_node->lsmm_size = accept.size + conn_param->private_data_len;
 	irdma_sc_send_lsmm(&iwqp->sc_qp, iwqp->ietf_mem.va, cm_node->lsmm_size,
 			   ibmr->lkey);
 
 	if (iwqp->page)
-		kunmap(iwqp->page);
+		kunmap_local(iwqp->sc_qp.qp_uk.sq_base);
 
 	iwqp->cm_id = cm_id;
 	cm_node->cm_id = cm_id;
@@ -4093,10 +4093,10 @@ static void irdma_cm_event_connected(struct irdma_cm_event *event)
 	irdma_cm_init_tsa_conn(iwqp, cm_node);
 	read0 = (cm_node->send_rdma0_op == SEND_RDMA_READ_ZERO);
 	if (iwqp->page)
-		iwqp->sc_qp.qp_uk.sq_base = kmap(iwqp->page);
+		iwqp->sc_qp.qp_uk.sq_base = kmap_local_page(iwqp->page);
 	irdma_sc_send_rtt(&iwqp->sc_qp, read0);
 	if (iwqp->page)
-		kunmap(iwqp->page);
+		kunmap_local(iwqp->sc_qp.qp_uk.sq_base);
 
 	attr.qp_state = IB_QPS_RTS;
 	cm_node->qhash_set = false;
-- 
2.28.0.rc0.12.gb6a658bd00c9

Re: [PATCH V2] RDMA/irdma: Remove use of kmap()

[Jason Gunthorpe]

On Tue, Jun 22, 2021 at 09:56:22AM -0700, ira.weiny@intel.com wrote:
> From: Ira Weiny <ira.weiny@intel.com>
> 
> kmap() is being deprecated and will break uses of device dax after PKS
> protection is introduced.[1]
> 
> The kmap() used in the irdma CM driver is thread local.  Therefore
> kmap_local_page() is sufficient to use and may provide performance benefits
> as well.  kmap_local_page() will work with device dax and pgmap
> protected pages.
> 
> Use kmap_local_page() instead of kmap().
> 
> [1] https://lore.kernel.org/lkml/20201009195033.3208459-59-ira.weiny@intel.com/
> 
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> ---
> Changes for V2:
> 	Move to the new irdma driver for 5.14
> ---
>  drivers/infiniband/hw/irdma/cm.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)

Applied to for-next, thanks

Jason

[PATCH 3/4] RDMA/siw: Remove kmap()

[ira.weiny]

From: Ira Weiny <ira.weiny@intel.com>

kmap() is being deprecated and will break uses of device dax after PKS
protection is introduced.[1]

These uses of kmap() in the SIW driver are thread local.  Therefore
kmap_local_page() is sufficient to use and will work with pgmap
protected pages when those are implemnted.

There is one more use of kmap() in this driver which is split into its
own patch because kmap_local_page() has strict ordering rules and the
use of the kmap_mask over multiple segments must be handled carefully.
Therefore, that conversion is handled in a stand alone patch.

Use kmap_local_page() instead of kmap() in the 'easy' cases.

[1] https://lore.kernel.org/lkml/20201009195033.3208459-59-ira.weiny@intel.com/

Signed-off-by: Ira Weiny <ira.weiny@intel.com>
---
 drivers/infiniband/sw/siw/siw_qp_tx.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/infiniband/sw/siw/siw_qp_tx.c b/drivers/infiniband/sw/siw/siw_qp_tx.c
index 7989c4043db4..db68a10d12cd 100644
--- a/drivers/infiniband/sw/siw/siw_qp_tx.c
+++ b/drivers/infiniband/sw/siw/siw_qp_tx.c
@@ -76,7 +76,7 @@ static int siw_try_1seg(struct siw_iwarp_tx *c_tx, void *paddr)
 			if (unlikely(!p))
 				return -EFAULT;
 
-			buffer = kmap(p);
+			buffer = kmap_local_page(p);
 
 			if (likely(PAGE_SIZE - off >= bytes)) {
 				memcpy(paddr, buffer + off, bytes);
@@ -84,7 +84,7 @@ static int siw_try_1seg(struct siw_iwarp_tx *c_tx, void *paddr)
 				unsigned long part = bytes - (PAGE_SIZE - off);
 
 				memcpy(paddr, buffer + off, part);
-				kunmap(p);
+				kunmap_local(buffer);
 
 				if (!mem->is_pbl)
 					p = siw_get_upage(mem->umem,
@@ -96,10 +96,10 @@ static int siw_try_1seg(struct siw_iwarp_tx *c_tx, void *paddr)
 				if (unlikely(!p))
 					return -EFAULT;
 
-				buffer = kmap(p);
+				buffer = kmap_local_page(p);
 				memcpy(paddr + part, buffer, bytes - part);
 			}
-			kunmap(p);
+			kunmap_local(buffer);
 		}
 	}
 	return (int)bytes;
@@ -485,6 +485,7 @@ static int siw_tx_hdt(struct siw_iwarp_tx *c_tx, struct socket *s)
 
 		while (sge_len) {
 			size_t plen = min((int)PAGE_SIZE - fp_off, sge_len);
+			void *kaddr;
 
 			if (!is_kva) {
 				struct page *p;
@@ -517,10 +518,11 @@ static int siw_tx_hdt(struct siw_iwarp_tx *c_tx, struct socket *s)
 							iov[seg].iov_base,
 							plen);
 				} else if (do_crc) {
+					kaddr = kmap_local_page(p);
 					crypto_shash_update(c_tx->mpa_crc_hd,
-							    kmap(p) + fp_off,
+							    kaddr + fp_off,
 							    plen);
-					kunmap(p);
+					kunmap_local(kaddr);
 				}
 			} else {
 				u64 va = sge->laddr + sge_off;
-- 
2.28.0.rc0.12.gb6a658bd00c9

Re: [PATCH 3/4] RDMA/siw: Remove kmap()

[Jason Gunthorpe]

On Mon, Jun 21, 2021 at 11:14:21PM -0700, ira.weiny@intel.com wrote:
> From: Ira Weiny <ira.weiny@intel.com>
> 
> kmap() is being deprecated and will break uses of device dax after PKS
> protection is introduced.[1]
> 
> These uses of kmap() in the SIW driver are thread local.  Therefore
> kmap_local_page() is sufficient to use and will work with pgmap
> protected pages when those are implemnted.
> 
> There is one more use of kmap() in this driver which is split into its
> own patch because kmap_local_page() has strict ordering rules and the
> use of the kmap_mask over multiple segments must be handled carefully.
> Therefore, that conversion is handled in a stand alone patch.
> 
> Use kmap_local_page() instead of kmap() in the 'easy' cases.
> 
> [1] https://lore.kernel.org/lkml/20201009195033.3208459-59-ira.weiny@intel.com/
> 
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> ---
>  drivers/infiniband/sw/siw/siw_qp_tx.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)

Applied to for-next, thanks

Jason

[PATCH 4/4] RDMA/siw: Convert siw_tx_hdt() to kmap_local_page()

[ira.weiny]

From: Ira Weiny <ira.weiny@intel.com>

kmap() is being deprecated and will break uses of device dax after PKS
protection is introduced.[1]

The use of kmap() in siw_tx_hdt() is all thread local therefore
kmap_local_page() is a sufficient replacement and will work with pgmap
protected pages when those are implemented.

kmap_local_page() mappings are tracked in a stack and must be unmapped
in the opposite order they were mapped in.

siw_tx_hdt() tracks pages used in a page_array.  It uses that array to
unmap pages which were mapped on function exit.  Not all entries in the
array are mapped and this is tracked in kmap_mask.

kunmap_local() takes a mapped address rather than a page.  Declare a
mapped address array, page_array_addr, of the same size as the page
array to be used for unmapping.

Use kmap_local_page() instead of kmap() to map pages in the page_array.

Because segments are mapped into the page array in increasing index
order, modify siw_unmap_pages() to unmap pages in decreasing order.

The kmap_mask is no longer needed as the lack of an address in the
address array can indicate no unmap is required.

[1] https://lore.kernel.org/lkml/20201009195033.3208459-59-ira.weiny@intel.com/

Signed-off-by: Ira Weiny <ira.weiny@intel.com>
---
 drivers/infiniband/sw/siw/siw_qp_tx.c | 35 +++++++++++++++------------
 1 file changed, 20 insertions(+), 15 deletions(-)

diff --git a/drivers/infiniband/sw/siw/siw_qp_tx.c b/drivers/infiniband/sw/siw/siw_qp_tx.c
index db68a10d12cd..e70aba23f6e7 100644
--- a/drivers/infiniband/sw/siw/siw_qp_tx.c
+++ b/drivers/infiniband/sw/siw/siw_qp_tx.c
@@ -396,13 +396,17 @@ 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 **pp, unsigned long kmap_mask)
+static void siw_unmap_pages(void **addrs, int len)
 {
-	while (kmap_mask) {
-		if (kmap_mask & BIT(0))
-			kunmap(*pp);
-		pp++;
-		kmap_mask >>= 1;
+	int i;
+
+	/*
+	 * Work backwards through the array to honor the kmap_local_page()
+	 * ordering requirements.
+	 */
+	for (i = (len-1); i >= 0; i--) {
+		if (addrs[i])
+			kunmap_local(addrs[i]);
 	}
 }
 
@@ -427,13 +431,15 @@ static int siw_tx_hdt(struct siw_iwarp_tx *c_tx, struct socket *s)
 	struct siw_sge *sge = &wqe->sqe.sge[c_tx->sge_idx];
 	struct kvec iov[MAX_ARRAY];
 	struct page *page_array[MAX_ARRAY];
+	void *page_array_addr[MAX_ARRAY];
 	struct msghdr msg = { .msg_flags = MSG_DONTWAIT | MSG_EOR };
 
 	int seg = 0, do_crc = c_tx->do_crc, is_kva = 0, rv;
 	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;
+
+	memset(page_array_addr, 0, sizeof(page_array_addr));
 
 	if (c_tx->state == SIW_SEND_HDR) {
 		if (c_tx->use_sendpage) {
@@ -498,7 +504,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)) {
-					siw_unmap_pages(page_array, kmap_mask);
+					siw_unmap_pages(page_array_addr, MAX_ARRAY);
 					wqe->processed -= c_tx->bytes_unsent;
 					rv = -EFAULT;
 					goto done_crc;
@@ -506,11 +512,10 @@ static int siw_tx_hdt(struct siw_iwarp_tx *c_tx, struct socket *s)
 				page_array[seg] = p;
 
 				if (!c_tx->use_sendpage) {
-					iov[seg].iov_base = kmap(p) + fp_off;
-					iov[seg].iov_len = plen;
+					page_array_addr[seg] = kmap_local_page(page_array[seg]);
 
-					/* Remember for later kunmap() */
-					kmap_mask |= BIT(seg);
+					iov[seg].iov_base = page_array_addr[seg] + fp_off;
+					iov[seg].iov_len = plen;
 
 					if (do_crc)
 						crypto_shash_update(
@@ -518,7 +523,7 @@ static int siw_tx_hdt(struct siw_iwarp_tx *c_tx, struct socket *s)
 							iov[seg].iov_base,
 							plen);
 				} else if (do_crc) {
-					kaddr = kmap_local_page(p);
+					kaddr = kmap_local_page(page_array[seg]);
 					crypto_shash_update(c_tx->mpa_crc_hd,
 							    kaddr + fp_off,
 							    plen);
@@ -542,7 +547,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");
-				siw_unmap_pages(page_array, kmap_mask);
+				siw_unmap_pages(page_array_addr, MAX_ARRAY);
 				wqe->processed -= c_tx->bytes_unsent;
 				rv = -EMSGSIZE;
 				goto done_crc;
@@ -593,7 +598,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);
-		siw_unmap_pages(page_array, kmap_mask);
+		siw_unmap_pages(page_array_addr, MAX_ARRAY);
 	}
 	if (rv < (int)hdr_len) {
 		/* Not even complete hdr pushed or negative rv */
-- 
2.28.0.rc0.12.gb6a658bd00c9

Re: [PATCH 4/4] RDMA/siw: Convert siw_tx_hdt() to kmap_local_page()

[Bernard Metzler]

-----ira.weiny@intel.com wrote: -----

>To: "Jason Gunthorpe" <jgg@ziepe.ca>
>From: ira.weiny@intel.com
>Date: 06/22/2021 08:14AM
>Cc: "Ira Weiny" <ira.weiny@intel.com>, "Mike Marciniszyn"
><mike.marciniszyn@cornelisnetworks.com>, "Dennis Dalessandro"
><dennis.dalessandro@cornelisnetworks.com>, "Doug Ledford"
><dledford@redhat.com>, "Faisal Latif" <faisal.latif@intel.com>,
>"Shiraz Saleem" <shiraz.saleem@intel.com>, "Bernard Metzler"
><bmt@zurich.ibm.com>, "Kamal Heib" <kheib@redhat.com>,
>linux-rdma@vger.kernel.org, linux-kernel@vger.kernel.org
>Subject: [EXTERNAL] [PATCH 4/4] RDMA/siw: Convert siw_tx_hdt() to
>kmap_local_page()
>
>From: Ira Weiny <ira.weiny@intel.com>
>
>kmap() is being deprecated and will break uses of device dax after
>PKS
>protection is introduced.[1]
>
>The use of kmap() in siw_tx_hdt() is all thread local therefore
>kmap_local_page() is a sufficient replacement and will work with
>pgmap
>protected pages when those are implemented.
>
>kmap_local_page() mappings are tracked in a stack and must be
>unmapped
>in the opposite order they were mapped in.
>
>siw_tx_hdt() tracks pages used in a page_array.  It uses that array
>to
>unmap pages which were mapped on function exit.  Not all entries in
>the
>array are mapped and this is tracked in kmap_mask.
>
>kunmap_local() takes a mapped address rather than a page.  Declare a
>mapped address array, page_array_addr, of the same size as the page
>array to be used for unmapping.
>

Hi Ira, thanks for taking care of that!

I think we can avoid introducing another 'page_array_addr[]' array
here, which must be zeroed first and completely searched for
valid mappings during unmap, and also further bloats the
stack size of siw_tx_hdt(). I think we can go away with the
already available iov[].iov_base addresses array, masking addresses
with PAGE_MASK during unmapping to mask any first byte offset.
All kmap_local_page() mapping end up at that list. For unmapping
we can still rely on the kmap_mask bit field, which is more
efficient to initialize and search for valid mappings. Ordering
during unmapping can be guaranteed if we parse the bitmask
in reverse order. Let me know if you prefer me to propose
a change -- that siw_tx_hdt() thing became rather complex I
have to admit!

Best,
Bernard.

>Use kmap_local_page() instead of kmap() to map pages in the
>page_array.
>
>Because segments are mapped into the page array in increasing index
>order, modify siw_unmap_pages() to unmap pages in decreasing order.
>
>The kmap_mask is no longer needed as the lack of an address in the
>address array can indicate no unmap is required.
>
>[1]
>INVALID URI REMOVED
>lkml_20201009195033.3208459-2D59-2Dira.weiny-40intel.com_&d=DwIDAg&c=
>jf_iaSHvJObTbx-siA1ZOg&r=2TaYXQ0T-r8ZO1PP1alNwU_QJcRRLfmYTAgd3QCvqSc&
>m=wnRcc-qyXV_X7kyQfFYL6XPgmmakQxmo44BmjIon-w0&s=Y0aiKJ4EHZY8FJlI-uiPr
>xcBE95kmgn3iEz3p8d5VF4&e= 
>
>Signed-off-by: Ira Weiny <ira.weiny@intel.com>
>---
> drivers/infiniband/sw/siw/siw_qp_tx.c | 35
>+++++++++++++++------------
> 1 file changed, 20 insertions(+), 15 deletions(-)
>
>diff --git a/drivers/infiniband/sw/siw/siw_qp_tx.c
>b/drivers/infiniband/sw/siw/siw_qp_tx.c
>index db68a10d12cd..e70aba23f6e7 100644
>--- a/drivers/infiniband/sw/siw/siw_qp_tx.c
>+++ b/drivers/infiniband/sw/siw/siw_qp_tx.c
>@@ -396,13 +396,17 @@ 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 **pp, unsigned long
>kmap_mask)
>+static void siw_unmap_pages(void **addrs, int len)
> {
>-	while (kmap_mask) {
>-		if (kmap_mask & BIT(0))
>-			kunmap(*pp);
>-		pp++;
>-		kmap_mask >>= 1;
>+	int i;
>+
>+	/*
>+	 * Work backwards through the array to honor the kmap_local_page()
>+	 * ordering requirements.
>+	 */
>+	for (i = (len-1); i >= 0; i--) {
>+		if (addrs[i])
>+			kunmap_local(addrs[i]);
> 	}
> }
> 
>@@ -427,13 +431,15 @@ static int siw_tx_hdt(struct siw_iwarp_tx
>*c_tx, struct socket *s)
> 	struct siw_sge *sge = &wqe->sqe.sge[c_tx->sge_idx];
> 	struct kvec iov[MAX_ARRAY];
> 	struct page *page_array[MAX_ARRAY];
>+	void *page_array_addr[MAX_ARRAY];
> 	struct msghdr msg = { .msg_flags = MSG_DONTWAIT | MSG_EOR };
> 
> 	int seg = 0, do_crc = c_tx->do_crc, is_kva = 0, rv;
> 	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;
>+
>+	memset(page_array_addr, 0, sizeof(page_array_addr));
> 
> 	if (c_tx->state == SIW_SEND_HDR) {
> 		if (c_tx->use_sendpage) {
>@@ -498,7 +504,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)) {
>-					siw_unmap_pages(page_array, kmap_mask);
>+					siw_unmap_pages(page_array_addr, MAX_ARRAY);
> 					wqe->processed -= c_tx->bytes_unsent;
> 					rv = -EFAULT;
> 					goto done_crc;
>@@ -506,11 +512,10 @@ static int siw_tx_hdt(struct siw_iwarp_tx
>*c_tx, struct socket *s)
> 				page_array[seg] = p;
> 
> 				if (!c_tx->use_sendpage) {
>-					iov[seg].iov_base = kmap(p) + fp_off;
>-					iov[seg].iov_len = plen;
>+					page_array_addr[seg] = kmap_local_page(page_array[seg]);
> 
>-					/* Remember for later kunmap() */
>-					kmap_mask |= BIT(seg);
>+					iov[seg].iov_base = page_array_addr[seg] + fp_off;
>+					iov[seg].iov_len = plen;
> 
> 					if (do_crc)
> 						crypto_shash_update(
>@@ -518,7 +523,7 @@ static int siw_tx_hdt(struct siw_iwarp_tx *c_tx,
>struct socket *s)
> 							iov[seg].iov_base,
> 							plen);
> 				} else if (do_crc) {
>-					kaddr = kmap_local_page(p);
>+					kaddr = kmap_local_page(page_array[seg]);
> 					crypto_shash_update(c_tx->mpa_crc_hd,
> 							    kaddr + fp_off,
> 							    plen);
>@@ -542,7 +547,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");
>-				siw_unmap_pages(page_array, kmap_mask);
>+				siw_unmap_pages(page_array_addr, MAX_ARRAY);
> 				wqe->processed -= c_tx->bytes_unsent;
> 				rv = -EMSGSIZE;
> 				goto done_crc;
>@@ -593,7 +598,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);
>-		siw_unmap_pages(page_array, kmap_mask);
>+		siw_unmap_pages(page_array_addr, MAX_ARRAY);
> 	}
> 	if (rv < (int)hdr_len) {
> 		/* Not even complete hdr pushed or negative rv */
>-- 
>2.28.0.rc0.12.gb6a658bd00c9
>
>

Re: [PATCH 4/4] RDMA/siw: Convert siw_tx_hdt() to kmap_local_page()

[Ira Weiny]

On Tue, Jun 22, 2021 at 04:42:49PM +0000, Bernard Metzler wrote:
> -----ira.weiny@intel.com wrote: -----
> 
> >To: "Jason Gunthorpe" <jgg@ziepe.ca>
> >From: ira.weiny@intel.com
> >Date: 06/22/2021 08:14AM
> >Cc: "Ira Weiny" <ira.weiny@intel.com>, "Mike Marciniszyn"
> ><mike.marciniszyn@cornelisnetworks.com>, "Dennis Dalessandro"
> ><dennis.dalessandro@cornelisnetworks.com>, "Doug Ledford"
> ><dledford@redhat.com>, "Faisal Latif" <faisal.latif@intel.com>,
> >"Shiraz Saleem" <shiraz.saleem@intel.com>, "Bernard Metzler"
> ><bmt@zurich.ibm.com>, "Kamal Heib" <kheib@redhat.com>,
> >linux-rdma@vger.kernel.org, linux-kernel@vger.kernel.org
> >Subject: [EXTERNAL] [PATCH 4/4] RDMA/siw: Convert siw_tx_hdt() to
> >kmap_local_page()
> >
> >From: Ira Weiny <ira.weiny@intel.com>
> >
> >kmap() is being deprecated and will break uses of device dax after
> >PKS
> >protection is introduced.[1]
> >
> >The use of kmap() in siw_tx_hdt() is all thread local therefore
> >kmap_local_page() is a sufficient replacement and will work with
> >pgmap
> >protected pages when those are implemented.
> >
> >kmap_local_page() mappings are tracked in a stack and must be
> >unmapped
> >in the opposite order they were mapped in.
> >
> >siw_tx_hdt() tracks pages used in a page_array.  It uses that array
> >to
> >unmap pages which were mapped on function exit.  Not all entries in
> >the
> >array are mapped and this is tracked in kmap_mask.
> >
> >kunmap_local() takes a mapped address rather than a page.  Declare a
> >mapped address array, page_array_addr, of the same size as the page
> >array to be used for unmapping.
> >
> 
> Hi Ira, thanks for taking care of that!
> 
> I think we can avoid introducing another 'page_array_addr[]' array
> here, which must be zeroed first and completely searched for
> valid mappings during unmap, and also further bloats the
> stack size of siw_tx_hdt(). I think we can go away with the
> already available iov[].iov_base addresses array, masking addresses
> with PAGE_MASK during unmapping to mask any first byte offset.
> All kmap_local_page() mapping end up at that list. For unmapping
> we can still rely on the kmap_mask bit field, which is more
> efficient to initialize and search for valid mappings. Ordering
> during unmapping can be guaranteed if we parse the bitmask
> in reverse order. Let me know if you prefer me to propose
> a change -- that siw_tx_hdt() thing became rather complex I
> have to admit!

Seems not too bad, V2 sent.

I was concerned with the additional stack size but only 28 pointers (If I did
my math right) did not seem too bad.  It is redundant though so lets see if
I've gotten V2 right.

Thanks!
Ira

> 
> Best,
> Bernard.
> 
> >Use kmap_local_page() instead of kmap() to map pages in the
> >page_array.
> >
> >Because segments are mapped into the page array in increasing index
> >order, modify siw_unmap_pages() to unmap pages in decreasing order.
> >
> >The kmap_mask is no longer needed as the lack of an address in the
> >address array can indicate no unmap is required.
> >
> >[1]
> >INVALID URI REMOVED
> >lkml_20201009195033.3208459-2D59-2Dira.weiny-40intel.com_&d=DwIDAg&c=
> >jf_iaSHvJObTbx-siA1ZOg&r=2TaYXQ0T-r8ZO1PP1alNwU_QJcRRLfmYTAgd3QCvqSc&
> >m=wnRcc-qyXV_X7kyQfFYL6XPgmmakQxmo44BmjIon-w0&s=Y0aiKJ4EHZY8FJlI-uiPr
> >xcBE95kmgn3iEz3p8d5VF4&e= 
> >
> >Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> >---
> > drivers/infiniband/sw/siw/siw_qp_tx.c | 35
> >+++++++++++++++------------
> > 1 file changed, 20 insertions(+), 15 deletions(-)
> >
> >diff --git a/drivers/infiniband/sw/siw/siw_qp_tx.c
> >b/drivers/infiniband/sw/siw/siw_qp_tx.c
> >index db68a10d12cd..e70aba23f6e7 100644
> >--- a/drivers/infiniband/sw/siw/siw_qp_tx.c
> >+++ b/drivers/infiniband/sw/siw/siw_qp_tx.c
> >@@ -396,13 +396,17 @@ 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 **pp, unsigned long
> >kmap_mask)
> >+static void siw_unmap_pages(void **addrs, int len)
> > {
> >-	while (kmap_mask) {
> >-		if (kmap_mask & BIT(0))
> >-			kunmap(*pp);
> >-		pp++;
> >-		kmap_mask >>= 1;
> >+	int i;
> >+
> >+	/*
> >+	 * Work backwards through the array to honor the kmap_local_page()
> >+	 * ordering requirements.
> >+	 */
> >+	for (i = (len-1); i >= 0; i--) {
> >+		if (addrs[i])
> >+			kunmap_local(addrs[i]);
> > 	}
> > }
> > 
> >@@ -427,13 +431,15 @@ static int siw_tx_hdt(struct siw_iwarp_tx
> >*c_tx, struct socket *s)
> > 	struct siw_sge *sge = &wqe->sqe.sge[c_tx->sge_idx];
> > 	struct kvec iov[MAX_ARRAY];
> > 	struct page *page_array[MAX_ARRAY];
> >+	void *page_array_addr[MAX_ARRAY];
> > 	struct msghdr msg = { .msg_flags = MSG_DONTWAIT | MSG_EOR };
> > 
> > 	int seg = 0, do_crc = c_tx->do_crc, is_kva = 0, rv;
> > 	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;
> >+
> >+	memset(page_array_addr, 0, sizeof(page_array_addr));
> > 
> > 	if (c_tx->state == SIW_SEND_HDR) {
> > 		if (c_tx->use_sendpage) {
> >@@ -498,7 +504,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)) {
> >-					siw_unmap_pages(page_array, kmap_mask);
> >+					siw_unmap_pages(page_array_addr, MAX_ARRAY);
> > 					wqe->processed -= c_tx->bytes_unsent;
> > 					rv = -EFAULT;
> > 					goto done_crc;
> >@@ -506,11 +512,10 @@ static int siw_tx_hdt(struct siw_iwarp_tx
> >*c_tx, struct socket *s)
> > 				page_array[seg] = p;
> > 
> > 				if (!c_tx->use_sendpage) {
> >-					iov[seg].iov_base = kmap(p) + fp_off;
> >-					iov[seg].iov_len = plen;
> >+					page_array_addr[seg] = kmap_local_page(page_array[seg]);
> > 
> >-					/* Remember for later kunmap() */
> >-					kmap_mask |= BIT(seg);
> >+					iov[seg].iov_base = page_array_addr[seg] + fp_off;
> >+					iov[seg].iov_len = plen;
> > 
> > 					if (do_crc)
> > 						crypto_shash_update(
> >@@ -518,7 +523,7 @@ static int siw_tx_hdt(struct siw_iwarp_tx *c_tx,
> >struct socket *s)
> > 							iov[seg].iov_base,
> > 							plen);
> > 				} else if (do_crc) {
> >-					kaddr = kmap_local_page(p);
> >+					kaddr = kmap_local_page(page_array[seg]);
> > 					crypto_shash_update(c_tx->mpa_crc_hd,
> > 							    kaddr + fp_off,
> > 							    plen);
> >@@ -542,7 +547,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");
> >-				siw_unmap_pages(page_array, kmap_mask);
> >+				siw_unmap_pages(page_array_addr, MAX_ARRAY);
> > 				wqe->processed -= c_tx->bytes_unsent;
> > 				rv = -EMSGSIZE;
> > 				goto done_crc;
> >@@ -593,7 +598,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);
> >-		siw_unmap_pages(page_array, kmap_mask);
> >+		siw_unmap_pages(page_array_addr, MAX_ARRAY);
> > 	}
> > 	if (rv < (int)hdr_len) {
> > 		/* Not even complete hdr pushed or negative rv */
> >-- 
> >2.28.0.rc0.12.gb6a658bd00c9
> >
> >
> 

[PATCH V2] RDMA/siw: Convert siw_tx_hdt() to kmap_local_page()

[ira.weiny]

From: Ira Weiny <ira.weiny@intel.com>

kmap() is being deprecated and will break uses of device dax after PKS
protection is introduced.[1]

The use of kmap() in siw_tx_hdt() is all thread local therefore
kmap_local_page() is a sufficient replacement and will work with pgmap
protected pages when those are implemented.

siw_tx_hdt() tracks pages used in a page_array.  It uses that array to
unmap pages which were mapped on function exit.  Not all entries in the
array are mapped and this is tracked in kmap_mask.

kunmap_local() takes a mapped address rather than a page.  Alter
siw_unmap_pages() to take the iov array to reuse the iov_base address of
each mapping.  Use PAGE_MASK to get the proper address for
kunmap_local().

kmap_local_page() mappings are tracked in a stack and must be unmapped
in the opposite order they were mapped in.  Because segments are mapped
into the page array in increasing index order, modify siw_unmap_pages()
to unmap pages in decreasing order.

Use kmap_local_page() instead of kmap() to map pages in the page_array.

[1] https://lore.kernel.org/lkml/20201009195033.3208459-59-ira.weiny@intel.com/

Signed-off-by: Ira Weiny <ira.weiny@intel.com>

---
Changes for V2:
	From Bernard
		Reuse iov[].iov_base rather than declaring another array of
		pointers and preserve the use of kmap_mask to know which iov's
		were kmapped.

---
 drivers/infiniband/sw/siw/siw_qp_tx.c | 32 +++++++++++++++++----------
 1 file changed, 20 insertions(+), 12 deletions(-)

diff --git a/drivers/infiniband/sw/siw/siw_qp_tx.c b/drivers/infiniband/sw/siw/siw_qp_tx.c
index db68a10d12cd..fd3b9e6a67d7 100644
--- a/drivers/infiniband/sw/siw/siw_qp_tx.c
+++ b/drivers/infiniband/sw/siw/siw_qp_tx.c
@@ -396,13 +396,20 @@ 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 **pp, unsigned long kmap_mask)
+static void siw_unmap_pages(struct kvec *iov, unsigned long kmap_mask, int len)
 {
-	while (kmap_mask) {
-		if (kmap_mask & BIT(0))
-			kunmap(*pp);
-		pp++;
-		kmap_mask >>= 1;
+	int i;
+
+	/*
+	 * Work backwards through the array to honor the kmap_local_page()
+	 * ordering requirements.
+	 */
+	for (i = (len-1); i >= 0; i--) {
+		if (kmap_mask & BIT(i)) {
+			unsigned long addr = (unsigned long)iov[i].iov_base;
+
+			kunmap_local((void *)(addr & PAGE_MASK));
+		}
 	}
 }
 
@@ -498,7 +505,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)) {
-					siw_unmap_pages(page_array, kmap_mask);
+					siw_unmap_pages(iov, kmap_mask, MAX_ARRAY);
 					wqe->processed -= c_tx->bytes_unsent;
 					rv = -EFAULT;
 					goto done_crc;
@@ -506,11 +513,12 @@ static int siw_tx_hdt(struct siw_iwarp_tx *c_tx, struct socket *s)
 				page_array[seg] = p;
 
 				if (!c_tx->use_sendpage) {
-					iov[seg].iov_base = kmap(p) + fp_off;
-					iov[seg].iov_len = plen;
+					void *kaddr = kmap_local_page(page_array[seg]);
 
 					/* Remember for later kunmap() */
 					kmap_mask |= BIT(seg);
+					iov[seg].iov_base = kaddr + fp_off;
+					iov[seg].iov_len = plen;
 
 					if (do_crc)
 						crypto_shash_update(
@@ -518,7 +526,7 @@ static int siw_tx_hdt(struct siw_iwarp_tx *c_tx, struct socket *s)
 							iov[seg].iov_base,
 							plen);
 				} else if (do_crc) {
-					kaddr = kmap_local_page(p);
+					kaddr = kmap_local_page(page_array[seg]);
 					crypto_shash_update(c_tx->mpa_crc_hd,
 							    kaddr + fp_off,
 							    plen);
@@ -542,7 +550,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");
-				siw_unmap_pages(page_array, kmap_mask);
+				siw_unmap_pages(iov, kmap_mask, MAX_ARRAY);
 				wqe->processed -= c_tx->bytes_unsent;
 				rv = -EMSGSIZE;
 				goto done_crc;
@@ -593,7 +601,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);
-		siw_unmap_pages(page_array, kmap_mask);
+		siw_unmap_pages(iov, kmap_mask, MAX_ARRAY);
 	}
 	if (rv < (int)hdr_len) {
 		/* Not even complete hdr pushed or negative rv */
-- 
2.28.0.rc0.12.gb6a658bd00c9

Re: [PATCH V2] RDMA/siw: Convert siw_tx_hdt() to kmap_local_page()

[Bernard Metzler]

-----ira.weiny@intel.com wrote: -----

>To: "Jason Gunthorpe" <jgg@ziepe.ca>
>From: ira.weiny@intel.com
>Date: 06/22/2021 10:35PM
>Cc: "Ira Weiny" <ira.weiny@intel.com>, "Mike Marciniszyn"
><mike.marciniszyn@cornelisnetworks.com>, "Dennis Dalessandro"
><dennis.dalessandro@cornelisnetworks.com>, "Doug Ledford"
><dledford@redhat.com>, "Faisal Latif" <faisal.latif@intel.com>,
>"Shiraz Saleem" <shiraz.saleem@intel.com>, "Bernard Metzler"
><bmt@zurich.ibm.com>, "Kamal Heib" <kheib@redhat.com>,
>linux-rdma@vger.kernel.org, linux-kernel@vger.kernel.org
>Subject: [EXTERNAL] [PATCH V2] RDMA/siw: Convert siw_tx_hdt() to
>kmap_local_page()
>
>From: Ira Weiny <ira.weiny@intel.com>
>
>kmap() is being deprecated and will break uses of device dax after
>PKS
>protection is introduced.[1]
>
>The use of kmap() in siw_tx_hdt() is all thread local therefore
>kmap_local_page() is a sufficient replacement and will work with
>pgmap
>protected pages when those are implemented.
>
>siw_tx_hdt() tracks pages used in a page_array.  It uses that array
>to
>unmap pages which were mapped on function exit.  Not all entries in
>the
>array are mapped and this is tracked in kmap_mask.
>
>kunmap_local() takes a mapped address rather than a page.  Alter
>siw_unmap_pages() to take the iov array to reuse the iov_base address
>of
>each mapping.  Use PAGE_MASK to get the proper address for
>kunmap_local().
>
>kmap_local_page() mappings are tracked in a stack and must be
>unmapped
>in the opposite order they were mapped in.  Because segments are
>mapped
>into the page array in increasing index order, modify
>siw_unmap_pages()
>to unmap pages in decreasing order.
>
>Use kmap_local_page() instead of kmap() to map pages in the
>page_array.
>
>[1]
>INVALID URI REMOVED
>lkml_20201009195033.3208459-2D59-2Dira.weiny-40intel.com_&d=DwIDAg&c=
>jf_iaSHvJObTbx-siA1ZOg&r=2TaYXQ0T-r8ZO1PP1alNwU_QJcRRLfmYTAgd3QCvqSc&
>m=ujJBVqPLdVdVxXvOu_PlFL3NVC0Znds3FgxyrtWJtwM&s=WZIBAdwlCqPIRjsNOGlly
>gQ6Hsug6ObgrWgO_nvBGyc&e= 
>
>Signed-off-by: Ira Weiny <ira.weiny@intel.com>
>
>---
>Changes for V2:
>	From Bernard
>		Reuse iov[].iov_base rather than declaring another array of
>		pointers and preserve the use of kmap_mask to know which iov's
>		were kmapped.
>
>---
> drivers/infiniband/sw/siw/siw_qp_tx.c | 32
>+++++++++++++++++----------
> 1 file changed, 20 insertions(+), 12 deletions(-)
>
>diff --git a/drivers/infiniband/sw/siw/siw_qp_tx.c
>b/drivers/infiniband/sw/siw/siw_qp_tx.c
>index db68a10d12cd..fd3b9e6a67d7 100644
>--- a/drivers/infiniband/sw/siw/siw_qp_tx.c
>+++ b/drivers/infiniband/sw/siw/siw_qp_tx.c
>@@ -396,13 +396,20 @@ 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 **pp, unsigned long
>kmap_mask)
>+static void siw_unmap_pages(struct kvec *iov, unsigned long
>kmap_mask, int len)
> {
>-	while (kmap_mask) {
>-		if (kmap_mask & BIT(0))
>-			kunmap(*pp);
>-		pp++;
>-		kmap_mask >>= 1;
>+	int i;
>+
>+	/*
>+	 * Work backwards through the array to honor the kmap_local_page()
>+	 * ordering requirements.
>+	 */
>+	for (i = (len-1); i >= 0; i--) {
>+		if (kmap_mask & BIT(i)) {
>+			unsigned long addr = (unsigned long)iov[i].iov_base;
>+
>+			kunmap_local((void *)(addr & PAGE_MASK));
>+		}
> 	}
> }
> 
>@@ -498,7 +505,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)) {
>-					siw_unmap_pages(page_array, kmap_mask);
>+					siw_unmap_pages(iov, kmap_mask, MAX_ARRAY);
> 					wqe->processed -= c_tx->bytes_unsent;
> 					rv = -EFAULT;
> 					goto done_crc;
>@@ -506,11 +513,12 @@ static int siw_tx_hdt(struct siw_iwarp_tx
>*c_tx, struct socket *s)
> 				page_array[seg] = p;
> 
> 				if (!c_tx->use_sendpage) {
>-					iov[seg].iov_base = kmap(p) + fp_off;
>-					iov[seg].iov_len = plen;
>+					void *kaddr = kmap_local_page(page_array[seg]);

we can use 'kmap_local_page(p)' here
> 
> 					/* Remember for later kunmap() */
> 					kmap_mask |= BIT(seg);
>+					iov[seg].iov_base = kaddr + fp_off;
>+					iov[seg].iov_len = plen;
> 
> 					if (do_crc)
> 						crypto_shash_update(
>@@ -518,7 +526,7 @@ static int siw_tx_hdt(struct siw_iwarp_tx *c_tx,
>struct socket *s)
> 							iov[seg].iov_base,
> 							plen);

This patch does not apply for me. Would I have to install first
your [Patch 3/4] -- since the current patch references kmap_local_page()
already? Maybe it is better to apply if it would be just one siw
related patch in that series?



> 				} else if (do_crc) {
>-					kaddr = kmap_local_page(p);
>+					kaddr = kmap_local_page(page_array[seg]);

using 'kmap_local_page(p)' as you had it is straightforward
and I would prefer it.

> 					crypto_shash_update(c_tx->mpa_crc_hd,
> 							    kaddr + fp_off,
> 							    plen);
>@@ -542,7 +550,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");
>-				siw_unmap_pages(page_array, kmap_mask);
>+				siw_unmap_pages(iov, kmap_mask, MAX_ARRAY);

to minimize the iterations over the byte array in 'siw_unmap_pages()',
we may pass seg-1 instead of MAX_ARRAY


> 				wqe->processed -= c_tx->bytes_unsent;
> 				rv = -EMSGSIZE;
> 				goto done_crc;
>@@ -593,7 +601,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);
>-		siw_unmap_pages(page_array, kmap_mask);
>+		siw_unmap_pages(iov, kmap_mask, MAX_ARRAY);

to minimize the iterations over the byte array in 'siw_unmap_pages()',
we may pass seg instead of MAX_ARRAY

> 	}
> 	if (rv < (int)hdr_len) {
> 		/* Not even complete hdr pushed or negative rv */
>-- 
>2.28.0.rc0.12.gb6a658bd00c9
>
>

Re: [PATCH V2] RDMA/siw: Convert siw_tx_hdt() to kmap_local_page()

[Ira Weiny]

On Wed, Jun 23, 2021 at 02:36:45PM +0000, Bernard Metzler wrote:
> -----ira.weiny@intel.com wrote: -----
> 
> >@@ -506,11 +513,12 @@ static int siw_tx_hdt(struct siw_iwarp_tx
> >*c_tx, struct socket *s)
> > 				page_array[seg] = p;
> > 
> > 				if (!c_tx->use_sendpage) {
> >-					iov[seg].iov_base = kmap(p) + fp_off;
> >-					iov[seg].iov_len = plen;
> >+					void *kaddr = kmap_local_page(page_array[seg]);
> 
> we can use 'kmap_local_page(p)' here

Yes but I actually did this on purpose as it makes the code read clearly that
the mapping is 'seg' element of the array.  Do you prefer 'p' because this is a
performant path?

> > 
> > 					/* Remember for later kunmap() */
> > 					kmap_mask |= BIT(seg);
> >+					iov[seg].iov_base = kaddr + fp_off;
> >+					iov[seg].iov_len = plen;
> > 
> > 					if (do_crc)
> > 						crypto_shash_update(
> >@@ -518,7 +526,7 @@ static int siw_tx_hdt(struct siw_iwarp_tx *c_tx,
> >struct socket *s)
> > 							iov[seg].iov_base,
> > 							plen);
> 
> This patch does not apply for me. Would I have to install first
> your [Patch 3/4] -- since the current patch references kmap_local_page()
> already? Maybe it is better to apply if it would be just one siw
> related patch in that series?

Yes the other patch goes first.  I split it out to make this more difficult
change more reviewable.  I could squash them as it is probably straight forward
enough but I've been careful with this in other subsystems.

Jason, do you have any issue with squashing the 2 patches?

> 
> 
> 
> > 				} else if (do_crc) {
> >-					kaddr = kmap_local_page(p);
> >+					kaddr = kmap_local_page(page_array[seg]);
> 
> using 'kmap_local_page(p)' as you had it is straightforward
> and I would prefer it.

OK.  I think this reads cleaner but I can see 'p' being more performant.

> 
> > 					crypto_shash_update(c_tx->mpa_crc_hd,
> > 							    kaddr + fp_off,
> > 							    plen);
> >@@ -542,7 +550,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");
> >-				siw_unmap_pages(page_array, kmap_mask);
> >+				siw_unmap_pages(iov, kmap_mask, MAX_ARRAY);
> 
> to minimize the iterations over the byte array in 'siw_unmap_pages()',
> we may pass seg-1 instead of MAX_ARRAY

Sounds good.

> 
> 
> > 				wqe->processed -= c_tx->bytes_unsent;
> > 				rv = -EMSGSIZE;
> > 				goto done_crc;
> >@@ -593,7 +601,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);
> >-		siw_unmap_pages(page_array, kmap_mask);
> >+		siw_unmap_pages(iov, kmap_mask, MAX_ARRAY);
> 
> to minimize the iterations over the byte array in 'siw_unmap_pages()',
> we may pass seg instead of MAX_ARRAY

Will do.

Thanks for the review!  :-D
Ira

[PATCH V3] RDMA/siw: Convert siw_tx_hdt() to kmap_local_page()

[ira.weiny]

From: Ira Weiny <ira.weiny@intel.com>

kmap() is being deprecated and will break uses of device dax after PKS
protection is introduced.[1]

The use of kmap() in siw_tx_hdt() is all thread local therefore
kmap_local_page() is a sufficient replacement and will work with pgmap
protected pages when those are implemented.

siw_tx_hdt() tracks pages used in a page_array.  It uses that array to
unmap pages which were mapped on function exit.  Not all entries in the
array are mapped and this is tracked in kmap_mask.

kunmap_local() takes a mapped address rather than a page.  Alter
siw_unmap_pages() to take the iov array to reuse the iov_base address of
each mapping.  Use PAGE_MASK to get the proper address for
kunmap_local().

kmap_local_page() mappings are tracked in a stack and must be unmapped
in the opposite order they were mapped in.  Because segments are mapped
into the page array in increasing index order, modify siw_unmap_pages()
to unmap pages in decreasing order.

Use kmap_local_page() instead of kmap() to map pages in the page_array.

[1] https://lore.kernel.org/lkml/20201009195033.3208459-59-ira.weiny@intel.com/

Signed-off-by: Ira Weiny <ira.weiny@intel.com>

---
Jason, I went ahead and left this a separate patch.  Let me know if you really
want this and the other siw squashed.

Changes for V3:
	From Bernard
		Use 'p' in kmap_local_page()
		Use seg as length to siw_unmap_pages()

Changes for V2:
	From Bernard
		Reuse iov[].iov_base rather than declaring another array
		of pointers and preserve the use of kmap_mask to know
		which iov's were kmapped.
---
 drivers/infiniband/sw/siw/siw_qp_tx.c | 30 +++++++++++++++++----------
 1 file changed, 19 insertions(+), 11 deletions(-)

diff --git a/drivers/infiniband/sw/siw/siw_qp_tx.c b/drivers/infiniband/sw/siw/siw_qp_tx.c
index db68a10d12cd..89a5b75f7254 100644
--- a/drivers/infiniband/sw/siw/siw_qp_tx.c
+++ b/drivers/infiniband/sw/siw/siw_qp_tx.c
@@ -396,13 +396,20 @@ 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 **pp, unsigned long kmap_mask)
+static void siw_unmap_pages(struct kvec *iov, unsigned long kmap_mask, int len)
 {
-	while (kmap_mask) {
-		if (kmap_mask & BIT(0))
-			kunmap(*pp);
-		pp++;
-		kmap_mask >>= 1;
+	int i;
+
+	/*
+	 * Work backwards through the array to honor the kmap_local_page()
+	 * ordering requirements.
+	 */
+	for (i = (len-1); i >= 0; i--) {
+		if (kmap_mask & BIT(i)) {
+			unsigned long addr = (unsigned long)iov[i].iov_base;
+
+			kunmap_local((void *)(addr & PAGE_MASK));
+		}
 	}
 }
 
@@ -498,7 +505,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)) {
-					siw_unmap_pages(page_array, kmap_mask);
+					siw_unmap_pages(iov, kmap_mask, seg);
 					wqe->processed -= c_tx->bytes_unsent;
 					rv = -EFAULT;
 					goto done_crc;
@@ -506,11 +513,12 @@ static int siw_tx_hdt(struct siw_iwarp_tx *c_tx, struct socket *s)
 				page_array[seg] = p;
 
 				if (!c_tx->use_sendpage) {
-					iov[seg].iov_base = kmap(p) + fp_off;
-					iov[seg].iov_len = plen;
+					void *kaddr = kmap_local_page(p);
 
 					/* Remember for later kunmap() */
 					kmap_mask |= BIT(seg);
+					iov[seg].iov_base = kaddr + fp_off;
+					iov[seg].iov_len = plen;
 
 					if (do_crc)
 						crypto_shash_update(
@@ -542,7 +550,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");
-				siw_unmap_pages(page_array, kmap_mask);
+				siw_unmap_pages(iov, kmap_mask, seg-1);
 				wqe->processed -= c_tx->bytes_unsent;
 				rv = -EMSGSIZE;
 				goto done_crc;
@@ -593,7 +601,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);
-		siw_unmap_pages(page_array, kmap_mask);
+		siw_unmap_pages(iov, kmap_mask, seg+1);
 	}
 	if (rv < (int)hdr_len) {
 		/* Not even complete hdr pushed or negative rv */
-- 
2.28.0.rc0.12.gb6a658bd00c9

Re: [PATCH V3] RDMA/siw: Convert siw_tx_hdt() to kmap_local_page()

[Bernard Metzler]

-----ira.weiny@intel.com wrote: -----

>To: "Jason Gunthorpe" <jgg@ziepe.ca>
>From: ira.weiny@intel.com
>Date: 06/24/2021 12:16AM
>Cc: "Ira Weiny" <ira.weiny@intel.com>, "Mike Marciniszyn"
><mike.marciniszyn@cornelisnetworks.com>, "Dennis Dalessandro"
><dennis.dalessandro@cornelisnetworks.com>, "Doug Ledford"
><dledford@redhat.com>, "Faisal Latif" <faisal.latif@intel.com>,
>"Shiraz Saleem" <shiraz.saleem@intel.com>, "Bernard Metzler"
><bmt@zurich.ibm.com>, "Kamal Heib" <kheib@redhat.com>,
>linux-rdma@vger.kernel.org, linux-kernel@vger.kernel.org
>Subject: [EXTERNAL] [PATCH V3] RDMA/siw: Convert siw_tx_hdt() to
>kmap_local_page()
>
>From: Ira Weiny <ira.weiny@intel.com>
>
>kmap() is being deprecated and will break uses of device dax after
>PKS
>protection is introduced.[1]
>
>The use of kmap() in siw_tx_hdt() is all thread local therefore
>kmap_local_page() is a sufficient replacement and will work with
>pgmap
>protected pages when those are implemented.
>
>siw_tx_hdt() tracks pages used in a page_array.  It uses that array
>to
>unmap pages which were mapped on function exit.  Not all entries in
>the
>array are mapped and this is tracked in kmap_mask.
>
>kunmap_local() takes a mapped address rather than a page.  Alter
>siw_unmap_pages() to take the iov array to reuse the iov_base address
>of
>each mapping.  Use PAGE_MASK to get the proper address for
>kunmap_local().
>
>kmap_local_page() mappings are tracked in a stack and must be
>unmapped
>in the opposite order they were mapped in.  Because segments are
>mapped
>into the page array in increasing index order, modify
>siw_unmap_pages()
>to unmap pages in decreasing order.
>
>Use kmap_local_page() instead of kmap() to map pages in the
>page_array.
>
>[1]
>INVALID URI REMOVED
>lkml_20201009195033.3208459-2D59-2Dira.weiny-40intel.com_&d=DwIDAg&c=
>jf_iaSHvJObTbx-siA1ZOg&r=2TaYXQ0T-r8ZO1PP1alNwU_QJcRRLfmYTAgd3QCvqSc&
>m=eI4Db7iSlEKRl4l5pYKwY5rL5WXWWxahhxNciwy2lRA&s=vo11VhOvYbAkABdhV6htX
>TmXgFZeWbBZAFnPDvg7Bzs&e= 
>
>Signed-off-by: Ira Weiny <ira.weiny@intel.com>
>
>---
>Jason, I went ahead and left this a separate patch.  Let me know if
>you really
>want this and the other siw squashed.
>
>Changes for V3:
>	From Bernard
>		Use 'p' in kmap_local_page()
>		Use seg as length to siw_unmap_pages()
>
>Changes for V2:
>	From Bernard
>		Reuse iov[].iov_base rather than declaring another array
>		of pointers and preserve the use of kmap_mask to know
>		which iov's were kmapped.
>---
> drivers/infiniband/sw/siw/siw_qp_tx.c | 30
>+++++++++++++++++----------
> 1 file changed, 19 insertions(+), 11 deletions(-)
>
>diff --git a/drivers/infiniband/sw/siw/siw_qp_tx.c
>b/drivers/infiniband/sw/siw/siw_qp_tx.c
>index db68a10d12cd..89a5b75f7254 100644
>--- a/drivers/infiniband/sw/siw/siw_qp_tx.c
>+++ b/drivers/infiniband/sw/siw/siw_qp_tx.c
>@@ -396,13 +396,20 @@ 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 **pp, unsigned long
>kmap_mask)
>+static void siw_unmap_pages(struct kvec *iov, unsigned long
>kmap_mask, int len)
> {
>-	while (kmap_mask) {
>-		if (kmap_mask & BIT(0))
>-			kunmap(*pp);
>-		pp++;
>-		kmap_mask >>= 1;
>+	int i;
>+
>+	/*
>+	 * Work backwards through the array to honor the kmap_local_page()
>+	 * ordering requirements.
>+	 */
>+	for (i = (len-1); i >= 0; i--) {
>+		if (kmap_mask & BIT(i)) {
>+			unsigned long addr = (unsigned long)iov[i].iov_base;
>+
>+			kunmap_local((void *)(addr & PAGE_MASK));
>+		}
> 	}
> }
> 
>@@ -498,7 +505,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)) {
>-					siw_unmap_pages(page_array, kmap_mask);
>+					siw_unmap_pages(iov, kmap_mask, seg);
> 					wqe->processed -= c_tx->bytes_unsent;
> 					rv = -EFAULT;
> 					goto done_crc;
>@@ -506,11 +513,12 @@ static int siw_tx_hdt(struct siw_iwarp_tx
>*c_tx, struct socket *s)
> 				page_array[seg] = p;
> 
> 				if (!c_tx->use_sendpage) {
>-					iov[seg].iov_base = kmap(p) + fp_off;
>-					iov[seg].iov_len = plen;
>+					void *kaddr = kmap_local_page(p);
> 
> 					/* Remember for later kunmap() */
> 					kmap_mask |= BIT(seg);
>+					iov[seg].iov_base = kaddr + fp_off;
>+					iov[seg].iov_len = plen;
> 
> 					if (do_crc)
> 						crypto_shash_update(
>@@ -542,7 +550,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");
>-				siw_unmap_pages(page_array, kmap_mask);
>+				siw_unmap_pages(iov, kmap_mask, seg-1);
> 				wqe->processed -= c_tx->bytes_unsent;
> 				rv = -EMSGSIZE;
> 				goto done_crc;
>@@ -593,7 +601,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);
>-		siw_unmap_pages(page_array, kmap_mask);
>+		siw_unmap_pages(iov, kmap_mask, seg+1);

seg+1 is one to many, since the last segment references the iWarp
trailer (CRC). There are 2 reason for this multi-segment processing
in the transmit path. (1) efficiency and (2) MTU based packet framing.
The iov contains the complete iWarp frame with header, (potentially
multiple) data fragments, and the CRC. It gets pushed to TCP in one
go, praying for iWarp framing stays intact (which most time works).
So the code can collect data form multiple SGE's of a WRITE or
SEND and tries putting those into one frame, if MTU allows, and
adds header and trailer. 
The last segment (seg + 1) references the CRC, which is never kmap'ed.

I'll try the code next days, but it looks good otherwise!

Thanks very much!
> 	}
> 	if (rv < (int)hdr_len) {
> 		/* Not even complete hdr pushed or negative rv */
>-- 
>2.28.0.rc0.12.gb6a658bd00c9
>
>

Re: [PATCH V3] RDMA/siw: Convert siw_tx_hdt() to kmap_local_page()

[Ira Weiny]

On Thu, Jun 24, 2021 at 03:45:55PM +0000, Bernard Metzler wrote:
> 
> >@@ -593,7 +601,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);
> >-		siw_unmap_pages(page_array, kmap_mask);
> >+		siw_unmap_pages(iov, kmap_mask, seg+1);
> 
> seg+1 is one to many, since the last segment references the iWarp
> trailer (CRC). There are 2 reason for this multi-segment processing
> in the transmit path. (1) efficiency and (2) MTU based packet framing.
> The iov contains the complete iWarp frame with header, (potentially
> multiple) data fragments, and the CRC. It gets pushed to TCP in one
> go, praying for iWarp framing stays intact (which most time works).
> So the code can collect data form multiple SGE's of a WRITE or
> SEND and tries putting those into one frame, if MTU allows, and
> adds header and trailer. 
>
> The last segment (seg + 1) references the CRC, which is never kmap'ed.

siw_unmap_pages() take a length and seg is the index...

But ok so a further optimization...

Fair enough.

> 
> I'll try the code next days, but it looks good otherwise!
 
I believe this will work though.

Ira

> Thanks very much!
> > 	}
> > 	if (rv < (int)hdr_len) {
> > 		/* Not even complete hdr pushed or negative rv */
> >-- 
> >2.28.0.rc0.12.gb6a658bd00c9
> >
> >

[PATCH V4] RDMA/siw: Convert siw_tx_hdt() to kmap_local_page()

[ira.weiny]

From: Ira Weiny <ira.weiny@intel.com>

kmap() is being deprecated and will break uses of device dax after PKS
protection is introduced.[1]

The use of kmap() in siw_tx_hdt() is all thread local therefore
kmap_local_page() is a sufficient replacement and will work with pgmap
protected pages when those are implemented.

siw_tx_hdt() tracks pages used in a page_array.  It uses that array to
unmap pages which were mapped on function exit.  Not all entries in the
array are mapped and this is tracked in kmap_mask.

kunmap_local() takes a mapped address rather than a page.  Alter
siw_unmap_pages() to take the iov array to reuse the iov_base address of
each mapping.  Use PAGE_MASK to get the proper address for
kunmap_local().

kmap_local_page() mappings are tracked in a stack and must be unmapped
in the opposite order they were mapped in.  Because segments are mapped
into the page array in increasing index order, modify siw_unmap_pages()
to unmap pages in decreasing order.

Use kmap_local_page() instead of kmap() to map pages in the page_array.

[1] https://lore.kernel.org/lkml/20201009195033.3208459-59-ira.weiny@intel.com/

Signed-off-by: Ira Weiny <ira.weiny@intel.com>

---
Changes for V4:
	From Bernard
		Further optimize siw_unmap_pages() by eliminating the
		CRC page from the iov array.

Changes for V3:
	From Bernard
		Use 'p' in kmap_local_page()
		Use seg as length to siw_unmap_pages()

Changes for V2:
	From Bernard
		Reuse iov[].iov_base rather than declaring another array
		of pointers and preserve the use of kmap_mask to know
		which iov's were kmapped.
---
 drivers/infiniband/sw/siw/siw_qp_tx.c | 30 +++++++++++++++++----------
 1 file changed, 19 insertions(+), 11 deletions(-)

diff --git a/drivers/infiniband/sw/siw/siw_qp_tx.c b/drivers/infiniband/sw/siw/siw_qp_tx.c
index db68a10d12cd..1f4e60257700 100644
--- a/drivers/infiniband/sw/siw/siw_qp_tx.c
+++ b/drivers/infiniband/sw/siw/siw_qp_tx.c
@@ -396,13 +396,20 @@ 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 **pp, unsigned long kmap_mask)
+static void siw_unmap_pages(struct kvec *iov, unsigned long kmap_mask, int len)
 {
-	while (kmap_mask) {
-		if (kmap_mask & BIT(0))
-			kunmap(*pp);
-		pp++;
-		kmap_mask >>= 1;
+	int i;
+
+	/*
+	 * Work backwards through the array to honor the kmap_local_page()
+	 * ordering requirements.
+	 */
+	for (i = (len-1); i >= 0; i--) {
+		if (kmap_mask & BIT(i)) {
+			unsigned long addr = (unsigned long)iov[i].iov_base;
+
+			kunmap_local((void *)(addr & PAGE_MASK));
+		}
 	}
 }
 
@@ -498,7 +505,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)) {
-					siw_unmap_pages(page_array, kmap_mask);
+					siw_unmap_pages(iov, kmap_mask, seg);
 					wqe->processed -= c_tx->bytes_unsent;
 					rv = -EFAULT;
 					goto done_crc;
@@ -506,11 +513,12 @@ static int siw_tx_hdt(struct siw_iwarp_tx *c_tx, struct socket *s)
 				page_array[seg] = p;
 
 				if (!c_tx->use_sendpage) {
-					iov[seg].iov_base = kmap(p) + fp_off;
-					iov[seg].iov_len = plen;
+					void *kaddr = kmap_local_page(p);
 
 					/* Remember for later kunmap() */
 					kmap_mask |= BIT(seg);
+					iov[seg].iov_base = kaddr + fp_off;
+					iov[seg].iov_len = plen;
 
 					if (do_crc)
 						crypto_shash_update(
@@ -542,7 +550,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");
-				siw_unmap_pages(page_array, kmap_mask);
+				siw_unmap_pages(iov, kmap_mask, seg-1);
 				wqe->processed -= c_tx->bytes_unsent;
 				rv = -EMSGSIZE;
 				goto done_crc;
@@ -593,7 +601,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);
-		siw_unmap_pages(page_array, kmap_mask);
+		siw_unmap_pages(iov, kmap_mask, seg);
 	}
 	if (rv < (int)hdr_len) {
 		/* Not even complete hdr pushed or negative rv */
-- 
2.28.0.rc0.12.gb6a658bd00c9

Re: [PATCH V4] RDMA/siw: Convert siw_tx_hdt() to kmap_local_page()

[Bernard Metzler]

-----ira.weiny@intel.com wrote: -----

>To: "Jason Gunthorpe" <jgg@ziepe.ca>
>From: ira.weiny@intel.com
>Date: 06/24/2021 07:48PM
>Cc: "Ira Weiny" <ira.weiny@intel.com>, "Mike Marciniszyn"
><mike.marciniszyn@cornelisnetworks.com>, "Dennis Dalessandro"
><dennis.dalessandro@cornelisnetworks.com>, "Doug Ledford"
><dledford@redhat.com>, "Faisal Latif" <faisal.latif@intel.com>,
>"Shiraz Saleem" <shiraz.saleem@intel.com>, "Bernard Metzler"
><bmt@zurich.ibm.com>, "Kamal Heib" <kheib@redhat.com>,
>linux-rdma@vger.kernel.org, linux-kernel@vger.kernel.org
>Subject: [EXTERNAL] [PATCH V4] RDMA/siw: Convert siw_tx_hdt() to
>kmap_local_page()
>
>From: Ira Weiny <ira.weiny@intel.com>
>
>kmap() is being deprecated and will break uses of device dax after
>PKS
>protection is introduced.[1]
>
>The use of kmap() in siw_tx_hdt() is all thread local therefore
>kmap_local_page() is a sufficient replacement and will work with
>pgmap
>protected pages when those are implemented.
>
>siw_tx_hdt() tracks pages used in a page_array.  It uses that array
>to
>unmap pages which were mapped on function exit.  Not all entries in
>the
>array are mapped and this is tracked in kmap_mask.
>
>kunmap_local() takes a mapped address rather than a page.  Alter
>siw_unmap_pages() to take the iov array to reuse the iov_base address
>of
>each mapping.  Use PAGE_MASK to get the proper address for
>kunmap_local().
>
>kmap_local_page() mappings are tracked in a stack and must be
>unmapped
>in the opposite order they were mapped in.  Because segments are
>mapped
>into the page array in increasing index order, modify
>siw_unmap_pages()
>to unmap pages in decreasing order.
>
>Use kmap_local_page() instead of kmap() to map pages in the
>page_array.
>
>[1]
>INVALID URI REMOVED
>lkml_20201009195033.3208459-2D59-2Dira.weiny-40intel.com_&d=DwIDAg&c=
>jf_iaSHvJObTbx-siA1ZOg&r=2TaYXQ0T-r8ZO1PP1alNwU_QJcRRLfmYTAgd3QCvqSc&
>m=01QnZvj05j7vvgDChewVpHJlDytiIFuttai7VRUdJMs&s=zS4nDlvF_3MDi9wu7GaL6
>qooDhiboqP5ii5ozBeDpLE&e= 
>
>Signed-off-by: Ira Weiny <ira.weiny@intel.com>
>
>---
>Changes for V4:
>	From Bernard
>		Further optimize siw_unmap_pages() by eliminating the
>		CRC page from the iov array.
>
>Changes for V3:
>	From Bernard
>		Use 'p' in kmap_local_page()
>		Use seg as length to siw_unmap_pages()
>
>Changes for V2:
>	From Bernard
>		Reuse iov[].iov_base rather than declaring another array
>		of pointers and preserve the use of kmap_mask to know
>		which iov's were kmapped.
>---
> drivers/infiniband/sw/siw/siw_qp_tx.c | 30
>+++++++++++++++++----------
> 1 file changed, 19 insertions(+), 11 deletions(-)
>
>diff --git a/drivers/infiniband/sw/siw/siw_qp_tx.c
>b/drivers/infiniband/sw/siw/siw_qp_tx.c
>index db68a10d12cd..1f4e60257700 100644
>--- a/drivers/infiniband/sw/siw/siw_qp_tx.c
>+++ b/drivers/infiniband/sw/siw/siw_qp_tx.c
>@@ -396,13 +396,20 @@ 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 **pp, unsigned long
>kmap_mask)
>+static void siw_unmap_pages(struct kvec *iov, unsigned long
>kmap_mask, int len)
> {
>-	while (kmap_mask) {
>-		if (kmap_mask & BIT(0))
>-			kunmap(*pp);
>-		pp++;
>-		kmap_mask >>= 1;
>+	int i;
>+
>+	/*
>+	 * Work backwards through the array to honor the kmap_local_page()
>+	 * ordering requirements.
>+	 */
>+	for (i = (len-1); i >= 0; i--) {
>+		if (kmap_mask & BIT(i)) {
>+			unsigned long addr = (unsigned long)iov[i].iov_base;
>+
>+			kunmap_local((void *)(addr & PAGE_MASK));
>+		}
> 	}
> }
> 
>@@ -498,7 +505,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)) {
>-					siw_unmap_pages(page_array, kmap_mask);
>+					siw_unmap_pages(iov, kmap_mask, seg);
> 					wqe->processed -= c_tx->bytes_unsent;
> 					rv = -EFAULT;
> 					goto done_crc;
>@@ -506,11 +513,12 @@ static int siw_tx_hdt(struct siw_iwarp_tx
>*c_tx, struct socket *s)
> 				page_array[seg] = p;
> 
> 				if (!c_tx->use_sendpage) {
>-					iov[seg].iov_base = kmap(p) + fp_off;
>-					iov[seg].iov_len = plen;
>+					void *kaddr = kmap_local_page(p);
> 
> 					/* Remember for later kunmap() */
> 					kmap_mask |= BIT(seg);
>+					iov[seg].iov_base = kaddr + fp_off;
>+					iov[seg].iov_len = plen;
> 
> 					if (do_crc)
> 						crypto_shash_update(
>@@ -542,7 +550,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");
>-				siw_unmap_pages(page_array, kmap_mask);
>+				siw_unmap_pages(iov, kmap_mask, seg-1);
> 				wqe->processed -= c_tx->bytes_unsent;
> 				rv = -EMSGSIZE;
> 				goto done_crc;
>@@ -593,7 +601,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);
>-		siw_unmap_pages(page_array, kmap_mask);
>+		siw_unmap_pages(iov, kmap_mask, seg);
> 	}
> 	if (rv < (int)hdr_len) {
> 		/* Not even complete hdr pushed or negative rv */
>-- 
>2.28.0.rc0.12.gb6a658bd00c9
>
>
Sry my misconfigured email attached some HTML crap. So I did not
reach the list.

Tested V4 which works as intended. Thanks, Ira!

Reviewed-by: Bernard Metzler <bmt@zurich.ibm.com>

Re: [PATCH V4] RDMA/siw: Convert siw_tx_hdt() to kmap_local_page()

[Ira Weiny]

> >
> Sry my misconfigured email attached some HTML crap. So I did not
> reach the list.

NP...

> 
> Tested V4 which works as intended. Thanks, Ira!
> 
> Reviewed-by: Bernard Metzler <bmt@zurich.ibm.com>

Thanks!
Ira

Re: [PATCH V4] RDMA/siw: Convert siw_tx_hdt() to kmap_local_page()

[Jason Gunthorpe]

On Thu, Jun 24, 2021 at 10:48:14AM -0700, ira.weiny@intel.com wrote:
> From: Ira Weiny <ira.weiny@intel.com>
> 
> kmap() is being deprecated and will break uses of device dax after PKS
> protection is introduced.[1]
> 
> The use of kmap() in siw_tx_hdt() is all thread local therefore
> kmap_local_page() is a sufficient replacement and will work with pgmap
> protected pages when those are implemented.
> 
> siw_tx_hdt() tracks pages used in a page_array.  It uses that array to
> unmap pages which were mapped on function exit.  Not all entries in the
> array are mapped and this is tracked in kmap_mask.
> 
> kunmap_local() takes a mapped address rather than a page.  Alter
> siw_unmap_pages() to take the iov array to reuse the iov_base address of
> each mapping.  Use PAGE_MASK to get the proper address for
> kunmap_local().
> 
> kmap_local_page() mappings are tracked in a stack and must be unmapped
> in the opposite order they were mapped in.  Because segments are mapped
> into the page array in increasing index order, modify siw_unmap_pages()
> to unmap pages in decreasing order.
> 
> Use kmap_local_page() instead of kmap() to map pages in the page_array.
> 
> [1] https://lore.kernel.org/lkml/20201009195033.3208459-59-ira.weiny@intel.com/
> 
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> Reviewed-by: Bernard Metzler <bmt@zurich.ibm.com>
> ---
> Changes for V4:
> 	From Bernard
> 		Further optimize siw_unmap_pages() by eliminating the
> 		CRC page from the iov array.
> 
> Changes for V3:
> 	From Bernard
> 		Use 'p' in kmap_local_page()
> 		Use seg as length to siw_unmap_pages()
> 
> Changes for V2:
> 	From Bernard
> 		Reuse iov[].iov_base rather than declaring another array
> 		of pointers and preserve the use of kmap_mask to know
> 		which iov's were kmapped.
> ---
>  drivers/infiniband/sw/siw/siw_qp_tx.c | 30 +++++++++++++++++----------
>  1 file changed, 19 insertions(+), 11 deletions(-)

Applied to for-next, thanks

Jason