mlx4: remove limitation on LSO header size
Subsystems (3)
PATCH 1/1 mlx4: remove limitation on LSO header size
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.
--- 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(-)
drivers/infiniband/hw/mlx4/main.c +1 -1
@@ -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 ;
drivers/infiniband/hw/mlx4/qp.c +7 -10
@@ -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);
include/linux/mlx4/device.h +1 -0
@@ -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,
Signature · 6 lines
1.6.4.3 -- 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
drivers/infiniband/hw/mlx4/qp.c static void __set_data_seg(struct mlx4_wqe_data_seg *dseg, struct ib_sge *sg) · line 1474
Quoted 2 lines
drivers/infiniband/hw/mlx4/qp.c +1
-0+ *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...
drivers/infiniband/hw/mlx4/qp.c int mlx4_ib_post_send(struct ib_qp *ibqp, struct ib_send_wr *wr, · line 1521
Quoted 1 line
drivers/infiniband/hw/mlx4/qp.c +1
-0+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?
drivers/infiniband/hw/mlx4/qp.c int mlx4_ib_post_send(struct ib_qp *ibqp, struct ib_send_wr *wr, · line 1687
Quoted 1 line
drivers/infiniband/hw/mlx4/qp.c +1
-0+ (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.
Quoted 2 lines · Or Gerlitz
Quoted 1 line
Also, this patch will cause
IB_DEVICE_UD_TSOto be set only of FW versions that setMLX4_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.
Quoted 6 lines
So the driver doesn't use the actual value of the
max_gso_szcapability, 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_szis 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.
Quoted 6 lines
Quoted 6 lines
drivers/infiniband/hw/mlx4/main.c +0
-0if (dev->dev->caps.bmme_flags &MLX4_BMME_FLAG_RESERVED_LKEY ) props->device_cap_flags |=IB_DEVICE_LOCAL_DMA_LKEY ;drivers/infiniband/hw/mlx4/qp.c +0
-0
Quoted 8 lines
drivers/infiniband/hw/mlx4/qp.c static int send_wqe_overhead(enum ib_qp_type type, u32 flags) · line 264
Quoted 6 lines
drivers/infiniband/hw/mlx4/qp.c +1
-1@@ -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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 6 lines · Roland Dreier
drivers/infiniband/hw/mlx4/qp.c static void __set_data_seg(struct mlx4_wqe_data_seg *dseg, struct ib_sge *sg) · line 1474
Quoted 2 lines
drivers/infiniband/hw/mlx4/qp.c +1
-0+ *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.
Quoted 8 lines
With that said, see below...
drivers/infiniband/hw/mlx4/qp.c int mlx4_ib_post_send(struct ib_qp *ibqp, struct ib_send_wr *wr, · line 1521
Quoted 1 line
drivers/infiniband/hw/mlx4/qp.c +1
-0+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.
Quoted 11 lines
drivers/infiniband/hw/mlx4/qp.c int mlx4_ib_post_send(struct ib_qp *ibqp, struct ib_send_wr *wr, · line 1687
Quoted 2 lines
drivers/infiniband/hw/mlx4/qp.c +1
-0+ (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
__be32and set the value directly inbuild_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.
Quoted 11 lines
Quoted 6 lines
drivers/infiniband/hw/mlx4/qp.c static void __set_data_seg(struct mlx4_wqe_data_seg *dseg, struct ib_sge *sg) · line 1474
Quoted 2 lines
drivers/infiniband/hw/mlx4/qp.c +1
-0+ *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.
Quoted 6 lines
Quoted 3 lines
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.