Re: [PATCH] RDMA/siw: avoid hiding errors in siw_post_send()

[Bernard Metzler <bernard.metzler@linux.dev>]

On 04.09.2025 19:36, Stefan Metzmacher wrote:
> When we hit situations like "sq full", "too many sge's" or similar
> things while queueing sqes, we should remember the error before
> rv = siw_activate_tx(qp); clears the return value.
> 
> Currently we hide such errors and confuse the caller
> which is waiting (most likely forever) for a post completion
> to arrive.
> 
I agree with that overall observation. Thanks for pointing it out.

> Also if we already queued some sqes with success we need to process
This is what we already do. But we hide the initial error.

One tricky question is what to return if we have both an immediate
error and an error happening during starting/processing the send queue.
I think it is best to return the immediate error in that case since
the caller relates it with the bad_wr content. The caller may
even understand that if bad_wr is not assigned it is not an
immediate error.


> them as no error happened, but at the end we need to return the
> error relative to the bad_wr to the caller.
> 
> Cc: Bernard Metzler <bernard.metzler@linux.dev>
> Cc: linux-rdma@vger.kernel.org
> Fixes: 303ae1cdfdf7 ("rdma/siw: application interface")
> Signed-off-by: Stefan Metzmacher <metze@samba.org>
> ---
>   drivers/infiniband/sw/siw/siw_verbs.c | 20 ++++++++++++++++++++
>   1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/infiniband/sw/siw/siw_verbs.c b/drivers/infiniband/sw/siw/siw_verbs.c
> index 556a2b4b42ed..a3f548652c37 100644
> --- a/drivers/infiniband/sw/siw/siw_verbs.c
> +++ b/drivers/infiniband/sw/siw/siw_verbs.c
> @@ -770,6 +770,8 @@ int siw_post_send(struct ib_qp *base_qp, const struct ib_send_wr *wr,
>   
>   	unsigned long flags;
>   	int rv = 0;
> +	size_t num_queued = 0;

I don't like counters which are actually used as boolean.
Introducing an immediate error value and assigning it if needed
should be sufficient.


> +	int error = 0;
>   
>   	if (wr && !rdma_is_kernel_res(&qp->base_qp.res)) {
>   		siw_dbg_qp(qp, "wr must be empty for user mapped sq\n");
> @@ -948,9 +950,24 @@ int siw_post_send(struct ib_qp *base_qp, const struct ib_send_wr *wr,
>   		sqe->flags |= SIW_WQE_VALID;
>   
>   		qp->sq_put++;
> +		num_queued++;
>   		wr = wr->next;
>   	}
>   
> +	if (unlikely(rv < 0)) {
> +		/*
> +		 * If at least one was queued
> +		 * we should start the tx, but
> +		 * still return an error with
> +		 * the bad_wr at the end.
> +		 */
> +		error = rv;
> +		if (num_queued == 0) {
> +			spin_unlock_irqrestore(&qp->sq_lock, flags);
> +			goto skip_direct_sending;
> +		}
> +	}
> +
>   	/*
>   	 * Send directly if SQ processing is not in progress.
>   	 * Eventual immediate errors (rv < 0) do not affect the involved
> @@ -982,6 +999,9 @@ int siw_post_send(struct ib_qp *base_qp, const struct ib_send_wr *wr,
>   
>   	up_read(&qp->state_lock);
>   
> +	if (unlikely(error != 0))
> +		rv = error;
> +
>   	if (rv >= 0)
>   		return 0;
>   	/*

I unfortunately do not have access to build/test infrastructure until
September 18th, but just a source tree and an editor. Sorry about that.
What I would have in mind as a good patch is the following below
(untested, maybe even with spelling errors). Maybe you can take it from
there and test?

Thanks very much!
Bernard.

diff --git a/drivers/infiniband/sw/siw/siw_verbs.c b/drivers/infiniband/sw/siw/siw_verbs.c
index 2b2a7b8e93b0..7780e39b4e3e 100644
--- a/drivers/infiniband/sw/siw/siw_verbs.c
+++ b/drivers/infiniband/sw/siw/siw_verbs.c
@@ -769,7 +769,7 @@ int siw_post_send(struct ib_qp *base_qp, const struct ib_send_wr *wr,
	struct siw_wqe *wqe = tx_wqe(qp);

	unsigned long flags;
-	int rv = 0;
+	int rv = 0, imm_err = 0;

	if (wr && !rdma_is_kernel_res(&qp->base_qp.res)) {
		siw_dbg_qp(qp, "wr must be empty for user mapped sq\n");
@@ -958,6 +958,14 @@ int siw_post_send(struct ib_qp *base_qp, const struct ib_send_wr *wr,
	 * processing, if new work is already pending. But rv must be passed
	 * to caller.
	 */
+	if (unlikely(rv < 0)) {
+		/*
+		 * Immediate error
+		 */
+		siw_dbg_qp(qp, "Immediate error %d\n", rv);
+		imm_err = rv;
+		*bad_wr = wr;
+	}
	if (wqe->wr_status != SIW_WR_IDLE) {
		spin_unlock_irqrestore(&qp->sq_lock, flags);
		goto skip_direct_sending;
@@ -982,15 +990,10 @@ int siw_post_send(struct ib_qp *base_qp, const struct ib_send_wr *wr,

	up_read(&qp->state_lock);

-	if (rv >= 0)
-		return 0;
-	/*
-	 * Immediate error
-	 */
-	siw_dbg_qp(qp, "error %d\n", rv);
+	if unlikely(imm_err)
+		return imm_err;

-	*bad_wr = wr;
-	return rv;
+	return (rv >= 0) ? 0 : rv;
  }

  /*