arbitrary sg lists support

PATCH Superseded Max Gurtovoy 7 participants 26 messages Apr 13, 2017 – May 2, 2017

Subsystems (4)

InfiniBand NVMe Mellanox MLX5 IB driver iSCSI extensions for RDMA (iSER) initiator

lore

Share & tools

lore

Max Gurtovoy

author

2017-04-13 15:56 UTC #0 lore raw

v2 cover arbitrary sg lists support

This patch adds an arbitrary sg list support to NVMEoF host for capable devices (such as RDMA CX4/CX5 HCAs).
During the review we decided to add an helper function to ib_core that will be used by ULPs in order to avoid code duplication.

Changes from V1:
- added helper function to verbes api
- use helper in iSER code
- add reviewrs signatures

Max Gurtovoy (4):
IB/core: Add inline function to get sg mr type IB/iser: Use ib_get_sg_mr_type() helper
nvme: enable SG gaps support
nvme-rdma: add support for arbitrary sg lists mapping

 drivers/infiniband/ulp/iser/iser_verbs.c |    8 +-------
 drivers/nvme/host/core.c                 |    5 ++++-
 drivers/nvme/host/nvme.h                 |    1 +
 drivers/nvme/host/rdma.c                 |   11 +++++++----
 include/rdma/ib_verbs.h                  |   17 +++++++++++++++++
 5 files changed, 30 insertions(+), 12 deletions(-)

Max Gurtovoy

author

2017-04-13 15:56 UTC #1 lore raw

PATCH v2 1/4 IB/core: Add inline function to get sg mr type

ULPs such as iSER/NVMEoF can use memory regions that support arbitrary sg list mapping in order to allow the block layer to send us gappy bios and reduce the stack workload. Add an helper function to get IB_MR_TYPE_SG_GAPS mr_type if the device support this kind of mapping.

Signed-off-by Max Gurtovoy <maxg at mellanox.com>

Reviewed-by Leon Romanovsky <leon at kernel.org>

---
 include/rdma/ib_verbs.h |   17 +++++++++++++++++
 1 files changed, 17 insertions(+), 0 deletions(-)

include/rdma/ib_verbs.h +17 -0

index 99e4423..dd97169 100644
@@ -3375,6 +3375,23 @@ int ib_map_mr_sg(struct ib_mr *mr, struct scatterlist *sg, int sg_nents,
 int ib_sg_to_pages(struct ib_mr *mr, struct scatterlist *sgl, int sg_nents,
 		unsigned int *sg_offset, int (*set_page)(struct ib_mr *, u64));
 
+/**
+ * ib_get_sg_mr_type() - check if the device support arbitrary
+ *                       sg mapping and return a suitable mr type.
+ * @device: The device on which to check support.
+ *
+ * Return: IB_MR_TYPE_SG_GAPS if the device support.
+ *         Otherwise, return IB_MR_TYPE_MEM_REG.
+ */
+static inline enum ib_mr_type ib_get_sg_mr_type(struct ib_device *device)
+{
+	if (device->attrs.device_cap_flags & IB_DEVICE_SG_GAPS_REG)
+		return IB_MR_TYPE_SG_GAPS;
+	else
+		return IB_MR_TYPE_MEM_REG;
+
+}
+
 void ib_drain_rq(struct ib_qp *qp);
 void ib_drain_sq(struct ib_qp *qp);
 void ib_drain_qp(struct ib_qp *qp);

Signature · 1 line

1.7.1

Sagi Grimberg

maintainer

2017-04-20 11:24 UTC #2 lore raw

PATCH v2 1/4 IB/core: Add inline function to get sg mr type

Quoted 8 lines

include/rdma/ib_verbs.h +7 -0

+/**
+ * ib_get_sg_mr_type() - check if the device support arbitrary
+ *                       sg mapping and return a suitable mr type.
+ * @device: The device on which to check support.
+ *
+ * Return: IB_MR_TYPE_SG_GAPS if the device support.
+ *         Otherwise, return IB_MR_TYPE_MEM_REG.

include/rdma/ib_verbs.h int ib_map_mr_sg(struct ib_mr *mr, struct scatterlist *sg, int sg_nents, · line 3394

Quoted 10 lines

include/rdma/ib_verbs.h +10 -0

+ */
+static inline enum ib_mr_type ib_get_sg_mr_type(struct ib_device *device)
+{
+	if (device->attrs.device_cap_flags & IB_DEVICE_SG_GAPS_REG)
+		return IB_MR_TYPE_SG_GAPS;
+	else
+		return IB_MR_TYPE_MEM_REG;
+
+}
+

It does eliminate some code duplication, but will everyone prefer the GAPS type? I guess they can just not use it, maybe the naming should reflect that GAPS is preferred? (it doesn't come for free as the device might need more resources to support GAPS)

Not sure...

Max Gurtovoy

author

2017-04-13 15:56 UTC #3 lore raw

PATCH v2 2/4 IB/iser: Use ib_get_sg_mr_type() helper

In reply to Max Gurtovoy

Signed-off-by Max Gurtovoy <maxg at mellanox.com>

Reviewed-by Leon Romanovsky <leon at kernel.org>

---
 drivers/infiniband/ulp/iser/iser_verbs.c |    8 +-------
 1 files changed, 1 insertions(+), 7 deletions(-)

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

index c538a38..7e63ffc 100644
@@ -242,15 +242,9 @@ void iser_free_fmr_pool(struct ib_conn *ib_conn)
 		   unsigned int size)
 {
 	struct ib_device *ib_dev = device->ib_device;
-	enum ib_mr_type mr_type;
 	int ret;
 
-	if (ib_dev->attrs.device_cap_flags & IB_DEVICE_SG_GAPS_REG)
-		mr_type = IB_MR_TYPE_SG_GAPS;
-	else
-		mr_type = IB_MR_TYPE_MEM_REG;
-
-	res->mr = ib_alloc_mr(pd, mr_type, size);
+	res->mr = ib_alloc_mr(pd, ib_get_sg_mr_type(ib_dev), size);
 	if (IS_ERR(res->mr)) {
 		ret = PTR_ERR(res->mr);
 		iser_err("Failed to allocate ib_fast_reg_mr err=%d\n", ret);

Signature · 1 line

1.7.1

Max Gurtovoy

author

2017-04-13 15:56 UTC #4 lore raw

PATCH v2 3/4 nvme: enable SG gaps support

In reply to Max Gurtovoy

For controllers that can handle arbitrarily sized bios (e.g advanced RDMA ctrls) we can allow the block layer to pass us gaps by skip setting the queue virt_boundary.

Signed-off-by Max Gurtovoy <maxg at mellanox.com>

Reviewed-by Keith Busch <keith.busch at intel.com>

Reviewed-by Christoph Hellwig <hch at lst.de>

Reviewed-by Leon Romanovsky <leon at kernel.org>

---
 drivers/nvme/host/core.c |    5 ++++-
 drivers/nvme/host/nvme.h |    1 +
 2 files changed, 5 insertions(+), 1 deletions(-)

drivers/nvme/host/core.c +4 -1

index 9583a5f..72bc70e 100644
@@ -1252,7 +1252,10 @@ static void nvme_set_queue_limits(struct nvme_ctrl *ctrl,
 	}
 	if (ctrl->quirks & NVME_QUIRK_STRIPE_SIZE)
 		blk_queue_chunk_sectors(q, ctrl->max_hw_sectors);
-	blk_queue_virt_boundary(q, ctrl->page_size - 1);
+
+	if (!ctrl->sg_gaps_support)
+		blk_queue_virt_boundary(q, ctrl->page_size - 1);
+
 	if (ctrl->vwc & NVME_CTRL_VWC_PRESENT)
 		vwc = true;
 	blk_queue_write_cache(q, vwc, vwc);

drivers/nvme/host/nvme.h +1 -0

index 2aa20e3..ccb895a 100644
@@ -162,6 +162,7 @@ struct nvme_ctrl {
 	struct work_struct scan_work;
 	struct work_struct async_event_work;
 	struct delayed_work ka_work;
+	bool sg_gaps_support;
 
 	/* Power saving configuration */
 	u64 ps_max_latency_us;

Signature · 1 line

1.7.1

Sagi Grimberg

maintainer

2017-04-20 11:27 UTC #5 lore raw

PATCH v2 3/4 nvme: enable SG gaps support

drivers/nvme/host/core.c static void nvme_set_queue_limits(struct nvme_ctrl *ctrl, · line 1258

Quoted 8 lines

drivers/nvme/host/core.c +4 -1

 	if (ctrl->quirks & NVME_QUIRK_STRIPE_SIZE)
 		blk_queue_chunk_sectors(q, ctrl->max_hw_sectors);
-	blk_queue_virt_boundary(q, ctrl->page_size - 1);
+
+	if (!ctrl->sg_gaps_support)
+		blk_queue_virt_boundary(q, ctrl->page_size - 1);
+

I think it would be better if the ctrl will hold a virt_boundary provided by the fabric driver? Then we would always blk_queue_virt_boundary but with capable devices the fabric driver can set it to 0. This will also allow for dword aligned sgls to fit in pretty easily.

Christoph Hellwig

maintainer

2017-04-21 06:44 UTC #6 lore raw

PATCH v2 3/4 nvme: enable SG gaps support

Quoted 5 lines · Sagi Grimberg

I think it would be better if the ctrl will hold a virt_boundary provided by the fabric driver? Then we would always blk_queue_virt_boundary but with capable devices the fabric driver can set it to 0. This will also allow for dword aligned sgls to fit in pretty easily.

All our block I/O must be cache line aligned, so dword aligned SGLs should not be an issue. And for PRPs or MRs we'll always use the host page size.

But I don't think I really care in the end..

Sagi Grimberg

maintainer

2017-04-23 07:20 UTC #7 lore raw

PATCH v2 3/4 nvme: enable SG gaps support

Christoph,

Quoted 3 lines

All our block I/O must be cache line aligned, so dword aligned SGLs should not be an issue. And for PRPs or MRs we'll always use the host page size.

Where is that constraint coming from? Is that new? Back when I wrote GAP support in rdma I tested a single byte alignment via vectored direct IO and it seemed to be going through...

I also just tested Bart's unaligned test [1] on scsi_debug and it seems to go through as well (alignment and length are 4)...

[1]:
https://github.com/bvanassche/srp-test/blob/master/discontiguous-io/discontiguous-io.cpp

Christoph Hellwig

maintainer

2017-04-23 08:45 UTC #8 lore raw

PATCH v2 3/4 nvme: enable SG gaps support

Quoted 9 lines · Sagi Grimberg

Christoph,
Quoted 3 lines
All our block I/O must be cache line aligned, so dword aligned SGLs should not be an issue. And for PRPs or MRs we'll always use the host page size.
Where is that constraint coming from? Is that new? Back when I wrote GAP support in rdma I tested a single byte alignment via vectored direct IO and it seemed to be going through...

Each block queue has a ->dma_alignment field, which is set to 511 by default unless changed. Everything not aligned to that will be bounced in __blk_rq_map_user_iov and friends before we send it to the driver.

Quoted 2 lines

I also just tested Bart's unaligned test [1] on scsi_debug and it seems to go through as well (alignment and length are 4)...

SCSI reduces the alignment in __scsi_init_queue:

	/*
         * set a reasonable default alignment on word boundaries: the
	 * host and device may alter it using
	 * blk_queue_update_dma_alignment() later.
	 */
	blk_queue_dma_alignment(q, 0x03);

still would fit NVMe dword-alignment SGLs, though :)

Sagi Grimberg

maintainer

2017-04-23 09:45 UTC #9 lore raw

PATCH v2 3/4 nvme: enable SG gaps support

Quoted 5 lines

Each block queue has a ->dma_alignment field, which is set to 511 by default unless changed. Everything not aligned to that will be bounced in __blk_rq_map_user_iov and friends before we send it to the driver.

Thanks, I missed the default setting...

Quoted 11 lines

Quoted 2 lines
I also just tested Bart's unaligned test [1] on scsi_debug and it seems to go through as well (alignment and length are 4)...
SCSI reduces the alignment in __scsi_init_queue:
	/*
         * set a reasonable default alignment on word boundaries: the
	 * host and device may alter it using
	 * blk_queue_update_dma_alignment() later.
	 */
	blk_queue_dma_alignment(q, 0x03);

That explains why I was able to test that with iSER :)

I guess I didn't test single byte alignment after all, but I definitely remember using less than a sector...

Quoted 1 line

still would fit NVMe dword-alignment SGLs, though :)

I would say its useful to have, but its only relevant to ioctls as normal io we explicitly on sub logical block size alignment.

Anyways, I still think having ctrl->virt_boundary instead of ctrl->sg_gap_support is cleaner...

Max Gurtovoy

author

2017-04-23 13:23 UTC #10 lore raw

PATCH v2 3/4 nvme: enable SG gaps support

Quoted 25 lines · Sagi Grimberg

Quoted 5 lines
Each block queue has a ->dma_alignment field, which is set to 511 by default unless changed. Everything not aligned to that will be bounced in __blk_rq_map_user_iov and friends before we send it to the driver.
Thanks, I missed the default setting...
Quoted 11 lines
Quoted 2 lines
I also just tested Bart's unaligned test [1] on scsi_debug and it seems to go through as well (alignment and length are 4)...
SCSI reduces the alignment in __scsi_init_queue:
/*
* set a reasonable default alignment on word boundaries: the
* host and device may alter it using
* blk_queue_update_dma_alignment() later.
*/
blk_queue_dma_alignment(q, 0x03);
That explains why I was able to test that with iSER :)
I guess I didn't test single byte alignment after all, but I definitely remember using less than a sector...

Quoted 7 lines

Quoted 1 line
still would fit NVMe dword-alignment SGLs, though :)
I would say its useful to have, but its only relevant to ioctls as normal io we explicitly on sub logical block size alignment.
Anyways, I still think having ctrl->virt_boundary instead of ctrl->sg_gap_support is cleaner...

Sure, I tought about it and actually started my implementation like that. It wasn't so clean, though.

Christoph Hellwig

maintainer

2017-04-24 07:02 UTC #11 lore raw

PATCH v2 3/4 nvme: enable SG gaps support

In reply to Sagi Grimberg

Quoted 7 lines · Sagi Grimberg

Quoted 1 line
still would fit NVMe dword-alignment SGLs, though :)
I would say its useful to have, but its only relevant to ioctls as normal io we explicitly on sub logical block size alignment.
Anyways, I still think having ctrl->virt_boundary instead of ctrl->sg_gap_support is cleaner...

Ok, fine with me.

Max Gurtovoy

author

2017-04-13 15:56 UTC #12 lore raw

PATCH v2 4/4 nvme-rdma: add support for arbitrary sg lists mapping

In reply to Max Gurtovoy

If the device support arbitrary sg list mapping (device cap IB_DEVICE_SG_GAPS_REG set) we allocate the memory regions with IB_MR_TYPE_SG_GAPS and allow the block layer to send us gappy bio vectors. We use this functionality in iSER driver as well.

Signed-off-by Max Gurtovoy <maxg at mellanox.com>

Reviewed-by Leon Romanovsky <leon at kernel.org>

---
 drivers/nvme/host/rdma.c |   11 +++++++----
 1 files changed, 7 insertions(+), 4 deletions(-)

drivers/nvme/host/rdma.c +7 -4

index 47a479f..1c6f974 100644
@@ -294,8 +294,8 @@ static int nvme_rdma_reinit_request(void *data, struct request *rq)
 
 	ib_dereg_mr(req->mr);
 
-	req->mr = ib_alloc_mr(dev->pd, IB_MR_TYPE_MEM_REG,
-			ctrl->max_fr_pages);
+	req->mr = ib_alloc_mr(dev->pd, ib_get_sg_mr_type(dev->dev),
+			      ctrl->max_fr_pages);
 	if (IS_ERR(req->mr)) {
 		ret = PTR_ERR(req->mr);
 		req->mr = NULL;

@@ -348,8 +348,8 @@ static int __nvme_rdma_init_request(struct nvme_rdma_ctrl *ctrl,
 	if (ret)
 		return ret;
 
-	req->mr = ib_alloc_mr(dev->pd, IB_MR_TYPE_MEM_REG,
-			ctrl->max_fr_pages);
+	req->mr = ib_alloc_mr(dev->pd, ib_get_sg_mr_type(ibdev),
+			      ctrl->max_fr_pages);
 	if (IS_ERR(req->mr)) {
 		ret = PTR_ERR(req->mr);
 		goto out_free_qe;

@@ -1608,6 +1608,9 @@ static int nvme_rdma_configure_admin_queue(struct nvme_rdma_ctrl *ctrl)
 	ctrl->ctrl.sqsize =
 		min_t(int, NVME_CAP_MQES(ctrl->cap) + 1, ctrl->ctrl.sqsize);
 
+	if (ctrl->device->dev->attrs.device_cap_flags & IB_DEVICE_SG_GAPS_REG)
+		ctrl->ctrl.sg_gaps_support = true;
+
 	error = nvme_enable_ctrl(&ctrl->ctrl, ctrl->cap);
 	if (error)
 		goto out_cleanup_queue;

Signature · 1 line

1.7.1

Bart Van Assche

2017-04-13 16:31 UTC #13 lore raw

v2 cover arbitrary sg lists support

In reply to Max Gurtovoy

Quoted 4 lines · Max Gurtovoy

This patch adds an arbitrary sg list support to NVMEoF host for capable devices (such as RDMA CX4/CX5 HCAs).
During the review we decided to add an helper function to ib_core that will be used by ULPs in order to avoid code duplication.

Hello Max,

A question that is not related to the ULP drivers touched by this patch series:
some time ago registration of sg-lists with gaps was disabled in the SRP initiator because arbitrary sg-lists triggered CQE dumps in combination with the mlx5 driver. Does this mean that this issue has been resolved? If so, what mlx5 driver and/or firmware version are needed to reenable arbitrary sg-list support in the SRP initiator?

Thanks,

Bart.

Max Gurtovoy

author

2017-04-13 16:46 UTC #14 lore raw

v2 cover arbitrary sg lists support

Quoted 6 lines · Bart Van Assche

Quoted 4 lines · Max Gurtovoy
This patch adds an arbitrary sg list support to NVMEoF host for capable devices (such as RDMA CX4/CX5 HCAs).
During the review we decided to add an helper function to ib_core that will be used by ULPs in order to avoid code duplication.

Quoted 9 lines

Hello Max,
A question that is not related to the ULP drivers touched by this patch series:
some time ago registration of sg-lists with gaps was disabled in the SRP initiator because arbitrary sg-lists triggered CQE dumps in combination with the mlx5 driver. Does this mean that this issue has been resolved? If so, what mlx5 driver and/or firmware version are needed to reenable arbitrary sg-list support in the SRP initiator?

Hi Bart,
IsraelR and I couldn't repro this issue even once on our labs. I suggest we take it with Laurence and try to figure it out.
I can try installing the latest FW on CX4/5 cards in Laurence Lab and run the test suite again.

Quoted 4 lines

Thanks,
Bart.

Laurence Oberman

2017-04-13 16:59 UTC #15 lore raw

v2 cover arbitrary sg lists support

----- Original Message -----

Quoted 30 lines

From: "Max Gurtovoy" <maxg at mellanox.com>
To: "Bart Van Assche" <Bart.VanAssche at sandisk.com>, hch at lst.de, "keith busch" <keith.busch at intel.com>, linux-nvme at lists.infradead.org, linux-rdma at vger.kernel.org, leon at kernel.org
Cc: vladimirk at mellanox.com, "Laurence Oberman" <loberman at redhat.com>
Sent: Thursday, April 13, 2017 12:46:27 PM
Subject: Re: [PATCH v2 0/4] arbitrary sg lists support
Quoted 8 lines · Bart Van Assche
Quoted 4 lines · Max Gurtovoy
This patch adds an arbitrary sg list support to NVMEoF host for capable devices (such as RDMA CX4/CX5 HCAs).
During the review we decided to add an helper function to ib_core that will be used by ULPs in order to avoid code duplication.
Hello Max,
Quoted 11 lines
A question that is not related to the ULP drivers touched by this patch series:
some time ago registration of sg-lists with gaps was disabled in the SRP initiator because arbitrary sg-lists triggered CQE dumps in combination with the
mlx5 driver. Does this mean that this issue has been resolved? If so, what mlx5
driver and/or firmware version are needed to reenable arbitrary sg-list support
in the SRP initiator?

Quoted 11 lines

Hi Bart,
IsraelR and I couldn't repro this issue even once on our labs. I suggest we take it with Laurence and try to figure it out.
I can try installing the latest FW on CX4/5 cards in Laurence Lab and run the test suite again.
Quoted 4 lines
Thanks,
Bart.

Hello
That's fine.

Lab is always ready for the SRP tests, I keep it that way. The lab has mlx5 back-to-back iwth opensm and EDR100

Regards
Laurence

Sagi Grimberg

maintainer

2017-04-20 11:30 UTC #16 lore raw

v2 cover arbitrary sg lists support

In reply to Bart Van Assche

Quoted 6 lines

Quoted 5 lines
This patch adds an arbitrary sg list support to NVMEoF host for capable devices (such as RDMA CX4/CX5 HCAs).
During the review we decided to add an helper function to ib_core that will be used by ULPs in order to avoid code duplication.

Quoted 8 lines

Hello Max,
A question that is not related to the ULP drivers touched by this patch series:
some time ago registration of sg-lists with gaps was disabled in the SRP initiator because arbitrary sg-lists triggered CQE dumps in combination with the mlx5 driver. Does this mean that this issue has been resolved? If so, what mlx5 driver and/or firmware version are needed to reenable arbitrary sg-list support in the SRP initiator?

I'm concerned about the fact that the SRP issue wasn't resolved. There might be a bug hiding there.

Max, Leon, my recollection was that the error syndrome you saw with SRP described "too much sgls for a single mr". If this is true, then I still think we need a different device capability for it.

I'm even thinking we need to remove gaps support from iSER until this is addressed.

Leon Romanovsky

2017-04-23 10:34 UTC #17 lore raw

v2 cover arbitrary sg lists support

Quoted 15 lines · Sagi Grimberg

Quoted 6 lines
Quoted 5 lines
This patch adds an arbitrary sg list support to NVMEoF host for capable devices (such as RDMA CX4/CX5 HCAs).
During the review we decided to add an helper function to ib_core that will be used by ULPs in order to avoid code duplication.
Quoted 8 lines
Hello Max,
A question that is not related to the ULP drivers touched by this patch series:
some time ago registration of sg-lists with gaps was disabled in the SRP initiator because arbitrary sg-lists triggered CQE dumps in combination with the mlx5 driver. Does this mean that this issue has been resolved? If so, what mlx5 driver and/or firmware version are needed to reenable arbitrary sg-list support in the SRP initiator?

Quoted 6 lines

I'm concerned about the fact that the SRP issue wasn't resolved. There might be a bug hiding there.
Max, Leon, my recollection was that the error syndrome you saw with SRP described "too much sgls for a single mr". If this is true, then I still think we need a different device capability for it.

I don't remember that Max succeeded to reproduce it.

Quoted 3 lines

I'm even thinking we need to remove gaps support from iSER until this is addressed.

Better to find the root cause.

Thanks
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-nvme/attachments/20170423/6c617d56/attachment.sig>

Sagi Grimberg

maintainer

2017-04-23 11:49 UTC #18 lore raw

v2 cover arbitrary sg lists support

Quoted 9 lines

Quoted 7 lines
I'm concerned about the fact that the SRP issue wasn't resolved. There might be a bug hiding there.
Max, Leon, my recollection was that the error syndrome you saw with SRP described "too much sgls for a single mr". If this is true, then I still think we need a different device capability for it.
I don't remember that Max succeeded to reproduce it.

You said Artemy was on this issue:
https://www.spinics.net/lists/linux-rdma/msg46390.html

and I asked you a question that will help clear things up but never received an answer:
https://www.spinics.net/lists/linux-rdma/msg46394.html

Leon Romanovsky

2017-04-23 12:12 UTC #19 lore raw

v2 cover arbitrary sg lists support

Quoted 4 lines · Sagi Grimberg

Quoted 4 lines
Quoted 4 lines
I'm concerned about the fact that the SRP issue wasn't resolved. There might be a bug hiding there.

Quoted 9 lines

Quoted 5 lines
Quoted 3 lines
Max, Leon, my recollection was that the error syndrome you saw with SRP described "too much sgls for a single mr". If this is true, then I still think we need a different device capability for it.
I don't remember that Max succeeded to reproduce it.
You said Artemy was on this issue:
https://www.spinics.net/lists/linux-rdma/msg46390.html

At the end, he proposed unrelated fix.

Quoted 3 lines

and I asked you a question that will help clear things up but never received an answer:
https://www.spinics.net/lists/linux-rdma/msg46394.html

i asked and didn't get answer too.
https://www.spinics.net/lists/linux-rdma/msg46407.html

Thanks

Quoted 4 lines

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-nvme/attachments/20170423/f3f09b25/attachment.sig>

Sagi Grimberg

maintainer

2017-04-23 12:28 UTC #20 lore raw

v2 cover arbitrary sg lists support

Quoted 3 lines

i asked and didn't get answer too.
https://www.spinics.net/lists/linux-rdma/msg46407.html

OK, I suppose that the question was to Laurence, but I'll try to answer.

We can't really say, this reproduced for 4M transfers, which iSER does not support without a applying higher max_sectors modparam. But even with that, its a different code path as SRP unlike iSER can use multiple MRs for a single transaction.

So short answer, can't say...

Max Gurtovoy

author

2017-04-23 13:35 UTC #21 lore raw

v2 cover arbitrary sg lists support

Quoted 7 lines · Sagi Grimberg

Quoted 3 lines
i asked and didn't get answer too.
https://www.spinics.net/lists/linux-rdma/msg46407.html
OK, I suppose that the question was to Laurence, but I'll try to answer.

Quoted 6 lines

We can't really say, this reproduced for 4M transfers, which iSER does not support without a applying higher max_sectors modparam. But even with that, its a different code path as SRP unlike iSER can use multiple MRs for a single transaction.
So short answer, can't say...

I actually answered here:
https://www.spinics.net/lists/linux-rdma/msg46412.html

Sagi,
As you mentioned the IO path is different in SRP and iSER so IMO we should start with SRP debugging. NVMf is similar to iSER (and not SRP) and that's why I think we can apply this patchset.
If you want to wait till we debug SRP issue it's fine, but I can't repro it in my lab so it can take longer.

Lourance,
maybe you can update your FW to the latest CX4 from our site and try to repro this issue ?

thanks,
Max.

Sagi Grimberg

maintainer

2017-04-23 14:35 UTC #22 lore raw

v2 cover arbitrary sg lists support

Quoted 3 lines

I actually answered here:
https://www.spinics.net/lists/linux-rdma/msg46412.html

Did anything progress with the debug?

Quoted 4 lines

Sagi,
As you mentioned the IO path is different in SRP and iSER so IMO we should start with SRP debugging. NVMf is similar to iSER (and not SRP) and that's why I think we can apply this patchset.

My concern is that this might be broken due to the fact that we don't know what triggers this.

Quoted 2 lines

If you want to wait till we debug SRP issue it's fine, but I can't repro it in my lab so it can take longer.

I'd prefer not to take a non-mandatory feature that is not guaranteed to work.

Quoted 3 lines

Lourance,
maybe you can update your FW to the latest CX4 from our site and try to repro this issue ?

Laurence,

Can you please enable srp_add_one debug:

echo "func srp_add_one +p" > /sys/kernel/debug/dynamic_debug/control

In addition apply the following:
--

(quoted diff) +0 -0

b/drivers/infiniband/hw/mlx5/mr.c
index d9c6c0ea750b..040fbc387e4f 100644

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

@@ -1403,6 +1403,8 @@ mlx5_alloc_priv_descs(struct ib_device *device,
         int add_size;
         int ret;
 
+       WARN_ON_ONCE(ndescs > device->attr.max_fast_reg_page_list_len);
+
         add_size = max_t(int, MLX5_UMR_ALIGN - ARCH_KMALLOC_MINALIGN, 0);
 
         mr->descs_alloc = kzalloc(size + add_size, GFP_KERNEL);

Cheers,
Sagi.

Doug Ledford

2017-05-01 18:50 UTC #23 lore raw

v2 cover arbitrary sg lists support

Quoted 17 lines · Sagi Grimberg

Quoted 3 lines
I actually answered here:
https://www.spinics.net/lists/linux-rdma/msg46412.html
Did anything progress with the debug?
Quoted 6 lines
Sagi,
As you mentioned the IO path is different in SRP and iSER so IMO we should start with SRP debugging. NVMf is similar to iSER (and not SRP)
and that's why I think we can apply this patchset.
My concern is that this might be broken due to the fact that we don't know what triggers this.
Quoted 1 line

Quoted 6 lines

Quoted 3 lines
If you want to wait till we debug SRP issue it's fine, but I can't repro
it in my lab so it can take longer.
I'd prefer not to take a non-mandatory feature that is not guaranteed to work.

Indeed. ?I've left this series on hold (actually, I've removed it from patchworks). ?When we get this straightened, then please resubmit.

Signature · 4 lines

Doug Ledford <dledford at redhat.com>
? ? GPG KeyID: B826A3330E572FDD
? ?
Key fingerprint = AE6B 1BDA 122B 23B4 265B ?1274 B826 A333 0E57 2FDD

Max Gurtovoy

author

2017-05-02 16:14 UTC #24 lore raw

v2 cover arbitrary sg lists support

Quoted 22 lines · Doug Ledford

Quoted 21 lines · Sagi Grimberg
Quoted 3 lines
I actually answered here:
https://www.spinics.net/lists/linux-rdma/msg46412.html
Did anything progress with the debug?
Quoted 6 lines
Sagi,
As you mentioned the IO path is different in SRP and iSER so IMO we should start with SRP debugging. NVMf is similar to iSER (and not SRP)
and that's why I think we can apply this patchset.
My concern is that this might be broken due to the fact that we don't know what triggers this.
Quoted 4 lines
If you want to wait till we debug SRP issue it's fine, but I can't repro
it in my lab so it can take longer.

Quoted 6 lines

Quoted 2 lines
I'd prefer not to take a non-mandatory feature that is not guaranteed to work.
Indeed. I've left this series on hold (actually, I've removed it from patchworks). When we get this straightened, then please resubmit.

Sure, no problem.

Max.

Laurence Oberman

2017-05-02 16:39 UTC #25 lore raw

v2 cover arbitrary sg lists support

----- Original Message -----

Quoted 39 lines

From: "Max Gurtovoy" <maxg at mellanox.com>
To: "Doug Ledford" <dledford at redhat.com>, "Sagi Grimberg" <sagi at grimberg.me>, "Leon Romanovsky" <leon at kernel.org>
Cc: "Bart Van Assche" <Bart.VanAssche at sandisk.com>, hch at lst.de, "keith busch" <keith.busch at intel.com>, linux-nvme at lists.infradead.org, linux-rdma at vger.kernel.org, vladimirk at mellanox.com
Sent: Tuesday, May 2, 2017 12:14:33 PM
Subject: Re: [PATCH v2 0/4] arbitrary sg lists support
Quoted 22 lines · Doug Ledford
Quoted 21 lines · Sagi Grimberg
Quoted 3 lines
I actually answered here:
https://www.spinics.net/lists/linux-rdma/msg46412.html
Did anything progress with the debug?
Quoted 6 lines
Sagi,
As you mentioned the IO path is different in SRP and iSER so IMO we should start with SRP debugging. NVMf is similar to iSER (and not SRP)
and that's why I think we can apply this patchset.
My concern is that this might be broken due to the fact that we don't know what triggers this.
Quoted 4 lines
If you want to wait till we debug SRP issue it's fine, but I can't repro
it in my lab so it can take longer.
Quoted 6 lines
Quoted 2 lines
I'd prefer not to take a non-mandatory feature that is not guaranteed to work.
Indeed. I've left this series on hold (actually, I've removed it from patchworks). When we get this straightened, then please resubmit.

Quoted 8 lines

Sure, no problem.
Max.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Max
The setup is in place, send me whatever it is you need for debug here. Patches, instrumentation etc.
I will be happy to gather the debug.
Regards
Laurence