Re: [PATCH net v4] net: mana: Add support to process throttled EQEs
From: Paolo Abeni
Date: Mon Nov 02 2026 - 16:47:12 EST
On 11/1/26 09:58, netdev-bot+sashiko@xxxxxxxxxx wrote:
Thank you for your contribution! Sashiko AI review found 2 potentialI understand from discussion on v3 that this will be handled separately,
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?
am I correct?
/P