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

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 · Medium: 1 · 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] Incomplete fix: a throttle EQE never services RDMA (mana_ib)
  CQs, so the CQ-stall bug the patch claims to fix is still reachable.
- [Medium] mana_gd_destroy_cq() now calls synchronize_net() for every CQ
  that was attached to a parent EQ, which is every mana_en TX and RX CQ.
- [Low] Claimed race: mana_create_txq() and mana_create_rxq() write
  gc->cq_table[] (mana_en.c:2855, 3181) before netif_napi_add_*() (2860,…

--

net: mana: Add support to process throttled EQEs

This patch handles GDMA_EQE_THROTTLE (type 4) events. Each EQ records
up to two child CQs, and their callbacks are invoked when a throttle EQE
arrives. It also advertises a new driver capability bit, adds an
eq_throttle_events ethtool counter, and adds RCU grace period waits to
the CQ and HWC teardown paths.

> diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c b/drivers/net/ethernet/microsoft/mana/gdma_main.c
> index 8e9bfc1d6a2ae..d74ed00bb7834 100644
> --- a/drivers/net/ethernet/microsoft/mana/gdma_main.c
> +++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c
> @@ -904,6 +904,27 @@ ssize_t mana_gd_read_ring(struct gdma_queue *q, char __user *buf,
>  	return copied;
>  }
>  
> +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;
> +
> +		if (cq->cq.callback)
> +			cq->cq.callback(cq->cq.context, cq);

[Severity: Low]
This may not be reachable today, but is cq_table membership enough to
show that the CQ is ready for its callback?

mana_create_txq() and mana_create_rxq() publish the CQ in cq_table
before they add NAPI. For example, in mana_create_rxq():

    gc->cq_table[cq->gdma_id] = cq->gdma_cq;

    netif_napi_add_weight_locked(ndev, &cq->napi, mana_poll, 1);

If a GDMA_EQE_THROTTLE arrived on the parent EQ in that window, this
loop would call mana_schedule_napi()->napi_schedule_irqoff() on a
napi_struct that is not initialized yet.

In mana_alloc_queues() the EQs are freshly created. During the RX
window, the only other CQ on EQ i is TX CQ i. That CQ carries no
traffic while port_is_up is still false, so with the throttle semantics
described in the commit message this looks unreachable. It does depend
on that ordering, and on the hardware never sending a throttle EQE
without pending completions.

Would it be more robust to publish the cq_table entry only after
netif_napi_add_*() has run?

> +	}
> +}

[ ... ]

> @@ -1220,25 +1246,49 @@ 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;
> +		}
>  }

[Severity: High]
Can a throttle EQE leave the mana_ib CQs unserviced?

child_cq[] is only filled here, and only when spec->cq.parent_eq is
set. In practice that covers the mana_en TX/RX CQs and the HWC CQ.

The RDMA CQs never get here with a parent:

- mana_ib_create_kernel_queue() in drivers/infiniband/hw/mana/main.c
  builds its spec as below, so parent_eq is NULL:

    struct gdma_queue_spec spec = {};

- User CQs from mana_ib_create_queue() never go through
  mana_gd_create_cq().

- mana_ib_install_cq_cb() only does:

    gc->cq_table[cq->queue.id] = gdma_cq;

The hardware still binds these CQs to GDMA EQs. In
mana_ib_gd_create_cq():

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

and in mana_ib_create_qp_rss() and mana_ib_create_qp_raw():

    cq_spec.attached_eq = eq->eq->id;

where those EQs are created by mana_ib_cfg_vport() when RDMA owns the
vport.

These EQs are serviced by mana_gd_intr()->mana_gd_process_eqe(). On a
GDMA_EQE_THROTTLE, mana_gd_schedule_eq_cqs() walks an all-NULL
child_cq[], and mana_ib_cq_handler() never runs.

Wouldn't that leave RDMA consumers waiting on comp_handler with
completions pending? That is the same queue stall the commit message
describes.

GDMA_DRV_CAP_FLAG_1_THROTTLED_EVENT_QUEUE is added to
GDMA_DRV_CAP_FLAGS1. mana_gd_verify_vf_version() sends that once for
the whole device, which mana_en and mana_ib share. If firmware only
emits type 4 EQEs after this bit is set, does this add a new stall for
RDMA CQs? If firmware always emits them, is the fix incomplete for
those EQs?

Separately, once both GDMA_EQ_MAX_CHILD_CQ slots are taken, the loop
above returns without recording the CQ and without any warning. Many
RDMA CQs can share one comp_vector EQ, and that seems to be the case
most likely to fill an EQ and trigger throttling.

Would this need a per-EQ list of CQs, or a walk over every CQ whose
parent is this EQ, rather than a fixed 2-entry array?

>  
>  static void mana_gd_destroy_cq(struct gdma_context *gc,
>  			       struct gdma_queue *queue)
>  {

[ ... ]

> -	gc->cq_table[id] = NULL;
> +	/* Wait for irq reader holding the cq */
> +	if (attached)
> +		synchronize_net();
>  }

[Severity: Medium]
Does this add one RCU grace period per CQ to every port teardown?

Every mana_en TX and RX CQ is attached to a parent EQ, and they are
destroyed one at a time:

mana_dealloc_queues()
  mana_destroy_txq()        /* loops over apc->num_queues */
    mana_deinit_cq()
      mana_gd_destroy_queue()
        mana_gd_destroy_cq()
          synchronize_net()

The same happens for each RX CQ through
mana_destroy_rxqs()->mana_destroy_rxq().

These paths run under RTNL: mana_close(), and mana_detach() from
ethtool -L/-G, MTU changes, XDP setup, and queue reset. Under RTNL,
synchronize_net() does:

    if (from_cleanup_net() || rtnl_is_locked())
        synchronize_rcu_expedited();

With MANA_MAX_NUM_QUEUES at 64, one teardown can run up to 128
expedited grace periods back to back while RTNL is held.

The HWC path also waits twice: once here for its CQ, and once in the
new synchronize_rcu() in mana_hwc_destroy_channel().

Could the child_cq[] and cq_table entries be cleared for all CQs first,
with a single grace period before the queues are freed?

[ ... ]

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