[ofa-general] Re: [PATCH 3/10] IB/core: Add LSO support
Eli Cohen
eli at dev.mellanox.co.il
Wed Apr 2 03:04:39 PDT 2008
Oof, that was a bad one and the following patch fixes the problem.
diff --git a/drivers/infiniband/hw/mlx4/qp.c b/drivers/infiniband/hw/mlx4/qp.c
index f805e8a..4eaee27 100644
--- a/drivers/infiniband/hw/mlx4/qp.c
+++ b/drivers/infiniband/hw/mlx4/qp.c
@@ -255,7 +255,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) ? 64 : 0);
case IB_QPT_UC:
return sizeof (struct mlx4_wqe_ctrl_seg) +
sizeof (struct mlx4_wqe_raddr_seg);
and the explanation is this. Since '+' precedes the '?' operator, the
expression evaluated is:
sizeof (struct mlx4_wqe_ctrl_seg) + sizeof (struct mlx4_wqe_datagram_seg) +
(flags & MLX4_IB_QP_LSO)
which is obviously true so the value returned is 64. The parentheses
around the '?' gives the desired result.
On Tue, 2008-04-01 at 12:41 -0700, Roland Dreier wrote:
> > would like me to re-generate the mlx4 LSO patch to match this commit or
> > would you do the adjustments?
>
> Sorry for being so slow.
>
> Anyway I did the adjustments as below. I also removed the "reserve"
> variable and moved the 64 byte extra for LSO into send_wqe_overhead(),
> since it seemed that the only place where you used send_wqe_overhead()
> without adding in reserve was actually a bug.
>
> I also did various changes other places, and maybe introduced a bug:
> when I try NPtcp between two systems (once running unmodified
> 2.6.25-rc8, the other running my for-2.6.26 branch, both with ConnectX
> with FW 2.3.000), on the side with the LSO patch, I eventually get a
> "local length error" or "local QP operation err" on a send. It is an
> LSO send of length 63744 with 17 fragments and an mss of 1992, so it
> should be segmented into 32 packets. Some of these sends complete
> successfully but eventually one fails. I'm still debugging but maybe
> you have some idea?
>
> When I get the local QP operation error, I get this in case it helps:
>
> local QP operation err (QPN 000048, WQE index affa, vendor syndrome 6f, opcode = 5e)
> CQE contents 00000048 00000000 00000000 00000000 00000000 00000000 affa6f02 0000005e
>
> - R.
>
> From 141035c707b81638659ada01f456d066f2b353f7 Mon Sep 17 00:00:00 2001
> From: Eli Cohen <eli at dev.mellanox.co.il>
> Date: Tue, 25 Mar 2008 15:35:12 +0200
> Subject: [PATCH] IB/mlx4: Add IPoIB LSO support
>
> Add TSO support to the mlx4_ib driver.
>
> Signed-off-by: Eli Cohen <eli at mellanox.co.il>
> Signed-off-by: Roland Dreier <rolandd at cisco.com>
> ---
> drivers/infiniband/hw/mlx4/cq.c | 3 +
> drivers/infiniband/hw/mlx4/main.c | 2 +
> drivers/infiniband/hw/mlx4/mlx4_ib.h | 5 ++
> drivers/infiniband/hw/mlx4/qp.c | 72 +++++++++++++++++++++++++++++----
> drivers/net/mlx4/fw.c | 9 ++++
> drivers/net/mlx4/fw.h | 1 +
> drivers/net/mlx4/main.c | 1 +
> include/linux/mlx4/device.h | 1 +
> include/linux/mlx4/qp.h | 5 ++
> 9 files changed, 90 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/infiniband/hw/mlx4/cq.c b/drivers/infiniband/hw/mlx4/cq.c
> index d2e32b0..7d70af7 100644
> --- a/drivers/infiniband/hw/mlx4/cq.c
> +++ b/drivers/infiniband/hw/mlx4/cq.c
> @@ -420,6 +420,9 @@ static int mlx4_ib_poll_one(struct mlx4_ib_cq *cq,
> case MLX4_OPCODE_BIND_MW:
> wc->opcode = IB_WC_BIND_MW;
> break;
> + case MLX4_OPCODE_LSO:
> + wc->opcode = IB_WC_LSO;
> + break;
> }
> } else {
> wc->byte_len = be32_to_cpu(cqe->byte_cnt);
> diff --git a/drivers/infiniband/hw/mlx4/main.c b/drivers/infiniband/hw/mlx4/main.c
> index 6ea4746..e9330a0 100644
> --- a/drivers/infiniband/hw/mlx4/main.c
> +++ b/drivers/infiniband/hw/mlx4/main.c
> @@ -101,6 +101,8 @@ 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)
> + props->device_cap_flags |= IB_DEVICE_UD_TSO;
>
> props->vendor_id = be32_to_cpup((__be32 *) (out_mad->data + 36)) &
> 0xffffff;
> diff --git a/drivers/infiniband/hw/mlx4/mlx4_ib.h b/drivers/infiniband/hw/mlx4/mlx4_ib.h
> index 3726e45..3f8bd0a 100644
> --- a/drivers/infiniband/hw/mlx4/mlx4_ib.h
> +++ b/drivers/infiniband/hw/mlx4/mlx4_ib.h
> @@ -110,6 +110,10 @@ struct mlx4_ib_wq {
> unsigned tail;
> };
>
> +enum mlx4_ib_qp_flags {
> + MLX4_IB_QP_LSO = 1 << 0
> +};
> +
> struct mlx4_ib_qp {
> struct ib_qp ibqp;
> struct mlx4_qp mqp;
> @@ -129,6 +133,7 @@ struct mlx4_ib_qp {
> struct mlx4_mtt mtt;
> int buf_size;
> struct mutex mutex;
> + u32 flags;
> u8 port;
> u8 alt_port;
> u8 atomic_rd_en;
> diff --git a/drivers/infiniband/hw/mlx4/qp.c b/drivers/infiniband/hw/mlx4/qp.c
> index 320c25f..8ddb97e 100644
> --- a/drivers/infiniband/hw/mlx4/qp.c
> +++ b/drivers/infiniband/hw/mlx4/qp.c
> @@ -71,6 +71,7 @@ enum {
>
> static const __be32 mlx4_ib_opcode[] = {
> [IB_WR_SEND] = __constant_cpu_to_be32(MLX4_OPCODE_SEND),
> + [IB_WR_LSO] = __constant_cpu_to_be32(MLX4_OPCODE_LSO),
> [IB_WR_SEND_WITH_IMM] = __constant_cpu_to_be32(MLX4_OPCODE_SEND_IMM),
> [IB_WR_RDMA_WRITE] = __constant_cpu_to_be32(MLX4_OPCODE_RDMA_WRITE),
> [IB_WR_RDMA_WRITE_WITH_IMM] = __constant_cpu_to_be32(MLX4_OPCODE_RDMA_WRITE_IMM),
> @@ -242,7 +243,7 @@ static void mlx4_ib_qp_event(struct mlx4_qp *qp, enum mlx4_event type)
> }
> }
>
> -static int send_wqe_overhead(enum ib_qp_type type)
> +static int send_wqe_overhead(enum ib_qp_type type, u32 flags)
> {
> /*
> * UD WQEs must have a datagram segment.
> @@ -253,7 +254,8 @@ static int send_wqe_overhead(enum ib_qp_type type)
> switch (type) {
> case IB_QPT_UD:
> return sizeof (struct mlx4_wqe_ctrl_seg) +
> - sizeof (struct mlx4_wqe_datagram_seg);
> + sizeof (struct mlx4_wqe_datagram_seg) +
> + (flags & MLX4_IB_QP_LSO) ? 64 : 0;
> case IB_QPT_UC:
> return sizeof (struct mlx4_wqe_ctrl_seg) +
> sizeof (struct mlx4_wqe_raddr_seg);
> @@ -315,7 +317,7 @@ static int set_kernel_sq_size(struct mlx4_ib_dev *dev, struct ib_qp_cap *cap,
> /* Sanity check SQ size before proceeding */
> if (cap->max_send_wr > dev->dev->caps.max_wqes ||
> cap->max_send_sge > dev->dev->caps.max_sq_sg ||
> - cap->max_inline_data + send_wqe_overhead(type) +
> + cap->max_inline_data + send_wqe_overhead(type, qp->flags) +
> sizeof (struct mlx4_wqe_inline_seg) > dev->dev->caps.max_sq_desc_sz)
> return -EINVAL;
>
> @@ -329,7 +331,7 @@ static int set_kernel_sq_size(struct mlx4_ib_dev *dev, struct ib_qp_cap *cap,
>
> s = max(cap->max_send_sge * sizeof (struct mlx4_wqe_data_seg),
> cap->max_inline_data + sizeof (struct mlx4_wqe_inline_seg)) +
> - send_wqe_overhead(type);
> + send_wqe_overhead(type, qp->flags);
>
> /*
> * Hermon supports shrinking WQEs, such that a single work
> @@ -394,7 +396,8 @@ static int set_kernel_sq_size(struct mlx4_ib_dev *dev, struct ib_qp_cap *cap,
> }
>
> qp->sq.max_gs = ((qp->sq_max_wqes_per_wr << qp->sq.wqe_shift) -
> - send_wqe_overhead(type)) / sizeof (struct mlx4_wqe_data_seg);
> + send_wqe_overhead(type, qp->flags)) /
> + sizeof (struct mlx4_wqe_data_seg);
>
> qp->buf_size = (qp->rq.wqe_cnt << qp->rq.wqe_shift) +
> (qp->sq.wqe_cnt << qp->sq.wqe_shift);
> @@ -503,6 +506,9 @@ static int create_qp_common(struct mlx4_ib_dev *dev, struct ib_pd *pd,
> } else {
> qp->sq_no_prefetch = 0;
>
> + if (init_attr->create_flags & IB_QP_CREATE_IPOIB_UD_LSO)
> + qp->flags |= MLX4_IB_QP_LSO;
> +
> err = set_kernel_sq_size(dev, &init_attr->cap, init_attr->qp_type, qp);
> if (err)
> goto err;
> @@ -673,7 +679,11 @@ struct ib_qp *mlx4_ib_create_qp(struct ib_pd *pd,
> struct mlx4_ib_qp *qp;
> int err;
>
> - if (init_attr->create_flags)
> + /* We only support LSO, and only for kernel UD QPs. */
> + if (init_attr->create_flags & ~IB_QP_CREATE_IPOIB_UD_LSO)
> + return ERR_PTR(-EINVAL);
> + if (init_attr->create_flags & IB_QP_CREATE_IPOIB_UD_LSO &&
> + (pd->uobject || init_attr->qp_type != IB_QPT_UD))
> return ERR_PTR(-EINVAL);
>
> switch (init_attr->qp_type) {
> @@ -879,10 +889,15 @@ static int __mlx4_ib_modify_qp(struct ib_qp *ibqp,
> }
> }
>
> - if (ibqp->qp_type == IB_QPT_GSI || ibqp->qp_type == IB_QPT_SMI ||
> - ibqp->qp_type == IB_QPT_UD)
> + if (ibqp->qp_type == IB_QPT_GSI || ibqp->qp_type == IB_QPT_SMI)
> context->mtu_msgmax = (IB_MTU_4096 << 5) | 11;
> - else if (attr_mask & IB_QP_PATH_MTU) {
> + else if (ibqp->qp_type == IB_QPT_UD) {
> + if (qp->flags & MLX4_IB_QP_LSO)
> + context->mtu_msgmax = (IB_MTU_4096 << 5) |
> + ilog2(dev->dev->caps.max_gso_sz);
> + else
> + context->mtu_msgmax = (IB_MTU_4096 << 5) | 11;
> + } else if (attr_mask & IB_QP_PATH_MTU) {
> if (attr->path_mtu < IB_MTU_256 || attr->path_mtu > IB_MTU_4096) {
> printk(KERN_ERR "path MTU (%u) is invalid\n",
> attr->path_mtu);
> @@ -1399,6 +1414,34 @@ static void __set_data_seg(struct mlx4_wqe_data_seg *dseg, struct ib_sge *sg)
> dseg->addr = cpu_to_be64(sg->addr);
> }
>
> +static int build_lso_seg(struct mlx4_lso_seg *wqe, struct ib_send_wr *wr,
> + struct mlx4_ib_qp *qp, unsigned *lso_seg_len)
> +{
> + unsigned halign = ALIGN(wr->wr.ud.hlen, 16);
> +
> + /*
> + * This is a temporary limitation and will be removed in
> + * a forthcoming FW release:
> + */
> + if (unlikely(wr->wr.ud.hlen) > 60)
> + return -EINVAL;
> +
> + if (unlikely(!(qp->flags & MLX4_IB_QP_LSO) &&
> + wr->num_sge > qp->sq.max_gs - (halign >> 4)))
> + return -EINVAL;
> +
> + memcpy(wqe->header, wr->wr.ud.header, wr->wr.ud.hlen);
> +
> + /* make sure LSO header is written before overwriting stamping */
> + wmb();
> +
> + wqe->mss_hdr_size = cpu_to_be32((wr->wr.ud.mss - wr->wr.ud.hlen) << 16 |
> + wr->wr.ud.hlen);
> +
> + *lso_seg_len = halign;
> + return 0;
> +}
> +
> int mlx4_ib_post_send(struct ib_qp *ibqp, struct ib_send_wr *wr,
> struct ib_send_wr **bad_wr)
> {
> @@ -1412,6 +1455,7 @@ int mlx4_ib_post_send(struct ib_qp *ibqp, struct ib_send_wr *wr,
> unsigned ind;
> int uninitialized_var(stamp);
> int uninitialized_var(size);
> + unsigned seglen;
> int i;
>
> spin_lock_irqsave(&qp->sq.lock, flags);
> @@ -1490,6 +1534,16 @@ int mlx4_ib_post_send(struct ib_qp *ibqp, struct ib_send_wr *wr,
> set_datagram_seg(wqe, wr);
> wqe += sizeof (struct mlx4_wqe_datagram_seg);
> size += sizeof (struct mlx4_wqe_datagram_seg) / 16;
> +
> + if (wr->opcode == IB_WR_LSO) {
> + err = build_lso_seg(wqe, wr, qp, &seglen);
> + if (err) {
> + *bad_wr = wr;
> + goto out;
> + }
> + wqe += seglen;
> + size += seglen / 16;
> + }
> break;
>
> case IB_QPT_SMI:
> diff --git a/drivers/net/mlx4/fw.c b/drivers/net/mlx4/fw.c
> index f494c3e..d82f275 100644
> --- a/drivers/net/mlx4/fw.c
> +++ b/drivers/net/mlx4/fw.c
> @@ -133,6 +133,7 @@ int mlx4_QUERY_DEV_CAP(struct mlx4_dev *dev, struct mlx4_dev_cap *dev_cap)
> #define QUERY_DEV_CAP_MAX_AV_OFFSET 0x27
> #define QUERY_DEV_CAP_MAX_REQ_QP_OFFSET 0x29
> #define QUERY_DEV_CAP_MAX_RES_QP_OFFSET 0x2b
> +#define QUERY_DEV_CAP_MAX_GSO_OFFSET 0x2d
> #define QUERY_DEV_CAP_MAX_RDMA_OFFSET 0x2f
> #define QUERY_DEV_CAP_RSZ_SRQ_OFFSET 0x33
> #define QUERY_DEV_CAP_ACK_DELAY_OFFSET 0x35
> @@ -215,6 +216,13 @@ int mlx4_QUERY_DEV_CAP(struct mlx4_dev *dev, struct mlx4_dev_cap *dev_cap)
> dev_cap->max_requester_per_qp = 1 << (field & 0x3f);
> MLX4_GET(field, outbox, QUERY_DEV_CAP_MAX_RES_QP_OFFSET);
> dev_cap->max_responder_per_qp = 1 << (field & 0x3f);
> + MLX4_GET(field, outbox, QUERY_DEV_CAP_MAX_GSO_OFFSET);
> + field &= 0x1f;
> + if (!field)
> + dev_cap->max_gso_sz = 0;
> + else
> + dev_cap->max_gso_sz = 1 << field;
> +
> MLX4_GET(field, outbox, QUERY_DEV_CAP_MAX_RDMA_OFFSET);
> dev_cap->max_rdma_global = 1 << (field & 0x3f);
> MLX4_GET(field, outbox, QUERY_DEV_CAP_ACK_DELAY_OFFSET);
> @@ -377,6 +385,7 @@ int mlx4_QUERY_DEV_CAP(struct mlx4_dev *dev, struct mlx4_dev_cap *dev_cap)
> dev_cap->max_sq_desc_sz, dev_cap->max_sq_sg);
> mlx4_dbg(dev, "Max RQ desc size: %d, max RQ S/G: %d\n",
> dev_cap->max_rq_desc_sz, dev_cap->max_rq_sg);
> + mlx4_dbg(dev, "Max GSO size: %d\n", dev_cap->max_gso_sz);
>
> dump_dev_cap_flags(dev, dev_cap->flags);
>
> diff --git a/drivers/net/mlx4/fw.h b/drivers/net/mlx4/fw.h
> index e16dec8..306cb9b 100644
> --- a/drivers/net/mlx4/fw.h
> +++ b/drivers/net/mlx4/fw.h
> @@ -96,6 +96,7 @@ struct mlx4_dev_cap {
> u8 bmme_flags;
> u32 reserved_lkey;
> u64 max_icm_sz;
> + int max_gso_sz;
> };
>
> struct mlx4_adapter {
> diff --git a/drivers/net/mlx4/main.c b/drivers/net/mlx4/main.c
> index 08bfc13..7cfbe75 100644
> --- a/drivers/net/mlx4/main.c
> +++ b/drivers/net/mlx4/main.c
> @@ -159,6 +159,7 @@ static int mlx4_dev_cap(struct mlx4_dev *dev, struct mlx4_dev_cap *dev_cap)
> dev->caps.page_size_cap = ~(u32) (dev_cap->min_page_sz - 1);
> dev->caps.flags = dev_cap->flags;
> dev->caps.stat_rate_support = dev_cap->stat_rate_support;
> + dev->caps.max_gso_sz = dev_cap->max_gso_sz;
>
> return 0;
> }
> diff --git a/include/linux/mlx4/device.h b/include/linux/mlx4/device.h
> index 6cdf813..ff7df1a 100644
> --- a/include/linux/mlx4/device.h
> +++ b/include/linux/mlx4/device.h
> @@ -186,6 +186,7 @@ struct mlx4_caps {
> u32 flags;
> u16 stat_rate_support;
> u8 port_width_cap[MLX4_MAX_PORTS + 1];
> + int max_gso_sz;
> };
>
> struct mlx4_buf_list {
> diff --git a/include/linux/mlx4/qp.h b/include/linux/mlx4/qp.h
> index 31f9eb3..cf0bf4e 100644
> --- a/include/linux/mlx4/qp.h
> +++ b/include/linux/mlx4/qp.h
> @@ -219,6 +219,11 @@ struct mlx4_wqe_datagram_seg {
> __be32 reservd[2];
> };
>
> +struct mlx4_lso_seg {
> + __be32 mss_hdr_size;
> + __be32 header[0];
> +};
> +
> struct mlx4_wqe_bind_seg {
> __be32 flags1;
> __be32 flags2;
More information about the general
mailing list