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

["Vishal Moola (Oracle)" <vishal.moola@gmail.com>]

On Mon, Sep 15, 2025 at 06:45:51PM -0400, Michael S. Tsirkin wrote:
> 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!

Hi Michael, you shouldn't need to worry about this as Andrew will take
all patches in the patchset through the mm tree. See v3 for the updated
version that will be merged:
https://lore.kernel.org/linux-mm/20250903185921.1785167-8-vishal.moola@gmail.com/

I get why you might've gotten confused. The 'fold' patch I was referencing
was included as an attachment rather than inline (but I've figured out a
way to inline things for the future).

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