Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Medium] virtio_balloon: inaccurate hint tracepoint in virtio_balloon_report_free_page() - [Low] virtio_balloon: unguarded virtio_balloon_migrate tracepoint definition --
commit 366e84eaf1398af638dddd244580ba2505eef10a Author: Qiang Liu <[email protected]> virtio_balloon: add tracepoints for free page hinting and migration This commit adds trace_virtio_balloon_free_page_hint and trace_virtio_balloon_migrate tracepoints to record free page hinting rounds and balloon page migration attempts. > diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c > index c4fdc6815b41b..faf78c639f3c1 100644 > --- a/drivers/virtio/virtio_balloon.c > +++ b/drivers/virtio/virtio_balloon.c > @@ -803,6 +803,7 @@ static void virtio_balloon_report_free_page(struct > virtio_balloon *vb) > { > int err; > struct device *dev = &vb->vdev->dev; > + unsigned int cmd_id = virtio_balloon_cmd_id_received(vb); > > /* Start by sending the received cmd id to host with an outbuf. */ > err = send_cmd_id_start(vb); > @@ -817,6 +818,9 @@ static void virtio_balloon_report_free_page(struct > virtio_balloon *vb) > err = send_cmd_id_stop(vb); > if (unlikely(err)) > dev_err(dev, "Failed to send a stop id, err = %d\n", err); > + > + trace_virtio_balloon_free_page_hint(cmd_id, > + > READ_ONCE(vb->num_free_page_blocks)); > } [Severity: Medium] Can reading cmd_id via a separate virtio_balloon_cmd_id_received(vb) call before send_cmd_id_start(vb) record a different command ID than the one actually used for the round, and does vb->num_free_page_blocks match the documented semantics of outstanding_blocks? First, virtio_balloon_cmd_id_received(vb) tests and clears VIRTIO_BALLOON_CONFIG_READ_CMD_ID before reading config space: drivers/virtio/virtio_balloon.c:send_cmd_id_start() { ... vb->cmd_id_active = cpu_to_virtio32(vb->vdev, virtio_balloon_cmd_id_received(vb)); ... } If a config interrupt arrives between the virtio_balloon_cmd_id_received(vb) call in virtio_balloon_report_free_page() and the second call inside send_cmd_id_start(vb), cmd_id in the tracepoint will not match the vb->cmd_id_active sent to the host. Second, the comment in include/trace/events/virtio_balloon.h for virtio_balloon_free_page_hint states that outstanding_blocks is the number of hint blocks handed to the host but not yet reclaimed via a DONE/STOP command at the end of the reporting round, and that a high value means the host did not fully drain the round. However, in report_free_page_func() -> virtio_balloon_report_free_page(): drivers/virtio/virtio_balloon.c:report_free_page_func() { cmd_id_received = virtio_balloon_cmd_id_received(vb); if (cmd_id_received == VIRTIO_BALLOON_CMD_ID_DONE) { /* Pass ULONG_MAX to give back all the free pages */ return_free_pages_to_mm(vb, ULONG_MAX); } else if (cmd_id_received != VIRTIO_BALLOON_CMD_ID_STOP && cmd_id_received != virtio32_to_cpu(vb->vdev, vb->cmd_id_active)) { virtio_balloon_report_free_page(vb); } } While send_free_pages() and send_cmd_id_stop() detach used buffers from free_page_vq via virtqueue_get_buf(), the allocated pages remain on vb->free_page_list and counted in vb->num_free_page_blocks until the host later sends VIRTIO_BALLOON_CMD_ID_DONE and triggers a subsequent report_free_page_func() run calling return_free_pages_to_mm(). Receiving VIRTIO_BALLOON_CMD_ID_STOP also does not reclaim blocks from vb->free_page_list. As a result, at the end of virtio_balloon_report_free_page(), isn't vb->num_free_page_blocks simply the total number of blocks accumulated on vb->free_page_list so far, rather than blocks the host failed to drain from the virtqueue? > > static void report_free_page_func(struct work_struct *work) > @@ -869,8 +873,11 @@ static int virtballoon_migratepage(struct > balloon_dev_info *vb_dev_info, > * recursion in the case it ends up triggering memory compaction > * while it is attempting to inflate the ballon. > */ > - if (!mutex_trylock(&vb->balloon_lock)) > + if (!mutex_trylock(&vb->balloon_lock)) { > + trace_virtio_balloon_migrate(page_to_pfn(page), > + page_to_pfn(newpage), -EAGAIN); > return -EAGAIN; > + } > > /* balloon's page migration 1st step -- inflate "newpage" */ > vb->num_pfns = VIRTIO_BALLOON_PAGES_PER_PAGE; > @@ -883,6 +890,8 @@ static int virtballoon_migratepage(struct > balloon_dev_info *vb_dev_info, > tell_host(vb, vb->deflate_vq); > > mutex_unlock(&vb->balloon_lock); > + > + trace_virtio_balloon_migrate(page_to_pfn(page), page_to_pfn(newpage), > 0); > return 0; > } > #endif /* CONFIG_BALLOON_MIGRATION */ > diff --git a/include/trace/events/virtio_balloon.h > b/include/trace/events/virtio_balloon.h > index 559deb997e885..c29f833fa1889 100644 > --- a/include/trace/events/virtio_balloon.h > +++ b/include/trace/events/virtio_balloon.h [ ... ] > +TRACE_EVENT(virtio_balloon_migrate, > + > + TP_PROTO(unsigned long old_pfn, unsigned long new_pfn, long ret), > + > + TP_ARGS(old_pfn, new_pfn, ret), > + > + TP_STRUCT__entry( > + __field(unsigned long, old_pfn) > + __field(unsigned long, new_pfn) > + __field(long, ret) > + ), > + > + TP_fast_assign( > + __entry->old_pfn = old_pfn; > + __entry->new_pfn = new_pfn; > + __entry->ret = ret; > + ), > + > + TP_printk("old_pfn=0x%lx new_pfn=0x%lx ret=%ld", > + __entry->old_pfn, __entry->new_pfn, __entry->ret) > +); [Severity: Low] Should TRACE_EVENT(virtio_balloon_migrate, ...) in include/trace/events/virtio_balloon.h be guarded by #ifdef CONFIG_BALLOON_MIGRATION? In drivers/virtio/virtio_balloon.c, virtballoon_migratepage() and both call sites of trace_virtio_balloon_migrate() are compiled only when CONFIG_BALLOON_MIGRATION is enabled. When building with CONFIG_VIRTIO_BALLOON enabled and CONFIG_BALLOON_MIGRATION disabled, the tracepoint has no callers, which triggers an unused tracepoint build warning when CONFIG_WARN_ON_UNUSED_TRACEPOINTS is enabled and emits unreachable trace event definitions into the image. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
