From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1759288AbcG1DHG (ORCPT ); Wed, 27 Jul 2016 23:07:06 -0400 Received: from mga02.intel.com ([134.134.136.20]:56418 "EHLO mga02.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1162150AbcG1DG5 convert rfc822-to-8bit (ORCPT ); Wed, 27 Jul 2016 23:06:57 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.28,432,1464678000"; d="scan'208";a="146660799" From: "Li, Liang Z" To: "Michael S. Tsirkin" 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" , "dgilbert@redhat.com" , "quintela@redhat.com" , Andrew Morton , "Vlastimil Babka" , Mel Gorman , "Paolo Bonzini" , Cornelia Huck , Amit Shah Subject: RE: [PATCH v2 repost 4/7] virtio-balloon: speed up inflate/deflate process Thread-Topic: [PATCH v2 repost 4/7] virtio-balloon: speed up inflate/deflate process Thread-Index: AQHR56aT5WMR4KLffEuOEzyB2S9p86AsSHmAgADRwqA= Date: Thu, 28 Jul 2016 03:06:37 +0000 Message-ID: References: <1469582616-5729-1-git-send-email-liang.z.li@intel.com> <1469582616-5729-5-git-send-email-liang.z.li@intel.com> <20160728002243-mutt-send-email-mst@kernel.org> In-Reply-To: <20160728002243-mutt-send-email-mst@kernel.org> Accept-Language: zh-CN, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiYTRkOWMxMDMtZmZjYi00OTFmLWFjYjctOTc2YjY2MGExMjBjIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE1LjkuNi42IiwiVHJ1c3RlZExhYmVsSGFzaCI6Im5teHB3VGVTZEJLeTVhUmtWb1J0a1wvVXM3ZFpSbTVKRzNqWk8zNjNBYVR3PSJ9 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 > > + * VIRTIO_BALLOON_PFNS_LIMIT is used to limit the size of page bitmap > > + * to prevent a very large page bitmap, there are two reasons for this: > > + * 1) to save memory. > > + * 2) allocate a large bitmap may fail. > > + * > > + * The actual limit of pfn is determined by: > > + * pfn_limit = min(max_pfn, VIRTIO_BALLOON_PFNS_LIMIT); > > + * > > + * If system has more pages than VIRTIO_BALLOON_PFNS_LIMIT, we will > > +scan > > + * the page list and send the PFNs with several times. To reduce the > > + * overhead of scanning the page list. VIRTIO_BALLOON_PFNS_LIMIT > > +should > > + * be set with a value which can cover most cases. > > So what if it covers 1/32 of the memory? We'll do 32 exits and not 1, still not a > big deal for a big guest. > The issue here is the overhead is too high for scanning the page list for 32 times. Limit the page bitmap size to a fixed value is better for a big guest? > > + */ > > +#define VIRTIO_BALLOON_PFNS_LIMIT ((32 * (1ULL << 30)) >> > PAGE_SHIFT) > > +/* 32GB */ > > I already said this with a smaller limit. > > 2<< 30 is 2G but that is not a useful comment. > pls explain what is the reason for this selection. > > Still applies here. > I will add the comment for this. > > - sg_init_one(&sg, vb->pfns, sizeof(vb->pfns[0]) * vb->num_pfns); > > + if (virtio_has_feature(vb->vdev, > VIRTIO_BALLOON_F_PAGE_BITMAP)) { > > + struct balloon_bmap_hdr *hdr = vb->bmap_hdr; > > + unsigned long bmap_len; > > + > > + /* cmd and req_id are not used here, set them to 0 */ > > + hdr->cmd = cpu_to_virtio16(vb->vdev, 0); > > + hdr->page_shift = cpu_to_virtio16(vb->vdev, PAGE_SHIFT); > > + hdr->reserved = cpu_to_virtio16(vb->vdev, 0); > > + hdr->req_id = cpu_to_virtio64(vb->vdev, 0); > > no need to swap 0, just fill it in. in fact you allocated all 0s so no need to touch > these fields at all. > Will change in v3. > > @@ -489,7 +612,7 @@ static int virtballoon_migratepage(struct > > balloon_dev_info *vb_dev_info, static int virtballoon_probe(struct > > virtio_device *vdev) { > > struct virtio_balloon *vb; > > - int err; > > + int err, hdr_len; > > > > if (!vdev->config->get) { > > dev_err(&vdev->dev, "%s failure: config access disabled\n", > @@ > > -508,6 +631,18 @@ static int virtballoon_probe(struct virtio_device *vdev) > > spin_lock_init(&vb->stop_update_lock); > > vb->stop_update = false; > > vb->num_pages = 0; > > + vb->pfn_limit = VIRTIO_BALLOON_PFNS_LIMIT; > > + vb->pfn_limit = min(vb->pfn_limit, get_max_pfn()); > > + vb->bmap_len = ALIGN(vb->pfn_limit, BITS_PER_LONG) / > > + BITS_PER_BYTE + 2 * sizeof(unsigned long); > > What are these 2 longs in aid of? > The rounddown(vb->start_pfn, BITS_PER_LONG) and roundup(vb->end_pfn, BITS_PER_LONG) may cause (vb->end_pfn - vb->start_pfn) > vb->pfn_limit, so we need extra space to save the bitmap for this case. 2 longs are enough. > > + hdr_len = sizeof(struct balloon_bmap_hdr); > > + vb->bmap_hdr = kzalloc(hdr_len + vb->bmap_len, GFP_KERNEL); > > So it can go up to 1MByte but adding header size etc you need a higher order > allocation. This is a waste, there is no need to have a power of two allocation. > Start from the other side. Say "I want to allocate 32KBytes for the bitmap". > Subtract the header and you get bitmap size. > Calculate the pfn limit from there. > Indeed, will change. Thanks a lot! Liang