Re: [Qemu-devel] [PATCH v17 4/6] virtio-balloon: VIRTIO_BALLOON_F_SG

[Tetsuo Handa]

Wei Wang wrote:
> @@ -164,6 +284,8 @@ static unsigned fill_balloon(struct virtio_balloon *vb, 
> size_t num)
>                       break;
>               }
>  
> +             if (use_sg && xb_set_page(vb, page, &pfn_min, &pfn_max) < 0)

Isn't this leaking "page" ?

> +                     break;
>               balloon_page_push(&pages, page);
>       }
>  



> @@ -184,8 +307,12 @@ static unsigned fill_balloon(struct virtio_balloon *vb, 
> size_t num)
>  
>       num_allocated_pages = vb->num_pfns;
>       /* Did we get any? */
> -     if (vb->num_pfns != 0)
> -             tell_host(vb, vb->inflate_vq);
> +     if (vb->num_pfns) {
> +             if (use_sg)
> +                     tell_host_sgs(vb, vb->inflate_vq, pfn_min, pfn_max);

Please describe why tell_host_sgs() can work without __GFP_DIRECT_RECLAIM 
allocation,
for tell_host_sgs() is called with vb->balloon_lock mutex held.

> +             else
> +                     tell_host(vb, vb->inflate_vq);
> +     }
>       mutex_unlock(&vb->balloon_lock);
>  
>       return num_allocated_pages;



> @@ -223,7 +353,13 @@ static unsigned leak_balloon(struct virtio_balloon *vb, 
> size_t num)
>               page = balloon_page_dequeue(vb_dev_info);
>               if (!page)
>                       break;
> -             set_page_pfns(vb, vb->pfns + vb->num_pfns, page);
> +             if (use_sg) {
> +                     if (xb_set_page(vb, page, &pfn_min, &pfn_max) < 0)

Isn't this leaking "page" ?

If this is inside vb->balloon_lock mutex (isn't this?), xb_set_page() must not
use __GFP_DIRECT_RECLAIM allocation, for leak_balloon_sg_oom() will be blocked
on vb->balloon_lock mutex.

> +                             break;
> +             } else {
> +                     set_page_pfns(vb, vb->pfns + vb->num_pfns, page);
> +             }
> +
>               list_add(&page->lru, &pages);
>               vb->num_pages -= VIRTIO_BALLOON_PAGES_PER_PAGE;
>       }