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

[Sahil Chandna <sahilchandna@linux.microsoft.com>]

On 03-11-2026 03:16, Paolo Abeni wrote:
> On 11/1/26 09:58, netdev-bot+sashiko@kernel.org wrote:
>> 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?
> I understand from discussion on v3 that this will be handled separately,
> am I correct?
> 
Yes, that is correct. I have discussed this already with Wei Hu for RDMA
CQ handling.

Regards,
Sahil
> /P