Re: [PATCH rdma-next v8 7/9] RDMA/mana_ib: Poll RC completions using PSN and FSN progress

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] RDMA/mana_ib: early budget return on unsignaled WQEs in mana_advance_send()
--

commit 9f65c018972a66e7f324e13ff6cdbb436c550724
Author: Konstantin Taranov <kotaranov@microsoft.com>

RDMA/mana_ib: Poll RC completions using PSN and FSN progress

This commit adds RC completion polling for receive completions, armed-PSN
notifications, and MMQ completions, retiring requester and MMQ shadow
entries using sequence numbers while honoring fence dependencies and
rearming the next signal PSN on armed completions.

> diff --git a/drivers/infiniband/hw/mana/cq.c b/drivers/infiniband/hw/mana/cq.c
> index 843e2c64b32bc..cd77e305c38f9 100644
> --- a/drivers/infiniband/hw/mana/cq.c
> +++ b/drivers/infiniband/hw/mana/cq.c

[ ... ]

> @@ -390,17 +425,105 @@ static void handle_rq_cqe(struct mana_ib_qp *qp, struct gdma_comp *cqe,
>  	shadow_queue_advance_consumer(&qp->shadow_rq);
>  }
>  
> +/*
> + * A fenced WQE can acknowledge the other send queue. Retire those dependencies
> + * first, without losing the fence when the caller's WC array fills up.
> + */
> +static bool mana_advance_send(struct mana_ib_qp *qp, bool mmq, u32 seq,
> +			      struct mana_cq_poll *poll)
> +{
> +	struct shadow_queue *shadow = mmq ? &qp->shadow_mmq : &qp->shadow_sq;
> +	struct shadow_queue *other = mmq ? &qp->shadow_sq : &qp->shadow_mmq;
> +	struct shadow_wqe_header *shadow_wqe;
> +
> +	while ((shadow_wqe = shadow_queue_get_next_to_consume(shadow))) {
> +		if (PSN_LT(seq, mmq ? shadow_wqe->fsn : shadow_wqe->wqe_offset_or_psn))
> +			break;
> +		if (poll->produced == poll->budget)
> +			return false;
> +		if (shadow_wqe->flags & MANA_WQ_FENCE_WC) {
> +			u32 dependency = mmq ? shadow_wqe->wqe_offset_or_psn : shadow_wqe->fsn;
> +			struct shadow_wqe_header *other_wqe;
> +
> +			while ((other_wqe = shadow_queue_get_next_to_consume(other))) {
> +				if (PSN_LT(dependency, mmq ? other_wqe->wqe_offset_or_psn :
> +							     other_wqe->fsn))
> +					break;
> +				mana_complete_send(qp, !mmq, poll, VENDOR_ERR_OK);
> +				if (poll->produced == poll->budget)
> +					return false;
> +			}
> +		}
> +
> +		mana_complete_send(qp, mmq, poll, VENDOR_ERR_OK);
> +	}
> +
> +	return true;
> +}
> +
> +static bool handle_mmq_cqe(struct mana_ib_qp *qp, u32 fsn,
> +			   struct mana_cq_poll *poll)
> +{
> +	return mana_advance_send(qp, true, fsn, poll);
> +}
> +
> +static bool handle_psn_cqe(struct mana_ib_qp *qp, u32 psn,
> +			   struct mana_cq_poll *poll)
> +{
> +	struct mana_ib_dev *mdev = container_of(qp->ibqp.device, struct mana_ib_dev, ib_dev);
> +	struct shadow_wqe_header *shadow_wqe;
> +	struct gdma_queue *recv_q;
> +	bool consumed;
> +	u32 arm_psn;
> +
> +	if (qp->ibqp.qp_type != IB_QPT_RC)
> +		return true;
> +
> +	recv_q = qp->rc_qp.queues[MANA_RC_RECV_QUEUE_REQUESTER].kmem;
> +
> +	consumed = mana_advance_send(qp, false, psn, poll);
> +	if (consumed) {
> +		shadow_wqe = shadow_queue_get_next_to_signal(&qp->shadow_sq);
> +		arm_psn = shadow_wqe ? shadow_wqe->wqe_offset_or_psn : psn + 1;
> +		arm_psn &= MANA_WQE_OFFSET_MASK;
> +		mana_gd_wq_ring_doorbell_ext(mdev_to_gc(mdev), recv_q, arm_psn,
> +					     SET_ARM_BIT, MANA_PSN_CLIENT_OFFSET);
> +	}
> +
> +	return consumed;
> +}

[Severity: High]
Can mana_advance_send() return false when only unsignaled WQEs remain to be
retired, causing handle_psn_cqe() to skip rearming the requester PSN
doorbell?

In mana_complete_send(), when vendor_error is VENDOR_ERR_OK, mana_fill_wc()
is only called when MANA_WQ_NO_SIGNAL_WC is not set:

	if (vendor_error || !(wqe->flags & MANA_WQ_NO_SIGNAL_WC))
		mana_fill_wc(qp, poll, wqe, wqe->send_opcode, vendor_error);

Unsignaled WQEs do not consume an entry in poll->wc or increment
poll->produced.

However, mana_advance_send() checks poll->produced == poll->budget
unconditionally at the start of the outer loop before checking whether
shadow_wqe has MANA_WQ_NO_SIGNAL_WC set, and again at the end of the inner
fence loop right after mana_complete_send(qp, !mmq, poll, VENDOR_ERR_OK).

If a caller posts a signaled WR followed by an unsignaled WR that is
acknowledged by the same CQE (for example, a signaled IB_WR_REG_MR on MMQ
followed by a fenced unsignaled SQ WR, or a signaled SQ WR followed by an
unsignaled SQ WR covered by the same CQE_TYPE_ARMED_CMPL) and polls with
budget equal to the number of signaled WRs, completing the signaled WR
brings poll->produced to poll->budget. Before retiring the remaining
acknowledged unsignaled WQE, mana_advance_send() sees poll->produced ==
poll->budget and returns false.

When that happens via mana_handle_cqe() -> handle_psn_cqe(), consumed is
false, so handle_psn_cqe() skips calling mana_gd_wq_ring_doorbell_ext() on
recv_q and leaves cq->has_pending_cqe set even though no signaled WCs remain
for that CQE. If the caller waits for completions of subsequent signaled WRs
before polling again, hardware will not generate another CQE_TYPE_ARMED_CMPL
and the RC send queue can stall.

Should mana_advance_send() only check poll->produced == poll->budget when the
next WQE to be completed does not have MANA_WQ_NO_SIGNAL_WC set?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103111729.729886-1-kotaranov@linux.microsoft.com?part=7