[PATCH v2 0/2] Handle mlx4 max_sge_rd correctly

[PATCH v2 0/2] Handle mlx4 max_sge_rd correctly

[Sagi Grimberg]

This addresses a specific mlx4 issue where the max_sge_rd
is actually smaller than max_sge (rdma reads with max_sge
entries completes with error).

The second patch removes the explicit work-around from the
iser target code.

Changes from v1:
- Fixed driver rdma segment size to be 16 bytes

Changes from v0:
- Used a dedicated enumeration MLX4_MAX_SGE_RD and added
  a root cause analysis to patch change log.

- Fixed isert qp creation to be max_sge but construct rdma
  work request with the minimum of max_sge and max_sge_rd
  as non-rdma sends (login rsp) take 2 sges (and some devices
  have max_sge_rd = 1.

Sagi Grimberg (2):
  mlx4: Expose correct max_sge_rd limit
  iser-target: Remove explicit mlx4 work-around

 drivers/infiniband/hw/mlx4/main.c       |    2 +-
 drivers/infiniband/ulp/isert/ib_isert.c |   13 +++----------
 include/linux/mlx4/device.h             |   11 +++++++++++
 3 files changed, 15 insertions(+), 11 deletions(-)

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

[PATCH v2 1/2] mlx4: Expose correct max_sge_rd limit

[Sagi Grimberg]

mlx4 devices (ConnectX-2, ConnectX-3) has a limitation
where rdma read work queue entries cannot exceed 512 bytes.
A rdma_read wqe needs to fit in 512 bytes:
- wqe control segment (16 bytes)
- rdma segment (16 bytes)
- scatter elements (16 bytes each)

So max_sge_rd should be: (512 - 16 - 16) / 16 = 30.

Signed-off-by: Sagi Grimberg <sagig@mellanox.com>
Reviewed-by: Steve Wise <swise@opengridcomputing.com>
---
 drivers/infiniband/hw/mlx4/main.c |    2 +-
 include/linux/mlx4/device.h       |   11 +++++++++++
 2 files changed, 12 insertions(+), 1 deletions(-)

diff --git a/drivers/infiniband/hw/mlx4/main.c b/drivers/infiniband/hw/mlx4/main.c
index 7026580..e0232a2 100644
--- a/drivers/infiniband/hw/mlx4/main.c
+++ b/drivers/infiniband/hw/mlx4/main.c
@@ -499,7 +499,7 @@ static int mlx4_ib_init_device_flags(struct ib_device *ibdev)
 	ibdev->max_qp_wr	   = dev->dev->caps.max_wqes - MLX4_IB_SQ_MAX_SPARE;
 	ibdev->max_sge		   = min(dev->dev->caps.max_sq_sg,
 					 dev->dev->caps.max_rq_sg);
-	ibdev->max_sge_rd	   = ibdev->max_sge;
+	ibdev->max_sge_rd	   = MLX4_MAX_SGE_RD;
 	ibdev->max_cq		   = dev->dev->quotas.cq;
 	ibdev->max_cqe		   = dev->dev->caps.max_cqes;
 	ibdev->max_mr		   = dev->dev->quotas.mpt;
diff --git a/include/linux/mlx4/device.h b/include/linux/mlx4/device.h
index dac6872..3dcae41 100644
--- a/include/linux/mlx4/device.h
+++ b/include/linux/mlx4/device.h
@@ -427,6 +427,17 @@ enum {
 };
 
 enum {
+	/*
+	 * Max wqe size for rdma read is 512 bytes, so this
+	 * limits our max_sge_rd as the wqe needs to fit:
+	 * - ctrl segment (16 bytes)
+	 * - rdma segment (16 bytes)
+	 * - scatter elements (16 bytes each)
+	 */
+	MLX4_MAX_SGE_RD	= (512 - 16 - 16) / 16
+};
+
+enum {
 	MLX4_DEV_PMC_SUBTYPE_GUID_INFO	 = 0x14,
 	MLX4_DEV_PMC_SUBTYPE_PORT_INFO	 = 0x15,
 	MLX4_DEV_PMC_SUBTYPE_PKEY_TABLE	 = 0x16,
-- 
1.7.1

[PATCH v2 2/2] iser-target: Remove explicit mlx4 work-around

[Sagi Grimberg]

The driver now exposes sufficient limits so we can
avoid having mlx4 specific work-around.

Signed-off-by: Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Reviewed-by: Steve Wise <swise-7bPotxP6k4+P2YhJcF5u+vpXobYPEAuW@public.gmane.org>
---
 drivers/infiniband/ulp/isert/ib_isert.c |   13 +++----------
 1 files changed, 3 insertions(+), 10 deletions(-)

diff --git a/drivers/infiniband/ulp/isert/ib_isert.c b/drivers/infiniband/ulp/isert/ib_isert.c
index 96336a9..eb985f9 100644
--- a/drivers/infiniband/ulp/isert/ib_isert.c
+++ b/drivers/infiniband/ulp/isert/ib_isert.c
@@ -141,16 +141,9 @@ isert_create_qp(struct isert_conn *isert_conn,
 	attr.recv_cq = comp->cq;
 	attr.cap.max_send_wr = ISERT_QP_MAX_REQ_DTOS;
 	attr.cap.max_recv_wr = ISERT_QP_MAX_RECV_DTOS + 1;
-	/*
-	 * FIXME: Use devattr.max_sge - 2 for max_send_sge as
-	 * work-around for RDMA_READs with ConnectX-2.
-	 *
-	 * Also, still make sure to have at least two SGEs for
-	 * outgoing control PDU responses.
-	 */
-	attr.cap.max_send_sge = max(2, device->ib_device->max_sge - 2);
-	isert_conn->max_sge = attr.cap.max_send_sge;
-
+	attr.cap.max_send_sge = device->ib_device->max_sge;
+	isert_conn->max_sge = min(device->ib_device->max_sge,
+				  device->ib_device->max_sge_rd);
 	attr.cap.max_recv_sge = 1;
 	attr.sq_sig_type = IB_SIGNAL_REQ_WR;
 	attr.qp_type = IB_QPT_RC;
-- 
1.7.1

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

Re: [PATCH v2 0/2] Handle mlx4 max_sge_rd correctly

[Sagi Grimberg]

On 28/10/2015 13:28, Sagi Grimberg wrote:
> This addresses a specific mlx4 issue where the max_sge_rd
> is actually smaller than max_sge (rdma reads with max_sge
> entries completes with error).
>
> The second patch removes the explicit work-around from the
> iser target code.
>
> Changes from v1:
> - Fixed driver rdma segment size to be 16 bytes
>
> Changes from v0:
> - Used a dedicated enumeration MLX4_MAX_SGE_RD and added
>    a root cause analysis to patch change log.
>
> - Fixed isert qp creation to be max_sge but construct rdma
>    work request with the minimum of max_sge and max_sge_rd
>    as non-rdma sends (login rsp) take 2 sges (and some devices
>    have max_sge_rd = 1.
>
> Sagi Grimberg (2):
>    mlx4: Expose correct max_sge_rd limit
>    iser-target: Remove explicit mlx4 work-around
>
>   drivers/infiniband/hw/mlx4/main.c       |    2 +-
>   drivers/infiniband/ulp/isert/ib_isert.c |   13 +++----------
>   include/linux/mlx4/device.h             |   11 +++++++++++
>   3 files changed, 15 insertions(+), 11 deletions(-)
>

Doug,

Any reply on this patchset?
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 0/2] Handle mlx4 max_sge_rd correctly

[Christoph Hellwig]

On Tue, Nov 10, 2015 at 12:36:44PM +0200, Sagi Grimberg wrote:
> Any reply on this patchset?

Did we ever make progress on this?

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

Re: [PATCH v2 0/2] Handle mlx4 max_sge_rd correctly

[Sagi Grimberg]

> Did we ever make progress on this?

Just up to Doug to pull it in.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 0/2] Handle mlx4 max_sge_rd correctly

[Christoph Hellwig]

On Thu, Dec 03, 2015 at 05:07:35PM +0200, Sagi Grimberg wrote:
> 
> >Did we ever make progress on this?
> 
> Just up to Doug to pull it in.

Doug, any chance to get this into the 4.4 queue?  It's annoying
having to work around this driver bug in every new ULP.

Re: [PATCH v2 1/2] mlx4: Expose correct max_sge_rd limit

[Doug Ledford]

[-- Attachment #1: Type: text/plain, Size: 2468 bytes --]

On 10/28/2015 07:28 AM, Sagi Grimberg wrote:
> mlx4 devices (ConnectX-2, ConnectX-3) has a limitation
> where rdma read work queue entries cannot exceed 512 bytes.
> A rdma_read wqe needs to fit in 512 bytes:
> - wqe control segment (16 bytes)
> - rdma segment (16 bytes)
> - scatter elements (16 bytes each)
> 
> So max_sge_rd should be: (512 - 16 - 16) / 16 = 30.
> 
> Signed-off-by: Sagi Grimberg <sagig@mellanox.com>
> Reviewed-by: Steve Wise <swise@opengridcomputing.com>
> ---
>  drivers/infiniband/hw/mlx4/main.c |    2 +-
>  include/linux/mlx4/device.h       |   11 +++++++++++
>  2 files changed, 12 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/infiniband/hw/mlx4/main.c b/drivers/infiniband/hw/mlx4/main.c
> index 7026580..e0232a2 100644
> --- a/drivers/infiniband/hw/mlx4/main.c
> +++ b/drivers/infiniband/hw/mlx4/main.c
> @@ -499,7 +499,7 @@ static int mlx4_ib_init_device_flags(struct ib_device *ibdev)
>  	ibdev->max_qp_wr	   = dev->dev->caps.max_wqes - MLX4_IB_SQ_MAX_SPARE;
>  	ibdev->max_sge		   = min(dev->dev->caps.max_sq_sg,
>  					 dev->dev->caps.max_rq_sg);
> -	ibdev->max_sge_rd	   = ibdev->max_sge;
> +	ibdev->max_sge_rd	   = MLX4_MAX_SGE_RD;
>  	ibdev->max_cq		   = dev->dev->quotas.cq;
>  	ibdev->max_cqe		   = dev->dev->caps.max_cqes;
>  	ibdev->max_mr		   = dev->dev->quotas.mpt;

What code tree did you make this patch from?  Because in an upstream 4.4
kernel, there is no function mlx4_ib_init_device_flags(), but there is
mlx4_ib_query_device() that does much the same thing.  But in there,
there is no ibdev->max_sge_rd, there is props->max_sge_rd.  It's like
this came from an entirely different source base somewhere.

> diff --git a/include/linux/mlx4/device.h b/include/linux/mlx4/device.h
> index dac6872..3dcae41 100644
> --- a/include/linux/mlx4/device.h
> +++ b/include/linux/mlx4/device.h
> @@ -427,6 +427,17 @@ enum {
>  };
>  
>  enum {
> +	/*
> +	 * Max wqe size for rdma read is 512 bytes, so this
> +	 * limits our max_sge_rd as the wqe needs to fit:
> +	 * - ctrl segment (16 bytes)
> +	 * - rdma segment (16 bytes)
> +	 * - scatter elements (16 bytes each)
> +	 */
> +	MLX4_MAX_SGE_RD	= (512 - 16 - 16) / 16
> +};
> +
> +enum {
>  	MLX4_DEV_PMC_SUBTYPE_GUID_INFO	 = 0x14,
>  	MLX4_DEV_PMC_SUBTYPE_PORT_INFO	 = 0x15,
>  	MLX4_DEV_PMC_SUBTYPE_PKEY_TABLE	 = 0x16,
> 


-- 
Doug Ledford <dledford@redhat.com>
              GPG KeyID: 0E572FDD



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]

Re: [PATCH v2 0/2] Handle mlx4 max_sge_rd correctly

[Doug Ledford]

[-- Attachment #1: Type: text/plain, Size: 783 bytes --]

On 12/08/2015 12:01 PM, Christoph Hellwig wrote:
> On Thu, Dec 03, 2015 at 05:07:35PM +0200, Sagi Grimberg wrote:
>>
>>> Did we ever make progress on this?
>>
>> Just up to Doug to pull it in.
> 
> Doug, any chance to get this into the 4.4 queue?  It's annoying
> having to work around this driver bug in every new ULP.

I've pulled these in.  Both of them failed to apply, and the first one
was just flat baffling in terms of how Sagi is working from a source
base so different than the one in the upstream kernel.  They should be
double checked just to make sure I'm not missing something since they
were so far off from applying to an upstream kernel.


-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
              GPG KeyID: 0E572FDD



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]

Re: [PATCH v2 0/2] Handle mlx4 max_sge_rd correctly

[Sagi Grimberg]

> I've pulled these in.  Both of them failed to apply, and the first one
> was just flat baffling in terms of how Sagi is working from a source
> base so different than the one in the upstream kernel.  They should be
> double checked just to make sure I'm not missing something since they
> were so far off from applying to an upstream kernel.

Hi Doug,

I mentioned this in v1. This patch set is applied over Christoph's
device attributes patch. Will it go in as well?

Re: [PATCH v2 0/2] Handle mlx4 max_sge_rd correctly

[Doug Ledford]

[-- Attachment #1: Type: text/plain, Size: 844 bytes --]

On 12/08/2015 01:13 PM, Sagi Grimberg wrote:
> 
>> I've pulled these in.  Both of them failed to apply, and the first one
>> was just flat baffling in terms of how Sagi is working from a source
>> base so different than the one in the upstream kernel.  They should be
>> double checked just to make sure I'm not missing something since they
>> were so far off from applying to an upstream kernel.
> 
> Hi Doug,
> 
> I mentioned this in v1. This patch set is applied over Christoph's
> device attributes patch. Will it go in as well?

No, that's too big and not the right sort of fix for 4.4-rc.  I had it
on my radar for getting into for-next.  So, it'll have to be fixed up to
apply over the top of your patches which will already be applied.


-- 
Doug Ledford <dledford@redhat.com>
              GPG KeyID: 0E572FDD



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]

Re: [PATCH v2 0/2] Handle mlx4 max_sge_rd correctly

[Doug Ledford]

[-- Attachment #1: Type: text/plain, Size: 993 bytes --]

On 12/08/2015 01:17 PM, Doug Ledford wrote:
> On 12/08/2015 01:13 PM, Sagi Grimberg wrote:
>>
>>> I've pulled these in.  Both of them failed to apply, and the first one
>>> was just flat baffling in terms of how Sagi is working from a source
>>> base so different than the one in the upstream kernel.  They should be
>>> double checked just to make sure I'm not missing something since they
>>> were so far off from applying to an upstream kernel.
>>
>> Hi Doug,
>>
>> I mentioned this in v1. This patch set is applied over Christoph's
>> device attributes patch. Will it go in as well?
> 
> No, that's too big and not the right sort of fix for 4.4-rc.  I had it
> on my radar for getting into for-next.  So, it'll have to be fixed up to
> apply over the top of your patches which will already be applied.

BTW, it's already pushed out, you can look at it now.


-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
              GPG KeyID: 0E572FDD



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]

Re: [PATCH v2 0/2] Handle mlx4 max_sge_rd correctly

[Sagi Grimberg]

> BTW, it's already pushed out, you can look at it now.

Looks fine.

Thanks