IB: Replace safe uses for ib_get_dma_mr with pd-&#62;local_dma_lkey

Acked-by Christoph Hellwig <[email protected]>

---
 drivers/infiniband/core/verbs.c | 47 ++++++++++++++++++++++++++++++++++++-----
 include/rdma/ib_verbs.h         |  9 ++------
 2 files changed, 44 insertions(+), 12 deletions(-)

drivers/infiniband/core/verbs.c +42 -5

index bac3fb406a74..bb561c8e51ad 100644
@@ -213,24 +213,61 @@ EXPORT_SYMBOL(rdma_port_get_link_layer);
 
 /* Protection domains */
 
+/**
+ * ib_alloc_pd - Allocates an unused protection domain.
+ * @device: The device on which to allocate the protection domain.
+ *
+ * A protection domain object provides an association between QPs, shared
+ * receive queues, address handles, memory regions, and memory windows.
+ *
+ * Every PD has a local_dma_lkey which can be used as the lkey value for local
+ * memory operations.
+ */
 struct ib_pd *ib_alloc_pd(struct ib_device *device)
 {
 	struct ib_pd *pd;
+	struct ib_device_attr devattr;
+	int rc;
+
+	rc = ib_query_device(device, &devattr);
+	if (rc)
+		return ERR_PTR(rc);
 
 	pd = device->alloc_pd(device, NULL, NULL);
+	if (IS_ERR(pd))
+		return pd;
+
+	pd->device = device;
+	pd->uobject = NULL;
+	pd->local_mr = NULL;
+	atomic_set(&pd->usecnt, 0);
+
+	if (devattr.device_cap_flags & IB_DEVICE_LOCAL_DMA_LKEY)
+		pd->local_dma_lkey = device->local_dma_lkey;
+	else {
+		struct ib_mr *mr;
+
+		mr = ib_get_dma_mr(pd, IB_ACCESS_LOCAL_WRITE);
+		if (IS_ERR(mr)) {
+			ib_dealloc_pd(pd);
+			return (struct ib_pd *)mr;
+		}
 
-	if (!IS_ERR(pd)) {
-		pd->device  = device;
-		pd->uobject = NULL;
-		atomic_set(&pd->usecnt, 0);
+		pd->local_mr = mr;
+		pd->local_dma_lkey = pd->local_mr->lkey;
 	}
-
 	return pd;
 }
 EXPORT_SYMBOL(ib_alloc_pd);
 
 int ib_dealloc_pd(struct ib_pd *pd)
 {
+	if (pd->local_mr) {
+		if (ib_dereg_mr(pd->local_mr))
+			return -EBUSY;
+		pd->local_mr = NULL;
+	}
+
 	if (atomic_read(&pd->usecnt))
 		return -EBUSY;

include/rdma/ib_verbs.h +2 -7

index b0f898e3b2e7..eaec3081fb87 100644
@@ -1252,9 +1252,11 @@ struct ib_udata {
 };
 
 struct ib_pd {
+	u32			local_dma_lkey;
 	struct ib_device       *device;
 	struct ib_uobject      *uobject;
 	atomic_t          	usecnt; /* count all resources */
+	struct ib_mr	       *local_mr;
 };
 
 struct ib_xrcd {

@@ -2135,13 +2137,6 @@ int ib_find_gid(struct ib_device *device, union ib_gid *gid,
 int ib_find_pkey(struct ib_device *device,
 		 u8 port_num, u16 pkey, u16 *index);
 
-/**
- * ib_alloc_pd - Allocates an unused protection domain.
- * @device: The device on which to allocate the protection domain.
- *
- * A protection domain object provides an association between QPs, shared
- * receive queues, address handles, memory regions, and memory windows.
- */
 struct ib_pd *ib_alloc_pd(struct ib_device *device);
 
 /**

Signature · 6 lines

2.1.4

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

Haggai Eran

2015-08-02 13:09 UTC #3 lore raw

Re: [PATCH v2 01/12] IB/core: Guarantee that a local_dma_lkey is available

drivers/infiniband/core/verbs.c EXPORT_SYMBOL(rdma_port_get_link_layer); · line 270

Quoted 8 lines · Jason Gunthorpe

int ib_dealloc_pd(struct ib_pd *pd)
{
drivers/infiniband/core/verbs.c +6 -0
+	if (pd->local_mr) {
+		if (ib_dereg_mr(pd->local_mr))
+			return -EBUSY;
+		pd->local_mr = NULL;
+	}
+

It looks like ib_uverbs_alloc_pd calls ib_device.alloc_pd() directly, and some drivers don't use kzalloc for allocating the pd, so the ib_dereg_mr call above results in a general protection fault.

Haggai

Jason Gunthorpe

2015-08-04 03:21 UTC #4 lore raw

Re: [PATCH v2 01/12] IB/core: Guarantee that a local_dma_lkey is available

Quoted 13 lines · Haggai Eran

drivers/infiniband/core/verbs.c EXPORT_SYMBOL(rdma_port_get_link_layer); · line 270
Quoted 8 lines · Jason Gunthorpe
int ib_dealloc_pd(struct ib_pd *pd)
{
drivers/infiniband/core/verbs.c +6 -0
+	if (pd->local_mr) {
+		if (ib_dereg_mr(pd->local_mr))
+			return -EBUSY;
+		pd->local_mr = NULL;
+	}
+
It looks like ib_uverbs_alloc_pd calls ib_device.alloc_pd() directly, and some drivers don't use kzalloc for allocating the pd, so the ib_dereg_mr call above results in a general protection fault.

Indeed it does..

I'll clean up the dealloc_pd mess too.

Jason

Jason Gunthorpe

author

2015-07-30 23:22 UTC #5 lore raw

PATCH v2 3/12 IB/ipoib: Remove ib_get_dma_mr calls

The pd now has a local_dma_lkey member which completely replaces ib_get_dma_mr, use it instead.

Signed-off-by Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>

---
 drivers/infiniband/ulp/ipoib/ipoib.h       |  1 -
 drivers/infiniband/ulp/ipoib/ipoib_cm.c    |  2 +-
 drivers/infiniband/ulp/ipoib/ipoib_verbs.c | 18 +++---------------
 3 files changed, 4 insertions(+), 17 deletions(-)

drivers/infiniband/ulp/ipoib/ipoib.h +0 -1

index 79859c4d43c9..ca2873698d75 100644
@@ -342,7 +342,6 @@ struct ipoib_dev_priv {
 	u16		  pkey;
 	u16		  pkey_index;
 	struct ib_pd	 *pd;
-	struct ib_mr	 *mr;
 	struct ib_cq	 *recv_cq;
 	struct ib_cq	 *send_cq;
 	struct ib_qp	 *qp;

drivers/infiniband/ulp/ipoib/ipoib_cm.c +1 -1

index ee39be6ccfb0..206227bb385f 100644
@@ -332,7 +332,7 @@ static void ipoib_cm_init_rx_wr(struct net_device *dev,
 	int i;
 
 	for (i = 0; i < priv->cm.num_frags; ++i)
-		sge[i].lkey = priv->mr->lkey;
+		sge[i].lkey = priv->pd->local_dma_lkey;
 
 	sge[0].length = IPOIB_CM_HEAD_SIZE;
 	for (i = 1; i < priv->cm.num_frags; ++i)

drivers/infiniband/ulp/ipoib/ipoib_verbs.c +3 ~~-15~~

index 9e6ee82a8fd7..3423256d3500 100644
@@ -152,12 +152,6 @@ int ipoib_transport_dev_init(struct net_device *dev, struct ib_device *ca)
 		return -ENODEV;
 	}
 
-	priv->mr = ib_get_dma_mr(priv->pd, IB_ACCESS_LOCAL_WRITE);
-	if (IS_ERR(priv->mr)) {
-		printk(KERN_WARNING "%s: ib_get_dma_mr failed\n", ca->name);
-		goto out_free_pd;
-	}
-
 	/*
 	 * the various IPoIB tasks assume they will never race against
 	 * themselves, so always use a single thread workqueue

@@ -165,7 +159,7 @@ int ipoib_transport_dev_init(struct net_device *dev, struct ib_device *ca)
 	priv->wq = create_singlethread_workqueue("ipoib_wq");
 	if (!priv->wq) {
 		printk(KERN_WARNING "ipoib: failed to allocate device WQ\n");
-		goto out_free_mr;
+		goto out_free_pd;
 	}
 
 	size = ipoib_recvq_size + 1;

@@ -224,13 +218,13 @@ int ipoib_transport_dev_init(struct net_device *dev, struct ib_device *ca)
 	priv->dev->dev_addr[3] = (priv->qp->qp_num      ) & 0xff;
 
 	for (i = 0; i < MAX_SKB_FRAGS + 1; ++i)
-		priv->tx_sge[i].lkey = priv->mr->lkey;
+		priv->tx_sge[i].lkey = priv->pd->local_dma_lkey;
 
 	priv->tx_wr.opcode	= IB_WR_SEND;
 	priv->tx_wr.sg_list	= priv->tx_sge;
 	priv->tx_wr.send_flags	= IB_SEND_SIGNALED;
 
-	priv->rx_sge[0].lkey = priv->mr->lkey;
+	priv->rx_sge[0].lkey = priv->pd->local_dma_lkey;
 
 	priv->rx_sge[0].length = IPOIB_UD_BUF_SIZE(priv->max_ib_mtu);
 	priv->rx_wr.num_sge = 1;

@@ -253,9 +247,6 @@ out_free_wq:
 	destroy_workqueue(priv->wq);
 	priv->wq = NULL;
 
-out_free_mr:
-	ib_dereg_mr(priv->mr);
-
 out_free_pd:
 	ib_dealloc_pd(priv->pd);

@@ -288,9 +279,6 @@ void ipoib_transport_dev_cleanup(struct net_device *dev)
 		priv->wq = NULL;
 	}
 
-	if (ib_dereg_mr(priv->mr))
-		ipoib_warn(priv, "ib_dereg_mr failed\n");
-
 	if (ib_dealloc_pd(priv->pd))
 		ipoib_warn(priv, "ib_dealloc_pd failed\n");

Signature · 6 lines

2.1.4

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

Jason Gunthorpe

author

2015-07-30 23:22 UTC #6 lore raw

PATCH v2 8/12 IB/srp: Use pd->local_dma_lkey

Replace all leys with pd->local_dma_lkey. This driver does not support iWarp, so this is safe.

The insecure use of ib_get_dma_mr is thus isolated to an rkey, and will have to be fixed separately.

Signed-off-by Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>

Reviewed-by Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/[email protected]>

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

drivers/infiniband/ulp/srp/ib_srp.c +1 -1

index 31a20b462266..19a1356f8b2a 100644
@@ -3146,7 +3146,7 @@ static ssize_t srp_create_target(struct device *dev,
 	target->io_class	= SRP_REV16A_IB_IO_CLASS;
 	target->scsi_host	= target_host;
 	target->srp_host	= host;
-	target->lkey		= host->srp_dev->mr->lkey;
+	target->lkey		= host->srp_dev->pd->local_dma_lkey;
 	target->rkey		= host->srp_dev->mr->rkey;
 	target->cmd_sg_cnt	= cmd_sg_entries;
 	target->sg_tablesize	= indirect_sg_entries ? : cmd_sg_entries;

Signature · 6 lines

2.1.4

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

Bart Van Assche

Reviewed-by on v2 8/12

2015-07-31 23:05 UTC #7 lore raw

Re: [PATCH v2 08/12] IB/srp: Use pd->local_dma_lkey

Quoted 18 lines · Jason Gunthorpe

Replace all leys with pd->local_dma_lkey. This driver does not support iWarp, so this is safe.
The insecure use of ib_get_dma_mr is thus isolated to an rkey, and will have to be fixed separately.
Signed-off-by Jason Gunthorpe <[email protected]>
Reviewed-by Sagi Grimberg <[email protected]>
---
  drivers/infiniband/ulp/srp/ib_srp.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
drivers/infiniband/ulp/srp/ib_srp.c +0 -0
index 31a20b462266..19a1356f8b2a 100644

drivers/infiniband/ulp/srp/ib_srp.c static ssize_t srp_create_target(struct device *dev, · line 3152

Quoted 8 lines

drivers/infiniband/ulp/srp/ib_srp.c +1 -1

@@ -3146,7 +3146,7 @@ static ssize_t srp_create_target(struct device *dev,
  	target->io_class	= SRP_REV16A_IB_IO_CLASS;
  	target->scsi_host	= target_host;
  	target->srp_host	= host;
-	target->lkey		= host->srp_dev->mr->lkey;
+	target->lkey		= host->srp_dev->pd->local_dma_lkey;
  	target->rkey		= host->srp_dev->mr->rkey;
  	target->cmd_sg_cnt	= cmd_sg_entries;
  	target->sg_tablesize	= indirect_sg_entries ? : cmd_sg_entries;

Reviewed-by Bart Van Assche <[email protected]>

Jason Gunthorpe

author

2015-07-30 23:22 UTC #8 lore raw

PATCH v2 12/12 rds/ib: Remove ib_get_dma_mr calls

The pd now has a local_dma_lkey member which completely replaces ib_get_dma_mr, use it instead.

Signed-off-by Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>

---
 net/rds/ib.c      | 8 --------
 net/rds/ib.h      | 2 --
 net/rds/ib_cm.c   | 4 +---
 net/rds/ib_recv.c | 6 +++---
 net/rds/ib_send.c | 8 ++++----
 5 files changed, 8 insertions(+), 20 deletions(-)

net/rds/ib.c +0 -8

index ba2dffeff608..56c570131667 100644
@@ -99,8 +99,6 @@ static void rds_ib_dev_free(struct work_struct *work)
 
 	if (rds_ibdev->mr_pool)
 		rds_ib_destroy_mr_pool(rds_ibdev->mr_pool);
-	if (rds_ibdev->mr)
-		ib_dereg_mr(rds_ibdev->mr);
 	if (rds_ibdev->pd)
 		ib_dealloc_pd(rds_ibdev->pd);

@@ -164,12 +162,6 @@ static void rds_ib_add_one(struct ib_device *device)
 		goto put_dev;
 	}
 
-	rds_ibdev->mr = ib_get_dma_mr(rds_ibdev->pd, IB_ACCESS_LOCAL_WRITE);
-	if (IS_ERR(rds_ibdev->mr)) {
-		rds_ibdev->mr = NULL;
-		goto put_dev;
-	}
-
 	rds_ibdev->mr_pool = rds_ib_create_mr_pool(rds_ibdev);
 	if (IS_ERR(rds_ibdev->mr_pool)) {
 		rds_ibdev->mr_pool = NULL;

net/rds/ib.h +0 -2

index 86d88ec5d556..36f7d808ffaa 100644
@@ -100,7 +100,6 @@ struct rds_ib_connection {
 	/* alphabet soup, IBTA style */
 	struct rdma_cm_id	*i_cm_id;
 	struct ib_pd		*i_pd;
-	struct ib_mr		*i_mr;
 	struct ib_cq		*i_send_cq;
 	struct ib_cq		*i_recv_cq;

@@ -173,7 +172,6 @@ struct rds_ib_device {
 	struct list_head	conn_list;
 	struct ib_device	*dev;
 	struct ib_pd		*pd;
-	struct ib_mr		*mr;
 	struct rds_ib_mr_pool	*mr_pool;
 	unsigned int		fmr_max_remaps;
 	unsigned int		max_fmrs;

net/rds/ib_cm.c +1 -3

index 0da2a45b33bd..a75e8832bc23 100644
@@ -269,7 +269,6 @@ static int rds_ib_setup_qp(struct rds_connection *conn)
 
 	/* Protection domain and memory range */
 	ic->i_pd = rds_ibdev->pd;
-	ic->i_mr = rds_ibdev->mr;
 
 	cq_attr.cqe = ic->i_send_ring.w_nr + 1;
 	ic->i_send_cq = ib_create_cq(dev, rds_ib_send_cq_comp_handler,

@@ -375,7 +374,7 @@ static int rds_ib_setup_qp(struct rds_connection *conn)
 
 	rds_ib_recv_init_ack(ic);
 
-	rdsdebug("conn %p pd %p mr %p cq %p %p\n", conn, ic->i_pd, ic->i_mr,
+	rdsdebug("conn %p pd %p cq %p %p\n", conn, ic->i_pd,
 		 ic->i_send_cq, ic->i_recv_cq);
 
 out:

@@ -678,7 +677,6 @@ void rds_ib_conn_shutdown(struct rds_connection *conn)
 
 		ic->i_cm_id = NULL;
 		ic->i_pd = NULL;
-		ic->i_mr = NULL;
 		ic->i_send_cq = NULL;
 		ic->i_recv_cq = NULL;
 		ic->i_send_hdrs = NULL;

net/rds/ib_recv.c +3 -3

index cac5b4506ee3..0ceb4c60d2a3 100644
@@ -62,12 +62,12 @@ void rds_ib_recv_init_ring(struct rds_ib_connection *ic)
 		sge = &recv->r_sge[0];
 		sge->addr = ic->i_recv_hdrs_dma + (i * sizeof(struct rds_header));
 		sge->length = sizeof(struct rds_header);
-		sge->lkey = ic->i_mr->lkey;
+		sge->lkey = ic->i_pd->local_dma_lkey;
 
 		sge = &recv->r_sge[1];
 		sge->addr = 0;
 		sge->length = RDS_FRAG_SIZE;
-		sge->lkey = ic->i_mr->lkey;
+		sge->lkey = ic->i_pd->local_dma_lkey;
 	}
 }

@@ -520,7 +520,7 @@ void rds_ib_recv_init_ack(struct rds_ib_connection *ic)
 
 	sge->addr = ic->i_ack_dma;
 	sge->length = sizeof(struct rds_header);
-	sge->lkey = ic->i_mr->lkey;
+	sge->lkey = ic->i_pd->local_dma_lkey;
 
 	wr->sg_list = sge;
 	wr->num_sge = 1;

net/rds/ib_send.c +4 -4

index 5d0a704fa039..f6c829d43373 100644
@@ -202,9 +202,9 @@ void rds_ib_send_init_ring(struct rds_ib_connection *ic)
 		sge = &send->s_sge[0];
 		sge->addr = ic->i_send_hdrs_dma + (i * sizeof(struct rds_header));
 		sge->length = sizeof(struct rds_header);
-		sge->lkey = ic->i_mr->lkey;
+		sge->lkey = ic->i_pd->local_dma_lkey;
 
-		send->s_sge[1].lkey = ic->i_mr->lkey;
+		send->s_sge[1].lkey = ic->i_pd->local_dma_lkey;
 	}
 }

@@ -813,7 +813,7 @@ int rds_ib_xmit_atomic(struct rds_connection *conn, struct rm_atomic_op *op)
 	/* Convert our struct scatterlist to struct ib_sge */
 	send->s_sge[0].addr = ib_sg_dma_address(ic->i_cm_id->device, op->op_sg);
 	send->s_sge[0].length = ib_sg_dma_len(ic->i_cm_id->device, op->op_sg);
-	send->s_sge[0].lkey = ic->i_mr->lkey;
+	send->s_sge[0].lkey = ic->i_pd->local_dma_lkey;
 
 	rdsdebug("rva %Lx rpa %Lx len %u\n", op->op_remote_addr,
 		 send->s_sge[0].addr, send->s_sge[0].length);

@@ -927,7 +927,7 @@ int rds_ib_xmit_rdma(struct rds_connection *conn, struct rm_rdma_op *op)
 			send->s_sge[j].addr =
 				 ib_sg_dma_address(ic->i_cm_id->device, scat);
 			send->s_sge[j].length = len;
-			send->s_sge[j].lkey = ic->i_mr->lkey;
+			send->s_sge[j].lkey = ic->i_pd->local_dma_lkey;
 
 			sent += len;
 			rdsdebug("ic %p sent %d remote_addr %llu\n", ic, sent, remote_addr);

Signature · 6 lines

2.1.4

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

santosh shilimkar

2015-08-14 02:47 UTC #9 lore raw

Re: [PATCH v2 12/12] rds/ib: Remove ib_get_dma_mr calls

Quoted 12 lines · Jason Gunthorpe

The pd now has a local_dma_lkey member which completely replaces ib_get_dma_mr, use it instead.
Signed-off-by Jason Gunthorpe <[email protected]>
---
  net/rds/ib.c      | 8 --------
  net/rds/ib.h      | 2 --
  net/rds/ib_cm.c   | 4 +---
  net/rds/ib_recv.c | 6 +++---
  net/rds/ib_send.c | 8 ++++----
  5 files changed, 8 insertions(+), 20 deletions(-)

I wanted to try this series earlier but couldn't do it because of broken RDS RDMA. Now I have that fixed with bunch of patches soon to be posted, tried the series. It works as expected.

The rds change looks also straight forward since ib_get_dma_mr() is being used for local write.

So feel free to add below tag if you need one.

Tested-Acked-by Santosh Shilimkar <[email protected]>

Jason Gunthorpe

2015-07-30 23:22 UTC #10 lore raw

PATCH v2 2/12 IB/mad: Remove ib_get_dma_mr calls

The pd now has a local_dma_lkey member which completely replaces ib_get_dma_mr, use it instead.

---
 drivers/infiniband/core/mad.c      | 26 +++-----------------------
 drivers/infiniband/core/mad_priv.h |  1 -
 include/rdma/ib_mad.h              |  1 -
 3 files changed, 3 insertions(+), 25 deletions(-)

drivers/infiniband/core/mad.c +3 ~~-23~~

index 786fc51bf04b..7c728a2d1d56 100644
@@ -338,13 +338,6 @@ struct ib_mad_agent *ib_register_mad_agent(struct ib_device *device,
 		goto error1;
 	}
 
-	mad_agent_priv->agent.mr = ib_get_dma_mr(port_priv->qp_info[qpn].qp->pd,
-						 IB_ACCESS_LOCAL_WRITE);
-	if (IS_ERR(mad_agent_priv->agent.mr)) {
-		ret = ERR_PTR(-ENOMEM);
-		goto error2;
-	}
-
 	if (mad_reg_req) {
 		reg_req = kmemdup(mad_reg_req, sizeof *reg_req, GFP_KERNEL);
 		if (!reg_req) {

@@ -429,8 +422,6 @@ error4:
 	spin_unlock_irqrestore(&port_priv->reg_lock, flags);
 	kfree(reg_req);
 error3:
-	ib_dereg_mr(mad_agent_priv->agent.mr);
-error2:
 	kfree(mad_agent_priv);
 error1:
 	return ret;

@@ -590,7 +581,6 @@ static void unregister_mad_agent(struct ib_mad_agent_private *mad_agent_priv)
 	wait_for_completion(&mad_agent_priv->comp);
 
 	kfree(mad_agent_priv->reg_req);
-	ib_dereg_mr(mad_agent_priv->agent.mr);
 	kfree(mad_agent_priv);
 }

@@ -1038,7 +1028,7 @@ struct ib_mad_send_buf * ib_create_send_mad(struct ib_mad_agent *mad_agent,
 
 	mad_send_wr->mad_agent_priv = mad_agent_priv;
 	mad_send_wr->sg_list[0].length = hdr_len;
-	mad_send_wr->sg_list[0].lkey = mad_agent->mr->lkey;
+	mad_send_wr->sg_list[0].lkey = mad_agent->qp->pd->local_dma_lkey;
 
 	/* OPA MADs don't have to be the full 2048 bytes */
 	if (opa && base_version == OPA_MGMT_BASE_VERSION &&

@@ -1047,7 +1037,7 @@ struct ib_mad_send_buf * ib_create_send_mad(struct ib_mad_agent *mad_agent,
 	else
 		mad_send_wr->sg_list[1].length = mad_size - hdr_len;
 
-	mad_send_wr->sg_list[1].lkey = mad_agent->mr->lkey;
+	mad_send_wr->sg_list[1].lkey = mad_agent->qp->pd->local_dma_lkey;
 
 	mad_send_wr->send_wr.wr_id = (unsigned long) mad_send_wr;
 	mad_send_wr->send_wr.sg_list = mad_send_wr->sg_list;

@@ -2885,7 +2875,7 @@ static int ib_mad_post_receive_mads(struct ib_mad_qp_info *qp_info,
 	struct ib_mad_queue *recv_queue = &qp_info->recv_queue;
 
 	/* Initialize common scatter list fields */
-	sg_list.lkey = (*qp_info->port_priv->mr).lkey;
+	sg_list.lkey = qp_info->port_priv->pd->local_dma_lkey;
 
 	/* Initialize common receive WR fields */
 	recv_wr.next = NULL;

@@ -3201,13 +3191,6 @@ static int ib_mad_port_open(struct ib_device *device,
 		goto error4;
 	}
 
-	port_priv->mr = ib_get_dma_mr(port_priv->pd, IB_ACCESS_LOCAL_WRITE);
-	if (IS_ERR(port_priv->mr)) {
-		dev_err(&device->dev, "Couldn't get ib_mad DMA MR\n");
-		ret = PTR_ERR(port_priv->mr);
-		goto error5;
-	}
-
 	if (has_smi) {
 		ret = create_mad_qp(&port_priv->qp_info[0], IB_QPT_SMI);
 		if (ret)

@@ -3248,8 +3231,6 @@ error8:
 error7:
 	destroy_mad_qp(&port_priv->qp_info[0]);
 error6:
-	ib_dereg_mr(port_priv->mr);
-error5:
 	ib_dealloc_pd(port_priv->pd);
 error4:
 	ib_destroy_cq(port_priv->cq);

@@ -3284,7 +3265,6 @@ static int ib_mad_port_close(struct ib_device *device, int port_num)
 	destroy_workqueue(port_priv->wq);
 	destroy_mad_qp(&port_priv->qp_info[1]);
 	destroy_mad_qp(&port_priv->qp_info[0]);
-	ib_dereg_mr(port_priv->mr);
 	ib_dealloc_pd(port_priv->pd);
 	ib_destroy_cq(port_priv->cq);
 	cleanup_recv_queue(&port_priv->qp_info[1]);

drivers/infiniband/core/mad_priv.h +0 -1

index 5be89f98928f..4a4f7aad0978 100644
@@ -199,7 +199,6 @@ struct ib_mad_port_private {
 	int port_num;
 	struct ib_cq *cq;
 	struct ib_pd *pd;
-	struct ib_mr *mr;
 
 	spinlock_t reg_lock;
 	struct ib_mad_mgmt_version_table version[MAX_MGMT_VERSION];

include/rdma/ib_mad.h +0 -1

index c8422d5a5a91..1f27023c919a 100644
@@ -388,7 +388,6 @@ enum {
 struct ib_mad_agent {
 	struct ib_device	*device;
 	struct ib_qp		*qp;
-	struct ib_mr		*mr;
 	ib_mad_recv_handler	recv_handler;
 	ib_mad_send_handler	send_handler;
 	ib_mad_snoop_handler	snoop_handler;

Signature · 1 line

2.1.4

Jason Gunthorpe

2015-07-30 23:22 UTC #11 lore raw

PATCH v2 4/12 IB/mlx4: Remove ib_get_dma_mr calls

The pd now has a local_dma_lkey member which completely replaces ib_get_dma_mr, use it instead.

---
 drivers/infiniband/hw/mlx4/mad.c     | 23 ++++-------------------
 drivers/infiniband/hw/mlx4/mlx4_ib.h |  1 -
 2 files changed, 4 insertions(+), 20 deletions(-)

drivers/infiniband/hw/mlx4/mad.c +4 ~~-19~~

index 68b3dfa922bf..1cd75ff02251 100644
@@ -580,7 +580,7 @@ int mlx4_ib_send_to_slave(struct mlx4_ib_dev *dev, int slave, u8 port,
 
 	list.addr = tun_qp->tx_ring[tun_tx_ix].buf.map;
 	list.length = sizeof (struct mlx4_rcv_tunnel_mad);
-	list.lkey = tun_ctx->mr->lkey;
+	list.lkey = tun_ctx->pd->local_dma_lkey;
 
 	wr.wr.ud.ah = ah;
 	wr.wr.ud.port_num = port;

@@ -1133,7 +1133,7 @@ static int mlx4_ib_post_pv_qp_buf(struct mlx4_ib_demux_pv_ctx *ctx,
 
 	sg_list.addr = tun_qp->ring[index].map;
 	sg_list.length = size;
-	sg_list.lkey = ctx->mr->lkey;
+	sg_list.lkey = ctx->pd->local_dma_lkey;
 
 	recv_wr.next = NULL;
 	recv_wr.sg_list = &sg_list;

@@ -1244,7 +1244,7 @@ int mlx4_ib_send_to_wire(struct mlx4_ib_dev *dev, int slave, u8 port,
 
 	list.addr = sqp->tx_ring[wire_tx_ix].buf.map;
 	list.length = sizeof (struct mlx4_mad_snd_buf);
-	list.lkey = sqp_ctx->mr->lkey;
+	list.lkey = sqp_ctx->pd->local_dma_lkey;
 
 	wr.wr.ud.ah = ah;
 	wr.wr.ud.port_num = port;

@@ -1827,19 +1827,12 @@ static int create_pv_resources(struct ib_device *ibdev, int slave, int port,
 		goto err_cq;
 	}
 
-	ctx->mr = ib_get_dma_mr(ctx->pd, IB_ACCESS_LOCAL_WRITE);
-	if (IS_ERR(ctx->mr)) {
-		ret = PTR_ERR(ctx->mr);
-		pr_err("Couldn't get tunnel DMA MR (%d)\n", ret);
-		goto err_pd;
-	}
-
 	if (ctx->has_smi) {
 		ret = create_pv_sqp(ctx, IB_QPT_SMI, create_tun);
 		if (ret) {
 			pr_err("Couldn't create %s QP0 (%d)\n",
 			       create_tun ? "tunnel for" : "",  ret);
-			goto err_mr;
+			goto err_pd;
 		}
 	}

@@ -1876,10 +1869,6 @@ err_qp0:
 		ib_destroy_qp(ctx->qp[0].qp);
 	ctx->qp[0].qp = NULL;
 
-err_mr:
-	ib_dereg_mr(ctx->mr);
-	ctx->mr = NULL;
-
 err_pd:
 	ib_dealloc_pd(ctx->pd);
 	ctx->pd = NULL;

@@ -1916,8 +1905,6 @@ static void destroy_pv_resources(struct mlx4_ib_dev *dev, int slave, int port,
 		ib_destroy_qp(ctx->qp[1].qp);
 		ctx->qp[1].qp = NULL;
 		mlx4_ib_free_pv_qp_bufs(ctx, IB_QPT_GSI, 1);
-		ib_dereg_mr(ctx->mr);
-		ctx->mr = NULL;
 		ib_dealloc_pd(ctx->pd);
 		ctx->pd = NULL;
 		ib_destroy_cq(ctx->cq);

@@ -2050,8 +2037,6 @@ static void mlx4_ib_free_sqp_ctx(struct mlx4_ib_demux_pv_ctx *sqp_ctx)
 		ib_destroy_qp(sqp_ctx->qp[1].qp);
 		sqp_ctx->qp[1].qp = NULL;
 		mlx4_ib_free_pv_qp_bufs(sqp_ctx, IB_QPT_GSI, 0);
-		ib_dereg_mr(sqp_ctx->mr);
-		sqp_ctx->mr = NULL;
 		ib_dealloc_pd(sqp_ctx->pd);
 		sqp_ctx->pd = NULL;
 		ib_destroy_cq(sqp_ctx->cq);

drivers/infiniband/hw/mlx4/mlx4_ib.h +0 -1

index 334387f63358..9066fc2406cc 100644
@@ -415,7 +415,6 @@ struct mlx4_ib_demux_pv_ctx {
 	struct ib_device *ib_dev;
 	struct ib_cq *cq;
 	struct ib_pd *pd;
-	struct ib_mr *mr;
 	struct work_struct work;
 	struct workqueue_struct *wq;
 	struct mlx4_ib_demux_pv_qp qp[2];

Signature · 1 line

2.1.4

Jason Gunthorpe

2015-07-30 23:22 UTC #12 lore raw

PATCH v2 5/12 IB/mlx5: Remove ib_get_dma_mr calls

The pd now has a local_dma_lkey member which completely replaces ib_get_dma_mr, use it instead.

---
 drivers/infiniband/hw/mlx5/main.c    | 13 -------------
 drivers/infiniband/hw/mlx5/mlx5_ib.h |  1 -
 drivers/infiniband/hw/mlx5/mr.c      |  5 ++---
 3 files changed, 2 insertions(+), 17 deletions(-)

drivers/infiniband/hw/mlx5/main.c +0 ~~-13~~

index 085c24b4b603..72ab0bdb415a 100644
@@ -1121,7 +1121,6 @@ static void destroy_umrc_res(struct mlx5_ib_dev *dev)
 
 	mlx5_ib_destroy_qp(dev->umrc.qp);
 	ib_destroy_cq(dev->umrc.cq);
-	ib_dereg_mr(dev->umrc.mr);
 	ib_dealloc_pd(dev->umrc.pd);
 }

@@ -1136,7 +1135,6 @@ static int create_umr_res(struct mlx5_ib_dev *dev)
 	struct ib_pd *pd;
 	struct ib_cq *cq;
 	struct ib_qp *qp;
-	struct ib_mr *mr;
 	struct ib_cq_init_attr cq_attr = {};
 	int ret;

@@ -1154,13 +1152,6 @@ static int create_umr_res(struct mlx5_ib_dev *dev)
 		goto error_0;
 	}
 
-	mr = ib_get_dma_mr(pd,  IB_ACCESS_LOCAL_WRITE);
-	if (IS_ERR(mr)) {
-		mlx5_ib_dbg(dev, "Couldn't create DMA MR for sync UMR QP\n");
-		ret = PTR_ERR(mr);
-		goto error_1;
-	}
-
 	cq_attr.cqe = 128;
 	cq = ib_create_cq(&dev->ib_dev, mlx5_umr_cq_handler, NULL, NULL,
 			  &cq_attr);

@@ -1218,7 +1209,6 @@ static int create_umr_res(struct mlx5_ib_dev *dev)
 
 	dev->umrc.qp = qp;
 	dev->umrc.cq = cq;
-	dev->umrc.mr = mr;
 	dev->umrc.pd = pd;
 
 	sema_init(&dev->umrc.sem, MAX_UMR_WR);

@@ -1240,9 +1230,6 @@ error_3:
 	ib_destroy_cq(cq);
 
 error_2:
-	ib_dereg_mr(mr);
-
-error_1:
 	ib_dealloc_pd(pd);
 
 error_0:

drivers/infiniband/hw/mlx5/mlx5_ib.h +0 -1

index 7cae09836481..446d80427773 100644
@@ -349,7 +349,6 @@ struct umr_common {
 	struct ib_pd	*pd;
 	struct ib_cq	*cq;
 	struct ib_qp	*qp;
-	struct ib_mr	*mr;
 	/* control access to UMR QP
 	 */
 	struct semaphore	sem;

drivers/infiniband/hw/mlx5/mr.c +2 -3

index bc9a0de897cb..4c92ca8a4181 100644
@@ -690,12 +690,11 @@ static void prep_umr_reg_wqe(struct ib_pd *pd, struct ib_send_wr *wr,
 			     int access_flags)
 {
 	struct mlx5_ib_dev *dev = to_mdev(pd->device);
-	struct ib_mr *mr = dev->umrc.mr;
 	struct mlx5_umr_wr *umrwr = (struct mlx5_umr_wr *)&wr->wr.fast_reg;
 
 	sg->addr = dma;
 	sg->length = ALIGN(sizeof(u64) * n, 64);
-	sg->lkey = mr->lkey;
+	sg->lkey = dev->umrc.pd->local_dma_lkey;
 
 	wr->next = NULL;
 	wr->send_flags = 0;

@@ -926,7 +925,7 @@ int mlx5_ib_update_mtt(struct mlx5_ib_mr *mr, u64 start_page_index, int npages,
 		sg.addr = dma;
 		sg.length = ALIGN(npages * sizeof(u64),
 				MLX5_UMR_MTT_ALIGNMENT);
-		sg.lkey = dev->umrc.mr->lkey;
+		sg.lkey = dev->umrc.pd->local_dma_lkey;
 
 		wr.send_flags = MLX5_IB_SEND_UMR_FAIL_IF_FREE |
 				MLX5_IB_SEND_UMR_UPDATE_MTT;

Signature · 1 line

2.1.4

Jason Gunthorpe

2015-07-30 23:22 UTC #13 lore raw

PATCH v2 6/12 IB/iser: Use pd->local_dma_lkey

Replace all leys with pd->local_dma_lkey. This driver does not support iWarp, so this is safe.

The insecure use of ib_get_dma_mr is thus isolated to an rkey, and this looks trivially fixed by forcing the use of registration in a future patch.

---
 drivers/infiniband/ulp/iser/iscsi_iser.c     | 2 +-
 drivers/infiniband/ulp/iser/iser_initiator.c | 8 ++++----
 drivers/infiniband/ulp/iser/iser_memory.c    | 2 +-
 drivers/infiniband/ulp/iser/iser_verbs.c     | 2 +-
 4 files changed, 7 insertions(+), 7 deletions(-)

drivers/infiniband/ulp/iser/iscsi_iser.c +1 -1

index 6a594aac2290..f44c6b879329 100644
@@ -204,7 +204,7 @@ iser_initialize_task_headers(struct iscsi_task *task,
 	tx_desc->dma_addr = dma_addr;
 	tx_desc->tx_sg[0].addr   = tx_desc->dma_addr;
 	tx_desc->tx_sg[0].length = ISER_HEADERS_LEN;
-	tx_desc->tx_sg[0].lkey   = device->mr->lkey;
+	tx_desc->tx_sg[0].lkey   = device->pd->local_dma_lkey;
 
 	iser_task->iser_conn = iser_conn;
 out:

drivers/infiniband/ulp/iser/iser_initiator.c +4 -4

index 3e2118e8ed87..2d02f042c69a 100644
@@ -173,8 +173,8 @@ static void iser_create_send_desc(struct iser_conn	*iser_conn,
 
 	tx_desc->num_sge = 1;
 
-	if (tx_desc->tx_sg[0].lkey != device->mr->lkey) {
-		tx_desc->tx_sg[0].lkey = device->mr->lkey;
+	if (tx_desc->tx_sg[0].lkey != device->pd->local_dma_lkey) {
+		tx_desc->tx_sg[0].lkey = device->pd->local_dma_lkey;
 		iser_dbg("sdesc %p lkey mismatch, fixing\n", tx_desc);
 	}
 }

@@ -291,7 +291,7 @@ int iser_alloc_rx_descriptors(struct iser_conn *iser_conn,
 		rx_sg = &rx_desc->rx_sg;
 		rx_sg->addr   = rx_desc->dma_addr;
 		rx_sg->length = ISER_RX_PAYLOAD_SIZE;
-		rx_sg->lkey   = device->mr->lkey;
+		rx_sg->lkey   = device->pd->local_dma_lkey;
 	}
 
 	iser_conn->rx_desc_head = 0;

@@ -543,7 +543,7 @@ int iser_send_control(struct iscsi_conn *conn,
 
 		tx_dsg->addr    = iser_conn->login_req_dma;
 		tx_dsg->length  = task->data_count;
-		tx_dsg->lkey    = device->mr->lkey;
+		tx_dsg->lkey    = device->pd->local_dma_lkey;
 		mdesc->num_sge = 2;
 	}

drivers/infiniband/ulp/iser/iser_memory.c +1 -1

index f0cdc961eb11..3129a42150ff 100644
@@ -393,7 +393,7 @@ iser_reg_dma(struct iser_device *device, struct iser_data_buf *mem,
 {
 	struct scatterlist *sg = mem->sg;
 
-	reg->sge.lkey = device->mr->lkey;
+	reg->sge.lkey = device->pd->local_dma_lkey;
 	reg->rkey = device->mr->rkey;
 	reg->sge.addr = ib_sg_dma_address(device->ib_device, &sg[0]);
 	reg->sge.length = ib_sg_dma_len(device->ib_device, &sg[0]);

drivers/infiniband/ulp/iser/iser_verbs.c +1 -1

index 5c9f565ea0e8..52268356c79e 100644
@@ -1017,7 +1017,7 @@ int iser_post_recvl(struct iser_conn *iser_conn)
 
 	sge.addr   = iser_conn->login_resp_dma;
 	sge.length = ISER_RX_LOGIN_SIZE;
-	sge.lkey   = ib_conn->device->mr->lkey;
+	sge.lkey   = ib_conn->device->pd->local_dma_lkey;
 
 	rx_wr.wr_id   = (uintptr_t)iser_conn->login_resp_buf;
 	rx_wr.sg_list = &sge;

Signature · 1 line

2.1.4

Jason Gunthorpe

2015-07-30 23:22 UTC #14 lore raw

PATCH v2 7/12 iser-target: Remove ib_get_dma_mr calls

The pd now has a local_dma_lkey member which completely replaces ib_get_dma_mr, use it instead.

---
 drivers/infiniband/ulp/isert/ib_isert.c | 33 +++++++++++----------------------
 drivers/infiniband/ulp/isert/ib_isert.h |  1 -
 2 files changed, 11 insertions(+), 23 deletions(-)

drivers/infiniband/ulp/isert/ib_isert.c +11 ~~-22~~

index 771700963127..00d75d977131 100644
@@ -235,7 +235,7 @@ isert_alloc_rx_descriptors(struct isert_conn *isert_conn)
 		rx_sg = &rx_desc->rx_sg;
 		rx_sg->addr = rx_desc->dma_addr;
 		rx_sg->length = ISER_RX_PAYLOAD_SIZE;
-		rx_sg->lkey = device->mr->lkey;
+		rx_sg->lkey = device->pd->local_dma_lkey;
 	}
 
 	isert_conn->rx_desc_head = 0;

@@ -385,22 +385,12 @@ isert_create_device_ib_res(struct isert_device *device)
 		goto out_cq;
 	}
 
-	device->mr = ib_get_dma_mr(device->pd, IB_ACCESS_LOCAL_WRITE);
-	if (IS_ERR(device->mr)) {
-		ret = PTR_ERR(device->mr);
-		isert_err("failed to create dma mr, device %p, ret=%d\n",
-			  device, ret);
-		goto out_mr;
-	}
-
 	/* Check signature cap */
 	device->pi_capable = dev_attr->device_cap_flags &
 			     IB_DEVICE_SIGNATURE_HANDOVER ? true : false;
 
 	return 0;
 
-out_mr:
-	ib_dealloc_pd(device->pd);
 out_cq:
 	isert_free_comps(device);
 	return ret;

@@ -411,7 +401,6 @@ isert_free_device_ib_res(struct isert_device *device)
 {
 	isert_info("device %p\n", device);
 
-	ib_dereg_mr(device->mr);
 	ib_dealloc_pd(device->pd);
 	isert_free_comps(device);
 }

@@ -1086,8 +1075,8 @@ isert_create_send_desc(struct isert_conn *isert_conn,
 	tx_desc->num_sge = 1;
 	tx_desc->isert_cmd = isert_cmd;
 
-	if (tx_desc->tx_sg[0].lkey != device->mr->lkey) {
-		tx_desc->tx_sg[0].lkey = device->mr->lkey;
+	if (tx_desc->tx_sg[0].lkey != device->pd->local_dma_lkey) {
+		tx_desc->tx_sg[0].lkey = device->pd->local_dma_lkey;
 		isert_dbg("tx_desc %p lkey mismatch, fixing\n", tx_desc);
 	}
 }

@@ -1110,7 +1099,7 @@ isert_init_tx_hdrs(struct isert_conn *isert_conn,
 	tx_desc->dma_addr = dma_addr;
 	tx_desc->tx_sg[0].addr	= tx_desc->dma_addr;
 	tx_desc->tx_sg[0].length = ISER_HEADERS_LEN;
-	tx_desc->tx_sg[0].lkey = device->mr->lkey;
+	tx_desc->tx_sg[0].lkey = device->pd->local_dma_lkey;
 
 	isert_dbg("Setup tx_sg[0].addr: 0x%llx length: %u lkey: 0x%x\n",
 		  tx_desc->tx_sg[0].addr, tx_desc->tx_sg[0].length,

@@ -1143,7 +1132,7 @@ isert_rdma_post_recvl(struct isert_conn *isert_conn)
 	memset(&sge, 0, sizeof(struct ib_sge));
 	sge.addr = isert_conn->login_req_dma;
 	sge.length = ISER_RX_LOGIN_SIZE;
-	sge.lkey = isert_conn->device->mr->lkey;
+	sge.lkey = isert_conn->device->pd->local_dma_lkey;
 
 	isert_dbg("Setup sge: addr: %llx length: %d 0x%08x\n",
 		sge.addr, sge.length, sge.lkey);

@@ -1193,7 +1182,7 @@ isert_put_login_tx(struct iscsi_conn *conn, struct iscsi_login *login,
 
 		tx_dsg->addr	= isert_conn->login_rsp_dma;
 		tx_dsg->length	= length;
-		tx_dsg->lkey	= isert_conn->device->mr->lkey;
+		tx_dsg->lkey	= isert_conn->device->pd->local_dma_lkey;
 		tx_desc->num_sge = 2;
 	}
 	if (!login->login_failed) {

@@ -2210,7 +2199,7 @@ isert_put_response(struct iscsi_conn *conn, struct iscsi_cmd *cmd)
 		isert_cmd->pdu_buf_len = pdu_len;
 		tx_dsg->addr	= isert_cmd->pdu_buf_dma;
 		tx_dsg->length	= pdu_len;
-		tx_dsg->lkey	= device->mr->lkey;
+		tx_dsg->lkey	= device->pd->local_dma_lkey;
 		isert_cmd->tx_desc.num_sge = 2;
 	}

@@ -2338,7 +2327,7 @@ isert_put_reject(struct iscsi_cmd *cmd, struct iscsi_conn *conn)
 	isert_cmd->pdu_buf_len = ISCSI_HDR_LEN;
 	tx_dsg->addr	= isert_cmd->pdu_buf_dma;
 	tx_dsg->length	= ISCSI_HDR_LEN;
-	tx_dsg->lkey	= device->mr->lkey;
+	tx_dsg->lkey	= device->pd->local_dma_lkey;
 	isert_cmd->tx_desc.num_sge = 2;
 
 	isert_init_send_wr(isert_conn, isert_cmd, send_wr);

@@ -2379,7 +2368,7 @@ isert_put_text_rsp(struct iscsi_cmd *cmd, struct iscsi_conn *conn)
 		isert_cmd->pdu_buf_len = txt_rsp_len;
 		tx_dsg->addr	= isert_cmd->pdu_buf_dma;
 		tx_dsg->length	= txt_rsp_len;
-		tx_dsg->lkey	= device->mr->lkey;
+		tx_dsg->lkey	= device->pd->local_dma_lkey;
 		isert_cmd->tx_desc.num_sge = 2;
 	}
 	isert_init_send_wr(isert_conn, isert_cmd, send_wr);

@@ -2420,7 +2409,7 @@ isert_build_rdma_wr(struct isert_conn *isert_conn, struct isert_cmd *isert_cmd,
 		ib_sge->addr = ib_sg_dma_address(ib_dev, tmp_sg) + page_off;
 		ib_sge->length = min_t(u32, data_left,
 				ib_sg_dma_len(ib_dev, tmp_sg) - page_off);
-		ib_sge->lkey = device->mr->lkey;
+		ib_sge->lkey = device->pd->local_dma_lkey;
 
 		isert_dbg("RDMA ib_sge: addr: 0x%llx  length: %u lkey: %x\n",
 			  ib_sge->addr, ib_sge->length, ib_sge->lkey);

@@ -2594,7 +2583,7 @@ isert_fast_reg_mr(struct isert_conn *isert_conn,
 	u32 page_off;
 
 	if (mem->dma_nents == 1) {
-		sge->lkey = device->mr->lkey;
+		sge->lkey = device->pd->local_dma_lkey;
 		sge->addr = ib_sg_dma_address(ib_dev, &mem->sg[0]);
 		sge->length = ib_sg_dma_len(ib_dev, &mem->sg[0]);
 		isert_dbg("sge: addr: 0x%llx  length: %u lkey: %x\n",

drivers/infiniband/ulp/isert/ib_isert.h +0 -1

index 9ec23a786c02..6a04ba3c0f72 100644
@@ -209,7 +209,6 @@ struct isert_device {
 	int			refcount;
 	struct ib_device	*ib_device;
 	struct ib_pd		*pd;
-	struct ib_mr		*mr;
 	struct isert_comp	*comps;
 	int                     comps_used;
 	struct list_head	dev_node;

Signature · 1 line

2.1.4

Jason Gunthorpe

2015-07-30 23:22 UTC #15 lore raw

PATCH v2 9/12 IB/srp: Do not create an all physical insecure rkey by default

The ULP only needs this if the insecure register_always performance optimization is enabled, or if FRWR/FMR is not supported in the driver.

Do not create an all physical MR unless it is needed to support either of those modes. Default register_always to true so the out of the box configuration does not create an insecure all physical MR.

---
 drivers/infiniband/ulp/srp/ib_srp.c | 31 +++++++++++++++++++++----------
 drivers/infiniband/ulp/srp/ib_srp.h |  2 +-
 2 files changed, 22 insertions(+), 11 deletions(-)

drivers/infiniband/ulp/srp/ib_srp.c +21 ~~-10~~

index 19a1356f8b2a..a8003079c232 100644
@@ -69,7 +69,7 @@ static unsigned int cmd_sg_entries;
 static unsigned int indirect_sg_entries;
 static bool allow_ext_sg;
 static bool prefer_fr;
-static bool register_always;
+static bool register_always = true;
 static int topspin_workarounds = 1;
 
 module_param(srp_sg_tablesize, uint, 0444);

@@ -3147,7 +3147,8 @@ static ssize_t srp_create_target(struct device *dev,
 	target->scsi_host	= target_host;
 	target->srp_host	= host;
 	target->lkey		= host->srp_dev->pd->local_dma_lkey;
-	target->rkey		= host->srp_dev->mr->rkey;
+	if (host->srp_dev->rkey_mr)
+		target->rkey		= host->srp_dev->rkey_mr->rkey;
 	target->cmd_sg_cnt	= cmd_sg_entries;
 	target->sg_tablesize	= indirect_sg_entries ? : cmd_sg_entries;
 	target->allow_ext_sg	= allow_ext_sg;

@@ -3378,6 +3379,7 @@ static void srp_add_one(struct ib_device *device)
 	struct srp_host *host;
 	int mr_page_shift, p;
 	u64 max_pages_per_mr;
+	unsigned int mr_flags = 0;
 
 	dev_attr = kmalloc(sizeof *dev_attr, GFP_KERNEL);
 	if (!dev_attr)

@@ -3396,8 +3398,11 @@ static void srp_add_one(struct ib_device *device)
 			    device->map_phys_fmr && device->unmap_fmr);
 	srp_dev->has_fr = (dev_attr->device_cap_flags &
 			   IB_DEVICE_MEM_MGT_EXTENSIONS);
-	if (!srp_dev->has_fmr && !srp_dev->has_fr)
+	if (!srp_dev->has_fmr && !srp_dev->has_fr) {
 		dev_warn(&device->dev, "neither FMR nor FR is supported\n");
+		/* Fall back to using an insecure all physical rkey */
+		mr_flags |= IB_ACCESS_REMOTE_READ | IB_ACCESS_REMOTE_WRITE;
+	}
 
 	srp_dev->use_fast_reg = (srp_dev->has_fr &&
 				 (!srp_dev->has_fmr || prefer_fr));

@@ -3433,12 +3438,17 @@ static void srp_add_one(struct ib_device *device)
 	if (IS_ERR(srp_dev->pd))
 		goto free_dev;
 
-	srp_dev->mr = ib_get_dma_mr(srp_dev->pd,
-				    IB_ACCESS_LOCAL_WRITE |
-				    IB_ACCESS_REMOTE_READ |
-				    IB_ACCESS_REMOTE_WRITE);
-	if (IS_ERR(srp_dev->mr))
-		goto err_pd;
+	if (!register_always)
+		mr_flags |= IB_ACCESS_REMOTE_READ | IB_ACCESS_REMOTE_WRITE;
+
+	if (mr_flags) {
+		srp_dev->rkey_mr = ib_get_dma_mr(
+		    srp_dev->pd, IB_ACCESS_LOCAL_WRITE | mr_flags);
+		if (IS_ERR(srp_dev->rkey_mr))
+			goto err_pd;
+	} else
+		srp_dev->rkey_mr = NULL;
+
 
 	for (p = rdma_start_port(device); p <= rdma_end_port(device); ++p) {
 		host = srp_add_port(srp_dev, p);

@@ -3495,7 +3505,8 @@ static void srp_remove_one(struct ib_device *device)
 		kfree(host);
 	}
 
-	ib_dereg_mr(srp_dev->mr);
+	if (srp_dev->rkey_mr)
+		ib_dereg_mr(srp_dev->rkey_mr);
 	ib_dealloc_pd(srp_dev->pd);
 
 	kfree(srp_dev);

drivers/infiniband/ulp/srp/ib_srp.h +1 -1

index 17ee3f80ba55..8b241f17f8b8 100644
@@ -95,7 +95,7 @@ struct srp_device {
 	struct list_head	dev_list;
 	struct ib_device       *dev;
 	struct ib_pd	       *pd;
-	struct ib_mr	       *mr;
+	struct ib_mr	       *rkey_mr;
 	u64			mr_page_mask;
 	int			mr_page_size;
 	int			mr_max_size;

Signature · 1 line

2.1.4

Christoph Hellwig

2015-08-03 15:39 UTC #16 lore raw

Re: [PATCH v2 09/12] IB/srp: Do not create an all physical insecure rkey by default

In addition to the comments on the cover letter I think your changes to srp_add_one could use this incremental cleanup:

drivers/infiniband/ulp/srp/ib_srp.c +7 -5

index a546256..5e2cb53 100644
@@ -3381,7 +3381,7 @@ static void srp_add_one(struct ib_device *device)
 	struct srp_host *host;
 	int mr_page_shift, p;
 	u64 max_pages_per_mr;
-	unsigned int mr_flags = 0;
+	bool need_phys_rkey = false;
 
 	dev_attr = kmalloc(sizeof *dev_attr, GFP_KERNEL);
 	if (!dev_attr)

@@ -3403,7 +3403,7 @@ static void srp_add_one(struct ib_device *device)
 	if (!srp_dev->has_fmr && !srp_dev->has_fr) {
 		dev_warn(&device->dev, "neither FMR nor FR is supported\n");
 		/* Fall back to using an insecure all physical rkey */
-		mr_flags |= IB_ACCESS_REMOTE_READ | IB_ACCESS_REMOTE_WRITE;
+		need_phys_rkey = true;
 	}
 
 	srp_dev->use_fast_reg = (srp_dev->has_fr &&

@@ -3441,11 +3441,13 @@ static void srp_add_one(struct ib_device *device)
 		goto free_dev;
 
 	if (!register_always)
-		mr_flags |= IB_ACCESS_REMOTE_READ | IB_ACCESS_REMOTE_WRITE;
+		need_phys_rkey = true;
 
-	if (mr_flags) {
+	if (need_phys_rkey) {
 		srp_dev->rkey_mr = ib_get_dma_mr(
-		    srp_dev->pd, IB_ACCESS_LOCAL_WRITE | mr_flags);
+		    srp_dev->pd, IB_ACCESS_LOCAL_WRITE |
+		    		 IB_ACCESS_REMOTE_READ |
+				 IB_ACCESS_REMOTE_WRITE);
 		if (IS_ERR(srp_dev->rkey_mr))
 			goto err_pd;
 	} else

Bart Van Assche

2015-08-03 17:18 UTC #17 lore raw

Re: [PATCH v2 09/12] IB/srp: Do not create an all physical insecure rkey by default

drivers/infiniband/ulp/srp/ib_srp.c static unsigned int cmd_sg_entries; · line 73

Quoted 11 lines · Jason Gunthorpe

drivers/infiniband/ulp/srp/ib_srp.c +1 -1

index 19a1356f8b2a..a8003079c232 100644
@@ -69,7 +69,7 @@ static unsigned int cmd_sg_entries;
  static unsigned int indirect_sg_entries;
  static bool allow_ext_sg;
  static bool prefer_fr;
-static bool register_always;
+static bool register_always = true;
  static int topspin_workarounds = 1;

Hello Jason,

Can you modify this patch such that the default value of prefer_fr is changed from false into true ? The only reason I had not made fast registration the default myself when I introduced FR support in the SRP initiator driver is because at that time the FR code in the SRP initiator was brand new. However, since the introduction of FR support in the SRP initiator that code has been tested extensively (at least inside SanDisk).

Thanks,

Bart.

Jason Gunthorpe

2015-07-30 23:22 UTC #18 lore raw

PATCH v2 10/12 ib_srpt: Remove ib_get_dma_mr calls

The pd now has a local_dma_lkey member which completely replaces ib_get_dma_mr, use it instead.

---
 drivers/infiniband/ulp/srpt/ib_srpt.c | 15 ++++-----------
 drivers/infiniband/ulp/srpt/ib_srpt.h |  1 -
 2 files changed, 4 insertions(+), 12 deletions(-)

drivers/infiniband/ulp/srpt/ib_srpt.c +4 ~~-11~~

index 60ff0a2390e5..20adb96ba0b2 100644
@@ -783,7 +783,7 @@ static int srpt_post_recv(struct srpt_device *sdev,
 
 	list.addr = ioctx->ioctx.dma;
 	list.length = srp_max_req_size;
-	list.lkey = sdev->mr->lkey;
+	list.lkey = sdev->pd->local_dma_lkey;
 
 	wr.next = NULL;
 	wr.sg_list = &list;

@@ -818,7 +818,7 @@ static int srpt_post_send(struct srpt_rdma_ch *ch,
 
 	list.addr = ioctx->ioctx.dma;
 	list.length = len;
-	list.lkey = sdev->mr->lkey;
+	list.lkey = sdev->pd->local_dma_lkey;
 
 	wr.next = NULL;
 	wr.wr_id = encode_wr_id(SRPT_SEND, ioctx->ioctx.index);

@@ -1206,7 +1206,7 @@ static int srpt_map_sg_to_ib_sge(struct srpt_rdma_ch *ch,
 
 		while (rsize > 0 && tsize > 0) {
 			sge->addr = dma_addr;
-			sge->lkey = ch->sport->sdev->mr->lkey;
+			sge->lkey = ch->sport->sdev->pd->local_dma_lkey;
 
 			if (rsize >= dma_len) {
 				sge->length =

@@ -3211,10 +3211,6 @@ static void srpt_add_one(struct ib_device *device)
 	if (IS_ERR(sdev->pd))
 		goto free_dev;
 
-	sdev->mr = ib_get_dma_mr(sdev->pd, IB_ACCESS_LOCAL_WRITE);
-	if (IS_ERR(sdev->mr))
-		goto err_pd;
-
 	sdev->srq_size = min(srpt_srq_size, sdev->dev_attr.max_srq_wr);
 
 	srq_attr.event_handler = srpt_srq_event;

@@ -3226,7 +3222,7 @@ static void srpt_add_one(struct ib_device *device)
 
 	sdev->srq = ib_create_srq(sdev->pd, &srq_attr);
 	if (IS_ERR(sdev->srq))
-		goto err_mr;
+		goto err_pd;
 
 	pr_debug("%s: create SRQ #wr= %d max_allow=%d dev= %s\n",
 		 __func__, sdev->srq_size, sdev->dev_attr.max_srq_wr,

@@ -3311,8 +3307,6 @@ err_cm:
 	ib_destroy_cm_id(sdev->cm_id);
 err_srq:
 	ib_destroy_srq(sdev->srq);
-err_mr:
-	ib_dereg_mr(sdev->mr);
 err_pd:
 	ib_dealloc_pd(sdev->pd);
 free_dev:

@@ -3358,7 +3352,6 @@ static void srpt_remove_one(struct ib_device *device)
 	srpt_release_sdev(sdev);
 
 	ib_destroy_srq(sdev->srq);
-	ib_dereg_mr(sdev->mr);
 	ib_dealloc_pd(sdev->pd);
 
 	srpt_free_ioctx_ring((struct srpt_ioctx **)sdev->ioctx_ring, sdev,

drivers/infiniband/ulp/srpt/ib_srpt.h +0 -1

index 21f8df67522a..5faad8acd789 100644
@@ -393,7 +393,6 @@ struct srpt_port {
 struct srpt_device {
 	struct ib_device	*device;
 	struct ib_pd		*pd;
-	struct ib_mr		*mr;
 	struct ib_srq		*srq;
 	struct ib_cm_id		*cm_id;
 	struct ib_device_attr	dev_attr;

Signature · 1 line

2.1.4

Jason Gunthorpe

2015-07-30 23:22 UTC #19 lore raw

PATCH v2 11/12 net/9p: Remove ib_get_dma_mr calls

The pd now has a local_dma_lkey member which completely replaces ib_get_dma_mr, use it instead.

Tested-by Dominique Martinet <[email protected]>

---
 net/9p/trans_rdma.c | 26 ++------------------------
 1 file changed, 2 insertions(+), 24 deletions(-)

net/9p/trans_rdma.c +2 ~~-24~~

index 37a78d20c0f6..ba1210253f5e 100644
@@ -94,8 +94,6 @@ struct p9_trans_rdma {
 	struct ib_pd *pd;
 	struct ib_qp *qp;
 	struct ib_cq *cq;
-	struct ib_mr *dma_mr;
-	u32 lkey;
 	long timeout;
 	int sq_depth;
 	struct semaphore sq_sem;

@@ -382,9 +380,6 @@ static void rdma_destroy_trans(struct p9_trans_rdma *rdma)
 	if (!rdma)
 		return;
 
-	if (rdma->dma_mr && !IS_ERR(rdma->dma_mr))
-		ib_dereg_mr(rdma->dma_mr);
-
 	if (rdma->qp && !IS_ERR(rdma->qp))
 		ib_destroy_qp(rdma->qp);

@@ -415,7 +410,7 @@ post_recv(struct p9_client *client, struct p9_rdma_context *c)
 
 	sge.addr = c->busa;
 	sge.length = client->msize;
-	sge.lkey = rdma->lkey;
+	sge.lkey = rdma->pd->local_dma_lkey;
 
 	wr.next = NULL;
 	c->wc_op = IB_WC_RECV;

@@ -506,7 +501,7 @@ dont_need_post_recv:
 
 	sge.addr = c->busa;
 	sge.length = c->req->tc->size;
-	sge.lkey = rdma->lkey;
+	sge.lkey = rdma->pd->local_dma_lkey;
 
 	wr.next = NULL;
 	c->wc_op = IB_WC_SEND;

@@ -647,7 +642,6 @@ rdma_create_trans(struct p9_client *client, const char *addr, char *args)
 	struct p9_trans_rdma *rdma;
 	struct rdma_conn_param conn_param;
 	struct ib_qp_init_attr qp_attr;
-	struct ib_device_attr devattr;
 	struct ib_cq_init_attr cq_attr = {};
 
 	/* Parse the transport specific mount options */

@@ -700,11 +694,6 @@ rdma_create_trans(struct p9_client *client, const char *addr, char *args)
 	if (err || (rdma->state != P9_RDMA_ROUTE_RESOLVED))
 		goto error;
 
-	/* Query the device attributes */
-	err = ib_query_device(rdma->cm_id->device, &devattr);
-	if (err)
-		goto error;
-
 	/* Create the Completion Queue */
 	cq_attr.cqe = opts.sq_depth + opts.rq_depth + 1;
 	rdma->cq = ib_create_cq(rdma->cm_id->device, cq_comp_handler,

@@ -719,17 +708,6 @@ rdma_create_trans(struct p9_client *client, const char *addr, char *args)
 	if (IS_ERR(rdma->pd))
 		goto error;
 
-	/* Cache the DMA lkey in the transport */
-	rdma->dma_mr = NULL;
-	if (devattr.device_cap_flags & IB_DEVICE_LOCAL_DMA_LKEY)
-		rdma->lkey = rdma->cm_id->device->local_dma_lkey;
-	else {
-		rdma->dma_mr = ib_get_dma_mr(rdma->pd, IB_ACCESS_LOCAL_WRITE);
-		if (IS_ERR(rdma->dma_mr))
-			goto error;
-		rdma->lkey = rdma->dma_mr->lkey;
-	}
-
 	/* Create the Queue Pair */
 	memset(&qp_attr, 0, sizeof qp_attr);
 	qp_attr.event_handler = qp_event_handler;

Signature · 1 line

2.1.4

Christoph Hellwig

2015-07-31 07:42 UTC #20 lore raw

Hi Jason,

this series look fine to me, although I don't feel comfortable enough to give a Reviewed-by: tag for the RDMA subsystem yet.

Steve Wise

Reviewed-by on the v2 series

2015-07-31 13:26 UTC #21 lore raw

Series looks good.

Reviewed-by Steve Wise <[email protected]>

Bart Van Assche

2015-07-31 22:20 UTC #22 lore raw

Quoted 3 lines · Jason Gunthorpe

All patches are compile tested. I've done basic testing up to and including the IPoIB patch, the rest required specialized setups I don't have access to, but are fairly straightforward.

Hello Jason,

SRP login fails with this patch series applied on top of Linux kernel v4.2-rc4. At the target side the following message appears every time the SRP initiator tries to log in: "ib_srpt: RDMA t 5 for idx 0 failed with status 10" (10=remote access error). That causes the initiator to receive a flush error (5).

Bart.

A message that is not in the archive (sent off-list or not yet received): [email protected]

Jason Gunthorpe

author

2015-07-31 22:31 UTC #24 lore raw

Quoted 5 lines · Bart Van Assche

Quoted 3 lines · Jason Gunthorpe
All patches are compile tested. I've done basic testing up to and including the IPoIB patch, the rest required specialized setups I don't have access to, but are fairly straightforward.

Quoted 7 lines

Hello Jason,
SRP login fails with this patch series applied on top of Linux kernel v4.2-rc4. At the target side the following message appears every time the SRP initiator tries to log in: "ib_srpt: RDMA t 5 for idx 0 failed with status 10" (10=remote access error). That causes the initiator to receive a flush error (5).

Sounds like a bad rkey was sent to the target? I'm guessing #9 is the cause? I'm guessing that target->rkey is being used someplace even though register_always is set?

Doug, in this case, I'd just drop the impacted SRP patches.. Hopefully just #9...

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

A message that is not in the archive (sent off-list or not yet received): 20150731223153.GA1518-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org

Bart Van Assche

2015-07-31 23:04 UTC #26 lore raw

Quoted 14 lines · Jason Gunthorpe

Quoted 5 lines · Bart Van Assche
Quoted 3 lines · Jason Gunthorpe
All patches are compile tested. I've done basic testing up to and including the IPoIB patch, the rest required specialized setups I don't have access to, but are fairly straightforward.
Quoted 7 lines
Hello Jason,
SRP login fails with this patch series applied on top of Linux kernel v4.2-rc4. At the target side the following message appears every time the SRP initiator tries to log in: "ib_srpt: RDMA t 5 for idx 0 failed with status 10" (10=remote access error). That causes the initiator to receive a flush error (5).

Quoted 6 lines

Sounds like a bad rkey was sent to the target? I'm guessing #9 is the cause? I'm guessing that target->rkey is being used someplace even though register_always is set?
Doug, in this case, I'd just drop the impacted SRP patches.. Hopefully just #9...

Hello Jason,

With only patches 1/12 and 8/12 applied my test passes.

Bart.

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

A message that is not in the archive (sent off-list or not yet received): [email protected]

Jason Gunthorpe

author

2015-07-31 23:14 UTC #28 lore raw

Quoted 1 line · Bart Van Assche

With only patches 1/12 and 8/12 applied my test passes.

Thanks Bart!

If you like, I can try and work on #9 (no promises :(), but I think I'll need some way to test it here.

Do you by chance have a straightforward recipe to setup SRP and SRPT on two Linux's for this simple purpose?

Doug, just drop #9 'IB/srp: Do not create an all physical insecure rkey by default' please, thanks.

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

A message that is not in the archive (sent off-list or not yet received): 20150731231430.GA1955-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org

Bart Van Assche

2015-07-31 23:32 UTC #30 lore raw

Quoted 2 lines · Jason Gunthorpe

Do you by chance have a straightforward recipe to setup SRP and SRPT on two Linux's for this simple purpose?

Some time ago I wrote a script that loads the upstream SRP target driver and creates two LUNs for local RAM disks (attached to this e-mail). The only disadvantage of that script is that it is based on the obsolete lio-utils package.

In case you would prefer the SCST SRP target driver, the following instructions should be sufficient to build, install, load and configure it on an RPM based system (please replace the GUID shown below by a GUID of a port of the HCA in your the target system):

svn co svn://svn.code.sf.net/p/scst/svn/trunk scst-trunk ||
git clone https://github.com/bvanassche/scst.git scst-trunk
cd scst-trunk
make rpm
su
rpm -U {,scstadmin/}rpmbuilddir/RPMS/x86_64/*.rpm cat <<EOF >/etc/scst.conf

HANDLER vdisk_blockio {
    DEVICE brd {
	filename /dev/ram0
    }
}
TARGET_DRIVER ib_srpt {
    TARGET fe80:0000:0000:0000:24be:05ff:ffa9:cbb1 {
        enabled 1
        LUN 0 brd
    }
}

EOF
modprobe brd
/etc/init.d/scst restart

Bart.

#!/bin/bash

filesize=$((16*1024*1024))
hcas="$(cat /sys/devices/*/*/*/infiniband/*/ports/*/gids/0 | sed 's/^$....$:$....$:$....$:$....$:$....$:$....$:$....$:$....$$/0x0000\2\3\4\5\6\7\8 0x\5\6\7\8 0x\1\2\3\4\5\6\7\8/')" # Output of cat /sys/devices/*/*/*/infiniband/*/ports/*/gids/0 on the remote

# systems
initiators="\
0x00000000000000000002c9030003cca7 \
0x00000000000000000002c9030003cca8 \
0x00000000000000000002c9030005f34f \
0x00000000000000000002c9030005f350 \
0x00000000000000000002c90300a34271 \
0x00000000000000000002c90300a34272 \
0x00000000000000000002c90300fab7f1 \
0x00000000000000000002c90300fab7f2"

if [ ! -e /sys/module/configfs ]; then
modprobe configfs
fi
if ! mount | grep -qw configfs; then
mount -t configfs none /sys/kernel/config
fi

if cd /sys/kernel/config/target >&/dev/null; then
for hca in ${hcas}; do
if [ -e srpt/$hca/$hca/enable ]; then
echo 0 >srpt/$hca/$hca/enable
fi
done
fi
cd /

rm -f /sys/kernel/config/target/srpt/*/*/acls/*/*/* >&/dev/null rmdir /sys/kernel/config/target/srpt/*/*/acls/*/* >&/dev/null rmdir /sys/kernel/config/target/srpt/*/*/acls/* >&/dev/null rm -f /sys/kernel/config/target/srpt/*/*/lun/*/* >&/dev/null rmdir /sys/kernel/config/target/srpt/*/*/lun/* >&/dev/null rmdir /sys/kernel/config/target/srpt/*/* >&/dev/null rmdir /sys/kernel/config/target/srpt/* >&/dev/null rmdir /sys/kernel/config/target/srpt >&/dev/null

if [ -e /sys/module/ib_srpt ]; then
rmmod ib_srpt
fi

rmdir /sys/kernel/config/target/core/*/* >&/dev/null rmdir /sys/kernel/config/target/core/* >&/dev/null

#find /sys/kernel/config/target

for m in ib_srpt target_core_pscsi target_core_iblock target_core_file target_core_stgt target_core_user target_core_mod do
if [ -e /sys/module/$m ]; then
rmmod $m
fi
done

if [ "$1" = "stop" ]; then
exit 0
fi

modprobe target_core_mod

if [ -e /sys/kernel/debug/dynamic_debug/control ]; then
#echo 'module target_core_mod +p' > /sys/kernel/debug/dynamic_debug/control :
fi

insmod /lib/modules/$(uname -r)/kernel/drivers/infiniband/ulp/srpt/ib_srpt.ko srp_max_req_size=4200 || exit $?

if [ -e /sys/kernel/debug/dynamic_debug/control ]; then
echo 'module ib_srpt +p' > /sys/kernel/debug/dynamic_debug/control
fi

modprobe target_core_file || exit $?
if [ ! -e /dev/ramdisk ]; then
dd if=/dev/zero of=/dev/ramdisk bs=${filesize} count=1
fi

if false && cd /sys/kernel/debug/tracing; then
echo function >current_tracer
{ echo 'srpt_*'; echo 'transport_*'; } >set_ftrace_filter echo 1 >tracing_on
fi

vdev0="fileio_0/vdev0"
vdev1="fileio_1/vdev2"
vdev2="rd_dr_0/vdev1"
vdevs="$vdev0 $vdev1"
vdev_count=2
tcm_node --fileio $vdev0 /dev/ramdisk ${filesize} tcm_node --fileio $vdev1 /dev/ramdisk 65536 if [ -e /sys/kernel/config/target/core/rd_dr_0 ]; then
tcm_node --ramdisk $vdev2 2
vdevs="$vdevs vdev2"
vdev_count=3
fi
cd /sys/kernel/config/target || exit $?
mkdir srpt || exit $?
cd srpt || exit $?
for hca in ${hcas}; do
mkdir $hca
[ -e $hca ] || continue
echo $hca
mkdir $hca/$hca
i=0
for v in ${vdevs}; do

    mkdir $hca/$hca/lun/lun_$i
    ( cd $hca/$hca/lun/lun_$i && ln -s ../../../../../core/$v )
    i=$((i+1))
  done
  for ini in ${initiators}; do
  (
    cd $hca/$hca/acls
    mkdir ${ini}
    cd ${ini}
    for ((i = 0; i < $vdev_count; i++)) do
      ( mkdir lun_$i && cd lun_$i && ln -s ../../../lun/lun_$i )
    done

)
echo 1 >$hca/$hca/enable
done
done

Doug Ledford

2015-08-01 20:05 UTC #31 lore raw

In reply to Bart Van Assche

Quoted 6 lines · Jason Gunthorpe

Quoted 1 line · Bart Van Assche
With only patches 1/12 and 8/12 applied my test passes.
Thanks Bart!

Quoted 8 lines

If you like, I can try and work on #9 (no promises :(), but I think I'll need some way to test it here.
Do you by chance have a straightforward recipe to setup SRP and SRPT on two Linux's for this simple purpose?
Doug, just drop #9 'IB/srp: Do not create an all physical insecure rkey by default' please, thanks.

I've pulled in the series for 4.3. I've included patch #9, but that's on the understanding that if it can't be fixed before the initial pull request, I'll drop it at that time. I have the resources to test this, so I can work on it, and that factors into my path forward here.

Signature · 2 lines

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

A message that is not in the archive (sent off-list or not yet received): [email protected]

Bart Van Assche

2015-08-11 20:50 UTC #33 lore raw

Quoted 13 lines · Doug Ledford

Quoted 12 lines · Jason Gunthorpe
Quoted 1 line · Bart Van Assche
With only patches 1/12 and 8/12 applied my test passes.
Thanks Bart!
If you like, I can try and work on #9 (no promises :(), but I think I'll need some way to test it here.
Do you by chance have a straightforward recipe to setup SRP and SRPT on two Linux's for this simple purpose?

Quoted 7 lines

Quoted 2 lines
Doug, just drop #9 'IB/srp: Do not create an all physical insecure rkey by default' please, thanks.
I've pulled in the series for 4.3. I've included patch #9, but that's on the understanding that if it can't be fixed before the initial pull request, I'll drop it at that time. I have the resources to test this, so I can work on it, and that factors into my path forward here.

Hi Doug,

Do you feel comfortable enough to queue the nine patches that replace Jason's patch #9 for kernel v4.3 (see also http://thread.gmane.org/gmane.linux.drivers.rdma/28153/focus=28419) or would you rather prefer to take out patch #9 and queue these nine patches for kernel v4.4 ? Please advise.

Thanks,

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

A message that is not in the archive (sent off-list or not yet received): [email protected]

Doug Ledford

2015-08-14 13:36 UTC #35 lore raw

Quoted 22 lines · Bart Van Assche

Quoted 13 lines · Doug Ledford
Quoted 12 lines · Jason Gunthorpe
Quoted 1 line · Bart Van Assche
With only patches 1/12 and 8/12 applied my test passes.
Thanks Bart!
If you like, I can try and work on #9 (no promises :(), but I think I'll need some way to test it here.
Do you by chance have a straightforward recipe to setup SRP and SRPT on two Linux's for this simple purpose?
Quoted 7 lines
Quoted 2 lines
Doug, just drop #9 'IB/srp: Do not create an all physical insecure rkey by default' please, thanks.
I've pulled in the series for 4.3. I've included patch #9, but that's on the understanding that if it can't be fixed before the initial pull request, I'll drop it at that time. I have the resources to test this, so I can work on it, and that factors into my path forward here.

Quoted 7 lines

Hi Doug,
Do you feel comfortable enough to queue the nine patches that replace Jason's patch #9 for kernel v4.3 (see also
http://thread.gmane.org/gmane.linux.drivers.rdma/28153/focus=28419) or would you rather prefer to take out patch #9 and queue these nine patches for kernel v4.4 ? Please advise.

I was planning on queueing up the 9 replacement patches.

Signature · 2 lines

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

A message that is not in the archive (sent off-list or not yet received): [email protected]

Bart Van Assche

2015-08-14 16:20 UTC #37 lore raw

Quoted 25 lines · Doug Ledford

Quoted 24 lines · Bart Van Assche
Quoted 13 lines · Doug Ledford
Quoted 12 lines · Jason Gunthorpe
Quoted 1 line · Bart Van Assche
With only patches 1/12 and 8/12 applied my test passes.
Thanks Bart!
If you like, I can try and work on #9 (no promises :(), but I think I'll need some way to test it here.
Do you by chance have a straightforward recipe to setup SRP and SRPT on two Linux's for this simple purpose?
Quoted 7 lines
Quoted 2 lines
Doug, just drop #9 'IB/srp: Do not create an all physical insecure rkey by default' please, thanks.
I've pulled in the series for 4.3. I've included patch #9, but that's on the understanding that if it can't be fixed before the initial pull request, I'll drop it at that time. I have the resources to test this, so I can work on it, and that factors into my path forward here.
Hi Doug,

Quoted 7 lines

Quoted 5 lines
Do you feel comfortable enough to queue the nine patches that replace Jason's patch #9 for kernel v4.3 (see also
http://thread.gmane.org/gmane.linux.drivers.rdma/28153/focus=28419) or would you rather prefer to take out patch #9 and queue these nine patches for kernel v4.4 ? Please advise.
I was planning on queueing up the 9 replacement patches.

Hello Doug,

I hope that someone who has access to a DDN system has the time to test this patch series against a DDN array. The memory key that is associated with the indirect data buffer descriptor is never used by the upstream SRP target driver (ib_srpt) but is used by DDN arrays to transfer the indirect data buffer descriptor. DDN arrays support indirect data buffers that are larger than what fits in the SRP_CMD information unit but the upstream SRP target driver not.

Bart.

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

A message that is not in the archive (sent off-list or not yet received): [email protected]

Doug Ledford

2015-08-15 02:08 UTC #39 lore raw

Quoted 36 lines · Bart Van Assche

Quoted 25 lines · Doug Ledford
Quoted 24 lines · Bart Van Assche
Quoted 13 lines · Doug Ledford
Quoted 12 lines · Jason Gunthorpe
Quoted 1 line · Bart Van Assche
With only patches 1/12 and 8/12 applied my test passes.
Thanks Bart!
If you like, I can try and work on #9 (no promises :(), but I think I'll need some way to test it here.
Do you by chance have a straightforward recipe to setup SRP and SRPT on two Linux's for this simple purpose?
Quoted 7 lines
Quoted 2 lines
Doug, just drop #9 'IB/srp: Do not create an all physical insecure rkey by default' please, thanks.
I've pulled in the series for 4.3. I've included patch #9, but that's on the understanding that if it can't be fixed before the initial pull request, I'll drop it at that time. I have the resources to test this, so I can work on it, and that factors into my path forward here.
Hi Doug,
Quoted 7 lines
Quoted 5 lines
Do you feel comfortable enough to queue the nine patches that replace Jason's patch #9 for kernel v4.3 (see also
http://thread.gmane.org/gmane.linux.drivers.rdma/28153/focus=28419) or would you rather prefer to take out patch #9 and queue these nine patches for kernel v4.4 ? Please advise.
I was planning on queueing up the 9 replacement patches.
Hello Doug,

Quoted 7 lines

I hope that someone who has access to a DDN system has the time to test this patch series against a DDN array. The memory key that is associated with the indirect data buffer descriptor is never used by the upstream SRP target driver (ib_srpt) but is used by DDN arrays to transfer the indirect data buffer descriptor. DDN arrays support indirect data buffers that are larger than what fits in the SRP_CMD information unit but the upstream SRP target driver not.

It will be ready to go very early in the merge window, so they would have plenty of time to test. Even pre-merge window if they wanted to.

Ben, can you find out if there would be an ability to test an upstream kernel against a DDN array where you are?

Signature · 2 lines

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

Christoph Hellwig

2015-08-03 15:24 UTC #40 lore raw

Quoted 5 lines · Bart Van Assche

Quoted 3 lines · Jason Gunthorpe
All patches are compile tested. I've done basic testing up to and including the IPoIB patch, the rest required specialized setups I don't have access to, but are fairly straightforward.

Quoted 7 lines

Hello Jason,
SRP login fails with this patch series applied on top of Linux kernel v4.2-rc4. At the target side the following message appears every time the SRP initiator tries to log in: "ib_srpt: RDMA t 5 for idx 0 failed with status 10" (10=remote access error). That causes the initiator to receive a flush error (5).

Could this be caused because srp_map_sg_entry falls back to the phys mapping for unaligned large requests in line 1389 in Dougs tree:

	if ((!dev->use_fast_reg && dma_addr & ~dev->mr_page_mask) ||
	    dma_len > dev->mr_max_size) {
		ret = srp_finish_mapping(state, ch);
		if (ret)
			return ret;

		srp_map_desc(state, dma_addr, dma_len, target->rkey);
		srp_map_update_start(state, NULL, 0, 0);
		return 0;
	}

Bart, do you know what hardware this workaround is for?

Also the SRP driver still falls back to phys registrations if the better MR methods fail, something that will stop working with Jasons patch. It's something that looks a little questionable and which other ULDs don't do either, so it would be good time to review this practice.

Additionally the SRP_DATA_DESC_INDIRECT case always uses the global rkey, so whenever we hits this things are going to break.

Jason, I suspect it might be a good idea to rename the rkey field to something like global_rkey, and make sure it's guarded by a proper conditional shared by all users.

A message that is not in the archive (sent off-list or not yet received): [email protected]

Bart Van Assche

2015-08-03 18:33 UTC #42 lore raw

Quoted 10 lines · Christoph Hellwig

Quoted 5 lines · Bart Van Assche
SRP login fails with this patch series applied on top of Linux kernel v4.2-rc4. At the target side the following message appears every time the SRP initiator tries to log in: "ib_srpt: RDMA t 5 for idx 0 failed with status 10" (10=remote access error). That causes the initiator to receive a flush error (5).
Could this be caused because srp_map_sg_entry falls back to the phys mapping for unaligned large requests in line 1389 in Dougs tree:

Quoted 10 lines

	if ((!dev->use_fast_reg && dma_addr & ~dev->mr_page_mask) ||
	    dma_len > dev->mr_max_size) {
		ret = srp_finish_mapping(state, ch);
		if (ret)
			return ret;

		srp_map_desc(state, dma_addr, dma_len, target->rkey);
		srp_map_update_start(state, NULL, 0, 0);
		return 0;
	}

This morning I added WARN_ON_ONCE(!target->rkey) statements in the code
paths that use target->rkey and that revealed that the above code
was indeed the culprit. Changing the default registration method from FMR into FR should be sufficient to avoid that the above code is triggered for HCA's that support FR (e.g. ConnectX-3 and ConnectX-4). For HCA's that support FMR but not FR, how about telling the block layer that data for the SRP initiator driver should be aligned at a 4KB boundary ? In srp_add_one() one can see that the finest granularity the SRP initiator driver supports for memory registration is 4KB (mr_page_shift = ...).

Quoted 1 line

Bart, do you know what hardware this workaround is for?

I hope the HW vendors can comment on this. Sorry but I'm not sure which HCA models and/or firmware versions do not support FMR mapping with a non-zero offset.

Quoted 5 lines

Also the SRP driver still falls back to phys registrations if the better MR methods fail, something that will stop working with Jasons patch. It's something that looks a little questionable and which other ULDs don't do either, so it would be good time to review this practice.

Agreed.

Quoted 2 lines

Additionally the SRP_DATA_DESC_INDIRECT case always uses the global rkey, so whenever we hits this things are going to break.

Indeed ... I think avoiding passing the global rkey over the wire in this case means registering the indirect table explicitly. Some SRP target implementations (e.g. the upstream ib_srpt driver) do not support partial indirect tables and fetch the indirect table from the SRP_CMD information unit. This means that the upstream ib_srpt driver never uses the rkey that is passed in the indirect data buffer descriptor. Although the upstream SRP target driver would keep working fine if an invalid rkey would be passed in the indirect data buffer header, I haven't found any clause in the SRP specification that allows to do this even if the entire indirect descriptor is passed in the SRP_CMD IU.

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

A message that is not in the archive (sent off-list or not yet received): [email protected]

Jason Gunthorpe

author

2015-08-04 18:09 UTC #44 lore raw

Quoted 5 lines · Bart Van Assche

Quoted 1 line
Bart, do you know what hardware this workaround is for?
I hope the HW vendors can comment on this. Sorry but I'm not sure which HCA models and/or firmware versions do not support FMR mapping with a non-zero offset.

Perhaps David can remember why he added this:

commit 8f26c9ff9cd0317ad867bce972f69e0c6c2cbe3c
IB/srp: rework mapping engine to use multiple FMR entries

+ /* If we start at an offset into the FMR page, don't merge into

+        * the current FMR. Finish it out, and use the kernel's MR for this
+        * sg entry. This is to avoid potential bugs on some SRP targets
+        * that were never quite defined, but went away when the initiator
+        * avoided using FMR on such page fragments.
+        */

+ if (dma_addr & ~dev->fmr_page_mask || dma_len > dev->fmr_max_size) {
+ ret = srp_map_finish_fmr(state, target);
+ if (ret)
+ return ret;
+

+               srp_map_desc(state, dma_addr, dma_len, target->rkey);
+               srp_map_update_start(state, NULL, 0, 0);
+               return 0;
        }

The way it is phrased makes it seem like a target bug, not a HCA bug?

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

A message that is not in the archive (sent off-list or not yet received): 20150804180933.GB5038-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org

David Dillow

2015-08-05 06:41 UTC #46 lore raw

Quoted 10 lines · Jason Gunthorpe

Quoted 5 lines · Bart Van Assche
Quoted 1 line
Bart, do you know what hardware this workaround is for?
I hope the HW vendors can comment on this. Sorry but I'm not sure which HCA models and/or firmware versions do not support FMR mapping with a non-zero offset.
Perhaps David can remember why he added this:
commit 8f26c9ff9cd0317ad867bce972f69e0c6c2cbe3c

It's originally from commit 559ce8f150d7d031c79c4d79173860f1bdfe3ce4, and the list's attempts at code archaeology failed us:
http://article.gmane.org/gmane.linux.drivers.rdma/7149

Quoted 1 line

The way it is phrased makes it seem like a target bug, not a HCA bug?

Yes, the original commit is explicit about it being a target issue. Since it mentioned the boogeyman words "data corruption", I kept the work around in the FMR work.

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

A message that is not in the archive (sent off-list or not yet received): 1438756876.5698.2.camel-a7a0dvSY7KqLUyTwlgNVppKKF0rrzTr+@public.gmane.org

Jason Gunthorpe

author

2015-08-05 19:51 UTC #48 lore raw

Quoted 8 lines · David Dillow

Quoted 7 lines · Jason Gunthorpe
Quoted 5 lines · Bart Van Assche
Quoted 1 line
Bart, do you know what hardware this workaround is for?
I hope the HW vendors can comment on this. Sorry but I'm not sure which HCA models and/or firmware versions do not support FMR mapping with a non-zero offset.

Quoted 7 lines

Quoted 3 lines
Perhaps David can remember why he added this:
commit 8f26c9ff9cd0317ad867bce972f69e0c6c2cbe3c
It's originally from commit 559ce8f150d7d031c79c4d79173860f1bdfe3ce4, and the list's attempts at code archaeology failed us:
http://article.gmane.org/gmane.linux.drivers.rdma/7149

Okay.. So over time we went from a clear target specific bug described 9 years ago in 559ce through chinese whispers to a general unspecific fear of non-zero offset FMR?

But nobody has described FMR failure in this way in the past 9 years with any specificity?

My random guesses would be broken mthca firmware at the start of the FMR feature (long since fixed) or a wonky target that is now 10 years obsolete..

If it was an HCA bug, I strongly have to think it is fixed now. We use FMR all over the place and SRP is the only area I've noticed this restriction..

If it is a target bug, then FRWR should trigger it as wel, so we are already about to revert that workaround.

I'm inclined to drop this entirely.. What do you think Bart?

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

A message that is not in the archive (sent off-list or not yet received): 20150805195122.GA31595-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org

Bart Van Assche

2015-08-05 21:45 UTC #50 lore raw

Quoted 28 lines · Jason Gunthorpe

Quoted 8 lines · David Dillow
Quoted 7 lines · Jason Gunthorpe
Quoted 5 lines · Bart Van Assche
Quoted 1 line
Bart, do you know what hardware this workaround is for?
I hope the HW vendors can comment on this. Sorry but I'm not sure which HCA models and/or firmware versions do not support FMR mapping with a non-zero offset.
Quoted 7 lines
Quoted 3 lines
Perhaps David can remember why he added this:
commit 8f26c9ff9cd0317ad867bce972f69e0c6c2cbe3c
It's originally from commit 559ce8f150d7d031c79c4d79173860f1bdfe3ce4, and the list's attempts at code archaeology failed us:
http://article.gmane.org/gmane.linux.drivers.rdma/7149
Okay.. So over time we went from a clear target specific bug described 9 years ago in 559ce through chinese whispers to a general unspecific fear of non-zero offset FMR?
But nobody has described FMR failure in this way in the past 9 years with any specificity?
My random guesses would be broken mthca firmware at the start of the FMR feature (long since fixed) or a wonky target that is now 10 years obsolete..

Quoted 8 lines

If it was an HCA bug, I strongly have to think it is fixed now. We use FMR all over the place and SRP is the only area I've noticed this restriction..
If it is a target bug, then FRWR should trigger it as wel, so we are already about to revert that workaround.
I'm inclined to drop this entirely.. What do you think Bart?

Even today I see memory corruption at the initiator side with FMR and not with FR if I leave out the alignment check. Since this only occurs with FMR and not with FR that excludes the target side as a possible cause. I will have a closer look at the srp_map_finish_fmr() function. Always passing 0 as the second argument of srp_map_desc() in srp_map_finish_fmr() is probably wrong. Before 2011 that second argument was the page offset of the first sg-list entry.

Bart.

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

A message that is not in the archive (sent off-list or not yet received): [email protected]

Bart Van Assche

2015-08-05 22:41 UTC #52 lore raw

Quoted 38 lines · Bart Van Assche

Quoted 28 lines · Jason Gunthorpe
Quoted 8 lines · David Dillow
Quoted 7 lines · Jason Gunthorpe
Quoted 5 lines · Bart Van Assche
Quoted 1 line
Bart, do you know what hardware this workaround is for?
I hope the HW vendors can comment on this. Sorry but I'm not sure which HCA models and/or firmware versions do not support FMR mapping with a non-zero offset.
Quoted 7 lines
Quoted 3 lines
Perhaps David can remember why he added this:
commit 8f26c9ff9cd0317ad867bce972f69e0c6c2cbe3c
It's originally from commit 559ce8f150d7d031c79c4d79173860f1bdfe3ce4, and the list's attempts at code archaeology failed us:
http://article.gmane.org/gmane.linux.drivers.rdma/7149
Okay.. So over time we went from a clear target specific bug described 9 years ago in 559ce through chinese whispers to a general unspecific fear of non-zero offset FMR?
But nobody has described FMR failure in this way in the past 9 years with any specificity?
My random guesses would be broken mthca firmware at the start of the FMR feature (long since fixed) or a wonky target that is now 10 years obsolete..
Quoted 8 lines
If it was an HCA bug, I strongly have to think it is fixed now. We use FMR all over the place and SRP is the only area I've noticed this restriction..
If it is a target bug, then FRWR should trigger it as wel, so we are already about to revert that workaround.
I'm inclined to drop this entirely.. What do you think Bart?

Quoted 7 lines

Even today I see memory corruption at the initiator side with FMR and not with FR if I leave out the alignment check. Since this only occurs with FMR and not with FR that excludes the target side as a possible cause. I will have a closer look at the srp_map_finish_fmr() function. Always passing 0 as the second argument of srp_map_desc() in srp_map_finish_fmr() is probably wrong. Before 2011 that second argument was the page offset of the first sg-list entry.

(replying to my own e-mail)

The patch below makes FMR registration work again for buffers that are not aligned on a page boundary.

Regarding the discussion in 2011 about FMR (http://thread.gmane.org/gmane.linux.drivers.rdma/7149): since in 2011 nobody recalled the root cause of the issue with non-page aligned FMR my proposal is to drop the page alignment check and if any issues occur to introduce a blacklist for the SRP target devices that have trouble with this.

Bart.

[PATCH] IB/srp: Restore FMR offset

Since srp_map_finish_fmr() is always called with base_dma_addr aligned on a page boundary this patch does not change any functionality. See also patch "IB/srp: rework mapping engine to use multiple FMR entries" (commit ID 8f26c9ff9cd0; January 2011).

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

drivers/infiniband/ulp/srp/ib_srp.c +4 -1

index 48201b3..cac444e 100644
@@ -1272,6 +1272,8 @@ static void srp_map_desc(struct srp_map_state *state, dma_addr_t dma_addr,
 static int srp_map_finish_fmr(struct srp_map_state *state,
 			      struct srp_rdma_ch *ch)
 {
+	struct srp_target_port *target = ch->target;
+	struct srp_device *dev = target->srp_host->srp_dev;
 	struct ib_pool_fmr *fmr;
 	u64 io_addr = 0;

@@ -1283,7 +1285,8 @@ static int srp_map_finish_fmr(struct srp_map_state *state,
 	*state->next_fmr++ = fmr;
 	state->nmdesc++;
 
-	srp_map_desc(state, 0, state->dma_len, fmr->fmr->rkey);
+	srp_map_desc(state, state->base_dma_addr & ~dev->mr_page_mask,
+		     state->dma_len, fmr->fmr->rkey);
 
 	return 0;
 }

Signature · 8 lines

2.1.4



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

A message that is not in the archive (sent off-list or not yet received): [email protected]

Jason Gunthorpe

author

2015-08-06 00:10 UTC #54 lore raw

Quoted 6 lines · Bart Van Assche

Regarding the discussion in 2011 about FMR
(http://thread.gmane.org/gmane.linux.drivers.rdma/7149): since in 2011 nobody recalled the root cause of the issue with non-page aligned FMR my proposal is to drop the page alignment check and if any issues occur to introduce a blacklist for the SRP target devices that have trouble with this.

Nice find, that sounds like a solid plan.

That looks like it leaves only the srp_map_data flow and the error case flow in srp_map_sg as remaining problems for the rkey patch? Any thoughts on adressing those?

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

A message that is not in the archive (sent off-list or not yet received): 20150806001006.GD2483-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org

Bart Van Assche

2015-08-06 00:19 UTC #56 lore raw

Quoted 8 lines · Jason Gunthorpe

Quoted 6 lines · Bart Van Assche
Regarding the discussion in 2011 about FMR
(http://thread.gmane.org/gmane.linux.drivers.rdma/7149): since in 2011 nobody recalled the root cause of the issue with non-page aligned FMR my proposal is to drop the page alignment check and if any issues occur to introduce a blacklist for the SRP target devices that have trouble with this.

Quoted 5 lines

Nice find, that sounds like a solid plan.
That looks like it leaves only the srp_map_data flow and the error case flow in srp_map_sg as remaining problems for the rkey patch? Any thoughts on adressing those?

Hello Jason,

A few experimental and untested patches are available for review at https://github.com/bvanassche/linux/tree/srp-initiator-for-next-experimental.

Bart.

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

A message that is not in the archive (sent off-list or not yet received): [email protected]

Jason Gunthorpe

author

2015-08-06 04:36 UTC #58 lore raw

Quoted 2 lines · Bart Van Assche

A few experimental and untested patches are available for review at
https://github.com/bvanassche/linux/tree/srp-initiator-for-next-experimental.

I'll leave this is in your hands then..

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

A message that is not in the archive (sent off-list or not yet received): 20150806043642.GA14153-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org

Bart Van Assche

2015-08-06 15:09 UTC #60 lore raw

Quoted 5 lines · Jason Gunthorpe

Quoted 2 lines · Bart Van Assche
A few experimental and untested patches are available for review at
https://github.com/bvanassche/linux/tree/srp-initiator-for-next-experimental.
I'll leave this is in your hands then..

Is there perhaps a tree available somewhere with the latest version of your patch series ?

Thanks,

Bart.

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

Bart Van Assche

2015-08-11 00:05 UTC #61 lore raw

cover IB/srp: Do not create an all physical insecure rkey by default

Quoted 6 lines · Jason Gunthorpe

Quoted 2 lines · Bart Van Assche
A few experimental and untested patches are available for review at
https://github.com/bvanassche/linux/tree/srp-initiator-for-next-experimental.
I'll leave this is in your hands then..

In case anyone would like to see what I am working on, I will post the patches I am currently testing in reply to this e-mail. The names of these patches are as follows:

0001-IB-srp-Re-enable-FMR-for-non-page-aligned-buffers.patch 0002-IB-srp-Use-multiple-registrations-for-large-memory-r.patch 0003-IB-srp-Add-memory-descriptor-array-pointer-range-che.patch 0004-IB-srp-Remove-the-memory-registration-backtracking-c.patch 0005-IB-srp-Remove-use_mr-argument-from-srp_map_sg_entry.patch 0006-IB-srp-Introduce-srp_device.use_fmr.patch 0007-IB-srp-Register-the-indirect-data-buffer-descriptor.patch 0008-IB-srp-Create-an-insecure-all-physical-rkey-only-if-.patch

These patches are also available at https://github.com/bvanassche/linux/tree/srp-initiator-for-next.

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

A message that is not in the archive (sent off-list or not yet received): [email protected]

Bart Van Assche

2015-08-11 00:06 UTC #63 lore raw

PATCH 1/8 IB/srp: Re-enable FMR for non-page aligned buffers

During a discussion in 2011 nobody recalled why FMR was not used for non-page aligned buffers (see also
http://thread.gmane.org/gmane.linux.drivers.rdma/7149). Re-enable FMR for such buffers. For the reason why the srp_map_fmr() function needs to be modified, see also patch "IB/srp: rework mapping engine to use multiple FMR entries" (commit ID 8f26c9ff9cd0; January 2011).

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

drivers/infiniband/ulp/srp/ib_srp.c +5 -9

index 2968b7b..887e8ca 100644
@@ -1272,6 +1272,8 @@ static void srp_map_desc(struct srp_map_state *state, dma_addr_t dma_addr,
 static int srp_map_finish_fmr(struct srp_map_state *state,
 			      struct srp_rdma_ch *ch)
 {
+	struct srp_target_port *target = ch->target;
+	struct srp_device *dev = target->srp_host->srp_dev;
 	struct ib_pool_fmr *fmr;
 	u64 io_addr = 0;

@@ -1283,7 +1285,8 @@ static int srp_map_finish_fmr(struct srp_map_state *state,
 	*state->next_fmr++ = fmr;
 	state->nmdesc++;
 
-	srp_map_desc(state, 0, state->dma_len, fmr->fmr->rkey);
+	srp_map_desc(state, state->base_dma_addr & ~dev->mr_page_mask,
+		     state->dma_len, fmr->fmr->rkey);
 
 	return 0;
 }

@@ -1390,14 +1393,7 @@ static int srp_map_sg_entry(struct srp_map_state *state,
 		return 0;
 	}
 
-	/*
-	 * Since not all RDMA HW drivers support non-zero page offsets for
-	 * FMR, if we start at an offset into a page, don't merge into the
-	 * current FMR mapping. Finish it out, and use the kernel's MR for
-	 * this sg entry.
-	 */
-	if ((!dev->use_fast_reg && dma_addr & ~dev->mr_page_mask) ||
-	    dma_len > dev->mr_max_size) {
+	if (dma_len > dev->mr_max_size) {
 		ret = srp_finish_mapping(state, ch);
 		if (ret)
 			return ret;

Signature · 6 lines

2.1.4

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

A message that is not in the archive (sent off-list or not yet received): [email protected]

Sagi Grimberg

2015-08-16 10:53 UTC #65 lore raw

Re: [PATCH 1/8] IB/srp: Re-enable FMR for non-page aligned buffers

Quoted 8 lines · Bart Van Assche

During a discussion in 2011 nobody recalled why FMR was not used for non-page aligned buffers (see also
http://thread.gmane.org/gmane.linux.drivers.rdma/7149). Re-enable FMR for such buffers. For the reason why the srp_map_fmr() function needs to be modified, see also patch "IB/srp: rework mapping engine to use multiple FMR entries" (commit ID 8f26c9ff9cd0; January 2011).
Signed-off-by Bart Van Assche <[email protected]>

Hi Bart,

Just wanted to confirm that this was tested with FMR and non-aligned IO. (the change log doesn't say it).
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 55D06BB3.7070905-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org

Bart Van Assche

2015-08-16 15:49 UTC #67 lore raw

Re: [PATCH 1/8] IB/srp: Re-enable FMR for non-page aligned buffers

Quoted 8 lines · Sagi Grimberg

Quoted 7 lines · Bart Van Assche
During a discussion in 2011 nobody recalled why FMR was not used for non-page aligned buffers (see also
http://thread.gmane.org/gmane.linux.drivers.rdma/7149). Re-enable FMR for such buffers. For the reason why the srp_map_fmr() function needs to be modified, see also patch "IB/srp: rework mapping engine to use multiple FMR entries" (commit ID 8f26c9ff9cd0; January 2011).

Quoted 6 lines

Quoted 1 line
Signed-off-by Bart Van Assche <[email protected]>
Hi Bart,
Just wanted to confirm that this was tested with FMR and non-aligned IO. (the change log doesn't say it).

Hello Sagi,

This patch series has been tested with FMR and I/O buffers that were not aligned to a page boundary by setting the ib_srp kernel module parameter prefer_fr to 0.

Bart.

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

Bart Van Assche

2015-08-11 00:06 UTC #68 lore raw

PATCH 2/8 IB/srp: Use multiple registrations for large memory regions

Instead of using the global rkey for large memory regions, use multiple registrations. See also the while (dma_len) loop further down in srp_map_sg_entry().

---
 drivers/infiniband/ulp/srp/ib_srp.c | 10 ----------
 1 file changed, 10 deletions(-)

drivers/infiniband/ulp/srp/ib_srp.c +0 ~~-10~~

index 887e8ca..b6b9a55 100644
@@ -1393,16 +1393,6 @@ static int srp_map_sg_entry(struct srp_map_state *state,
 		return 0;
 	}
 
-	if (dma_len > dev->mr_max_size) {
-		ret = srp_finish_mapping(state, ch);
-		if (ret)
-			return ret;
-
-		srp_map_desc(state, dma_addr, dma_len, target->rkey);
-		srp_map_update_start(state, NULL, 0, 0);
-		return 0;
-	}
-
 	/*
 	 * If this is the first sg that will be mapped via FMR or via FR, save
 	 * our position. We need to know the first unmapped entry, its index,

Signature · 6 lines

2.1.4

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

Bart Van Assche

2015-08-11 00:07 UTC #69 lore raw

PATCH 3/8 IB/srp: Add memory descriptor array pointer range checking

Although most paths through which a request is submitted check block layer parameters like the max_segments limit, these are not checked when an SG_IO or direct I/O request is submitted. Hence add a range check for the memory descriptor array pointer.

---
 drivers/infiniband/ulp/srp/ib_srp.c | 16 ++++++++++++----
 drivers/infiniband/ulp/srp/ib_srp.h | 10 ++++++++--
 2 files changed, 20 insertions(+), 6 deletions(-)

drivers/infiniband/ulp/srp/ib_srp.c +12 -4

index b6b9a55..f9fa220 100644
@@ -1277,12 +1277,15 @@ static int srp_map_finish_fmr(struct srp_map_state *state,
 	struct ib_pool_fmr *fmr;
 	u64 io_addr = 0;
 
+	if (state->fmr.next >= state->fmr.end)
+		return -ENOMEM;
+
 	fmr = ib_fmr_pool_map_phys(ch->fmr_pool, state->pages,
 				   state->npages, io_addr);
 	if (IS_ERR(fmr))
 		return PTR_ERR(fmr);
 
-	*state->next_fmr++ = fmr;
+	*state->fmr.next++ = fmr;
 	state->nmdesc++;
 
 	srp_map_desc(state, state->base_dma_addr & ~dev->mr_page_mask,

@@ -1301,6 +1304,9 @@ static int srp_map_finish_fr(struct srp_map_state *state,
 	struct srp_fr_desc *desc;
 	u32 rkey;
 
+	if (state->fr.next >= state->fr.end)
+		return -ENOMEM;
+
 	desc = srp_fr_pool_get(ch->fr_pool);
 	if (!desc)
 		return -ENOMEM;

@@ -1324,7 +1330,7 @@ static int srp_map_finish_fr(struct srp_map_state *state,
 				       IB_ACCESS_REMOTE_WRITE);
 	wr.wr.fast_reg.rkey = desc->mr->lkey;
 
-	*state->next_fr++ = desc;
+	*state->fr.next++ = desc;
 	state->nmdesc++;
 
 	srp_map_desc(state, state->base_dma_addr, state->dma_len,

@@ -1450,10 +1456,12 @@ static int srp_map_sg(struct srp_map_state *state, struct srp_rdma_ch *ch,
 	state->desc	= req->indirect_desc;
 	state->pages	= req->map_page;
 	if (dev->use_fast_reg) {
-		state->next_fr = req->fr_list;
+		state->fr.next = req->fr_list;
+		state->fr.end = req->fr_list + target->cmd_sg_cnt;
 		use_mr = !!ch->fr_pool;
 	} else {
-		state->next_fmr = req->fmr_list;
+		state->fmr.next = req->fmr_list;
+		state->fmr.end = req->fmr_list + target->cmd_sg_cnt;
 		use_mr = !!ch->fmr_pool;
 	}

drivers/infiniband/ulp/srp/ib_srp.h +8 -2

index 17ee3f8..2ab73bc 100644
@@ -282,8 +282,14 @@ struct srp_fr_pool {
  */
 struct srp_map_state {
 	union {
-		struct ib_pool_fmr **next_fmr;
-		struct srp_fr_desc **next_fr;
+		struct {
+			struct ib_pool_fmr **next;
+			struct ib_pool_fmr **end;
+		} fmr;
+		struct {
+			struct srp_fr_desc **next;
+			struct srp_fr_desc **end;
+		} fr;
 	};
 	struct srp_direct_buf  *desc;
 	u64		       *pages;

Signature · 6 lines

2.1.4

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

A message that is not in the archive (sent off-list or not yet received): [email protected]

Sagi Grimberg

2015-08-16 10:57 UTC #71 lore raw

Re: [PATCH 3/8] IB/srp: Add memory descriptor array pointer range checking

Quoted 3 lines · Bart Van Assche

Although most paths through which a request is submitted check block layer parameters like the max_segments limit, these are not checked when an SG_IO or direct I/O request is submitted.

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

A message that is not in the archive (sent off-list or not yet received): 55D06C9D.7030608-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org

Bart Van Assche

2015-08-16 15:51 UTC #73 lore raw

Re: [PATCH 3/8] IB/srp: Add memory descriptor array pointer range checking

Quoted 6 lines · Sagi Grimberg

Quoted 3 lines · Bart Van Assche
Although most paths through which a request is submitted check block layer parameters like the max_segments limit, these are not checked when an SG_IO or direct I/O request is submitted.
Why not check that there instead?

The scope of this patch series is to avoid that the SRP initiator needs to create a global memory key. Changing the behavior of SG_IO and/or direct I/O falls outside the scope of this patch series.

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

Bart Van Assche

2015-08-11 00:07 UTC #74 lore raw

PATCH 4/8 IB/srp: Remove the memory registration backtracking code

Mapping a discontiguous sg-list requires multiple memory regions and hence can exhaust the memory region pool. The SRP initiator already handles this by temporarily reducing the queue depth. This means that it is safe to remove the memory registration backtracking code. This patch has been tested with direct I/O sizes up to 256 MB.

---
 drivers/infiniband/ulp/srp/ib_srp.c | 62 ++++++++-----------------------------
 drivers/infiniband/ulp/srp/ib_srp.h |  6 ----
 2 files changed, 13 insertions(+), 55 deletions(-)

drivers/infiniband/ulp/srp/ib_srp.c +13 ~~-49~~

index f9fa220..660a4a4 100644
@@ -1364,15 +1364,6 @@ static int srp_finish_mapping(struct srp_map_state *state,
 	return ret;
 }
 
-static void srp_map_update_start(struct srp_map_state *state,
-				 struct scatterlist *sg, int sg_index,
-				 dma_addr_t dma_addr)
-{
-	state->unmapped_sg = sg;
-	state->unmapped_index = sg_index;
-	state->unmapped_addr = dma_addr;
-}
-
 static int srp_map_sg_entry(struct srp_map_state *state,
 			    struct srp_rdma_ch *ch,
 			    struct scatterlist *sg, int sg_index,

@@ -1399,23 +1390,12 @@ static int srp_map_sg_entry(struct srp_map_state *state,
 		return 0;
 	}
 
-	/*
-	 * If this is the first sg that will be mapped via FMR or via FR, save
-	 * our position. We need to know the first unmapped entry, its index,
-	 * and the first unmapped address within that entry to be able to
-	 * restart mapping after an error.
-	 */
-	if (!state->unmapped_sg)
-		srp_map_update_start(state, sg, sg_index, dma_addr);
-
 	while (dma_len) {
 		unsigned offset = dma_addr & ~dev->mr_page_mask;
 		if (state->npages == dev->max_pages_per_mr || offset != 0) {
 			ret = srp_finish_mapping(state, ch);
 			if (ret)
 				return ret;
-
-			srp_map_update_start(state, sg, sg_index, dma_addr);
 		}
 
 		len = min_t(unsigned int, dma_len, dev->mr_page_size - offset);

@@ -1434,11 +1414,8 @@ static int srp_map_sg_entry(struct srp_map_state *state,
 	 * boundaries.
 	 */
 	ret = 0;
-	if (len != dev->mr_page_size) {
+	if (len != dev->mr_page_size)
 		ret = srp_finish_mapping(state, ch);
-		if (!ret)
-			srp_map_update_start(state, NULL, 0, 0);
-	}
 	return ret;
 }

@@ -1448,9 +1425,8 @@ static int srp_map_sg(struct srp_map_state *state, struct srp_rdma_ch *ch,
 {
 	struct srp_target_port *target = ch->target;
 	struct srp_device *dev = target->srp_host->srp_dev;
-	struct ib_device *ibdev = dev->dev;
 	struct scatterlist *sg;
-	int i;
+	int i, ret;
 	bool use_mr;
 
 	state->desc	= req->indirect_desc;

@@ -1466,34 +1442,22 @@ static int srp_map_sg(struct srp_map_state *state, struct srp_rdma_ch *ch,
 	}
 
 	for_each_sg(scat, sg, count, i) {
-		if (srp_map_sg_entry(state, ch, sg, i, use_mr)) {
-			/*
-			 * Memory registration failed, so backtrack to the
-			 * first unmapped entry and continue on without using
-			 * memory registration.
-			 */
-			dma_addr_t dma_addr;
-			unsigned int dma_len;
-
-backtrack:
-			sg = state->unmapped_sg;
-			i = state->unmapped_index;
-
-			dma_addr = ib_sg_dma_address(ibdev, sg);
-			dma_len = ib_sg_dma_len(ibdev, sg);
-			dma_len -= (state->unmapped_addr - dma_addr);
-			dma_addr = state->unmapped_addr;
-			use_mr = false;
-			srp_map_desc(state, dma_addr, dma_len, target->rkey);
-		}
+		ret = srp_map_sg_entry(state, ch, sg, i, use_mr);
+		if (ret)
+			goto out;
 	}
 
-	if (use_mr && srp_finish_mapping(state, ch))
-		goto backtrack;
+	if (use_mr) {
+		ret = srp_finish_mapping(state, ch);
+		if (ret)
+			goto out;
+	}
 
 	req->nmdesc = state->nmdesc;
+	ret = 0;
 
-	return 0;
+out:
+	return ret;
 }
 
 static int srp_map_data(struct scsi_cmnd *scmnd, struct srp_rdma_ch *ch,

drivers/infiniband/ulp/srp/ib_srp.h +0 -6

index 2ab73bc..1e42418 100644
@@ -276,9 +276,6 @@ struct srp_fr_pool {
  * @npages:	    Number of page addresses in the pages[] array.
  * @nmdesc:	    Number of FMR or FR memory descriptors used for mapping.
  * @ndesc:	    Number of SRP buffer descriptors that have been filled in.
- * @unmapped_sg:    First element of the sg-list that is mapped via FMR or FR.
- * @unmapped_index: Index of the first element mapped via FMR or FR.
- * @unmapped_addr:  DMA address of the first element mapped via FMR or FR.
  */
 struct srp_map_state {
 	union {

@@ -299,9 +296,6 @@ struct srp_map_state {
 	unsigned int		npages;
 	unsigned int		nmdesc;
 	unsigned int		ndesc;
-	struct scatterlist     *unmapped_sg;
-	int			unmapped_index;
-	dma_addr_t		unmapped_addr;
 };
 
 #endif /* IB_SRP_H */

Signature · 6 lines

2.1.4

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

Bart Van Assche

2015-08-11 00:08 UTC #75 lore raw

PATCH 5/8 IB/srp: Remove use_mr argument from srp_map_sg_entry()

Move the srp_map_desc() call from inside srp_map_sg_entry() to srp_map_sg() such that the use_mr argument can be removed from srp_map_sg_entry().

---
 drivers/infiniband/ulp/srp/ib_srp.c | 36 +++++++++++++++---------------------
 1 file changed, 15 insertions(+), 21 deletions(-)

drivers/infiniband/ulp/srp/ib_srp.c +15 ~~-21~~

index 660a4a4..b8fc35b 100644
@@ -1260,6 +1260,8 @@ static void srp_map_desc(struct srp_map_state *state, dma_addr_t dma_addr,
 {
 	struct srp_direct_buf *desc = state->desc;
 
+	WARN_ON_ONCE(!dma_len);
+
 	desc->va = cpu_to_be64(dma_addr);
 	desc->key = cpu_to_be32(rkey);
 	desc->len = cpu_to_be32(dma_len);

@@ -1366,29 +1368,17 @@ static int srp_finish_mapping(struct srp_map_state *state,
 
 static int srp_map_sg_entry(struct srp_map_state *state,
 			    struct srp_rdma_ch *ch,
-			    struct scatterlist *sg, int sg_index,
-			    bool use_mr)
+			    struct scatterlist *sg, int sg_index)
 {
 	struct srp_target_port *target = ch->target;
 	struct srp_device *dev = target->srp_host->srp_dev;
 	struct ib_device *ibdev = dev->dev;
 	dma_addr_t dma_addr = ib_sg_dma_address(ibdev, sg);
 	unsigned int dma_len = ib_sg_dma_len(ibdev, sg);
-	unsigned int len;
+	unsigned int len = 0;
 	int ret;
 
-	if (!dma_len)
-		return 0;
-
-	if (!use_mr) {
-		/*
-		 * Once we're in direct map mode for a request, we don't
-		 * go back to FMR or FR mode, so no need to update anything
-		 * other than the descriptor.
-		 */
-		srp_map_desc(state, dma_addr, dma_len, target->rkey);
-		return 0;
-	}
+	WARN_ON_ONCE(!dma_len);
 
 	while (dma_len) {
 		unsigned offset = dma_addr & ~dev->mr_page_mask;

@@ -1441,16 +1431,20 @@ static int srp_map_sg(struct srp_map_state *state, struct srp_rdma_ch *ch,
 		use_mr = !!ch->fmr_pool;
 	}
 
-	for_each_sg(scat, sg, count, i) {
-		ret = srp_map_sg_entry(state, ch, sg, i, use_mr);
-		if (ret)
-			goto out;
-	}
-
 	if (use_mr) {
+		for_each_sg(scat, sg, count, i) {
+			ret = srp_map_sg_entry(state, ch, sg, i);
+			if (ret)
+				goto out;
+		}
 		ret = srp_finish_mapping(state, ch);
 		if (ret)
 			goto out;
+	} else {
+		for_each_sg(scat, sg, count, i) {
+			srp_map_desc(state, ib_sg_dma_address(dev->dev, sg),
+				     ib_sg_dma_len(dev->dev, sg), target->rkey);
+		}
 	}
 
 	req->nmdesc = state->nmdesc;

Signature · 6 lines

2.1.4

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

Bart Van Assche

2015-08-11 00:08 UTC #76 lore raw

PATCH 6/8 IB/srp: Introduce srp_device.use_fmr

Introduce the variable srp_device.use_fmr. Leave out the dev->has_fr / dev->has_fmr and ch->fr_pool / ch->fmr_pool checks since these are redundant. This patch does not change any functionality but makes the source code easier to read.

---
 drivers/infiniband/ulp/srp/ib_srp.c | 22 +++++++++++-----------
 drivers/infiniband/ulp/srp/ib_srp.h |  1 +
 2 files changed, 12 insertions(+), 11 deletions(-)

drivers/infiniband/ulp/srp/ib_srp.c +11 ~~-11~~

index b8fc35b..1029fd2 100644
@@ -546,7 +546,7 @@ static int srp_create_ch_ib(struct srp_rdma_ch *ch)
 	if (ret)
 		goto err_qp;
 
-	if (dev->use_fast_reg && dev->has_fr) {
+	if (dev->use_fast_reg) {
 		fr_pool = srp_alloc_fr_pool(target);
 		if (IS_ERR(fr_pool)) {
 			ret = PTR_ERR(fr_pool);

@@ -557,7 +557,7 @@ static int srp_create_ch_ib(struct srp_rdma_ch *ch)
 		if (ch->fr_pool)
 			srp_destroy_fr_pool(ch->fr_pool);
 		ch->fr_pool = fr_pool;
-	} else if (!dev->use_fast_reg && dev->has_fmr) {
+	} else if (dev->use_fmr) {
 		fmr_pool = srp_alloc_fmr_pool(target);
 		if (IS_ERR(fmr_pool)) {
 			ret = PTR_ERR(fmr_pool);

@@ -623,7 +623,7 @@ static void srp_free_ch_ib(struct srp_target_port *target,
 	if (dev->use_fast_reg) {
 		if (ch->fr_pool)
 			srp_destroy_fr_pool(ch->fr_pool);
-	} else {
+	} else if (dev->use_fmr) {
 		if (ch->fmr_pool)
 			ib_destroy_fmr_pool(ch->fmr_pool);
 	}

@@ -1085,7 +1085,7 @@ static void srp_unmap_data(struct scsi_cmnd *scmnd,
 		if (req->nmdesc)
 			srp_fr_pool_put(ch->fr_pool, req->fr_list,
 					req->nmdesc);
-	} else {
+	} else if (dev->use_fmr) {
 		struct ib_pool_fmr **pfmr;
 
 		for (i = req->nmdesc, pfmr = req->fmr_list; i > 0; i--, pfmr++)

@@ -1345,8 +1345,11 @@ static int srp_finish_mapping(struct srp_map_state *state,
 			      struct srp_rdma_ch *ch)
 {
 	struct srp_target_port *target = ch->target;
+	struct srp_device *dev = target->srp_host->srp_dev;
 	int ret = 0;
 
+	WARN_ON_ONCE(!dev->use_fast_reg && !dev->use_fmr);
+
 	if (state->npages == 0)
 		return 0;

@@ -1354,8 +1357,7 @@ static int srp_finish_mapping(struct srp_map_state *state,
 		srp_map_desc(state, state->base_dma_addr, state->dma_len,
 			     target->rkey);
 	else
-		ret = target->srp_host->srp_dev->use_fast_reg ?
-			srp_map_finish_fr(state, ch) :
+		ret = dev->use_fast_reg ? srp_map_finish_fr(state, ch) :
 			srp_map_finish_fmr(state, ch);
 
 	if (ret == 0) {

@@ -1417,21 +1419,18 @@ static int srp_map_sg(struct srp_map_state *state, struct srp_rdma_ch *ch,
 	struct srp_device *dev = target->srp_host->srp_dev;
 	struct scatterlist *sg;
 	int i, ret;
-	bool use_mr;
 
 	state->desc	= req->indirect_desc;
 	state->pages	= req->map_page;
 	if (dev->use_fast_reg) {
 		state->fr.next = req->fr_list;
 		state->fr.end = req->fr_list + target->cmd_sg_cnt;
-		use_mr = !!ch->fr_pool;
-	} else {
+	} else if (dev->use_fmr) {
 		state->fmr.next = req->fmr_list;
 		state->fmr.end = req->fmr_list + target->cmd_sg_cnt;
-		use_mr = !!ch->fmr_pool;
 	}
 
-	if (use_mr) {
+	if (dev->use_fast_reg || dev->use_fmr) {
 		for_each_sg(scat, sg, count, i) {
 			ret = srp_map_sg_entry(state, ch, sg, i);
 			if (ret)

@@ -3355,6 +3354,7 @@ static void srp_add_one(struct ib_device *device)
 
 	srp_dev->use_fast_reg = (srp_dev->has_fr &&
 				 (!srp_dev->has_fmr || prefer_fr));
+	srp_dev->use_fmr = !srp_dev->use_fast_reg && srp_dev->has_fmr;
 
 	/*
 	 * Use the smallest page size supported by the HCA, down to a

drivers/infiniband/ulp/srp/ib_srp.h +1 -0

index 1e42418..60a33c1 100644
@@ -102,6 +102,7 @@ struct srp_device {
 	int			max_pages_per_mr;
 	bool			has_fmr;
 	bool			has_fr;
+	bool			use_fmr;
 	bool			use_fast_reg;
 };

Signature · 6 lines

2.1.4

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

A message that is not in the archive (sent off-list or not yet received): [email protected]

Sagi Grimberg

2015-08-16 11:03 UTC #78 lore raw

Re: [PATCH 6/8] IB/srp: Introduce srp_device.use_fmr

Quoted 64 lines · Bart Van Assche

---
  drivers/infiniband/ulp/srp/ib_srp.c | 22 +++++++++++-----------
  drivers/infiniband/ulp/srp/ib_srp.h |  1 +
  2 files changed, 12 insertions(+), 11 deletions(-)

drivers/infiniband/ulp/srp/ib_srp.c +7 -4

index b8fc35b..1029fd2 100644
@@ -546,7 +546,7 @@ static int srp_create_ch_ib(struct srp_rdma_ch *ch)
  	if (ret)
  		goto err_qp;
 
-	if (dev->use_fast_reg && dev->has_fr) {
+	if (dev->use_fast_reg) {
  		fr_pool = srp_alloc_fr_pool(target);
  		if (IS_ERR(fr_pool)) {
  			ret = PTR_ERR(fr_pool);

@@ -557,7 +557,7 @@ static int srp_create_ch_ib(struct srp_rdma_ch *ch)
  		if (ch->fr_pool)
  			srp_destroy_fr_pool(ch->fr_pool);
  		ch->fr_pool = fr_pool;
-	} else if (!dev->use_fast_reg && dev->has_fmr) {
+	} else if (dev->use_fmr) {
  		fmr_pool = srp_alloc_fmr_pool(target);
  		if (IS_ERR(fmr_pool)) {
  			ret = PTR_ERR(fmr_pool);

@@ -623,7 +623,7 @@ static void srp_free_ch_ib(struct srp_target_port *target,
  	if (dev->use_fast_reg) {
  		if (ch->fr_pool)
  			srp_destroy_fr_pool(ch->fr_pool);
-	} else {
+	} else if (dev->use_fmr) {
  		if (ch->fmr_pool)
  			ib_destroy_fmr_pool(ch->fmr_pool);
  	}

@@ -1085,7 +1085,7 @@ static void srp_unmap_data(struct scsi_cmnd *scmnd,
  		if (req->nmdesc)
  			srp_fr_pool_put(ch->fr_pool, req->fr_list,
  					req->nmdesc);
-	} else {
+	} else if (dev->use_fmr) {
  		struct ib_pool_fmr **pfmr;
 
  		for (i = req->nmdesc, pfmr = req->fmr_list; i > 0; i--, pfmr++)

@@ -1345,8 +1345,11 @@ static int srp_finish_mapping(struct srp_map_state *state,
  			      struct srp_rdma_ch *ch)
  {
  	struct srp_target_port *target = ch->target;
+	struct srp_device *dev = target->srp_host->srp_dev;
  	int ret = 0;
 
+	WARN_ON_ONCE(!dev->use_fast_reg && !dev->use_fmr);
+
  	if (state->npages == 0)
  		return 0;

drivers/infiniband/ulp/srp/ib_srp.c static int srp_finish_mapping(struct srp_map_state *state, · line 1361

Quoted 7 lines

drivers/infiniband/ulp/srp/ib_srp.c +1 -2

@@ -1354,8 +1357,7 @@ static int srp_finish_mapping(struct srp_map_state *state,
  		srp_map_desc(state, state->base_dma_addr, state->dma_len,
  			     target->rkey);
  	else
-		ret = target->srp_host->srp_dev->use_fast_reg ?
-			srp_map_finish_fr(state, ch) :
+		ret = dev->use_fast_reg ? srp_map_finish_fr(state, ch) :
  			srp_map_finish_fmr(state, ch);

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

A message that is not in the archive (sent off-list or not yet received): 55D06E05.5060209-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org

Bart Van Assche

2015-08-16 15:57 UTC #80 lore raw

Re: [PATCH 6/8] IB/srp: Introduce srp_device.use_fmr

Quoted 11 lines · Sagi Grimberg

drivers/infiniband/ulp/srp/ib_srp.c static int srp_finish_mapping(struct srp_map_state *state, · line 1361
Quoted 8 lines · Bart Van Assche
drivers/infiniband/ulp/srp/ib_srp.c +1 -2
@@ -1354,8 +1357,7 @@ static int srp_finish_mapping(struct srp_map_state *state,
   		srp_map_desc(state, state->base_dma_addr, state->dma_len,
   			     target->rkey);
   	else
-		ret = target->srp_host->srp_dev->use_fast_reg ?
-			srp_map_finish_fr(state, ch) :
+		ret = dev->use_fast_reg ? srp_map_finish_fr(state, ch) :
   			srp_map_finish_fmr(state, ch);
Is this related to the patch?

This change could have been split out as a separate patch. If I have to repost this patch series I will convert this change into a separate patch.

Bart.

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

Bart Van Assche

2015-08-11 00:09 UTC #81 lore raw

PATCH 7/8 IB/srp: Register the indirect data buffer descriptor

Instead of always using the global rkey for the indirect data buffer descriptor, register that descriptor with the HCA if the kernel module parameter register_always has been set to Y.

---
 drivers/infiniband/ulp/srp/ib_srp.c | 57 +++++++++++++++++++++++++++++++++++--
 drivers/infiniband/ulp/srp/ib_srp.h |  4 +++
 2 files changed, 58 insertions(+), 3 deletions(-)

drivers/infiniband/ulp/srp/ib_srp.c +54 -3

index 1029fd2..2a16ab4 100644
@@ -1453,18 +1453,58 @@ out:
 	return ret;
 }
 
+/*
+ * Register the indirect data buffer descriptor with the HCA.
+ *
+ * Note: since the indirect data buffer descriptor has been allocated with
+ * kmalloc() it is guaranteed that this buffer is a physically contiguous
+ * memory buffer.
+ */
+static int srp_map_idb(struct srp_rdma_ch *ch, struct srp_request *req,
+		       void **next_mr, void **end_mr, u32 idb_len,
+		       __be32 *idb_rkey)
+{
+	struct srp_target_port *target = ch->target;
+	struct srp_device *dev = target->srp_host->srp_dev;
+	struct srp_map_state state;
+	struct srp_direct_buf idb_desc;
+	u64 idb_pages[1];
+	int ret;
+
+	memset(&state, 0, sizeof(state));
+	memset(&idb_desc, 0, sizeof(idb_desc));
+	state.gen.next = next_mr;
+	state.gen.end = end_mr;
+	state.desc = &idb_desc;
+	state.pages = idb_pages;
+	state.pages[0] = (req->indirect_dma_addr &
+			  dev->mr_page_mask);
+	state.npages = 1;
+	state.base_dma_addr = req->indirect_dma_addr;
+	state.dma_len = idb_len;
+	ret = srp_finish_mapping(&state, ch);
+	if (ret < 0)
+		goto out;
+
+	*idb_rkey = idb_desc.key;
+
+out:
+	return ret;
+}
+
 static int srp_map_data(struct scsi_cmnd *scmnd, struct srp_rdma_ch *ch,
 			struct srp_request *req)
 {
 	struct srp_target_port *target = ch->target;
 	struct scatterlist *scat;
 	struct srp_cmd *cmd = req->cmd->buf;
-	int len, nents, count;
+	int len, nents, count, ret;
 	struct srp_device *dev;
 	struct ib_device *ibdev;
 	struct srp_map_state state;
 	struct srp_indirect_buf *indirect_hdr;
-	u32 table_len;
+	u32 idb_len, table_len;
+	__be32 idb_rkey;
 	u8 fmt;
 
 	if (!scsi_sglist(scmnd) || scmnd->sc_data_direction == DMA_NONE)

@@ -1546,6 +1586,7 @@ static int srp_map_data(struct scsi_cmnd *scmnd, struct srp_rdma_ch *ch,
 
 	count = min(state.ndesc, target->cmd_sg_cnt);
 	table_len = state.ndesc * sizeof (struct srp_direct_buf);
+	idb_len = sizeof(struct srp_indirect_buf) + table_len;
 
 	fmt = SRP_DATA_DESC_INDIRECT;
 	len = sizeof(struct srp_cmd) + sizeof (struct srp_indirect_buf);

@@ -1554,8 +1595,18 @@ static int srp_map_data(struct scsi_cmnd *scmnd, struct srp_rdma_ch *ch,
 	memcpy(indirect_hdr->desc_list, req->indirect_desc,
 	       count * sizeof (struct srp_direct_buf));
 
+	if (register_always && (dev->use_fast_reg || dev->use_fmr)) {
+		ret = srp_map_idb(ch, req, state.gen.next, state.gen.end,
+				  idb_len, &idb_rkey);
+		if (ret < 0)
+			return ret;
+		req->nmdesc++;
+	} else {
+		idb_rkey = target->rkey;
+	}
+
 	indirect_hdr->table_desc.va = cpu_to_be64(req->indirect_dma_addr);
-	indirect_hdr->table_desc.key = cpu_to_be32(target->rkey);
+	indirect_hdr->table_desc.key = idb_rkey;
 	indirect_hdr->table_desc.len = cpu_to_be32(table_len);
 	indirect_hdr->len = cpu_to_be32(state.total_len);

drivers/infiniband/ulp/srp/ib_srp.h +4 -0

index 60a33c1..255b0e5 100644
@@ -288,6 +288,10 @@ struct srp_map_state {
 			struct srp_fr_desc **next;
 			struct srp_fr_desc **end;
 		} fr;
+		struct {
+			void		   **next;
+			void		   **end;
+		} gen;
 	};
 	struct srp_direct_buf  *desc;
 	u64		       *pages;

Signature · 6 lines

2.1.4

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

A message that is not in the archive (sent off-list or not yet received): [email protected]

Sagi Grimberg

2015-08-16 11:09 UTC #83 lore raw

Re: [PATCH 7/8] IB/srp: Register the indirect data buffer descriptor

Quoted 15 lines · Bart Van Assche

Instead of always using the global rkey for the indirect data buffer descriptor, register that descriptor with the HCA if the kernel module parameter register_always has been set to Y.
Signed-off-by Bart Van Assche <[email protected]>
---
  drivers/infiniband/ulp/srp/ib_srp.c | 57 +++++++++++++++++++++++++++++++++++--
  drivers/infiniband/ulp/srp/ib_srp.h |  4 +++
  2 files changed, 58 insertions(+), 3 deletions(-)
drivers/infiniband/ulp/srp/ib_srp.c +0 -0
index 1029fd2..2a16ab4 100644

drivers/infiniband/ulp/srp/ib_srp.c out: · line 1462

Quoted 10 lines

drivers/infiniband/ulp/srp/ib_srp.c +7 -0

@@ -1453,18 +1453,58 @@ out:
  	return ret;
  }
 
+/*
+ * Register the indirect data buffer descriptor with the HCA.
+ *
+ * Note: since the indirect data buffer descriptor has been allocated with
+ * kmalloc() it is guaranteed that this buffer is a physically contiguous
+ * memory buffer.
+ */

kdoc style?

Quoted 83 lines

drivers/infiniband/ulp/srp/ib_srp.c +47 -3

+static int srp_map_idb(struct srp_rdma_ch *ch, struct srp_request *req,
+		       void **next_mr, void **end_mr, u32 idb_len,
+		       __be32 *idb_rkey)
+{
+	struct srp_target_port *target = ch->target;
+	struct srp_device *dev = target->srp_host->srp_dev;
+	struct srp_map_state state;
+	struct srp_direct_buf idb_desc;
+	u64 idb_pages[1];
+	int ret;
+
+	memset(&state, 0, sizeof(state));
+	memset(&idb_desc, 0, sizeof(idb_desc));
+	state.gen.next = next_mr;
+	state.gen.end = end_mr;
+	state.desc = &idb_desc;
+	state.pages = idb_pages;
+	state.pages[0] = (req->indirect_dma_addr &
+			  dev->mr_page_mask);
+	state.npages = 1;
+	state.base_dma_addr = req->indirect_dma_addr;
+	state.dma_len = idb_len;
+	ret = srp_finish_mapping(&state, ch);
+	if (ret < 0)
+		goto out;
+
+	*idb_rkey = idb_desc.key;
+
+out:
+	return ret;
+}
+
  static int srp_map_data(struct scsi_cmnd *scmnd, struct srp_rdma_ch *ch,
  			struct srp_request *req)
  {
  	struct srp_target_port *target = ch->target;
  	struct scatterlist *scat;
  	struct srp_cmd *cmd = req->cmd->buf;
-	int len, nents, count;
+	int len, nents, count, ret;
  	struct srp_device *dev;
  	struct ib_device *ibdev;
  	struct srp_map_state state;
  	struct srp_indirect_buf *indirect_hdr;
-	u32 table_len;
+	u32 idb_len, table_len;
+	__be32 idb_rkey;
  	u8 fmt;
 
  	if (!scsi_sglist(scmnd) || scmnd->sc_data_direction == DMA_NONE)

@@ -1546,6 +1586,7 @@ static int srp_map_data(struct scsi_cmnd *scmnd, struct srp_rdma_ch *ch,
 
  	count = min(state.ndesc, target->cmd_sg_cnt);
  	table_len = state.ndesc * sizeof (struct srp_direct_buf);
+	idb_len = sizeof(struct srp_indirect_buf) + table_len;
 
  	fmt = SRP_DATA_DESC_INDIRECT;
  	len = sizeof(struct srp_cmd) + sizeof (struct srp_indirect_buf);

@@ -1554,8 +1595,18 @@ static int srp_map_data(struct scsi_cmnd *scmnd, struct srp_rdma_ch *ch,
  	memcpy(indirect_hdr->desc_list, req->indirect_desc,
  	       count * sizeof (struct srp_direct_buf));
 
+	if (register_always && (dev->use_fast_reg || dev->use_fmr)) {
+		ret = srp_map_idb(ch, req, state.gen.next, state.gen.end,
+				  idb_len, &idb_rkey);
+		if (ret < 0)
+			return ret;
+		req->nmdesc++;
+	} else {
+		idb_rkey = target->rkey;
+	}
+
  	indirect_hdr->table_desc.va = cpu_to_be64(req->indirect_dma_addr);
-	indirect_hdr->table_desc.key = cpu_to_be32(target->rkey);
+	indirect_hdr->table_desc.key = idb_rkey;
  	indirect_hdr->table_desc.len = cpu_to_be32(table_len);
  	indirect_hdr->len = cpu_to_be32(state.total_len);

drivers/infiniband/ulp/srp/ib_srp.h +0 -0

index 60a33c1..255b0e5 100644

drivers/infiniband/ulp/srp/ib_srp.h struct srp_map_state { · line 294

Quoted 7 lines

drivers/infiniband/ulp/srp/ib_srp.h +4 -0

@@ -288,6 +288,10 @@ struct srp_map_state {
  			struct srp_fr_desc **next;
  			struct srp_fr_desc **end;
  		} fr;
+		struct {
+			void		   **next;
+			void		   **end;
+		} gen;

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

A message that is not in the archive (sent off-list or not yet received): 55D06F56.4060005-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org

Bart Van Assche

2015-08-16 16:56 UTC #85 lore raw

Re: [PATCH 7/8] IB/srp: Register the indirect data buffer descriptor

Quoted 10 lines · Sagi Grimberg

Quoted 7 lines · Bart Van Assche
+/*
+ * Register the indirect data buffer descriptor with the HCA.
+ *
+ * Note: since the indirect data buffer descriptor has been allocated with
+ * kmalloc() it is guaranteed that this buffer is a physically contiguous
+ * memory buffer.
+ */
kdoc style?

If I have to repost this patch series I will convert this comment into kernel-doc style.

Quoted 6 lines

drivers/infiniband/ulp/srp/ib_srp.h +0 -0

index 60a33c1..255b0e5 100644
@@ -288,6 +288,10 @@ struct srp_map_state {
   			struct srp_fr_desc **next;

Quoted 8 lines

drivers/infiniband/ulp/srp/ib_srp.h struct srp_map_state { · line 294
Quoted 6 lines
drivers/infiniband/ulp/srp/ib_srp.h +4 -0
   			struct srp_fr_desc **end;
   		} fr;
+		struct {
+			void		   **next;
+			void		   **end;
+		} gen;
What does 'gen' stands for?

In this context "gen" stands for "generic". This patch introduces some code that needs to access the "next" and "end" pointers and that is used both for FMR and FR.

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

Bart Van Assche

2015-08-11 00:09 UTC #86 lore raw

PATCH 8/8 IB/srp: Create an insecure all physical rkey only if needed

The SRP initiator only needs this if the insecure register_always=N performance optimization is enabled, or if FRWR/FMR is not supported in the driver.

Signed-off-by Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>

[bvanassche: reworked and rebased this patch]

---
 drivers/infiniband/ulp/srp/ib_srp.c | 40 +++++++++++++++++++++----------------
 drivers/infiniband/ulp/srp/ib_srp.h |  4 ++--
 2 files changed, 25 insertions(+), 19 deletions(-)

drivers/infiniband/ulp/srp/ib_srp.c +23 ~~-17~~

index 2a16ab4..192f737 100644
@@ -68,8 +68,8 @@ static unsigned int srp_sg_tablesize;
 static unsigned int cmd_sg_entries;
 static unsigned int indirect_sg_entries;
 static bool allow_ext_sg;
-static bool prefer_fr;
-static bool register_always;
+static bool prefer_fr = true;
+static bool register_always = true;
 static int topspin_workarounds = 1;
 
 module_param(srp_sg_tablesize, uint, 0444);

@@ -1353,9 +1353,9 @@ static int srp_finish_mapping(struct srp_map_state *state,
 	if (state->npages == 0)
 		return 0;
 
-	if (state->npages == 1 && !register_always)
+	if (state->npages == 1 && target->global_mr)
 		srp_map_desc(state, state->base_dma_addr, state->dma_len,
-			     target->rkey);
+			     target->global_mr->rkey);
 	else
 		ret = dev->use_fast_reg ? srp_map_finish_fr(state, ch) :
 			srp_map_finish_fmr(state, ch);

@@ -1442,7 +1442,8 @@ static int srp_map_sg(struct srp_map_state *state, struct srp_rdma_ch *ch,
 	} else {
 		for_each_sg(scat, sg, count, i) {
 			srp_map_desc(state, ib_sg_dma_address(dev->dev, sg),
-				     ib_sg_dma_len(dev->dev, sg), target->rkey);
+				     ib_sg_dma_len(dev->dev, sg),
+				     target->global_mr->rkey);
 		}
 	}

@@ -1531,7 +1532,7 @@ static int srp_map_data(struct scsi_cmnd *scmnd, struct srp_rdma_ch *ch,
 	fmt = SRP_DATA_DESC_DIRECT;
 	len = sizeof (struct srp_cmd) +	sizeof (struct srp_direct_buf);
 
-	if (count == 1 && !register_always) {
+	if (count == 1 && target->global_mr) {
 		/*
 		 * The midlayer only generated a single gather/scatter
 		 * entry, or DMA mapping coalesced everything to a

@@ -1541,7 +1542,7 @@ static int srp_map_data(struct scsi_cmnd *scmnd, struct srp_rdma_ch *ch,
 		struct srp_direct_buf *buf = (void *) cmd->add_data;
 
 		buf->va  = cpu_to_be64(ib_sg_dma_address(ibdev, scat));
-		buf->key = cpu_to_be32(target->rkey);
+		buf->key = cpu_to_be32(target->global_mr->rkey);
 		buf->len = cpu_to_be32(ib_sg_dma_len(ibdev, scat));
 
 		req->nmdesc = 0;

@@ -1595,14 +1596,14 @@ static int srp_map_data(struct scsi_cmnd *scmnd, struct srp_rdma_ch *ch,
 	memcpy(indirect_hdr->desc_list, req->indirect_desc,
 	       count * sizeof (struct srp_direct_buf));
 
-	if (register_always && (dev->use_fast_reg || dev->use_fmr)) {
+	if (!target->global_mr) {
 		ret = srp_map_idb(ch, req, state.gen.next, state.gen.end,
 				  idb_len, &idb_rkey);
 		if (ret < 0)
 			return ret;
 		req->nmdesc++;
 	} else {
-		idb_rkey = target->rkey;
+		idb_rkey = target->global_mr->rkey;
 	}
 
 	indirect_hdr->table_desc.va = cpu_to_be64(req->indirect_dma_addr);

@@ -3150,7 +3151,7 @@ static ssize_t srp_create_target(struct device *dev,
 	target->scsi_host	= target_host;
 	target->srp_host	= host;
 	target->lkey		= host->srp_dev->pd->local_dma_lkey;
-	target->rkey		= host->srp_dev->mr->rkey;
+	target->global_mr	= host->srp_dev->global_mr;
 	target->cmd_sg_cnt	= cmd_sg_entries;
 	target->sg_tablesize	= indirect_sg_entries ? : cmd_sg_entries;
 	target->allow_ext_sg	= allow_ext_sg;

@@ -3438,12 +3439,16 @@ static void srp_add_one(struct ib_device *device)
 	if (IS_ERR(srp_dev->pd))
 		goto free_dev;
 
-	srp_dev->mr = ib_get_dma_mr(srp_dev->pd,
-				    IB_ACCESS_LOCAL_WRITE |
-				    IB_ACCESS_REMOTE_READ |
-				    IB_ACCESS_REMOTE_WRITE);
-	if (IS_ERR(srp_dev->mr))
-		goto err_pd;
+	if (!register_always || (!srp_dev->has_fmr && !srp_dev->has_fr)) {
+		srp_dev->global_mr = ib_get_dma_mr(srp_dev->pd,
+						   IB_ACCESS_LOCAL_WRITE |
+						   IB_ACCESS_REMOTE_READ |
+						   IB_ACCESS_REMOTE_WRITE);
+		if (IS_ERR(srp_dev->global_mr))
+			goto err_pd;
+	} else {
+		srp_dev->global_mr = NULL;
+	}
 
 	for (p = rdma_start_port(device); p <= rdma_end_port(device); ++p) {
 		host = srp_add_port(srp_dev, p);

@@ -3500,7 +3505,8 @@ static void srp_remove_one(struct ib_device *device)
 		kfree(host);
 	}
 
-	ib_dereg_mr(srp_dev->mr);
+	if (srp_dev->global_mr)
+		ib_dereg_mr(srp_dev->global_mr);
 	ib_dealloc_pd(srp_dev->pd);
 
 	kfree(srp_dev);

drivers/infiniband/ulp/srp/ib_srp.h +2 -2

index 255b0e5..3608f2e 100644
@@ -95,7 +95,7 @@ struct srp_device {
 	struct list_head	dev_list;
 	struct ib_device       *dev;
 	struct ib_pd	       *pd;
-	struct ib_mr	       *mr;
+	struct ib_mr	       *global_mr;
 	u64			mr_page_mask;
 	int			mr_page_size;
 	int			mr_max_size;

@@ -183,10 +183,10 @@ struct srp_target_port {
 	spinlock_t		lock;
 
 	/* read only in the hot path */
+	struct ib_mr		*global_mr;
 	struct srp_rdma_ch	*ch;
 	u32			ch_count;
 	u32			lkey;
-	u32			rkey;
 	enum srp_target_state	state;
 	unsigned int		max_iu_len;
 	unsigned int		cmd_sg_cnt;

Signature · 6 lines

2.1.4

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

Jason Gunthorpe

author

2015-08-11 05:40 UTC #87 lore raw

Re: [PATCH 0/9] IB/srp: Do not create an all physical insecure rkey by default