[PATCH v2 0/4] arbitrary sg lists support

[PATCH v2 0/4] arbitrary sg lists support

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

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

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

[Max Gurtovoy]

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

diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
index 99e4423..dd97169 100644
--- a/include/rdma/ib_verbs.h
+++ b/include/rdma/ib_verbs.h
@@ -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);
-- 
1.7.1

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

[Sagi Grimberg]

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

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

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

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

diff --git a/drivers/infiniband/ulp/iser/iser_verbs.c b/drivers/infiniband/ulp/iser/iser_verbs.c
index c538a38..7e63ffc 100644
--- a/drivers/infiniband/ulp/iser/iser_verbs.c
+++ b/drivers/infiniband/ulp/iser/iser_verbs.c
@@ -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);
-- 
1.7.1

[PATCH v2 3/4] nvme: enable SG gaps support

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

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 9583a5f..72bc70e 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -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);
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 2aa20e3..ccb895a 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -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;
-- 
1.7.1

[PATCH v2 3/4] nvme: enable SG gaps support

[Sagi Grimberg]

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

[PATCH v2 3/4] nvme: enable SG gaps support

[Christoph Hellwig]

On Thu, Apr 20, 2017@02:27:59PM +0300, Sagi Grimberg wrote:
> 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..

[PATCH v2 3/4] nvme: enable SG gaps support

[Sagi Grimberg]

Christoph,

> 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

[PATCH v2 3/4] nvme: enable SG gaps support

[Christoph Hellwig]

On Sun, Apr 23, 2017@10:20:59AM +0300, Sagi Grimberg wrote:
> Christoph,
>
>> 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.

> 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 :)

[PATCH v2 3/4] nvme: enable SG gaps support

[Sagi Grimberg]

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

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

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

[PATCH v2 3/4] nvme: enable SG gaps support

[Max Gurtovoy]

On 4/23/2017 12:45 PM, Sagi Grimberg wrote:
>
>> 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...
>
>>> 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...
>
>> 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.

[PATCH v2 3/4] nvme: enable SG gaps support

[Christoph Hellwig]

On Sun, Apr 23, 2017@12:45:39PM +0300, Sagi Grimberg wrote:
>> 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.

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

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

diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index 47a479f..1c6f974 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -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;
-- 
1.7.1

[PATCH v2 0/4] arbitrary sg lists support

[Bart Van Assche]

On Thu, 2017-04-13@18:56 +0300, Max Gurtovoy wrote:
> 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.

[PATCH v2 0/4] arbitrary sg lists support

[Max Gurtovoy]

On 4/13/2017 7:31 PM, Bart Van Assche wrote:
> On Thu, 2017-04-13@18:56 +0300, Max Gurtovoy wrote:
>> 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?
>

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.

> Thanks,
>
> Bart.
>

[PATCH v2 0/4] arbitrary sg lists support

[Laurence Oberman]

----- Original Message -----
> 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
> 
> 
> 
> On 4/13/2017 7:31 PM, Bart Van Assche wrote:
> > On Thu, 2017-04-13@18:56 +0300, Max Gurtovoy wrote:
> >> 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?
> >
> 
> 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.
> 
> > 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

[PATCH v2 0/4] arbitrary sg lists support

[Sagi Grimberg]

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

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.

[PATCH v2 0/4] arbitrary sg lists support

[Leon Romanovsky]

On Thu, Apr 20, 2017@02:30:46PM +0300, Sagi Grimberg wrote:
>
> > > 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?
>
> 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.

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

[PATCH v2 0/4] arbitrary sg lists support

[Sagi Grimberg]

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

[PATCH v2 0/4] arbitrary sg lists support

[Leon Romanovsky]

On Sun, Apr 23, 2017@02:49:43PM +0300, Sagi Grimberg wrote:
>
> > > 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
>

At the end, he proposed unrelated fix.

> 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


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

[PATCH v2 0/4] arbitrary sg lists support

[Sagi Grimberg]

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

[PATCH v2 0/4] arbitrary sg lists support

[Max Gurtovoy]

On 4/23/2017 3:28 PM, Sagi Grimberg wrote:
>
>> 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...

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.

[PATCH v2 0/4] arbitrary sg lists support

[Sagi Grimberg]

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

Did anything progress with the debug?

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

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

> 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:
--
diff --git a/drivers/infiniband/hw/mlx5/mr.c 
b/drivers/infiniband/hw/mlx5/mr.c
index d9c6c0ea750b..040fbc387e4f 100644
--- a/drivers/infiniband/hw/mlx5/mr.c
+++ b/drivers/infiniband/hw/mlx5/mr.c
@@ -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.

[PATCH v2 0/4] arbitrary sg lists support

[Doug Ledford]

On Sun, 2017-04-23@17:35 +0300, Sagi Grimberg wrote:
> > 
> > I actually answered here:
> > https://www.spinics.net/lists/linux-rdma/msg46412.html
> 
> Did anything progress with the debug?
> 
> > 
> > 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.
> 
> > 
> > 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.

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

[PATCH v2 0/4] arbitrary sg lists support

[Max Gurtovoy]

On 5/1/2017 9:50 PM, Doug Ledford wrote:
> On Sun, 2017-04-23@17:35 +0300, Sagi Grimberg wrote:
>>>
>>> I actually answered here:
>>> https://www.spinics.net/lists/linux-rdma/msg46412.html
>>
>> Did anything progress with the debug?
>>
>>>
>>> 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.
>>
>>>
>>> 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.
>

Sure, no problem.

Max.

[PATCH v2 0/4] arbitrary sg lists support

[Laurence Oberman]

----- Original Message -----
> 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
> 
> 
> 
> On 5/1/2017 9:50 PM, Doug Ledford wrote:
> > On Sun, 2017-04-23@17:35 +0300, Sagi Grimberg wrote:
> >>>
> >>> I actually answered here:
> >>> https://www.spinics.net/lists/linux-rdma/msg46412.html
> >>
> >> Did anything progress with the debug?
> >>
> >>>
> >>> 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.
> >>
> >>>
> >>> 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.
> >
> 
> 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