Re: [RFC RESEND 2/3] virtio_net: introduce an RX queue watchdog for stuck detection

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Missing watchdog state initialization when dynamically increasing receive queues, leading to immediate false stall warnings.
--

commit c9ed39984b2f85816232c0d723aba63e3a109800
Author: Longjun Tang <tanglongjun@kylinos.cn>

virtio_net: introduce an RX queue watchdog for stuck detection

This commit introduces a per-device watchdog to detect stalled RX queues in
virtio-net. The watchdog periodically scans queues and logs a warning if a
queue has a non-zero backlog but makes no progress and receives no interrupts.

> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index e34c52d059d39..c28de03c177b3 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c

[ ... ]

> @@ -3046,6 +3058,77 @@ static int virtnet_poll(struct napi_struct *napi, int budget)
>  	return received;
>  }
>  
> +/*
> + * This watchdog detects that state: a queue is considered stalled when
> + * it has a non-zero backlog, makes no consumption progress and receives
> + * no new interrupt for rx_watchdog_timeo seconds. On detection it logs
> + * a warning.
> + */
> +static void virtnet_rx_watchdog(struct timer_list *t)
> +{
> +	struct virtnet_info *vi = timer_container_of(vi, t, rx_watchdog);
> +	unsigned long timeout = rx_watchdog_timeo * HZ;
> +	int i;
> +
> +	if (!rx_watchdog_timeo)
> +		return;
> +
> +	for (i = 0; i < vi->curr_queue_pairs; i++) {
> +		struct receive_queue *rq = &vi->rq[i];
> +		u16 last_used = virtqueue_get_last_used_idx(rq->vq);
> +		u16 calls = rq->calls;
> +		bool backlog = virtqueue_poll(rq->vq, last_used);
> +
> +		if (!backlog || last_used != rq->watchdog_last_used_idx ||
> +		    calls != rq->watchdog_calls) {
> +			/* No pending data, or the queue made progress, or a
> +			 * new interrupt arrived: restart the window.
> +			 */
> +			rq->watchdog_last_used_idx = last_used;
> +			rq->watchdog_calls = calls;
> +			rq->watchdog_jiffies = jiffies;
> +			continue;
> +		}
> +
> +		if (time_after(jiffies, rq->watchdog_jiffies + timeout)) {

[Severity: Medium]
When receive queues are dynamically increased via ethtool, does this condition
falsely evaluate to true?

Looking at virtnet_set_queues(), it increases vi->curr_queue_pairs but doesn't
initialize watchdog_jiffies for the newly added queues:

virtnet_set_queues() {
    ...
    vi->curr_queue_pairs = queue_pairs;
    if (dev->flags & IFF_UP) {
        local_bh_disable();
        for (int i = 0; i < vi->curr_queue_pairs; ++i)
            virtqueue_napi_schedule(&vi->rq[i].napi, vi->rq[i].vq);
        local_bh_enable();
    }
    ...
}

If the watchdog timer executes right after NAPI fills the new ring and a
packet is immediately received (making backlog true), the watchdog evaluates
the time_after() condition.

Since watchdog_jiffies remains 0 for the new queues, would this immediately
log a false positive "RX queue stalled" warning with a huge stall time?

> +			unsigned int stall_ms =
> +				jiffies_to_msecs(jiffies - rq->watchdog_jiffies);
> +
> +			netdev_warn(vi->dev, "RX queue %u stalled for %u ms\n",
> +				    i, stall_ms);
> +
> +			/* Rate-limit to one event per timeout. */
> +			rq->watchdog_jiffies = jiffies;
> +		}
> +	}
> +
> +	mod_timer(&vi->rx_watchdog, jiffies + HZ);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260908092646.108865-1-lange_tang@163.com?part=2