On Mon, Jan 29, 2024 at 09:50:43AM -0500, Chuck Lever wrote:
> From: Chuck Lever <chuck.lever@xxxxxxxxxx>
>
> For upper layer protocols that request rw_ctxs, ib_create_qp()
> adjusts ib_qp_init_attr::max_send_wr to accommodate the WQEs those
> rw_ctxs will consume. See rdma_rw_init_qp() for details.
>
> To actually use those additional WQEs, svc_rdma_accept() needs to
> retrieve the corrected SQ depth after calling rdma_create_qp() and
> set newxprt->sc_sq_depth and newxprt->sc_sq_avail so that
> svc_rdma_send() and svc_rdma_post_chunk_ctxt() can utilize those
> WQEs.
>
> The NVMe target driver, for example, already does this properly.
>
> Fixes: 26fb2254dd33 ("svcrdma: Estimate Send Queue depth properly")
> Signed-off-by: Chuck Lever <chuck.lever@xxxxxxxxxx>
> ---
> net/sunrpc/xprtrdma/svc_rdma_transport.c | 36 ++++++++++++++++++------------
> 1 file changed, 22 insertions(+), 14 deletions(-)
After applying this one, workload tests trigger this set of errors
on my test NFS server (CX-5 in RoCE mode):
mlx5_core 0000:02:00.0: cq_err_event_notifier:517:(pid 4374): CQ error on CQN 0x407, syndrome 0x1
rocep2s0/1: QP 137 error: unrecognized status (0x41 0x0 0x0)
I think syndrome 0x1 is IB_EVENT_QP_FATAL ?
But the "unrecognized status" comes from mlx5_ib_qp_err_syndrome(),
which does this:
344 pr_err("%s/%d: QP %d error: %s (0x%x 0x%x 0x%x)\n",
345 ibqp->device->name, ibqp->port, ibqp->qp_num,
346 ib_wc_status_msg(
347 MLX5_GET(cqe_error_syndrome, err_syn, syndrome)),
348 MLX5_GET(cqe_error_syndrome, err_syn, vendor_error_syndrome),
349 MLX5_GET(cqe_error_syndrome, err_syn, hw_syndrome_type),
350 MLX5_GET(cqe_error_syndrome, err_syn, hw_error_syndrome));
I don't think the "syndrome" field contains a WC status code, so
invoking ib_wc_status_msg() to get a symbolic string seems wrong.
Anyway,
- Can someone with an mlx5 decoder ring tell me what 0x41 is?
- If someone sees an obvious error with how this patch has
set up the SQ and Send CQ, please hit me with a clue bat.
> diff --git a/net/sunrpc/xprtrdma/svc_rdma_transport.c b/net/sunrpc/xprtrdma/svc_rdma_transport.c
> index 4a038c7e86f9..75f1481fbca0 100644
> --- a/net/sunrpc/xprtrdma/svc_rdma_transport.c
> +++ b/net/sunrpc/xprtrdma/svc_rdma_transport.c
> @@ -370,12 +370,12 @@ static struct svc_xprt *svc_rdma_create(struct svc_serv *serv,
> */
> static struct svc_xprt *svc_rdma_accept(struct svc_xprt *xprt)
> {
> + unsigned int ctxts, rq_depth, sq_depth;
> struct svcxprt_rdma *listen_rdma;
> struct svcxprt_rdma *newxprt = NULL;
> struct rdma_conn_param conn_param;
> struct rpcrdma_connect_private pmsg;
> struct ib_qp_init_attr qp_attr;
> - unsigned int ctxts, rq_depth;
> struct ib_device *dev;
> int ret = 0;
> RPC_IFDEBUG(struct sockaddr *sap);
> @@ -422,24 +422,29 @@ static struct svc_xprt *svc_rdma_accept(struct svc_xprt *xprt)
> newxprt->sc_max_requests = rq_depth - 2;
> newxprt->sc_max_bc_requests = 2;
> }
> + sq_depth = rq_depth;
> +
> ctxts = rdma_rw_mr_factor(dev, newxprt->sc_port_num, RPCSVC_MAXPAGES);
> ctxts *= newxprt->sc_max_requests;
> - newxprt->sc_sq_depth = rq_depth + ctxts;
> - if (newxprt->sc_sq_depth > dev->attrs.max_qp_wr)
> - newxprt->sc_sq_depth = dev->attrs.max_qp_wr;
> - atomic_set(&newxprt->sc_sq_avail, newxprt->sc_sq_depth);
>
> newxprt->sc_pd = ib_alloc_pd(dev, 0);
> if (IS_ERR(newxprt->sc_pd)) {
> trace_svcrdma_pd_err(newxprt, PTR_ERR(newxprt->sc_pd));
> goto errout;
> }
> - newxprt->sc_sq_cq = ib_alloc_cq_any(dev, newxprt, newxprt->sc_sq_depth,
> +
> + /* The Completion Queue depth is the maximum number of signaled
> + * WRs expected to be in flight. Every Send WR is signaled, and
> + * each rw_ctx has a chain of WRs, but only one WR in each chain
> + * is signaled.
> + */
> + newxprt->sc_sq_cq = ib_alloc_cq_any(dev, newxprt, sq_depth + ctxts,
> IB_POLL_WORKQUEUE);
> if (IS_ERR(newxprt->sc_sq_cq))
> goto errout;
> - newxprt->sc_rq_cq =
> - ib_alloc_cq_any(dev, newxprt, rq_depth, IB_POLL_WORKQUEUE);
> + /* Every Receive WR is signaled. */
> + newxprt->sc_rq_cq = ib_alloc_cq_any(dev, newxprt, rq_depth,
> + IB_POLL_WORKQUEUE);
> if (IS_ERR(newxprt->sc_rq_cq))
> goto errout;
>
> @@ -448,7 +453,7 @@ static struct svc_xprt *svc_rdma_accept(struct svc_xprt *xprt)
> qp_attr.qp_context = &newxprt->sc_xprt;
> qp_attr.port_num = newxprt->sc_port_num;
> qp_attr.cap.max_rdma_ctxs = ctxts;
> - qp_attr.cap.max_send_wr = newxprt->sc_sq_depth - ctxts;
> + qp_attr.cap.max_send_wr = sq_depth;
> qp_attr.cap.max_recv_wr = rq_depth;
> qp_attr.cap.max_send_sge = newxprt->sc_max_send_sges;
> qp_attr.cap.max_recv_sge = 1;
> @@ -456,17 +461,20 @@ static struct svc_xprt *svc_rdma_accept(struct svc_xprt *xprt)
> qp_attr.qp_type = IB_QPT_RC;
> qp_attr.send_cq = newxprt->sc_sq_cq;
> qp_attr.recv_cq = newxprt->sc_rq_cq;
> - dprintk(" cap.max_send_wr = %d, cap.max_recv_wr = %d\n",
> - qp_attr.cap.max_send_wr, qp_attr.cap.max_recv_wr);
> - dprintk(" cap.max_send_sge = %d, cap.max_recv_sge = %d\n",
> - qp_attr.cap.max_send_sge, qp_attr.cap.max_recv_sge);
> -
> ret = rdma_create_qp(newxprt->sc_cm_id, newxprt->sc_pd, &qp_attr);
> if (ret) {
> trace_svcrdma_qp_err(newxprt, ret);
> goto errout;
> }
> + dprintk("svcrdma: cap.max_send_wr = %d, cap.max_recv_wr = %d\n",
> + qp_attr.cap.max_send_wr, qp_attr.cap.max_recv_wr);
> + dprintk(" cap.max_send_sge = %d, cap.max_recv_sge = %d\n",
> + qp_attr.cap.max_send_sge, qp_attr.cap.max_recv_sge);
> + dprintk(" send CQ depth = %d, recv CQ depth = %d\n",
> + sq_depth, rq_depth);
> newxprt->sc_qp = newxprt->sc_cm_id->qp;
> + newxprt->sc_sq_depth = qp_attr.cap.max_send_wr;
> + atomic_set(&newxprt->sc_sq_avail, newxprt->sc_sq_depth);
>
> if (!(dev->attrs.device_cap_flags & IB_DEVICE_MEM_MGT_EXTENSIONS))
> newxprt->sc_snd_w_inv = false;
>
>
>
--
Chuck Lever
