Re: [PATCH net v4] net: mana: Add support to process throttled EQEs

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 1 (1) · Medium: 0 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The patch tells firmware that every EQ on the function handles
  throttled EQEs.
- [Low] The patch marks only one side of the concurrent accesses.

Pre-existing issues:
- [High] mana_gd_destroy_cq() waits for a grace period
  (synchronize_net()) only when mana_gd_detach_cq() returns true,…

--

net: mana: Add support to process throttled EQEs

This commit handles GDMA_EQE_THROTTLE events. It records up to two child
CQs per EQ and calls their callbacks when a throttle EQE arrives. CQ
teardown now detaches the CQ from its parent EQ and from cq_table[]
before a grace period. The patch also advertises a new capability bit and
adds a per-EQ throttle counter to ethtool stats.

> diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c b/drivers/net/ethernet/microsoft/mana/gdma_main.c
> index 8c915e0087846..ab942d5bfe058 100644
> --- a/drivers/net/ethernet/microsoft/mana/gdma_main.c
> +++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c
> @@ -757,6 +757,27 @@ void mana_gd_free_service_wq(struct gdma_context *gc)
>  	gc->service_wq = NULL;
>  }
>  
> +static void mana_gd_schedule_eq_cqs(struct gdma_queue *eq)
> +{
> +	struct gdma_context *gc = eq->gdma_dev->gdma_context;
> +	struct gdma_queue *cq;
> +	u8 i;
> +
> +	for (i = 0; i < GDMA_EQ_MAX_CHILD_CQ; i++) {
> +		cq = rcu_dereference(eq->eq.child_cq[i]);
> +
> +		if (!cq)
> +			continue;
> +
> +		if (cq->id >= READ_ONCE(gc->max_num_cqs) ||
> +		    !gc->cq_table || gc->cq_table[cq->id] != cq)
> +			continue;

[Severity: Low]
This patch marks only the write side. mana_gd_detach_cq() clears
gc->cq_table[id] with WRITE_ONCE(), and mana_hwc_destroy_channel() now
sets gc->max_num_cqs with WRITE_ONCE().

The readers still use plain loads. Here that means gc->cq_table and
gc->cq_table[cq->id]. The GDMA_EQE_COMPLETION branch of
mana_gd_process_eqe() also reads both fields without READ_ONCE():

	if (WARN_ON_ONCE(cq_id >= gc->max_num_cqs))
		break;

	cq = gc->cq_table[cq_id];

Could KCSAN report these as data races? Should the readers use
READ_ONCE() to pair with the marked writers?

This is mostly an annotation issue. The completion-path race was already
there before this patch, and the loads in mana_gd_schedule_eq_cqs() only
compare the pointer value.

> +
> +		if (cq->cq.callback)
> +			cq->cq.callback(cq->cq.context, cq);
> +	}
> +}

[ ... ]

> @@ -1073,25 +1104,66 @@ static void mana_gd_create_cq(const struct gdma_queue_spec *spec,
>  			      struct gdma_queue *queue)
>  {
>  	u32 log2_num_entries = ilog2(spec->queue_size / GDMA_CQE_SIZE);
> +	struct gdma_queue *parent;
> +	u8 i;
>  
>  	queue->head |= INITIALIZED_OWNER_BIT(log2_num_entries);
> -	queue->cq.parent = spec->cq.parent_eq;
> +	parent = spec->cq.parent_eq;
> +	queue->cq.parent = parent;
>  	queue->cq.context = spec->cq.context;
>  	queue->cq.callback = spec->cq.callback;
> +
> +	if (!parent)
> +		return;
> +
> +	/* For throttled EQE store the child CQ */
> +	for (i = 0; i < GDMA_EQ_MAX_CHILD_CQ; i++) {
> +		if (!rcu_access_pointer(parent->eq.child_cq[i])) {
> +			rcu_assign_pointer(parent->eq.child_cq[i], queue);
> +			return;
> +		}
> +	}
> +
> +	WARN_ONCE(1, "mana: EQ %u has no free child CQ slot\n", parent->id);
>  }

[Severity: High]
Do mana_ib CQs ever end up in child_cq[]?

This is the only place that writes child_cq[], and it returns early when
spec->cq.parent_eq is NULL. mana_ib_create_kernel_queue() uses a zeroed
spec:

drivers/infiniband/hw/mana/main.c:mana_ib_create_kernel_queue() {
	struct gdma_queue_spec spec = {};
	...
	err = mana_gd_create_mana_wq_cq(mdev->gdma_dev, &spec, &queue->kmem);
	...
}

mana_ib_gd_create_cq() then binds the RNIC CQ in hardware to an mana_ib
EQ:

	req.eq_id = mdev->eqs[cq->comp_vector]->id;

mana_ib_install_cq_cb() adds it only to gc->cq_table[].

mana_ib_create_qp_rss() and mana_ib_create_qp_raw() attach raw and RSS
QP CQs to the per-vPort mana_en EQs:

	eq = &mpc->eqs[cq->comp_vector % mpc->num_queues];
	cq_spec.attached_eq = eq->eq->id;

These CQs are also registered only through mana_ib_install_cq_cb().

The patch also adds GDMA_DRV_CAP_FLAG_1_THROTTLED_EVENT_QUEUE to
GDMA_DRV_CAP_FLAGS1, which mana_gd_verify_vf_version() sends for the
whole function.

Suppose hardware folds one of these CQs' completions into a
GDMA_EQE_THROTTLE. mana_gd_schedule_eq_cqs() walks child_cq[], which is
empty on mdev->eqs and holds only the mana_en TX and RX CQs on mpc->eqs.
mana_ib_cq_handler() never runs, and nothing polls or re-arms the CQ.

Wouldn't that stall the RDMA CQ the same way the commit message
describes for TX?

A fixed two-entry child_cq[] also cannot represent an EQ shared by many
RDMA CQs on the same comp_vector.

The commit message says:

  "A mana_en EQ serves one TX CQ and one RX CQ, and the HWC EQ serves
  the HWC CQ."

Is that still true once raw QPs attach CQs to mpc->eqs?

If advertising the bit is what turns on throttle EQEs, this patch adds a
new RDMA stall. If it is not, the RDMA case stays unfixed. Could the
commit message cover mana_ib in either case?

> -static void mana_gd_destroy_cq(struct gdma_context *gc,
> -			       struct gdma_queue *queue)
> +/* Unlink queue from its parent EQ's child_cq[] and from gc->cq_table[].
> + *
> + * Return: true if queue was in the parent EQ's child_cq[]. In that case the
> + * caller must wait for an RCU grace period before freeing queue, because
> + * mana_gd_intr() may still be handling a throttle EQE for it.
> + */
> +bool mana_gd_detach_cq(struct gdma_context *gc, struct gdma_queue *queue)
>  {

[ ... ]

> +	/* Check if CQ might have been detached already, only clear if
> +	 * it still points to same queue .
> +	 */
> +	if (id < gc->max_num_cqs && READ_ONCE(gc->cq_table[id]) == queue)
> +		WRITE_ONCE(gc->cq_table[id], NULL);
>  
> -	if (!gc->cq_table[id])
> -		return;
> +	return attached;
> +}
>  
> -	gc->cq_table[id] = NULL;
> +static void mana_gd_destroy_cq(struct gdma_context *gc,
> +			       struct gdma_queue *queue)
> +{
> +	/* Wait for irq reader holding the cq */
> +	if (mana_gd_detach_cq(gc, queue))
> +		synchronize_net();
>  }

[Severity: High]
This isn't a bug introduced by this patch, but mana_gd_detach_cq() also
clears gc->cq_table[id]. The GDMA_EQE_COMPLETION branch of
mana_gd_process_eqe() reads that table and calls the callback under the
rcu_read_lock() in mana_gd_intr():

	cq = gc->cq_table[cq_id];
	...
	if (cq->cq.callback)
		cq->cq.callback(cq->cq.context, cq);

mana_gd_destroy_cq() only waits when the CQ was in child_cq[]. An
mana_ib kernel (kmem) CQ has parent_eq NULL, so detach returns false and
no grace period runs:

mana_ib_destroy_cq()
  mana_ib_remove_cq_cb()       <- returns early for kmem
  mana_ib_gd_destroy_cq()
  mana_ib_destroy_queue()
    mana_gd_destroy_queue(kmem)
      mana_gd_destroy_cq()     <- clears cq_table[id], no wait

Suppose an IRQ on another CPU has already loaded the pointer from
cq_table[]. Can it then call mana_ib_cq_handler() on a freed gdma_queue
and mana_ib_cq?

The old mana_gd_destroy_cq() also cleared the slot without a grace
period, and drivers/infiniband/hw/mana has no synchronize_rcu() or
call_rcu(). However, the new kernel-doc and the "Wait for irq reader
holding the cq" comment now describe a rule that leaves out the
cq_table[] reader.

User CQs look similar. mana_ib_remove_cq_cb() frees the entry before it
clears the slot:

	kfree(gc->cq_table[cq->queue.id]);
	gc->cq_table[cq->queue.id] = NULL;

Should the grace period depend on removal from cq_table[] instead of
child_cq[] membership?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261028084020.2051254-1-sahilchandna%40linux.microsoft.com