Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933061AbcJYBVc convert rfc822-to-8bit (ORCPT ); Mon, 24 Oct 2016 21:21:32 -0400 Received: from mga09.intel.com ([134.134.136.24]:51283 "EHLO mga09.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1758166AbcJYBVa (ORCPT ); Mon, 24 Oct 2016 21:21:30 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.31,544,1473145200"; d="scan'208";a="23291757" From: "Li, Liang Z" To: "Hansen, Dave" , "mst@redhat.com" CC: "linux-kernel@vger.kernel.org" , "virtualization@lists.linux-foundation.org" , "linux-mm@kvack.org" , "virtio-dev@lists.oasis-open.org" , "kvm@vger.kernel.org" , "qemu-devel@nongnu.org" , "quintela@redhat.com" , "dgilbert@redhat.com" , "pbonzini@redhat.com" , "cornelia.huck@de.ibm.com" , "amit.shah@redhat.com" Subject: RE: [RESEND PATCH v3 kernel 2/7] virtio-balloon: define new feature bit and page bitmap head Thread-Topic: [RESEND PATCH v3 kernel 2/7] virtio-balloon: define new feature bit and page bitmap head Thread-Index: AQHSK2WRkZsNTGeXVka44lM97FAiR6C3UMeAgAEToBA= Date: Tue, 25 Oct 2016 01:21:21 +0000 Message-ID: References: <1477031080-12616-1-git-send-email-liang.z.li@intel.com> <1477031080-12616-3-git-send-email-liang.z.li@intel.com> <580E3C07.701@intel.com> In-Reply-To: <580E3C07.701@intel.com> Accept-Language: zh-CN, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiZGFkNTRlYWUtYmUxMS00N2RmLWJiYmYtMGUwMjZkOGYxYzZiIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE1LjkuNi42IiwiVHJ1c3RlZExhYmVsSGFzaCI6Im1JQ3JKaUpjajRZeDFWeDNpclJpVWpQWDY5dTlqdk14aW4wUFlaUmE5SVk9In0= x-ctpclassification: CTP_IC x-originating-ip: [10.239.127.40] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 8BIT MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2264 Lines: 66 > 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