Re: [PATCH rdma-next v6 10/10] RDMA/mana_ib: Drain kernel receive and send queues

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] RDMA/mana_ib: redundant comp_handler calls and duplicated drain logic in qp.c
--

commit 5e7a469ad596532936441484def77d5229b5cbff
Author: Konstantin Taranov <kotaranov@microsoft.com>

RDMA/mana_ib: Drain kernel receive and send queues

This commit implements the drain_sq and drain_rq device operations for
mana_ib by transitioning RC QPs to the error state, posting a marker work
request, and waiting for its completion.

> diff --git a/drivers/infiniband/hw/mana/qp.c b/drivers/infiniband/hw/mana/qp.c
> index 1756e96b9c811..4854fc4c6d0dc 100644
> --- a/drivers/infiniband/hw/mana/qp.c
> +++ b/drivers/infiniband/hw/mana/qp.c
> @@ -1398,3 +1398,106 @@ int mana_ib_destroy_qp(struct ib_qp *ibqp, struct ib_udata *udata)
>  
>  	return -ENOENT;
>  }
> +
> +struct mana_ib_drain_cqe {
> +	struct ib_cqe cqe;
> +	struct completion done;
> +};
> +
> +static void mana_ib_drain_qp_done(struct ib_cq *cq, struct ib_wc *wc)
> +{
> +	struct mana_ib_drain_cqe *cqe = container_of(wc->wr_cqe,
> +						     struct mana_ib_drain_cqe,
> +						     cqe);
> +	complete(&cqe->done);
> +}
> +
> +void mana_ib_drain_sq(struct ib_qp *ibqp)
> +{
> +	struct ib_qp_attr attr = { .qp_state = IB_QPS_ERR };
> +	struct mana_ib_drain_cqe sdrain;
> +	struct ib_rdma_wr swr = {
> +		.wr = {
> +			.next = NULL,
> +			.wr_cqe = &sdrain.cqe,
> +			.opcode = IB_WR_SEND,
> +		},
> +	};
> +	const struct ib_send_wr *bad_swr;
> +	struct ib_cq *cq = ibqp->send_cq;
> +	int ret;
> +
> +	if (ibqp->qp_type != IB_QPT_RC) {
> +		ibdev_err(ibqp->device, "SQ drain is only supported for RC QPs (type %u)\n",
> +			  ibqp->qp_type);
> +		return;
> +	}
> +
> +	ret = ib_modify_qp(ibqp, &attr, IB_QP_STATE);
> +	if (ret) {
> +		ibdev_err(ibqp->device, "Failed to drain SQ: modify QP returned %d\n", ret);
> +		return;
> +	}
> +
> +	sdrain.cqe.done = mana_ib_drain_qp_done;
> +	init_completion(&sdrain.done);
> +
> +	ret = ib_post_send(ibqp, &swr.wr, &bad_swr);
> +	if (ret) {
> +		ibdev_err(ibqp->device, "Failed to post SQ drain WR: %d\n", ret);
> +		return;
> +	}
> +
> +	if (cq->poll_ctx != IB_POLL_DIRECT && cq->comp_handler)
> +		cq->comp_handler(cq, cq->cq_context);
> +
> +	if (cq->poll_ctx == IB_POLL_DIRECT) {
> +		while (!wait_for_completion_timeout(&sdrain.done, HZ / 10))
> +			ib_process_cq_direct(cq, -1);
> +	} else {
> +		wait_for_completion(&sdrain.done);
> +	}
> +}
> +
> +void mana_ib_drain_rq(struct ib_qp *ibqp)
> +{
> +	struct ib_qp_attr attr = { .qp_state = IB_QPS_ERR };
> +	struct mana_ib_drain_cqe rdrain;
> +	struct ib_recv_wr rwr = {
> +		.next = NULL,
> +		.wr_cqe = &rdrain.cqe,
> +	};
> +	struct ib_cq *cq = ibqp->recv_cq;
> +	int ret;
> +
> +	if (ibqp->qp_type != IB_QPT_RC) {
> +		ibdev_err(ibqp->device, "RQ drain is only supported for RC QPs (type %u)\n",
> +			  ibqp->qp_type);
> +		return;
> +	}
> +
> +	ret = ib_modify_qp(ibqp, &attr, IB_QP_STATE);
> +	if (ret) {
> +		ibdev_err(ibqp->device, "Failed to drain RQ: modify QP returned %d\n", ret);
> +		return;
> +	}
> +
> +	rdrain.cqe.done = mana_ib_drain_qp_done;
> +	init_completion(&rdrain.done);
> +
> +	ret = ib_post_recv(ibqp, &rwr, NULL);
> +	if (ret) {
> +		ibdev_err(ibqp->device, "Failed to post RQ drain WR: %d\n", ret);
> +		return;
> +	}
> +
> +	if (cq->poll_ctx != IB_POLL_DIRECT && cq->comp_handler)
> +		cq->comp_handler(cq, cq->cq_context);
> +
> +	if (cq->poll_ctx == IB_POLL_DIRECT) {
> +		while (!wait_for_completion_timeout(&rdrain.done, HZ / 10))
> +			ib_process_cq_direct(cq, -1);
> +	} else {
> +		wait_for_completion(&rdrain.done);
> +	}
> +}

[Severity: Low]
Are custom mana_ib_drain_sq() and mana_ib_drain_rq() callbacks needed in
mana_ib_dev_ops, and does calling cq->comp_handler() here invoke the
completion handler twice?

When ib_modify_qp() transitions the QP to IB_QPS_ERR,
mana_ib_modify_qp_state() calls mana_add_qp_to_error_cqs() to link
qp->send_err_node and qp->recv_err_node onto the CQ error lists.

Then, when ib_post_send() and ib_post_recv() run, mana_ib_post_send_rc()
and mana_ib_post_recv_rc() already call mana_ib_notify_flush(), which
invokes ibcq->comp_handler(ibcq, ibcq->cq_context) when ibcq->poll_ctx is
not IB_POLL_DIRECT. Calling cq->comp_handler() again right after
ib_post_send() and ib_post_recv() triggers a redundant second invocation.

More broadly, since mana_ib_modify_qp(), mana_ib_post_sq() (which supports
0-SGE IB_WR_RDMA_WRITE WRs), and mana_ib_post_rq() already handle software
flushing and CQ notification via mana_ib_notify_flush(), could mana_ib
simply rely on the generic __ib_drain_sq() and __ib_drain_rq() fallbacks in
drivers/infiniband/core/verbs.c instead of duplicating them here (which
also restricts drain_rq to IB_QPT_RC even though mana_ib_post_recv_ud()
supports flushing UD and GSI receive queues)?

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