From: Dongli Zhang <[email protected]> Sent: Wednesday, December 10, 2025
11:10 AM
>
> Hi David,
>
> On 12/10/25 12:09 AM, David Hildenbrand (Red Hat) wrote:
> > On 12/9/25 22:23, Dongli Zhang wrote:
> >> Do not set vb->pr_dev_info.report unconditionally if
> >> VIRTIO_BALLOON_F_REPORTING is not available.
> >
> > Can you share with us why you think that should be done? Please document the
> > "why" and not only the "what".
> >
> > Without VIRTIO_BALLOON_F_REPORTING, we'll never call
> > page_reporting_register(),
> > so it will never be used.
> >
> > But the compiler cannot optimize it out. It only happens during driver
> > loading,
> > so I am not sure it is worth the churn?
>
> When I was reading about the free-page reporting feature in virtio-balloon, I
> was confused as to why pr_dev_info.report was always configured
> unconditionally.
>
> Later, I looked at the implementation in the Hyper-V balloon driver and
> noticed
> that it even resets pr_dev_info.report back to NULL if
> page_reporting_register()
> fails (see line 1669).
The Hyper-V balloon driver does this because it uses the NULL in
pr_dev_info.report
to indicate if page_reporting_unregister() should be called when the driver
exits.
See disable_page_reporting(). Unlike the virtio balloon driver, the Hyper-V
balloon_probe() function succeeds even if page_reporting_register() fails, so
some indicator is needed on exit. I didn't look super carefully, but it
appears the
virtio balloon driver doesn't need such an indicator.
That said, I don't have opinion on the tradeoffs of this proposed change.
Michael
>
> 1651 static void enable_page_reporting(void)
> 1652 {
> 1653 int ret;
> 1654
> 1655 if
> (!hv_query_ext_cap(HV_EXT_CAPABILITY_MEMORY_COLD_DISCARD_HINT)) {
> 1656 pr_debug("Cold memory discard hint not supported by
> Hyper-V\n");
> 1657 return;
> 1658 }
> 1659
> 1660 BUILD_BUG_ON(PAGE_REPORTING_CAPACITY >
> HV_MEMORY_HINT_MAX_GPA_PAGE_RANGES);
> 1661 dm_device.pr_dev_info.report = hv_free_page_report;
> 1662 /*
> 1663 * We let the page_reporting_order parameter decide the order
> 1664 * in the page_reporting code
> 1665 */
> 1666 dm_device.pr_dev_info.order = 0;
> 1667 ret = page_reporting_register(&dm_device.pr_dev_info);
> 1668 if (ret < 0) {
> 1669 dm_device.pr_dev_info.report = NULL;
> 1670 pr_err("Failed to enable cold memory discard: %d\n",
> ret);
> 1671 } else {
> 1672 pr_info("Cold memory discard hint enabled with order
> %d\n",
> 1673 page_reporting_order);
> 1674 }
> 1675 }
>
> That's why I'd like to move the configuration of pr_dev_info.report inside the
> if statement.
>
> It's a purely non-functional change, intended only to make the initialization
> look cleaner.
>
> Apologies - I should have mentioned that this is a non-functional change in
> the
> commit message.
>
> Thank you very much!
>
> Dongli Zhang
>
>
>
> >
> >>
> >> Signed-off-by: Dongli Zhang <[email protected]>
> >> ---
> >> drivers/virtio/virtio_balloon.c | 3 ++-
> >> 1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/virtio/virtio_balloon.c
> >> b/drivers/virtio/virtio_balloon.c
> >> index 74fe59f5a78c..0c39f2415324 100644
> >> --- a/drivers/virtio/virtio_balloon.c
> >> +++ b/drivers/virtio/virtio_balloon.c
> >> @@ -1034,7 +1034,6 @@ static int virtballoon_probe(struct virtio_device
> >> *vdev)
> >> poison_val, &poison_val);
> >> }
> >> - vb->pr_dev_info.report = virtballoon_free_page_report;
> >> if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_REPORTING)) {
> >> unsigned int capacity;
> >> @@ -1044,6 +1043,8 @@ static int virtballoon_probe(struct virtio_device
> >> *vdev)
> >> goto out_unregister_oom;
> >> }
> >> + vb->pr_dev_info.report = virtballoon_free_page_report;
> >> +
> >> /*
> >> * The default page reporting order is @pageblock_order, which
> >> * corresponds to 512MB in size on ARM64 when 64KB base page
> >
> >
>
