Received-SPF: pass (google.com: domain of linux-nfs+bounces-1574-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Mon, 29 Jan 2024 12:26:14 -0500
From: Chuck Lever <chuck.lever@oracle.com>
To: Chuck Lever <cel@kernel.org>
Cc: linux-nfs@vger.kernel.org, linux-rdma@vger.kernel.org
Subject: Re: [PATCH v1 02/11] svcrdma: Use all allocated Send Queue entries
Message-ID: <ZbffthImvL3YspVT@tissot.1015granger.net>
References: <170653967395.24162.4661804176845293777.stgit@manet.1015granger.net>
 <170653984365.24162.652127313173673494.stgit@manet.1015granger.net>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <170653984365.24162.652127313173673494.stgit@manet.1015granger.net>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: 
	=?us-ascii?Q?BsvTYjFj27U3rxfWw+qf+J5eVP7TUbqmBvqBuArZ7H17acX05Fb975OmUJsV?=
 =?us-ascii?Q?jah7MqSjD0gi1zyMfjCSx9rhhDaqPA63KZCJGi8eOexB+9Ugi/M0kfKiwt1p?=
 =?us-ascii?Q?CewR5TACuzhYKpOMoRU8hCchVuqUwwwprbaTEQsJUwbHEZkWZ434k49DhyBl?=
 =?us-ascii?Q?uFlzsrXapNFcOWdeKtwPr68n5EPIZjh23bVQ9aY2awkdTbvguO6+qADUBVUb?=
 =?us-ascii?Q?kGhk2juaGmXO6vFI++UKeWlj5KZFqrnkv4GkNeLxKgVezdLrfoV9PYXAJ6aw?=
 =?us-ascii?Q?G4BaZ3Z+lwnePSX+KsuNRCn0rKLQmUNBwYlYMWa+oUe8NzXfxOam+VoJamJh?=
 =?us-ascii?Q?RFd/HC6HQBSJoT1k2dzCfPu6GfOxToACI20e5lLbe15R0hIal0bmemJjLlQu?=
 =?us-ascii?Q?I+vCW54yTJoJnBFAgHHxy7WZUcM3btGiPkuy5vnHMj5LFqkipFSOIgfKZ4U5?=
 =?us-ascii?Q?oiBBA401FINwIgrNKhi3kQUIEEHPgkKBKbjsl1hlBhyrlR8w3I9IqNwUJH30?=
 =?us-ascii?Q?d8qIm7TZvd9htwx5ql913TqPPHg7MxujoBLMMDMpWIvlwAQom+DoVh9y9IQo?=
 =?us-ascii?Q?E+oX4rm88OUhzg76kU52jE6JDLLNSPkBlgwPJcoPVc7gD2gmOqfbzHj8PUYc?=
 =?us-ascii?Q?+WkXRhMTO4y9gpLkDBuuaWmi2tQWrPECcJqTkb4xiEwaDJWPL0ke1RSHPKQo?=
 =?us-ascii?Q?QZLcGemsAC1EFs4KShIB8NWSw/eU+g5i2dgVVEsSD8/FeUEQzPzLBb2x1Bac?=
 =?us-ascii?Q?XgyEcq7eVAsTVC+CiHP4AkWzEU/LXVRz8OAJtCQtHmXYFyPqD+zxbTZgxKH+?=
 =?us-ascii?Q?vfIiZQuWsuGv05mNqert3mb4wqOnsm4LRETIVOuXeXQBd/kruulJ1zhnBSpf?=
 =?us-ascii?Q?VDPAsM/BTlyYBsxHekkgQUrczk2ia4yQx8XU2PpZfN5uCsfpMHP9YIC57I7E?=
 =?us-ascii?Q?LI2r0NLRUWWqEaim5URVeTP8dfOiYzR7q5IWa5CXyr25t0lndPYXR9iyeX+A?=
 =?us-ascii?Q?OdB6qK0c5YMe0e7XDa6X/w1UWVjimfNGEWtonZVi4GRVEdttte4foxYj3+ll?=
 =?us-ascii?Q?5pw+NNtLpbkQ4EKL9S/BCXI9qSOLwrXo+MR2mM6mY911dm0yqCiAeF4cxxc0?=
 =?us-ascii?Q?aVDt5BWkFYqEShd8KR7Tj+ZEZb4cwM/I8dbBoQoOZ/BM4obamAMEqEZ0PoDZ?=
 =?us-ascii?Q?mbwoNe919Z3ThjdmkjGCxtQKyf8b4rlynb26Neomw4CHwO8sA0cVvrCJIlt/?=
 =?us-ascii?Q?0Pem/VlcVzy1dsmD/LMwjxwLM+m512CtHR2LJ+wnKPHC6KUe5qMlu8u7r1Zd?=
 =?us-ascii?Q?ZaxvwNPfk7V3MJg+k7KkDxPH8rN4ad63FSHMfI3wH6RXVoYglixqY8Ovg0db?=
 =?us-ascii?Q?OIUv3s/Jb8gG1gtQYniix5oiSAcEdbE5MwK3DQ9DvRg3Y5HpP/Jt3me6+MI/?=
 =?us-ascii?Q?sLHTJfVgS5uIzllz6W0qRApV3qYvRO/uNWjd/J+OLNVl3eQzOIT95NhirQdN?=
 =?us-ascii?Q?8PfeCutVhwoB1Sdb/LQyu5s2QE7FhAPlZ9Q2YF2aqa3MkllaS0s9PFz2gZ8n?=
 =?us-ascii?Q?VlQPZzCv+sJObgBfsYW9IVtITzhmGk78SuRi1WRaggcJCaOqg4i5ajbKxs2p?=
 =?us-ascii?Q?lw=3D=3D?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	UUarHrJ4s0clhB4krj/NI7jYYCymt6D+iHbLM3pO1NNKRGlUfc8+ebYUd0N8kxVHzbK3cwXtSx44xpZ/sjTmnDGNVuzv9R2LuArv2dHgzh9AnA0ADPAvlD1/OW1MZa/5SfiD93Qo5azP6vaiNd3Iv0UmbRfQVdNHk+zky2U6UwU8Cp5IogplqVQagI2vpaXQVJgwzgeVPcWO/BDgTyxTjJS7H8y1G7mDCf3FZ3KpW161tqDq/pLvnCk6SDA4qUCqq5xLUd8KlRNWht8UBpqHyeRS8tNX4TbEsLkPdqeHrqfoTySdoKljAJC8fhg6L9hUdgR3iKgq0VNX8JLQYj/BYwrNOAdsY8+9J36M9YouTEOitltswbzcTZnbF8Fwp03lE82hISDfN7d3Zbj51yjgb5sP9zMnArRsXsXq4m1NwcnSnjO4NfGvGWAKrK1ewmMRVnfGzuTFJaxkVsghndwU+3MNiwLHbjFBx4FqpWuo5e5d6qVi6S2LqWFUj10GVmGFq8n1IKz1MALRpzbSCuh/WwpDUc1JMps65f1WNBR7osAZREtwOMv5nh81fBVKGx7RAGLXjZBVEyVdvQN000SdDH7lZ1On+fiCgeKsTYwZNHk=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: ace2e084-f08b-44d7-0ebe-08dc20ef6687
X-MS-Exchange-CrossTenant-AuthSource: BN0PR10MB5128.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 29 Jan 2024 17:26:17.0914
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: iBYyvD67Vw24ca+0N+tSgFPWGE4NCcGDKQ2FS56321W8Oe0SkvhyBiMMdImu4Wsaz7rTHpHJn/vsq+S9pPJutg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY5PR10MB5985
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.272,Aquarius:18.0.1011,Hydra:6.0.619,FMLib:17.11.176.26
 definitions=2024-01-29_10,2024-01-29_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 spamscore=0 bulkscore=0 malwarescore=0
 suspectscore=0 phishscore=0 mlxscore=0 mlxlogscore=999 adultscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2311290000
 definitions=main-2401290128
X-Proofpoint-GUID: upAF-4wF2MttkN6ZqL7GaoPjumUeAXPH
X-Proofpoint-ORIG-GUID: upAF-4wF2MttkN6ZqL7GaoPjumUeAXPH

On Mon, Jan 29, 2024 at 09:50:43AM -0500, Chuck Lever wrote:
> From: Chuck Lever <chuck.lever@oracle.com>
> 
> 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@oracle.com>
> ---
>  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