Re: [PATCH net 2/3] net: hns3: fix interface stuck after OOM with full hardware rx queue

[Jijie Shao <shaojijie@huawei.com>]

on 2026/9/19 23:37, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 6 potential
> issue(s) to consider:
>
> Critical: 0 · High: 0 · Medium: 5 · Low: 1
>
> - [Medium] The changelog misdescribes the pre-patch allocation-failure
>    handling of hns3_clean_rx_ring() and hides that the patch removes the…
> - [Medium] rx_oom_cnt is exported as a per-RX-queue counter (offset
>    inside struct hns3_enet_ring, printed as rxqN_...), but the state that…
> - [Medium] The new per-RX-queue allocation-failure counter is exported as
>    a driver-private ethtool -S string (HNS3_TQP_STAT("rx_oom_cnt",…
> - [Medium] In hns3_clean_rx_ring() the newly added `if (unlikely(err ==
>    -ENOMEM)) failure = true;` (hns3_enet.c:4507-4508) poisons the pre-…
> - [Medium] hns3_oom_task() returns early when HNS3_NIC_STATE_DOWN is set
>    (hns3_enet.c:4466-4467) without ever reaching the…
> - [Low] The per-vector OOM bit is read with hns3_ring_is_oom_state() at
>    hns3_enet.c:4531 and then unconditionally re-set by…
>
> --
>
> net: hns3: fix interface stuck after OOM with full hardware rx queue
>
> This patch adds a 250ms delayed_work that re-arms NAPI for vectors whose rx
> rings hit an allocation failure, tracked in a new per-vector bitmap. It also
> changes hns3_clean_rx_ring() to always return recv_pkts and adds a new
> rx_oom_cnt ethtool counter.
>
> A few questions below.
>
>> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
>> index 2f3aeade558cd..0c088feae03ca 100644
>> --- a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
>> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
> [ ... ]
>
>> @@ -4451,6 +4455,34 @@ static int hns3_handle_rx_bd(struct hns3_enet_ring *ring)
>>   	return 0;
>>   }
>>   
>> +static void hns3_oom_task(struct work_struct *work)
>> +{
>> +	struct hns3_nic_priv *priv = container_of(work, struct hns3_nic_priv,
>> +						  oom_task.work);
>> +	struct net_device *netdev = priv->netdev;
>> +	struct hnae3_handle *h = priv->ae_handle;
>> +	u16 i;
>> +
>> +	if (test_bit(HNS3_NIC_STATE_DOWN, &priv->state))
>> +		return;
> [Severity: Medium]
>
> Can this early return leave bits set in priv->oom_vector_bm forever?
>
> This is the only function that ever clears the bitmap, and the DOWN check
> returns before the test_and_clear_bit() loop. The up path does not reset it
> either, hns3_nic_net_up() only does:
>
> 	clear_bit(HNS3_NIC_STATE_DOWN, &priv->state);
>
> and priv->oom_vector_bm is zeroed only when hns3_nic_alloc_vector_data()
> reallocates it, which does not happen on a plain ifdown/ifup.
>
> Bits can be set while DOWN is set: hns3_lb_check_rx_ring() in hns3_ethtool.c
> calls hns3_clean_rx_ring() during the loopback selftest with the netdev
> stopped, and a work item queued just before hns3_nic_net_stop() will run
> after DOWN is set.
>
> After the interface comes back up, the new tail test in hns3_clean_rx_ring()
>
> 	if (unlikely(failure || hns3_ring_is_oom_state(ring)))
> 		hns3_oom_task_schedule(ring);
>
> then evaluates true on every poll of that vector, bumping rx_oom_cnt,
> re-setting the bit and re-queueing the work each time, until the work
> finally executes. Would clearing the bitmap in hns3_nic_net_up(), or
> clearing the bits before the DOWN early return, avoid that?

Yes, will be fixed in V2.

>
>> +
>> +	netif_dbg(h, rx_err, netdev, "oom napi_schedule 0x%*pb\n",
>> +		  priv->vector_num, priv->oom_vector_bm);
>> +	for (i = 0; i < priv->vector_num; i++)
>> +		if (test_and_clear_bit(i, priv->oom_vector_bm))
>> +			napi_schedule(&priv->tqp_vector[i].napi);
>> +}
>> +
>> +static void hns3_oom_task_schedule(struct hns3_enet_ring *ring)
>> +{
>> +	struct hns3_nic_priv *priv = netdev_priv(ring_to_netdev(ring));
>> +
>> +	hns3_ring_stats_update(ring, rx_oom_cnt);
>> +	hns3_ring_set_oom_state(ring);
>> +	schedule_delayed_work(&priv->oom_task,
>> +			      msecs_to_jiffies(HNS3_OOM_POLL_INTERVAL_MS));
>> +}
> [Severity: Medium]
>
> Is rx_oom_cnt charged to rings that never failed an allocation?
>
> The counter lives in struct hns3_enet_ring and is exported per rx queue, but
> the state that gates the increment is per vector, since
> hns3_ring_set_oom_state() and hns3_ring_is_oom_state() index
> priv->oom_vector_bm with ring->tqp_vector->idx.
>
> One vector commonly serves several rx rings:
>
> hns3_nic_alloc_vector_data()
> 	vector_num = min_t(u16, num_online_cpus(), tqp_num);
>
> hns3_nic_init_vector_data()
> 		u16 vector_i = i % priv->vector_num;
>
> and hns3_nic_common_poll() cleans all rx rings of the vector via
> hns3_for_each_ring(ring, tqp_vector->rx_group). So after ring A fails,
> hns3_clean_rx_ring() for sibling ring B sees hns3_ring_is_oom_state(ring)
> true and calls hns3_oom_task_schedule(ring), which increments B's
> rx_oom_cnt.
>
> Since the bit stays set until the work runs about 250 ms later, does the
> same single failure get re-counted on every poll in that window, making the
> value track polling frequency rather than the number of failures?
>
> Also, hns3_nic_alloc_rx_buffers() collapses all refill errors into its
> boolean return, including the dma_mapping_error() path:
>
> hns3_map_buffer()
> 	if (unlikely(dma_mapping_error(ring_to_dev(ring), cb->dma)))
> 		return -EIO;
>
> so should a mapping failure be reported as an OOM event? Bumping the
> counter at the actual allocation failure sites instead would avoid both
> issues.

You are right on both counts. The counter increments on every poll while the bit
is set (tracking polling frequency, not failure events), and sibling rings in
N:1 vector-to-ring configs are charged for failures they did not experience.

Will be fixed in v2. The counter will increment only on the 0→1 bitmap
transition using test_and_set_bit().

>
>> +
>>   int hns3_clean_rx_ring(struct hns3_enet_ring *ring, int budget,
>>   		       void (*rx_fn)(struct hns3_enet_ring *, struct sk_buff *))
>>   {
>> @@ -4472,6 +4504,9 @@ int hns3_clean_rx_ring(struct hns3_enet_ring *ring, int budget,
>>   
>>   		/* Poll one pkt */
>>   		err = hns3_handle_rx_bd(ring);
>> +		if (unlikely(err == -ENOMEM))
>> +			failure = true;
>> +
> [Severity: Medium]
>
> Does setting failure here skip the buffer refill and the head doorbell
> write at the exit block?
>
> The only -ENOMEM reaching here comes from hns3_alloc_skb(), which leaves
> ring->skb NULL:
>
> 	if (unlikely(!skb)) {
> 		hns3_rl_err(netdev, "alloc rx skb fail\n");
> 		hns3_ring_stats_update(ring, sw_err_cnt);
>
> 		return -ENOMEM;
>
> so control goes straight to the out: label with failure already true, and
> the short circuit there never evaluates the right operand:
>
> out:
> 	/* sync head pointer before exiting, since hardware will calculate
> 	 * FBD number with head pointer
> 	 */
> 	if (unused_count > 0)
> 		failure = failure ||
> 			  hns3_nic_alloc_rx_buffers(ring, unused_count);
>
> That means the up to 15 unused descriptors are not replenished and the
> HNS3_RING_RX_RING_HEAD_REG write inside hns3_nic_alloc_rx_buffers() does not
> happen for that poll, contrary to the comment above it. Buffer refill uses
> the page_pool cache or dev_alloc_pages() rather than napi_alloc_skb(), so it
> could still have succeeded.
>
> Before this patch, failure being true at out: implied
> hns3_nic_alloc_rx_buffers() had already run and written the head pointer.
> Would setting failure after the out: block, or using a separate variable
> for the head SKB failure, keep the refill attempt?


Correct. The skipped refill targets only pre-existing empty slots. The next
NAPI poll (re-armed by the delayed work) will attempt the refill again. Under
severe memory pressure the allocation may fail anyway.

No change in v2.


>
>>   		/* Do not get FE for the packet or failed to alloc skb */
>>   		if (unlikely(!ring->skb || err == -ENXIO)) {
>>   			goto out;
>> @@ -4493,7 +4528,10 @@ int hns3_clean_rx_ring(struct hns3_enet_ring *ring, int budget,
>>   		failure = failure ||
>>   			  hns3_nic_alloc_rx_buffers(ring, unused_count);
>>   
>> -	return failure ? budget : recv_pkts;
>> +	if (unlikely(failure || hns3_ring_is_oom_state(ring)))
>> +		hns3_oom_task_schedule(ring);
>> +
>> +	return recv_pkts;
>>   }
> [Severity: Medium]
>
> The changelog says:
>
>    "Also fix hns3_clean_rx_ring() to return recv_pkts instead of budget
>     on allocation failure, preventing NAPI busy-poll under low memory."
>
> Was the removed return really a bug? git blame attributes
> return failure ? budget : recv_pkts; to 68752b24f51a7 ("net: hns3: schedule
> the polling again when allocation fails"), which added it so that a buffer
> allocation failure reports an incomplete poll and keeps NAPI running:
>
> hns3_nic_common_poll()
> 		int rx_cleaned = hns3_clean_rx_ring(ring, rx_budget,
> 						    hns3_rx_skb);
> 		if (rx_cleaned >= rx_budget)
> 			clean_complete = false;
>
> With this change, the retry for hns3_nic_alloc_rx_buffers() failures moves
> from the very next poll to up to 250 ms later, and only if the delayed work
> actually runs. Could the changelog state that the return value change
> supersedes the recovery added by 68752b24f51a7, and that the two changes are
> now coupled, since the patch carries a Fixes: tag?
>
> Is the described deadlock also broader than what the code shows? Before this
> patch, only the napi_alloc_skb() failure in hns3_add_frag() returns -ENXIO
> with failure left false, so that was the single path that could complete
> NAPI with the ring undrained, which matches Fixes: 81ae0e0491f3. Refill
> failures kept polling.

Right — the return failure ? budget : recv_pkts was intentionally added by
68752b24f51a7 as a busy-poll recovery mechanism. The delayed_work supersedes
it; returning budget would cause the CPU spinning we're trying to avoid.

Will be fixed in v2.

>
> [Severity: Low]
>
> Is the read of the bit here and the set inside hns3_oom_task_schedule() racy
> against the work function?
>
> hns3_ring_is_oom_state() tests the bit, then hns3_ring_set_oom_state()
> unconditionally sets it again, as two separate non-atomic operations, while
> hns3_oom_task() does test_and_clear_bit() on another CPU. A poll that read
> the bit as true before the clear can set it again after the clear,
> resurrecting a request with no allocation failure behind it, so the work
> fires again 250 ms later, clears it and issues another napi_schedule().
>
> Would setting the bit only at the real allocation failure sites and gating
> this branch on failure alone, so that hns3_oom_task() is the only writer
> that clears and nobody re-sets without a failure, remove the resurrection
> cycle? Making the state per ring instead of per vector would have the same
> effect.

This race does exist. The worst-case effect is one spurious napi_schedule()
call, which is harmless. Acceptable given the simplicity of the per-vector
bitmap design.

No change in v2.

>
>>   
>>   static void hns3_update_rx_int_coalesce(struct hns3_enet_tqp_vector *tqp_vector)
> [ ... ]
>
>> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.h b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.h
>> index 933e3527ed824..27a09629cbcab 100644
>> --- a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.h
>> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.h
> [ ... ]
>
>> @@ -711,6 +714,20 @@ static inline unsigned int hns3_page_order(struct hns3_enet_ring *ring)
>>   #define hns3_rl_usec_to_reg(int_rl) ((int_rl) >> 2)
>>   #define hns3_rl_round_down(int_rl) round_down(int_rl, 4)
>>   
>> +static inline void hns3_ring_set_oom_state(struct hns3_enet_ring *ring)
>> +{
>> +	struct hns3_nic_priv *priv = netdev_priv(ring_to_netdev(ring));
>> +
>> +	set_bit(ring->tqp_vector->idx, priv->oom_vector_bm);
>> +}
>> +
>> +static inline bool hns3_ring_is_oom_state(struct hns3_enet_ring *ring)
>> +{
>> +	struct hns3_nic_priv *priv = netdev_priv(ring_to_netdev(ring));
>> +
>> +	return test_bit(ring->tqp_vector->idx, priv->oom_vector_bm);
>> +}
>> +
>>   void hns3_ethtool_set_ops(struct net_device *netdev);
> [ ... ]
>
>> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c b/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
>> index e7318f2363157..392653635bda2 100644
>> --- a/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
>> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
>> @@ -62,6 +62,7 @@ static const struct hns3_stats hns3_rxq_stats[] = {
>>   	HNS3_TQP_STAT("non_reuse_pg", non_reuse_pg),
>>   	HNS3_TQP_STAT("frag_alloc_err", frag_alloc_err),
>>   	HNS3_TQP_STAT("frag_alloc", frag_alloc),
>> +	HNS3_TQP_STAT("rx_oom_cnt", rx_oom_cnt),
>>   };
> [Severity: Medium]
>
> Should this new per-rx-queue allocation failure counter go through the
> standard netlink queue statistics instead of a private ethtool -S string?
>
> struct netdev_queue_stats_rx in include/net/netdev_queues.h already has:
>
> 	u64 alloc_fail;
>
> exposed as rx-alloc-fail and documented in
> Documentation/netlink/specs/netdev.yaml as "Number of times skb or buffer
> allocation failed on the Rx datapath. Allocation failure may, or may not
> result in a packet drop...", which matches what rx_oom_cnt counts.
>
> hns3 has no netdev_stat_ops/get_queue_stats_rx implementation today, so this
> adds a driver-private string for a counter category that
> Documentation/networking/statistics.rst assigns to netdev generic netlink.
> Would implementing get_queue_stats_rx() and filling stats->alloc_fail be
> preferable to adding the ethtool string?

Agreed in principle — struct netdev_queue_stats_rx.alloc_fail is the right
interface. Introducing get_queue_stats_rx() touches the statistics reporting
infrastructure and feels out of scope for this targeted fix.

No change in v2.