| Message ID | 1477031080-12616-3-git-send-email-liang.z.li@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <liang.z.li@intel.com> > Cc: Michael S. Tsirkin <mst@redhat.com> > Cc: Paolo Bonzini <pbonzini@redhat.com> > Cc: Cornelia Huck <cornelia.huck@de.ibm.com> > Cc: Amit Shah <amit.shah@redhat.com> > --- > 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_VQ 1 /* 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.
> 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
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 <liang.z.li@intel.com> > Cc: Michael S. Tsirkin <mst@redhat.com> > Cc: Paolo Bonzini <pbonzini@redhat.com> > Cc: Cornelia Huck <cornelia.huck@de.ibm.com> > Cc: Amit Shah <amit.shah@redhat.com> > --- > 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_VQ 1 /* 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
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_VQ 1 /* 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; +}; + #endif /* _LINUX_VIRTIO_BALLOON_H */
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 <liang.z.li@intel.com> Cc: Michael S. Tsirkin <mst@redhat.com> Cc: Paolo Bonzini <pbonzini@redhat.com> Cc: Cornelia Huck <cornelia.huck@de.ibm.com> Cc: Amit Shah <amit.shah@redhat.com> --- include/uapi/linux/virtio_balloon.h | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)