Re: [Qemu-devel] [PATCH v18 07/10] virtio-balloon: VIRTIO_BALLOON_F_SG

[Tetsuo Handa]

Wei Wang wrote:
> +static inline int xb_set_page(struct virtio_balloon *vb,
> +                            struct page *page,
> +                            unsigned long *pfn_min,
> +                            unsigned long *pfn_max)
> +{
> +     unsigned long pfn = page_to_pfn(page);
> +     int ret;
> +
> +     *pfn_min = min(pfn, *pfn_min);
> +     *pfn_max = max(pfn, *pfn_max);
> +
> +     do {
> +             ret = xb_preload_and_set_bit(&vb->page_xb, pfn,
> +                                          GFP_NOWAIT | __GFP_NOWARN);

It is a bit of pity that __GFP_NOWARN here is applied to only xb_preload().
Memory allocation by xb_set_bit() will after all emit warnings. Maybe

  xb_init(&vb->page_xb);
  vb->page_xb.gfp_mask |= __GFP_NOWARN;

is tolerable? Or, unconditionally apply __GFP_NOWARN at xb_init()?

  static inline void xb_init(struct xb *xb)
  {
          INIT_RADIX_TREE(&xb->xbrt, IDR_RT_MARKER | GFP_NOWAIT);
  }

> +     } while (unlikely(ret == -EAGAIN));
> +
> +     return ret;
> +}
> +



> @@ -172,11 +283,18 @@ static unsigned fill_balloon(struct virtio_balloon *vb, 
> size_t num)
>       vb->num_pfns = 0;
>  
>       while ((page = balloon_page_pop(&pages))) {
> +             if (use_sg) {
> +                     if (xb_set_page(vb, page, &pfn_min, &pfn_max) < 0) {
> +                             __free_page(page);
> +                             break;

You cannot "break;" without consuming all pages in "pages".

> +                     }
> +             } else {
> +                     set_page_pfns(vb, vb->pfns + vb->num_pfns, page);
> +             }
> +
>               balloon_page_enqueue(&vb->vb_dev_info, page);
>  
>               vb->num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE;
> -
> -             set_page_pfns(vb, vb->pfns + vb->num_pfns, page);
>               vb->num_pages += VIRTIO_BALLOON_PAGES_PER_PAGE;
>               if (!virtio_has_feature(vb->vdev,
>                                       VIRTIO_BALLOON_F_DEFLATE_ON_OOM))



> @@ -212,9 +334,12 @@ static unsigned leak_balloon(struct virtio_balloon *vb, 
> size_t num)
>       struct page *page;
>       struct balloon_dev_info *vb_dev_info = &vb->vb_dev_info;
>       LIST_HEAD(pages);
> +     bool use_sg = virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_SG);

You can pass use_sg as an argument to leak_balloon(). Then, you won't
need to define leak_balloon_sg_oom(). Since xbitmap allocation does not
use __GFP_DIRECT_RECLAIM, it is safe to reuse leak_balloon() for OOM path.
Just be sure to pass use_sg == false because memory allocation for
use_sg == true likely fails when called from OOM path. (But trying
use_sg == true for OOM path and then fallback to use_sg == false is not bad?)

> +     unsigned long pfn_max = 0, pfn_min = ULONG_MAX;
>  
> -     /* We can only do one array worth at a time. */
> -     num = min(num, ARRAY_SIZE(vb->pfns));
> +     /* Traditionally, we can only do one array worth at a time. */
> +     if (!use_sg)
> +             num = min(num, ARRAY_SIZE(vb->pfns));
>  
>       mutex_lock(&vb->balloon_lock);
>       /* We can't release more pages than taken */



> diff --git a/include/uapi/linux/virtio_balloon.h 
> b/include/uapi/linux/virtio_balloon.h
> index 343d7dd..37780a7 100644
> --- a/include/uapi/linux/virtio_balloon.h
> +++ b/include/uapi/linux/virtio_balloon.h
> @@ -34,6 +34,7 @@
>  #define VIRTIO_BALLOON_F_MUST_TELL_HOST      0 /* Tell before reclaiming 
> pages */
>  #define VIRTIO_BALLOON_F_STATS_VQ    1 /* Memory Stats virtqueue */
>  #define VIRTIO_BALLOON_F_DEFLATE_ON_OOM      2 /* Deflate balloon on OOM */
> +#define VIRTIO_BALLOON_F_SG          3 /* Use sg instead of PFN lists */

Want more explicit comment that PFN lists will be used on OOM and therefore
the host side must be prepared for both sg and PFN lists even if negotiated?