linux-kernel - RE: [PATCH 1/1] virtio_balloon: do not set pr_dev

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID:
 <BN7PR02MB4148C75ACA806BAEEEC4351BD4ABA@BN7PR02MB4148.namprd02.prod.outlook.com>
Date: Wed, 17 Dec 2025 04:52:57 +0000
From: Michael Kelley <mhklinux@...look.com>
To: Dongli Zhang <dongli.zhang@...cle.com>, "David Hildenbrand (Red Hat)"
	<david@...nel.org>, "virtualization@...ts.linux.dev"
	<virtualization@...ts.linux.dev>
CC: "jasowang@...hat.com" <jasowang@...hat.com>, "xuanzhuo@...ux.alibaba.com"
	<xuanzhuo@...ux.alibaba.com>, "eperezma@...hat.com" <eperezma@...hat.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH 1/1] virtio_balloon: do not set pr_dev_info.report
 unconditionally

From: Dongli Zhang <dongli.zhang@...cle.com> 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 <dongli.zhang@...cle.com>
> >> ---
> >>   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
> >
> >
>