Re: [PATCH 3/3] virtio_balloon: add tracepoints for free page hinting and migration

[sashiko-bot@kernel.org]

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 <liuqiang@kylinos.cn>

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/20261030025834.19160-1-liuqiangneo@163.com?part=3