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 <[email protected]> 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/[email protected]?part=2
