Re: [PATCH v2 7/7] virtio_balloon: Stop calling page_address() in free_pages()

["Michael S. Tsirkin" <mst@redhat.com>]

On Wed, Aug 27, 2025 at 11:29:22AM -0700, Vishal Moola (Oracle) wrote:
> On Wed, Aug 27, 2025 at 02:01:01PM +0200, David Hildenbrand wrote:
> > On 26.08.25 22:56, Vishal Moola (Oracle) wrote:
> > > free_pages() should be used when we only have a virtual address. We
> > > should call __free_pages() directly on our page instead.
> > > 
> > > Signed-off-by: Vishal Moola (Oracle) <vishal.moola@gmail.com>
> > > ---
> > >   drivers/virtio/virtio_balloon.c | 3 +--
> > >   1 file changed, 1 insertion(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> > > index eae65136cdfb..d4e6865ce355 100644
> > > --- a/drivers/virtio/virtio_balloon.c
> > > +++ b/drivers/virtio/virtio_balloon.c
> > > @@ -488,8 +488,7 @@ static unsigned long return_free_pages_to_mm(struct virtio_balloon *vb,
> > >   		page = balloon_page_pop(&vb->free_page_list);
> > >   		if (!page)
> > >   			break;
> > > -		free_pages((unsigned long)page_address(page),
> > > -			   VIRTIO_BALLOON_HINT_BLOCK_ORDER);
> > > +		__free_pages(page, VIRTIO_BALLOON_HINT_BLOCK_ORDER);
> > >   	}
> > >   	vb->num_free_page_blocks -= num_returned;
> > >   	spin_unlock_irq(&vb->free_page_list_lock);
> > 
> > I think you missed another nastiness of similar kind in
> > get_free_page_and_send() where we do
> > 
> > 	p = page_address(page);
> > 
> > Just to call
> > 
> > 	free_pages((unsigned long)p, VIRTIO_BALLOON_HINT_BLOCK_ORDER);
> 
> Thanks for catching that. Andrew can you fold the attached patch into
> this one please? It looks like the page_address() call is needed for other
> things, but since we're changing the file we might as well clean these
> up as well.
> 
> I imagine theres more of these lingering in the kernel, but theres so
> many callers and I only looked for the ones that were calling
> page_address() inline :(.

Confused what is folded where.
If you want me to apply these things please just fold whatever
you want folded and post.
Thanks!


> >From a7d439154c7990418da976e5864b91fce9d49d58 Mon Sep 17 00:00:00 2001
> From: "Vishal Moola (Oracle)" <vishal.moola@gmail.com>
> Date: Wed, 27 Aug 2025 11:10:22 -0700
> Subject: [PATCH] virtio_ballon: Call __free_pages() in
>  get_free_page_and_send()
> 
> free_pages() should be used when we only have a virtual address. We
> should call __free_pages() directly on our page instead.
> 
> Signed-off-by: Vishal Moola (Oracle) <vishal.moola@gmail.com>
> ---
>  drivers/virtio/virtio_balloon.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index d4e6865ce355..7f3fd72678eb 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -718,8 +718,7 @@ static int get_free_page_and_send(struct virtio_balloon *vb)
>  	if (vq->num_free > 1) {
>  		err = virtqueue_add_inbuf(vq, &sg, 1, p, GFP_KERNEL);
>  		if (unlikely(err)) {
> -			free_pages((unsigned long)p,
> -				   VIRTIO_BALLOON_HINT_BLOCK_ORDER);
> +			__free_pages(page, VIRTIO_BALLOON_HINT_BLOCK_ORDER);
>  			return err;
>  		}
>  		virtqueue_kick(vq);
> @@ -732,7 +731,7 @@ static int get_free_page_and_send(struct virtio_balloon *vb)
>  		 * The vq has no available entry to add this page block, so
>  		 * just free it.
>  		 */
> -		free_pages((unsigned long)p, VIRTIO_BALLOON_HINT_BLOCK_ORDER);
> +		__free_pages(page, VIRTIO_BALLOON_HINT_BLOCK_ORDER);
>  	}
>  
>  	return 0;
> -- 
> 2.51.0
>