Re: [RESEND PATCH v3 kernel 2/7] virtio-balloon: define new feature bit and page bitmap head
From: Dave Hansen <hidden>
Date: 2016-10-24 16:51:26
Also in:
kvm, lkml, qemu-devel, virtualization
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.
quoted hunk ↗ jump to hunk
Signed-off-by: Liang Li <redacted> Cc: Michael S. Tsirkin <mst@redhat.com> Cc: Paolo Bonzini <pbonzini@redhat.com> Cc: Cornelia Huck <redacted> Cc: Amit Shah <redacted> --- 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. -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>