Re: [PATCH rdma-next v5 08/10] RDMA/mana_ib: Flush and notify CQs when kernel QPs enter ERR

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] RDMA/mana_ib: missing CQ notification for WRs posted after IB_QPS_ERR
- [High] RDMA/mana_ib: lost software flush notification in mana_ib_arm_cq()
- [Medium] RDMA/mana_ib: missing IB_POLL_DIRECT check in mana_ib_post_send_ud()
--

commit 254e0a6be9165a866ac991ad943bce590f88dfb0
Author: Konstantin Taranov <kotaranov@microsoft.com>

RDMA/mana_ib: Flush and notify CQs when kernel QPs enter ERR

This commit links kernel QPs onto the CQ error lists when transitioning to
IB_QPS_ERR and invokes the CQ completion handlers so pending work requests
can be software-flushed.

> diff --git a/drivers/infiniband/hw/mana/cq.c b/drivers/infiniband/hw/mana/cq.c
> index cd77e305c38f9..080f09f3c634e 100644
> --- a/drivers/infiniband/hw/mana/cq.c
> +++ b/drivers/infiniband/hw/mana/cq.c
> @@ -589,7 +589,7 @@ static void mana_drain_gsi_sq(struct mana_ib_qp *qp)
>  		list_add_tail(&qp->send_err_node, &cq->send_err_qp_list);
>  	spin_unlock_irqrestore(&cq->cq_lock, flags);
>  
> -	if (cq->ibcq.comp_handler)
> +	if (cq->ibcq.poll_ctx != IB_POLL_DIRECT && cq->ibcq.comp_handler)
>  		cq->ibcq.comp_handler(&cq->ibcq, cq->ibcq.cq_context);
>  }

[Severity: Medium]
Should mana_ib_post_send_ud() in drivers/infiniband/hw/mana/wr.c also check
for IB_POLL_DIRECT before invoking comp_handler?

With this commit, mana_ib_modify_qp_state() calls mana_add_qp_to_error_cqs()
for any kernel QP entering IB_QPS_ERR, including kernel UD QPs, which links
qp->send_err_node onto send_cq->send_err_qp_list.

When mana_ib_post_send_ud() subsequently posts a WR on that QP, it checks
send_err_node and calls comp_handler without checking poll_ctx:

drivers/infiniband/hw/mana/wr.c:mana_ib_post_send_ud() {
	...
	spin_lock_irqsave(&cq->cq_lock, flags);
	flush_send = !list_empty(&qp->send_err_node);
	spin_unlock_irqrestore(&cq->cq_lock, flags);

	if (flush_send && cq->ibcq.comp_handler)
		cq->ibcq.comp_handler(&cq->ibcq, cq->ibcq.cq_context);
}

If the kernel UD QP uses a send CQ created with IB_POLL_DIRECT, will calling
comp_handler invoke ib_cq_completion_direct() and trigger its WARN_ONCE()?

> diff --git a/drivers/infiniband/hw/mana/qp.c b/drivers/infiniband/hw/mana/qp.c
> index cd2bc39fce6ec..e8c479c96cbf7 100644
> --- a/drivers/infiniband/hw/mana/qp.c
> +++ b/drivers/infiniband/hw/mana/qp.c
> @@ -771,6 +771,27 @@ static int mana_ib_create_uc_qp(struct ib_qp *ibqp, struct ib_pd *ibpd,
>  	return err;
>  }
>  
> +static void mana_add_qp_to_error_cqs(struct mana_ib_qp *qp)
> +{
> +	struct mana_ib_cq *send_cq = container_of(qp->ibqp.send_cq, struct mana_ib_cq, ibcq);
> +	struct mana_ib_cq *recv_cq = container_of(qp->ibqp.recv_cq, struct mana_ib_cq, ibcq);
> +	unsigned long flags;
> +
> +	if (WARN_ON_ONCE(qp->ibqp.uobject))
> +		return;
> +
> +	/* Keep ERR QPs linked until reset/destroy, including later drain WRs. */
> +	spin_lock_irqsave(&send_cq->cq_lock, flags);
> +	if (list_empty(&qp->send_err_node))
> +		list_add_tail(&qp->send_err_node, &send_cq->send_err_qp_list);
> +	spin_unlock_irqrestore(&send_cq->cq_lock, flags);
> +
> +	spin_lock_irqsave(&recv_cq->cq_lock, flags);
> +	if (list_empty(&qp->recv_err_node))
> +		list_add_tail(&qp->recv_err_node, &recv_cq->recv_err_qp_list);
> +	spin_unlock_irqrestore(&recv_cq->cq_lock, flags);
> +}

[Severity: High]
Can work requests posted concurrently with or after the transition to
IB_QPS_ERR be left unpolled in the shadow queues?

Once the QP is in IB_QPS_ERR in hardware, MANA hardware ignores WQ doorbells
for that QP and generates no hardware CQEs for subsequently posted WRs.

Unlike mana_ib_post_send_ud(), which checks !list_empty(&qp->send_err_node)
under cq->cq_lock and invokes cq->ibcq.comp_handler after ringing the
doorbell, mana_ib_post_send_rc(), mana_ib_post_recv_rc(), and
mana_ib_post_recv_ud() in drivers/infiniband/hw/mana/wr.c only queue the WR
to shadow_sq, shadow_mmq, or shadow_rq and ring the doorbell without checking
send_err_node or recv_err_node or invoking comp_handler:

drivers/infiniband/hw/mana/wr.c:mana_ib_post_send_rc() {
	...
	if (ring_sq)
		mana_gd_wq_ring_doorbell(mdev_to_gc(mdev),
					 qp->rc_qp.queues[MANA_RC_SEND_QUEUE_REQUESTER].kmem);

	spin_unlock_irqrestore(&qp->sq_lock, flags);
	return err;
}

At this commit, __ib_drain_sq() and __ib_drain_rq() call
ib_modify_qp(IB_QPS_ERR) and then post the drain WR via ib_post_send() or
ib_post_recv() without calling comp_handler again. If the CQ poll triggered
by mana_ib_notify_error_cqs() runs and finishes mana_ib_poll_cq() before the
drain WR is posted to shadow_sq or shadow_rq, wait_for_completion() hangs.

Even with the later commits in this series ("RDMA/mana_ib: Drain kernel
receive and send queues", which adds custom mana_ib_drain_sq() and
mana_ib_drain_rq() callbacks that call cq->comp_handler after posting the
drain WR, and "RDMA/mana_ib: Handle error CQEs for RC QPs", which adds
mana_ib_qp_error_work), any WR posted concurrently with or after
mana_ib_modify_qp(IB_QPS_ERR) outside mana_ib_drain_sq() or
mana_ib_drain_rq() — for example when mana_ib_qp_error_work asynchronously
moves the QP to IB_QPS_ERR while a ULP is posting WRs, or when a ULP moves
the QP to IB_QPS_ERR and posts a WR directly — will be queued to the shadow
queue after mana_ib_notify_error_cqs() without re-triggering CQ polling.

Should mana_ib_post_send_rc(), mana_ib_post_recv_rc(), and
mana_ib_post_recv_ud() also check whether the QP is on the CQ error list and
notify the CQ, similar to mana_ib_post_send_ud()?

[ ... ]

> @@ -1107,12 +1133,27 @@ static void mana_ib_modify_qp_state(struct ib_qp *ibqp, struct ib_qp_attr *attr,
>  						     SET_ARM_BIT, MANA_PSN_CLIENT_OFFSET);
>  		}
>  	}
> +
> +	return notify;
> +}
> +
> +static void mana_ib_notify_error_cqs(struct mana_ib_qp *qp)
> +{
> +	struct ib_cq *send_cq = qp->ibqp.send_cq;
> +	struct ib_cq *recv_cq = qp->ibqp.recv_cq;
> +
> +	if (send_cq->poll_ctx != IB_POLL_DIRECT && send_cq->comp_handler)
> +		send_cq->comp_handler(send_cq, send_cq->cq_context);
> +	if (recv_cq != send_cq && recv_cq->poll_ctx != IB_POLL_DIRECT &&
> +	    recv_cq->comp_handler)
> +		recv_cq->comp_handler(recv_cq, recv_cq->cq_context);
>  }

[Severity: High]
Can the software flush notification in mana_ib_notify_error_cqs() be lost on
IB_POLL_SOFTIRQ CQs if ib_poll_handler() is concurrently finishing a poll?

For IB_POLL_SOFTIRQ CQs, irq_poll_softirq() keeps IRQ_POLL_F_SCHED set in
cq->iop.state throughout __ib_process_cq() until ib_poll_handler() calls
irq_poll_complete(&cq->iop). Consider the following sequence:

CPU 0                                   CPU 1
ib_poll_handler()
  __ib_process_cq()
    mana_ib_poll_cq()
      mana_flush_completions()
      spin_unlock_irqrestore(&cq->cq_lock)
    /* executing wc->wr_cqe->done() */
                                        mana_ib_modify_qp()
                                          mana_add_qp_to_error_cqs()
                                          mana_ib_notify_error_cqs()
                                            ib_cq_completion_softirq()
                                              irq_poll_sched(&cq->iop)
                                              /* IRQ_POLL_F_SCHED still set,
                                               * returns without scheduling */
  irq_poll_complete(&cq->iop)
  ib_req_notify_cq(cq,
    IB_CQ_NEXT_COMP |
    IB_CQ_REPORT_MISSED_EVENTS)
    mana_ib_arm_cq()

In mana_ib_arm_cq() in drivers/infiniband/hw/mana/cq.c,
IB_CQ_REPORT_MISSED_EVENTS only checks cq->has_pending_cqe:

drivers/infiniband/hw/mana/cq.c:mana_ib_arm_cq() {
	...
	spin_lock_irqsave(&cq->cq_lock, irq_flags);
	mana_ib_cq_doorbell(cq, SET_ARM_BIT);
	if (flags & IB_CQ_REPORT_MISSED_EVENTS)
		missed = cq->has_pending_cqe;
	spin_unlock_irqrestore(&cq->cq_lock, irq_flags);

	return missed ? 1 : 0;
}

Because mana_ib_arm_cq() does not check whether send_err_qp_list or
recv_err_qp_list has unconsumed WQEs in shadow_sq, shadow_mmq, or shadow_rq,
and software-flushed WRs do not generate hardware CQ interrupts, won't
mana_ib_arm_cq() return 0 and leave the error QP's pending WRs (or a drain WR
notified via comp_handler during that window) unpolled?

[ ... ]

>
>base-commit: 978361dd2ccff357d4fe6aac6ce057d1bfae8312

([978361dd2ccf: RDMA/mana_ib: Implement software flushing of RC QPs])
([254e0a6be916: RDMA/mana_ib: Flush and notify CQs when kernel QPs enter ERR])
([daeb2bf3de14: RDMA/mana_ib: Drain kernel receive and send queues])
([33d06de2231e: RDMA/mana_ib: Handle error CQEs for RC QPs])
([a966dc60ab42: RDMA/mana_ib: Report port events for RC])
([acdb8e2b2de9: RDMA/mana_ib: Implement modify_qp for UC])
([763b5797538e: RDMA/mana_ib: Set the correct syndrome properly in CQE])
([32de7988de0a: RDMA/mana_ib: Configure interrupt coalescing timer for RC])
([bc57232325f5: RDMA/mana_ib: Set RC transport capabilities])
([737b4ca9ed86: RDMA/mana_ib: Enable the RC support])

-- 
2.51.0

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