Re: [PATCH net-next V2 1/2] net/mlx5e: Batch NAPI RCU grace periods on channel close

[Carolina Jubran <cjubran@nvidia.com>]

On 12/11/2026 20:50, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential
> issue(s) to consider.
>
> Critical: 0 · High: 2 · 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] PTP SQ recovery loses the RCU/NAPI grace period that used to
>    separate the PTP napi_disable from freeing the PTP CQs, which can…
> - [High] net/mlx5e: missing grace period after page_pool_destroy() in
>    channel close
> - [Low] The patch adds an unmentioned change in XSK teardown state
>    tracking and several copies of teardown sequences that will have to be…
>
> --
>
> net/mlx5e: Batch NAPI RCU grace periods on channel close
>
> This patch splits mlx5e channel deactivate and close into pre_sync and
> post_sync phases, so one synchronize_net() covers all channels instead
> of one per queue. NAPI instances are unlinked with
> __netif_napi_del_locked(), and a single grace period runs before the
> channels are freed. Single-queue paths keep their per-object
> synchronize_net() calls.
>
>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
>> index c3c2c5720c6f3..73b504ba8c8c6 100644
>> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> [ ... ]
>
>> @@ -2936,35 +2993,62 @@ static void mlx5e_activate_channel(struct mlx5e_channel *c)
> [ ... ]
>
>> -static void mlx5e_close_channel(struct mlx5e_channel *c)
>> +static void mlx5e_close_channel_pre_sync(struct mlx5e_channel *c)
>>   {
>>   	if (test_bit(MLX5E_CHANNEL_STATE_XSK, c->state))
>> -		mlx5e_close_xsk(c);
>> -	mlx5e_close_queues(c);
>> -	mlx5e_qos_close_queues(c);
>> -	netif_napi_del_locked(&c->napi);
>> +		mlx5e_close_xsk_pre_sync(c);
>> +
>> +	if (c->xdp)
>> +		mlx5e_close_xdpsq_pre_sync(&c->rq_xdpsq);
>>   
>> +	if (c->xdpsq)
>> +		mlx5e_close_xdpsq_pre_sync(c->xdpsq);
>> +}
>> +
>> +static void mlx5e_close_channel_post_sync(struct mlx5e_channel *c)
>> +{
>> +	if (c->xskrq.xsk_pool)
>> +		mlx5e_close_xsk_post_sync(c);
>> +	mlx5e_close_queues_post_sync(c);
>> +	mlx5e_qos_close_queues_post_sync(c);
>> +}
> [Severity: Low]
> This isn't a functional bug, but the XSK teardown decision here now uses
> c->xskrq.xsk_pool instead of MLX5E_CHANNEL_STATE_XSK. That is because
> mlx5e_close_channel_pre_sync() clears the state bit before post_sync
> runs.
>
> For the new check to work, the error path in mlx5e_open_xsk() in
> en/xsk/setup.c now does:
>
> 	c->xskrq.xsk_pool = NULL;
> 	c->xsksq.xsk_pool = NULL;
>
> The commit message mentions neither change. Could it explain that
> xsk_pool is now the liveness flag for XSK teardown?
It's documented in a code comment next to the change, which I think is
the right place for it.
>
> The patch also adds several copies of existing teardown sequences:
>
>    - mlx5e_close_queues_post_sync() is a near line-by-line copy of
>      mlx5e_close_queues(). The original is still used by the
>      err_close_queues path in mlx5e_open_channel().
>
>    - mlx5e_close_channels_post_sync() open-codes mlx5e_ptp_close() as
>      mlx5e_ptp_close_queues() plus kvfree(). mlx5e_ptp_close() is still
>      used by the error handling in mlx5e_open_channels() and by the PTP
>      recovery in en/reporter_tx.c.
>
>    - mlx5e_close_locked() open-codes the same sequence as
>      mlx5e_deactivate_priv_channels() followed by
>      mlx5e_close_channels(), with the same two grace periods.
>
> If a resource is later added to only one copy, either the batched close
> path or the error/recovery path will skip its cleanup.
>
> Could the single-object variants be written as pre_sync, then
> synchronize_net(), then post_sync? And could mlx5e_close_locked() keep
> calling the existing wrappers?
>
> [ ... ]

I will change in v3.

>> @@ -3060,32 +3144,67 @@ static int mlx5e_wait_channels_min_rx_wqes(struct mlx5e_channels *chs)
>>   	return err ? -ETIMEDOUT : 0;
>>   }
>>   
>> -static void mlx5e_deactivate_channels(struct mlx5e_channels *chs)
>> +static void mlx5e_deactivate_channels_pre_sync(struct mlx5e_channels *chs)
>>   {
>>   	int i;
>>   
>>   	if (chs->ptp)
>> -		mlx5e_ptp_deactivate_channel(chs->ptp);
>> +		mlx5e_ptp_deactivate_channel_pre_sync(chs->ptp);
>>   
>>   	for (i = 0; i < chs->num; i++)
>> -		mlx5e_deactivate_channel(chs->c[i]);
>> +		mlx5e_deactivate_channel_pre_sync(chs->c[i]);
>>   }
>>   
>> -void mlx5e_close_channels(struct mlx5e_channels *chs)
>> +static void mlx5e_deactivate_channels_post_sync(struct mlx5e_channels *chs)
>> +{
>> +	int i;
>> +
>> +	if (chs->ptp)
>> +		mlx5e_ptp_deactivate_channel_post_sync(chs->ptp);
>> +
>> +	for (i = 0; i < chs->num; i++)
>> +		mlx5e_deactivate_channel_post_sync(chs->c[i]);
>> +}
> [Severity: High]
> Does freeing the channels here need another grace period after the
> page pools are destroyed?
>
> mlx5e_alloc_rq() creates rq->page_pool with pp_params.napi set to
> rq->cq.napi. That is &c->napi for regular channels and &ptp->napi for
> PTP, and both are embedded in the structs freed here.
>
> When an in-flight skb backed by that pool is freed in softirq on
> another CPU, page_pool_napi_local() does READ_ONCE(pool->p.napi) and
> then reads napi->list_owner without any lock.
>
> pool->p.napi is only cleared in page_pool_disable_direct_recycling().
> That runs from page_pool_destroy(), reached from mlx5e_close_rq()
> through mlx5e_free_rq().
>
> Before this patch, mlx5e_close_channel() ran mlx5e_close_queues()
> first and only then called netif_napi_del_locked(). Its
> synchronize_net() covered any softirq that had loaded the old
> pool->p.napi before kvfree(c) ran.
>
> In the batched path, the only grace period runs between
> mlx5e_close_channels_pre_sync() and mlx5e_close_channels_post_sync().
> At that point pool->p.napi still points at the embedded napi.
>
> post_sync then calls mlx5e_ptp_close_queues() and
> mlx5e_close_channel_post_sync(), which clear pool->p.napi. It then
> immediately calls kvfree() on chs->ptp and chs->c[i].
>
> page_pool_scrub() only calls synchronize_net() when
> dma_dev_need_sync() is true. So on DMA-coherent platforms nothing
> waits here.
>
> A softirq that read pool->p.napi just before it was cleared can then
> read list_owner from freed memory. If the stale value matches the
> current CPU, it would also take the direct recycling path into a pool
> that is being torn down.
>
> This affects mlx5e_close_locked() and every mlx5e_close_channels()
> caller, for example ip link set down or a channel count change. The
> single-channel mlx5e_close_channel() still does
> netif_napi_del_locked() after post_sync, so it looks unaffected.
>
> Would it work to destroy all the queues in post_sync, issue a second
> synchronize_net(), and only then kvfree() the channels and PTP? That
> keeps the cost at a fixed number of grace periods. Or is the
> page_pool core meant to provide this guarantee, and I am missing it?

Right. In v3 I unlink the page pools in pre_sync, so the existing
synchronize_net() covers this.

>
> [Severity: High]
> Is the PTP SQ recovery path still safe after this change?
>
> mlx5e_tx_reporter_ptpsq_unhealthy_recover() in en/reporter_tx.c does:
>
> 	mlx5e_deactivate_priv_channels(priv);
>
> 	mlx5e_ptp_close(chs->ptp);
>
> Before this patch, mlx5e_deactivate_channels() ran
> mlx5e_ptp_deactivate_channel() first, which ends in
> napi_disable_locked(). It then ran mlx5e_deactivate_channel() for each
> channel. Those calls did several synchronize_net() calls through
> mlx5e_deactivate_rq(), mlx5e_deactivate_icosq() and
> mlx5e_deactivate_txqsq(). So a grace period always followed the PTP
> napi_disable_locked().
>
> Now the single synchronize_net() in mlx5e_deactivate_priv_channels()
> runs before post_sync. mlx5e_ptp_deactivate_channel_post_sync() calls
> napi_disable_locked(), and nothing waits after it.
>
> mlx5e_ptp_close() then frees the queues before netif_napi_del_locked():
>
> 	mlx5e_ptp_close_queues(c);
> 	netif_napi_del_locked(&c->napi);
>
> 	kvfree(c);
>
> The call chain mlx5e_ptp_close_queues()->mlx5e_close_cq()->
> mlx5e_free_cq()->mlx5_wq_destroy() frees the CQ buffer, and frees the
> doorbell record through mlx5_db_free().
>
> Meanwhile, mlx5e_ptp_napi_poll() keeps running under rcu_read_lock()
> after napi_complete_done():
>
> 	if (unlikely(!napi_complete_done(napi, work_done)))
> 		goto out;
>
> 	ch_stats->arm++;
>
> 	if (test_bit(MLX5E_PTP_STATE_TX, c->state)) {
> 		for (i = 0; i < c->num_tc; i++) {
> 			mlx5e_cq_arm(&c->ptpsq[i].txqsq.cq);
> 			mlx5e_cq_arm(&c->ptpsq[i].ts_cq);
>
> napi_disable_locked() only waits for NAPIF_STATE_SCHED and
> NAPIF_STATE_NPSVC to clear. Can it return while the poller is still in
> this arm section?
>
> If so, mlx5e_cq_arm() could write through mcq->arm_db into a doorbell
> record that has been freed, or that mlx5_db_alloc() has already given
> to another queue. It would also ring the UAR for a destroyed CQN.
>
> The other callers of mlx5e_deactivate_priv_channels() look fine:
> mlx5e_switch_priv_channels(), the ipoib paths and mlx5e_queue_start()
> go on to mlx5e_close_channels() or mlx5e_close_channel(), and both call
> synchronize_net() before freeing. So this seems limited to the PTP
> recovery path.
>
> The later patch in this series ("net/mlx5e: Batch ICOSQ trigger RCU
> synchronization") does not touch this path either.
>
> Would either of these close the window?
>
>    - Add synchronize_net() before mlx5e_ptp_close() in the recovery
>      path.
>
>    - Have mlx5e_ptp_close() call __netif_napi_del_locked() and
>      synchronize_net() before mlx5e_ptp_close_queues(), as the batched
>      close path now does.
>
> [ ... ]

In v3 mlx5e_ptp_close() calls __netif_napi_del_locked() and 
synchronize_net()
before freeing the queues.


Thanks!

pw-bot: cr