[PATCH 1/1] virtio_balloon: do not set pr_dev_info.report unconditionally

[PATCH 1/1] virtio_balloon: do not set pr_dev_info.report unconditionally

[Dongli Zhang]

Do not set vb->pr_dev_info.report unconditionally if
VIRTIO_BALLOON_F_REPORTING is not available.

Signed-off-by: Dongli Zhang <dongli.zhang@oracle.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
-- 
2.39.3

Re: [PATCH 1/1] virtio_balloon: do not set pr_dev_info.report unconditionally

[David Hildenbrand (Red Hat)]

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?

> 
> Signed-off-by: Dongli Zhang <dongli.zhang@oracle.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


-- 
Cheers

David

Re: [PATCH 1/1] virtio_balloon: do not set pr_dev_info.report unconditionally

[Dongli Zhang]

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).

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@oracle.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
> 
> 

RE: [PATCH 1/1] virtio_balloon: do not set pr_dev_info.report unconditionally

[Michael Kelley]

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

Re: [PATCH 1/1] virtio_balloon: do not set pr_dev_info.report unconditionally

[David Hildenbrand (Red Hat)]

On 12/17/25 05:52, Michael Kelley wrote:
> From: Dongli Zhang <dongli.zhang@oracle.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.

Nothing is broken AFAIKT.

The patch description should be updated to reflect why we would want to 
do that given that nothing is broken.

No strong opinion on the patch with an updated patch description from my 
side.

-- 
Cheers

David