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 [off-list ref]
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.
quoted hunk ↗ jump to hunk
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?
quoted hunk ↗ jump to hunk
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
[ ... ]
quoted hunk ↗ jump to hunk
+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/20261030025834.19160-1-liuqiangneo@163.com?part=3