Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] virtio: off-by-one in virtqueue_enable_cb_delayed_max() hangs page reporting - [Medium] virtio_balloon: broken callback suppression in virtballoon_free_page_report() --
commit e2548c4b5befc523db0e3106c2a724d4255859e6 Author: Michael S. Tsirkin <[email protected]> virtio_balloon: submit reported pages as individual buffers This commit submits each reported page in virtballoon_free_page_report() as a separate virtqueue buffer instead of a single scatter-gather descriptor chain to avoid exceeding the virtio specification's 2^32 byte limit. It uses virtqueue_enable_cb_delayed_max() and virtqueue_disable_cb() around wait_event() when draining the buffers. > diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c > index ab3e3d887e00f..64999f05f9f1d 100644 > --- a/drivers/virtio/virtio_balloon.c > +++ b/drivers/virtio/virtio_balloon.c > @@ -205,25 +205,47 @@ static int virtballoon_free_page_report(struct > page_reporting_dev_info *pr_dev_i [ ... ] > - /* When host has read buffer, this completes via balloon_ack */ > - wait_event(vb->acked, virtqueue_get_buf(vq, &unused)); > + if (nents) { > + virtqueue_kick(vq); > > - return 0; > + /* When host has read buffer, this completes via balloon_ack. > + * We report pages as individual buffers, so arm the callback > + * to only interrupt once all the queued buffers have been > + * used: otherwise we would get one interrupt per page. > + */ > + for (i = 0; i < nents; i++) { > + unsigned int unused; > + > + virtqueue_enable_cb_delayed_max(vq); [Severity: High] Can calling virtqueue_enable_cb_delayed_max(vq) here cause wait_event() to hang indefinitely when VIRTIO_RING_F_EVENT_IDX is negotiated? In drivers/virtio/virtio_ring.c, virtqueue_enable_cb_delayed_max_split() computes the event index as: bufs = (u16)(vq->split.avail_idx_shadow - vq->last_used_idx); virtio_store_mb(vq->weak_barriers, &vring_used_event(&vq->split.vring), cpu_to_virtio16(vq->vq.vdev, vq->last_used_idx + bufs)); if (unlikely((u16)(virtio16_to_cpu(vq->vq.vdev, vq->split.vring.used->idx) - vq->last_used_idx) > bufs)) { END_USE(vq); return false; } In include/uapi/linux/virtio_ring.h, vring_need_event() checks: static inline int vring_need_event(__u16 event_idx, __u16 new_idx, __u16 old) { return (__u16)(new_idx - event_idx - 1) < (__u16)(new_idx - old); } Because event_idx is 0-based, an interrupt is triggered when used->idx (new_idx) advances to event_idx + 1. When bufs buffers are queued at ring slots last_used_idx .. last_used_idx + bufs - 1, consuming all queued buffers advances used->idx to last_used_idx + bufs. By setting vring_used_event to last_used_idx + bufs instead of last_used_idx + bufs - 1, when the device finishes consuming all queued buffers (new_idx == event_idx), (__u16)(new_idx - event_idx - 1) underflows to 0xffff and vring_need_event() returns false. In addition, the race check tests (used->idx - last_used_idx) > bufs, which can never be true since the device cannot consume more than the bufs available buffers. Similarly, in virtqueue_enable_cb_delayed_max_packed(): bufs = vq->packed.vring.num - vq->vq.num_free; last_used_idx = READ_ONCE(vq->last_used_idx); wrap_counter = packed_used_wrap_counter(last_used_idx); used_idx = packed_last_used(last_used_idx) + bufs; if (used_idx >= vq->packed.vring.num) { used_idx -= vq->packed.vring.num; wrap_counter ^= 1; } vq->packed.vring.driver->off_wrap = cpu_to_le16(used_idx | (wrap_counter << VRING_PACKED_EVENT_F_WRAP_CTR)); This sets driver->off_wrap to packed_last_used(last_used_idx) + bufs (next_avail_idx, one descriptor past the last queued descriptor) instead of the last queued descriptor. Since that descriptor is not available to the device, the device never marks it used and never interrupts, hanging the page_reporting_process() -> page_reporting_cycle() -> virtballoon_free_page_report() worker in wait_event(). > + wait_event(vb->acked, > + virtqueue_get_buf(vq, &unused)); > + virtqueue_disable_cb(vq); > + } > + } [Severity: Medium] Does ordering virtqueue_enable_cb_delayed_max(), virtqueue_get_buf(), and virtqueue_disable_cb() this way defeat the delayed callback suppression? There appear to be three issues with the callback state machine in virtballoon_free_page_report(): 1. Reporting virtqueue callbacks are enabled by default in init_vqs(), and virtqueue_kick(vq) is called before callbacks are disabled or delayed, so the device is notified while interrupts are still armed for the first buffer. 2. Inside the loop, virtqueue_enable_cb_delayed_max(vq) is called before wait_event(vb->acked, virtqueue_get_buf(vq, &unused)). Because callbacks are enabled when virtqueue_get_buf() runs, virtqueue_get_buf_ctx_split() (and update_last_used_idx_packed() for packed rings) overwrites the delayed event threshold on every dequeued buffer with the very next single buffer: vq->last_used_idx++; /* If we expect an interrupt for the next entry, tell host * by writing event index and flush out the write before * the read in the next get_buf call. */ if (!(vq->split.avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT)) virtio_store_mb(vq->weak_barriers, &vring_used_event(&vq->split.vring), cpu_to_virtio16(vq->vq.vdev, vq->last_used_idx)); 3. When an interrupt fires, vring_interrupt() sets vq->event_triggered = true. After wait_event() wakes and virtqueue_get_buf() writes vring_used_event = vq->last_used_idx, the subsequent virtqueue_disable_cb(vq) call hits the early return in virtqueue_disable_cb_split() (and similarly in virtqueue_disable_cb_packed()): if (!(vq->split.avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT)) { vq->split.avail_flags_shadow |= VRING_AVAIL_F_NO_INTERRUPT; /* * If device triggered an event already it won't trigger one again: * no need to disable. */ if (vq->event_triggered) return; if (vq->event) /* TODO: this is a hack. Figure out a cleaner value to write. */ vring_used_event(&vq->split.vring) = 0x0; This leaves vring_used_event armed at vq->last_used_idx in the shared ring at the end of the batch, so the next call to virtballoon_free_page_report() kicks the virtqueue with interrupts armed for the first buffer. Should callbacks be kept disabled while draining available buffers with virtqueue_get_buf(), only arming virtqueue_enable_cb_delayed_max() when virtqueue_get_buf() returns NULL? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/ad6a530f49c4a0e182112a73789e31b5e91f3bb7.1793026707.git....@redhat.com?part=2
