Re: [Qemu-devel] [RESEND PATCH v3 kernel 2/7] virtio-balloon: define new feature bit and page bitmap head

[Michael S. Tsirkin]

On Fri, Oct 21, 2016 at 02:24:35PM +0800, Liang Li wrote:
> Add a new feature which supports sending the page information with
> a bitmap. The current implementation uses PFNs array, which is not
> very efficient. Using bitmap can improve the performance of
> inflating/deflating significantly
> 
> The page bitmap header will used to tell the host some information
> about the page bitmap. e.g. the page size, page bitmap length and
> start pfn.
> 
> Signed-off-by: Liang Li 
> Cc: Michael S. Tsirkin 
> Cc: Paolo Bonzini 
> Cc: Cornelia Huck 
> Cc: Amit Shah 
> ---
>  include/uapi/linux/virtio_balloon.h | 19 +++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/include/uapi/linux/virtio_balloon.h 
> b/include/uapi/linux/virtio_balloon.h
> index 343d7dd..d3b182a 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_VQ1 /* Memory Stats virtqueue */
>  #define VIRTIO_BALLOON_F_DEFLATE_ON_OOM  2 /* Deflate balloon on OOM */
> +#define VIRTIO_BALLOON_F_PAGE_BITMAP 3 /* Send page info with bitmap */
>  
>  /* Size of a PFN in the balloon interface. */
>  #define VIRTIO_BALLOON_PFN_SHIFT 12
> @@ -82,4 +83,22 @@ struct virtio_balloon_stat {
>   __virtio64 val;
>  } __attribute__((packed));
>  
> +/* Page bitmap header structure */
> +struct balloon_bmap_hdr {

Should be virtio_balloon.

> + /* Used to distinguish different request */

different requests? what are the legal values?

> + __virtio16 cmd;
> + /* Shift width of page in the bitmap */

In which units?

> + __virtio16 page_shift;
> + /* flag used to identify different status */

this comment does not seem to add any value.

> + __virtio16 flag;
> + /* Reserved */

this too

> + __virtio16 reserved;
> + /* ID of the request */
> + __virtio64 req_id;
> + /* The pfn of 0 bit in the bitmap */
> + __virtio64 start_pfn;
> + /* The length of the bitmap, in bytes */

Why not in bits?

> + __virtio64 bmap_len;
> +};
> +
>  #endif /* _LINUX_VIRTIO_BALLOON_H */
> -- 
> 1.8.3.1

Re: [Qemu-devel] [RESEND PATCH v3 kernel 2/7] virtio-balloon: define new feature bit and page bitmap head

[Li, Liang Z]

> On 10/20/2016 11:24 PM, Liang Li wrote:
> > Add a new feature which supports sending the page information with a
> > bitmap. The current implementation uses PFNs array, which is not very
> > efficient. Using bitmap can improve the performance of
> > inflating/deflating significantly
> 
> Why is it not efficient?  How is using a bitmap more efficient?  What kinds of
> cases is the bitmap inefficient?
> 
> > The page bitmap header will used to tell the host some information
> > about the page bitmap. e.g. the page size, page bitmap length and
> > start pfn.
> 
> Why did you choose to add these features to the structure?  What benefits
> do they add?
> 
> Could you describe your solution a bit here, and describe its strengths and
> weaknesses?
> 

Will elaborate the solution in V4.

> >  /* Size of a PFN in the balloon interface. */  #define
> > VIRTIO_BALLOON_PFN_SHIFT 12 @@ -82,4 +83,22 @@ struct
> > virtio_balloon_stat {
> > __virtio64 val;
> >  } __attribute__((packed));
> >
> > +/* Page bitmap header structure */
> > +struct balloon_bmap_hdr {
> > +   /* Used to distinguish different request */
> > +   __virtio16 cmd;
> > +   /* Shift width of page in the bitmap */
> > +   __virtio16 page_shift;
> > +   /* flag used to identify different status */
> > +   __virtio16 flag;
> > +   /* Reserved */
> > +   __virtio16 reserved;
> > +   /* ID of the request */
> > +   __virtio64 req_id;
> > +   /* The pfn of 0 bit in the bitmap */
> > +   __virtio64 start_pfn;
> > +   /* The length of the bitmap, in bytes */
> > +   __virtio64 bmap_len;
> > +};
> 
> FWIW this is totally unreadable.  Please do something like this:
> 
> > +struct balloon_bmap_hdr {
> > +   __virtio16 cmd; /* Used to distinguish different ...
> > +   __virtio16 page_shift;  /* Shift width of page in the bitmap */
> > +   __virtio16 flag;/* flag used to identify different...
> > +   __virtio16 reserved;/* Reserved */
> > +   __virtio64 req_id;  /* ID of the request */
> > +   __virtio64 start_pfn;   /* The pfn of 0 bit in the bitmap */
> > +   __virtio64 bmap_len;/* The length of the bitmap, in bytes */
> > +};
> 
> and please make an effort to add useful comments.  "/* Reserved */"
> seems like a waste of bytes to me.

OK. Maybe 'padding' is better than 'reserved' .

Thanks for your comments!

Liang

Re: [Qemu-devel] [RESEND PATCH v3 kernel 2/7] virtio-balloon: define new feature bit and page bitmap head

[Dave Hansen]

On 10/20/2016 11:24 PM, Liang Li wrote:
> Add a new feature which supports sending the page information with
> a bitmap. The current implementation uses PFNs array, which is not
> very efficient. Using bitmap can improve the performance of
> inflating/deflating significantly

Why is it not efficient?  How is using a bitmap more efficient?  What
kinds of cases is the bitmap inefficient?

> The page bitmap header will used to tell the host some information
> about the page bitmap. e.g. the page size, page bitmap length and
> start pfn.

Why did you choose to add these features to the structure?  What
benefits do they add?

Could you describe your solution a bit here, and describe its strengths
and weaknesses?

The same comments apply, even if (especially if) you change the data
structure.

> Signed-off-by: Liang Li 
> Cc: Michael S. Tsirkin 
> Cc: Paolo Bonzini 
> Cc: Cornelia Huck 
> Cc: Amit Shah 
> ---
>  include/uapi/linux/virtio_balloon.h | 19 +++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/include/uapi/linux/virtio_balloon.h 
> b/include/uapi/linux/virtio_balloon.h
> index 343d7dd..d3b182a 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_VQ1 /* Memory Stats virtqueue */
>  #define VIRTIO_BALLOON_F_DEFLATE_ON_OOM  2 /* Deflate balloon on OOM */
> +#define VIRTIO_BALLOON_F_PAGE_BITMAP 3 /* Send page info with bitmap */
>  
>  /* Size of a PFN in the balloon interface. */
>  #define VIRTIO_BALLOON_PFN_SHIFT 12
> @@ -82,4 +83,22 @@ struct virtio_balloon_stat {
>   __virtio64 val;
>  } __attribute__((packed));
>  
> +/* Page bitmap header structure */
> +struct balloon_bmap_hdr {
> + /* Used to distinguish different request */
> + __virtio16 cmd;
> + /* Shift width of page in the bitmap */
> + __virtio16 page_shift;
> + /* flag used to identify different status */
> + __virtio16 flag;
> + /* Reserved */
> + __virtio16 reserved;
> + /* ID of the request */
> + __virtio64 req_id;
> + /* The pfn of 0 bit in the bitmap */
> + __virtio64 start_pfn;
> + /* The length of the bitmap, in bytes */
> + __virtio64 bmap_len;
> +};

FWIW this is totally unreadable.  Please do something like this:

> +struct balloon_bmap_hdr {
> + __virtio16 cmd; /* Used to distinguish different ...
> + __virtio16 page_shift;  /* Shift width of page in the bitmap */
> + __virtio16 flag;/* flag used to identify different...
> + __virtio16 reserved;/* Reserved */
> + __virtio64 req_id;  /* ID of the request */
> + __virtio64 start_pfn;   /* The pfn of 0 bit in the bitmap */
> + __virtio64 bmap_len;/* The length of the bitmap, in bytes */
> +};

and please make an effort to add useful comments.  "/* Reserved */"
seems like a waste of bytes to me.

[Qemu-devel] [RESEND PATCH v3 kernel 2/7] virtio-balloon: define new feature bit and page bitmap head

[Liang Li]

Add a new feature which supports sending the page information with
a bitmap. The current implementation uses PFNs array, which is not
very efficient. Using bitmap can improve the performance of
inflating/deflating significantly

The page bitmap header will used to tell the host some information
about the page bitmap. e.g. the page size, page bitmap length and
start pfn.

Signed-off-by: Liang Li 
Cc: Michael S. Tsirkin 
Cc: Paolo Bonzini 
Cc: Cornelia Huck 
Cc: Amit Shah 
---
 include/uapi/linux/virtio_balloon.h | 19 +++
 1 file changed, 19 insertions(+)

diff --git a/include/uapi/linux/virtio_balloon.h 
b/include/uapi/linux/virtio_balloon.h
index 343d7dd..d3b182a 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_HOST0 /* Tell before reclaiming 
pages */
 #define VIRTIO_BALLOON_F_STATS_VQ  1 /* Memory Stats virtqueue */
 #define VIRTIO_BALLOON_F_DEFLATE_ON_OOM2 /* Deflate balloon on OOM */
+#define VIRTIO_BALLOON_F_PAGE_BITMAP   3 /* Send page info with bitmap */
 
 /* Size of a PFN in the balloon interface. */
 #define VIRTIO_BALLOON_PFN_SHIFT 12
@@ -82,4 +83,22 @@ struct virtio_balloon_stat {
__virtio64 val;
 } __attribute__((packed));
 
+/* Page bitmap header structure */
+struct balloon_bmap_hdr {
+   /* Used to distinguish different request */
+   __virtio16 cmd;
+   /* Shift width of page in the bitmap */
+   __virtio16 page_shift;
+   /* flag used to identify different status */
+   __virtio16 flag;
+   /* Reserved */
+   __virtio16 reserved;
+   /* ID of the request */
+   __virtio64 req_id;
+   /* The pfn of 0 bit in the bitmap */
+   __virtio64 start_pfn;
+   /* The length of the bitmap, in bytes */
+   __virtio64 bmap_len;
+};
+
 #endif /* _LINUX_VIRTIO_BALLOON_H */
-- 
1.8.3.1