Re: [PATCH net-next v01 1/4] Fixes: Solve an LRO stall issue

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · 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).

- [Medium] The commit message does not describe the change.
- [Low] The patch says it fixes a user-visible RX stall ("Solve an LRO
  stall issue") and the reviewer text it quotes asks for a Fixes: tag,…

--

Fixes: Solve an LRO stall issue

This patch changes hinic3_rx_poll() to return budget instead of nr_pkts
when the RX loop exits on the LRO replenish threshold. As a result,
hinic3_poll() stays in polling mode and does not complete NAPI or
re-enable the MSI-X vector.

> Fixes: Solve an LRO stall issue
>
> As noted in the subsequent review.
>
> This changes the return value when the loop exits on the LRO
> replenish threshold. It used to return nr_pkts and now returns budget.

[ ... ]

> This seems unrelated to statistics, and the commit message does not
> mention it. Was it intended? If it fixes an LRO stall, could it go in
> a separate patch with its own explanation and a Fixes: tag?

[Severity: Medium]
Could the commit message be rewritten so it describes this change on its
own?

The subject has no "hinic3:" prefix. It also starts with "Fixes:", and
tools that grep for Fixes: trailers can misread that.

"As noted in the subsequent review." points to a mailing list thread that
won't be in git history.

The rest of the body is the review text copied word for word. That
includes the questions "Was it intended?" and "could it go in a separate
patch with its own explanation and a Fixes: tag?", and they are left
unanswered.

The message doesn't say what stalls, when it stalls, or why returning
budget from hinic3_rx_poll() on the lro_replenish_thld exit fixes it. The
threshold defaults to HINIC3_LRO_REPLENISH_THLD, which is 256.

Is this the mechanism? Before the patch, the threshold break returned
nr_pkts < budget, so hinic3_poll() went on to run:

	if (likely(napi_complete_done(napi, work_done))) {
		hinic3_net_dim(nic_dev, irq_cfg);
		hinic3_set_msix_state(nic_dev->hwdev, irq_cfg->msix_entry_idx,
				      HINIC3_MSIX_ENABLE);

At that point, CQEs with RXDONE already set could still be in the ring.
If the device doesn't raise a new interrupt for CQEs it wrote before the
vector was re-enabled, those packets would stay there until more traffic
arrives.

Could the message say whether this device behaviour is what the patch
works around? The change was split out as the review asked, but the
explanation the review asked for seems to be missing.

> diff --git a/drivers/net/ethernet/huawei/hinic3/hinic3_rx.c b/drivers/net/ethernet/huawei/hinic3/hinic3_rx.c
> index 309ab59013797..9c4ab88998d47 100644
> --- a/drivers/net/ethernet/huawei/hinic3/hinic3_rx.c
> +++ b/drivers/net/ethernet/huawei/hinic3/hinic3_rx.c

[ ... ]

> @@ -581,12 +582,14 @@ int hinic3_rx_poll(struct hinic3_rxq *rxq, int budget)
>  
>  		rx_cqe->status = 0;
>  
> -		if (num_wqe >= nic_dev->lro_replenish_thld)
> +		if (num_wqe >= nic_dev->lro_replenish_thld) {
> +			poll_again = true;
>  			break;
> +		}
>  	}
>  
>  	if (rxq->delta >= HINIC3_RX_BUFFER_WRITE)
>  		hinic3_rx_fill_buffers(rxq);
>  
> -	return nr_pkts;
> +	return poll_again ? budget : nr_pkts;
>  }

[Severity: Low]
The patch is described as a fix for an RX stall, so should it carry a
Fixes: tag? The threshold early exit in hinic3_rx_poll(), and the
return nr_pkts it reaches, came in with:

Fixes: 17fcb3dc12bb ("hinic3: module initialization and tx/rx logic")

Without that tag and a net tree target, stable kernels that ship hinic3
won't pick up the fix.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1793946318.git.gongfan1%40huawei.com