[PATCH] mlx4: remove limitation on LSO header size

[PATCH] mlx4: remove limitation on LSO header size

[Eli Cohen]

Current code has a limitation as for the size of an LSO header not allowed to
cross a 64 byte boundary. This patch removes this limitation by setting the WQE
RR for large headers thus allowing LSO headers of any size. The extra buffer
reserved for MLX4_IB_QP_LSO QPs has been doubled, from 64 to 128 bytes,
assuming this is reasonable upper limit to header length.
Also, this patch will cause IB_DEVICE_UD_TSO to be set only of FW versions that
set MLX4_DEV_CAP_FLAG_BLH; e.g. FW version 2.6.000 and higher.

Signed-off-by: Eli Cohen <eli-VPRAkNaXOzVS1MOuV/RT9w@public.gmane.org>
---
 drivers/infiniband/hw/mlx4/main.c |    2 +-
 drivers/infiniband/hw/mlx4/qp.c   |   17 +++++++----------
 include/linux/mlx4/device.h       |    1 +
 3 files changed, 9 insertions(+), 11 deletions(-)

diff --git a/drivers/infiniband/hw/mlx4/main.c b/drivers/infiniband/hw/mlx4/main.c
index 3cb3f47..e596537 100644
--- a/drivers/infiniband/hw/mlx4/main.c
+++ b/drivers/infiniband/hw/mlx4/main.c
@@ -103,7 +103,7 @@ static int mlx4_ib_query_device(struct ib_device *ibdev,
 		props->device_cap_flags |= IB_DEVICE_UD_AV_PORT_ENFORCE;
 	if (dev->dev->caps.flags & MLX4_DEV_CAP_FLAG_IPOIB_CSUM)
 		props->device_cap_flags |= IB_DEVICE_UD_IP_CSUM;
-	if (dev->dev->caps.max_gso_sz)
+	if (dev->dev->caps.max_gso_sz && dev->dev->caps.flags & MLX4_DEV_CAP_FLAG_BLH)
 		props->device_cap_flags |= IB_DEVICE_UD_TSO;
 	if (dev->dev->caps.bmme_flags & MLX4_BMME_FLAG_RESERVED_LKEY)
 		props->device_cap_flags |= IB_DEVICE_LOCAL_DMA_LKEY;
diff --git a/drivers/infiniband/hw/mlx4/qp.c b/drivers/infiniband/hw/mlx4/qp.c
index 219b103..1b356cf 100644
--- a/drivers/infiniband/hw/mlx4/qp.c
+++ b/drivers/infiniband/hw/mlx4/qp.c
@@ -261,7 +261,7 @@ static int send_wqe_overhead(enum ib_qp_type type, u32 flags)
 	case IB_QPT_UD:
 		return sizeof (struct mlx4_wqe_ctrl_seg) +
 			sizeof (struct mlx4_wqe_datagram_seg) +
-			((flags & MLX4_IB_QP_LSO) ? 64 : 0);
+			((flags & MLX4_IB_QP_LSO) ? 128 : 0);
 	case IB_QPT_UC:
 		return sizeof (struct mlx4_wqe_ctrl_seg) +
 			sizeof (struct mlx4_wqe_raddr_seg);
@@ -1467,16 +1467,11 @@ static void __set_data_seg(struct mlx4_wqe_data_seg *dseg, struct ib_sge *sg)
 
 static int build_lso_seg(struct mlx4_wqe_lso_seg *wqe, struct ib_send_wr *wr,
 			 struct mlx4_ib_qp *qp, unsigned *lso_seg_len,
-			 __be32 *lso_hdr_sz)
+			 __be32 *lso_hdr_sz, int *blh)
 {
 	unsigned halign = ALIGN(sizeof *wqe + wr->wr.ud.hlen, 16);
 
-	/*
-	 * This is a temporary limitation and will be removed in
-	 * a forthcoming FW release:
-	 */
-	if (unlikely(halign > 64))
-		return -EINVAL;
+	*blh = unlikely(halign > 64) ? 1 : 0;
 
 	if (unlikely(!(qp->flags & MLX4_IB_QP_LSO) &&
 		     wr->num_sge > qp->sq.max_gs - (halign >> 4)))
@@ -1523,6 +1518,7 @@ int mlx4_ib_post_send(struct ib_qp *ibqp, struct ib_send_wr *wr,
 	__be32 *lso_wqe;
 	__be32 uninitialized_var(lso_hdr_sz);
 	int i;
+	int blh = 0;
 
 	spin_lock_irqsave(&qp->sq.lock, flags);
 
@@ -1616,7 +1612,7 @@ int mlx4_ib_post_send(struct ib_qp *ibqp, struct ib_send_wr *wr,
 			size += sizeof (struct mlx4_wqe_datagram_seg) / 16;
 
 			if (wr->opcode == IB_WR_LSO) {
-				err = build_lso_seg(wqe, wr, qp, &seglen, &lso_hdr_sz);
+				err = build_lso_seg(wqe, wr, qp, &seglen, &lso_hdr_sz, &blh);
 				if (unlikely(err)) {
 					*bad_wr = wr;
 					goto out;
@@ -1687,7 +1683,8 @@ int mlx4_ib_post_send(struct ib_qp *ibqp, struct ib_send_wr *wr,
 		}
 
 		ctrl->owner_opcode = mlx4_ib_opcode[wr->opcode] |
-			(ind & qp->sq.wqe_cnt ? cpu_to_be32(1 << 31) : 0);
+			(ind & qp->sq.wqe_cnt ? cpu_to_be32(1 << 31) : 0) |
+			(blh ? cpu_to_be32(1 << 6) : 0);
 
 		stamp = ind + qp->sq_spare_wqes;
 		ind += DIV_ROUND_UP(size * 16, 1U << qp->sq.wqe_shift);
diff --git a/include/linux/mlx4/device.h b/include/linux/mlx4/device.h
index ce7cc6c..e92d1bf 100644
--- a/include/linux/mlx4/device.h
+++ b/include/linux/mlx4/device.h
@@ -61,6 +61,7 @@ enum {
 	MLX4_DEV_CAP_FLAG_BAD_PKEY_CNTR	= 1 <<  8,
 	MLX4_DEV_CAP_FLAG_BAD_QKEY_CNTR	= 1 <<  9,
 	MLX4_DEV_CAP_FLAG_DPDP		= 1 << 12,
+	MLX4_DEV_CAP_FLAG_BLH		= 1 << 15,
 	MLX4_DEV_CAP_FLAG_MEM_WINDOW	= 1 << 16,
 	MLX4_DEV_CAP_FLAG_APM		= 1 << 17,
 	MLX4_DEV_CAP_FLAG_ATOMIC	= 1 << 18,
-- 
1.6.4.3

--
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] mlx4: remove limitation on LSO header size

[Roland Dreier]

 > +	*blh = unlikely(halign > 64) ? 1 : 0;

This idiom of "(boolean condition) ? 1 : 0" looks odd to me... doesn't
(halign > 64) already evaluate to 1 or 0 anyway?  Does the unlikely()
actually affect code generation here?

With that said, see below...

 > +	int blh = 0;

I assume this initialization is to avoid a compiler warning.  But the
code is actually correct without initializing blh -- so I think that we
save a tiny bit of code by doing uninitialized_var() instead?

 > +			(blh ? cpu_to_be32(1 << 6) : 0);

...given that the only use of blh is as a flag to decide what constant
to use here, does it generate better code to make blh be __be32 and set
the value directly in build_lso_seg, ie do:

	*blh = unlikely(halign > 64) ? cpu_to_be32(1 << 6) : 0;

and then use blh without ?: in mlx4_ib_post_send...

 - R.

Re: [ewg] [PATCH] mlx4: remove limitation on LSO header size

[Eli Cohen]

On Sun, Oct 04, 2009 at 10:12:16AM +0200, Or Gerlitz wrote:
>> Also, this patch will cause IB_DEVICE_UD_TSO to be set only of FW versions that set MLX4_DEV_CAP_FLAG_BLH; e.g. FW version 2.6.000 and higher.
> warning to users having an older firmware installed?

I am not sure this deserves a warning - after all it's just that the
feature is not supported.

> So the driver doesn't use the actual value of the max_gso_sz capability,  
> isn't this a bug?  the BLH bit  (any reason not the mention in the  
> change-log what these three letters stand for...?) serves you to support  
> large LSO headers, but isn't enough, max_gso_sz is related to the  
> payload and should be used, I think.
>
According to the spec, for LSO to be supported at all, max_gso_sz must
be none zero. Moreover, for BLH to be supported (BLH stands for Big
LSO Header), the MLX4_DEV_CAP_FLAG_BLH flag must be set and hence the
condition on both of them.


>>  	if (dev->dev->caps.bmme_flags & MLX4_BMME_FLAG_RESERVED_LKEY)
>>  		props->device_cap_flags |= IB_DEVICE_LOCAL_DMA_LKEY;
>> diff --git a/drivers/infiniband/hw/mlx4/qp.c b/drivers/infiniband/hw/mlx4/qp.c
>> index 219b103..1b356cf 100644
>> --- a/drivers/infiniband/hw/mlx4/qp.c
>> +++ b/drivers/infiniband/hw/mlx4/qp.c
>> @@ -261,7 +261,7 @@ static int send_wqe_overhead(enum ib_qp_type type, u32 flags)
>>  	case IB_QPT_UD:
>>  		return sizeof (struct mlx4_wqe_ctrl_seg) +
>>  			sizeof (struct mlx4_wqe_datagram_seg) +
>> -			((flags & MLX4_IB_QP_LSO) ? 64 : 0);
>> +			((flags & MLX4_IB_QP_LSO) ? 128 : 0);
> 64 , 128 ... here and later in build_lso_seg ,  how about defining some  
> human readable something?

OK, I will replace with an enum.
--
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] mlx4: remove limitation on LSO header size

[Eli Cohen]

On Wed, Oct 07, 2009 at 03:45:16PM -0700, Roland Dreier wrote:
> 
>  > +	*blh = unlikely(halign > 64) ? 1 : 0;
> 
> This idiom of "(boolean condition) ? 1 : 0" looks odd to me... doesn't
> (halign > 64) already evaluate to 1 or 0 anyway?  Does the unlikely()
> actually affect code generation here?
True, (halign > 64) is the same and is cleaner. As for the unlikely()
-- well it's already been there and besides, we're never sure if it
will improve anything so the same question could be asked for other
places in the code.


> 
> With that said, see below...
> 
>  > +	int blh = 0;
> 
> I assume this initialization is to avoid a compiler warning.  But the
> code is actually correct without initializing blh -- so I think that we
> save a tiny bit of code by doing uninitialized_var() instead?
We must initialize blh since it is used for any send request and not
just LSO opcodes. 


> 
>  > +			(blh ? cpu_to_be32(1 << 6) : 0);
> 
> ...given that the only use of blh is as a flag to decide what constant
> to use here, does it generate better code to make blh be __be32 and set
> the value directly in build_lso_seg, ie do:
> 
> 	*blh = unlikely(halign > 64) ? cpu_to_be32(1 << 6) : 0;
> 
> and then use blh without ?: in mlx4_ib_post_send...
> 

So we can let build_lso_header() put the corrent value for blh and
unconditionally "or" it into owner_opcode.

Re: [PATCH] mlx4: remove limitation on LSO header size

[Roland Dreier]

 > >  > +	*blh = unlikely(halign > 64) ? 1 : 0;

 > > This idiom of "(boolean condition) ? 1 : 0" looks odd to me... doesn't
 > > (halign > 64) already evaluate to 1 or 0 anyway?  Does the unlikely()
 > > actually affect code generation here?

 > True, (halign > 64) is the same and is cleaner. As for the unlikely()
 > -- well it's already been there and besides, we're never sure if it
 > will improve anything so the same question could be asked for other
 > places in the code.

I was just wondering in this case where you are just assigning the
boolean value of the expression to a variable how unlikely affects
things.  I can understand for conditional jumps how the compiler can
choose to make the likely case more efficient, but when there are no
jumps then I was just curious how the hint could help.

 > > I assume this initialization is to avoid a compiler warning.  But the
 > > code is actually correct without initializing blh -- so I think that we
 > > save a tiny bit of code by doing uninitialized_var() instead?

 > We must initialize blh since it is used for any send request and not
 > just LSO opcodes. 

So then this patch was buggy because blh was not reinitialized as we
loop through multiple work requests?  eg an LSO request followed by a
non-LSO request?

Anyway I'll look over the newer patch.