From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1162503AbcG0WAm (ORCPT ); Wed, 27 Jul 2016 18:00:42 -0400 Received: from mx1.redhat.com ([209.132.183.28]:35480 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1161863AbcG0WA1 (ORCPT ); Wed, 27 Jul 2016 18:00:27 -0400 Date: Thu, 28 Jul 2016 01:00:19 +0300 From: "Michael S. Tsirkin" To: Liang Li 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 7/7] virtio-balloon: tell host vm's free page info Message-ID: <20160728004606-mutt-send-email-mst@kernel.org> References: <1469582616-5729-1-git-send-email-liang.z.li@intel.com> <1469582616-5729-8-git-send-email-liang.z.li@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1469582616-5729-8-git-send-email-liang.z.li@intel.com> X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.38]); Wed, 27 Jul 2016 22:00:26 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Jul 27, 2016 at 09:23:36AM +0800, Liang Li wrote: > Support the request for vm's free page information, response with > a page bitmap. QEMU can make use of this free page bitmap to speed > up live migration process by skipping process the free pages. > > Signed-off-by: Liang Li > Cc: Michael S. Tsirkin > Cc: Andrew Morton > Cc: Vlastimil Babka > Cc: Mel Gorman > Cc: Paolo Bonzini > Cc: Cornelia Huck > Cc: Amit Shah > --- > drivers/virtio/virtio_balloon.c | 104 +++++++++++++++++++++++++++++++++++++--- > 1 file changed, 98 insertions(+), 6 deletions(-) > > diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c > index 2d18ff6..5ca4ad3 100644 > --- a/drivers/virtio/virtio_balloon.c > +++ b/drivers/virtio/virtio_balloon.c > @@ -62,10 +62,13 @@ module_param(oom_pages, int, S_IRUSR | S_IWUSR); > MODULE_PARM_DESC(oom_pages, "pages to free on OOM"); > > extern unsigned long get_max_pfn(void); > +extern int get_free_pages(unsigned long start_pfn, unsigned long end_pfn, > + unsigned long *bitmap, unsigned long len); > + > > struct virtio_balloon { > struct virtio_device *vdev; > - struct virtqueue *inflate_vq, *deflate_vq, *stats_vq; > + struct virtqueue *inflate_vq, *deflate_vq, *stats_vq, *misc_vq; > > /* The balloon servicing is delegated to a freezable workqueue. */ > struct work_struct update_balloon_stats_work; > @@ -89,6 +92,8 @@ struct virtio_balloon { > unsigned long pfn_limit; > /* Used to record the processed pfn range */ > unsigned long min_pfn, max_pfn, start_pfn, end_pfn; > + /* Request header */ > + struct balloon_req_hdr req_hdr; > /* > * The pages we've told the Host we're not using are enqueued > * at vb_dev_info->pages list. > @@ -373,6 +378,49 @@ static void update_balloon_stats(struct virtio_balloon *vb) > pages_to_bytes(available)); > } > > +static void update_free_pages_stats(struct virtio_balloon *vb, why _stats? > + unsigned long req_id) > +{ > + struct scatterlist sg_in, sg_out; > + unsigned long pfn = 0, bmap_len, max_pfn; > + struct virtqueue *vq = vb->misc_vq; > + struct balloon_bmap_hdr *hdr = vb->bmap_hdr; > + int ret = 1; > + > + max_pfn = get_max_pfn(); > + mutex_lock(&vb->balloon_lock); > + while (pfn < max_pfn) { > + memset(vb->page_bitmap, 0, vb->bmap_len); > + ret = get_free_pages(pfn, pfn + vb->pfn_limit, > + vb->page_bitmap, vb->bmap_len * BITS_PER_BYTE); > + hdr->cmd = cpu_to_virtio16(vb->vdev, BALLOON_GET_FREE_PAGES); > + hdr->page_shift = cpu_to_virtio16(vb->vdev, PAGE_SHIFT); > + hdr->req_id = cpu_to_virtio64(vb->vdev, req_id); > + hdr->start_pfn = cpu_to_virtio64(vb->vdev, pfn); > + bmap_len = vb->pfn_limit / BITS_PER_BYTE; > + if (!ret) { > + hdr->flag = cpu_to_virtio16(vb->vdev, > + BALLOON_FLAG_DONE); > + if (pfn + vb->pfn_limit > max_pfn) > + bmap_len = (max_pfn - pfn) / BITS_PER_BYTE; > + } else > + hdr->flag = cpu_to_virtio16(vb->vdev, > + BALLOON_FLAG_CONT); > + hdr->bmap_len = cpu_to_virtio64(vb->vdev, bmap_len); > + sg_init_one(&sg_out, hdr, > + sizeof(struct balloon_bmap_hdr) + bmap_len); Wait a second. This adds the same buffer multiple times in a loop. We will overwrite the buffer without waiting for hypervisor to process it. What did I miss? > + > + virtqueue_add_outbuf(vq, &sg_out, 1, vb, GFP_KERNEL); this can fail. you want to maybe make sure vq has enough space before you use it or check error and wait. > + virtqueue_kick(vq); why kick here within loop? wait until done. in fact kick outside lock is better for smp. > + pfn += vb->pfn_limit; > + } > + > + sg_init_one(&sg_in, &vb->req_hdr, sizeof(vb->req_hdr)); > + virtqueue_add_inbuf(vq, &sg_in, 1, &vb->req_hdr, GFP_KERNEL); > + virtqueue_kick(vq); > + mutex_unlock(&vb->balloon_lock); > +} > + > /* > * While most virtqueues communicate guest-initiated requests to the hypervisor, > * the stats queue operates in reverse. The driver initializes the virtqueue > @@ -511,18 +559,49 @@ static void update_balloon_size_func(struct work_struct *work) > queue_work(system_freezable_wq, work); > } > > +static void misc_handle_rq(struct virtio_balloon *vb) > +{ > + struct balloon_req_hdr *ptr_hdr; > + unsigned int len; > + > + ptr_hdr = virtqueue_get_buf(vb->misc_vq, &len); > + if (!ptr_hdr || len != sizeof(vb->req_hdr)) > + return; > + > + switch (ptr_hdr->cmd) { > + case BALLOON_GET_FREE_PAGES: > + update_free_pages_stats(vb, ptr_hdr->param); > + break; > + default: > + break; > + } > +} > + > +static void misc_request(struct virtqueue *vq) > +{ > + struct virtio_balloon *vb = vq->vdev->priv; > + > + misc_handle_rq(vb); > +} > + > static int init_vqs(struct virtio_balloon *vb) > { > - struct virtqueue *vqs[3]; > - vq_callback_t *callbacks[] = { balloon_ack, balloon_ack, stats_request }; > - static const char * const names[] = { "inflate", "deflate", "stats" }; > + struct virtqueue *vqs[4]; > + vq_callback_t *callbacks[] = { balloon_ack, balloon_ack, > + stats_request, misc_request }; > + static const char * const names[] = { "inflate", "deflate", "stats", > + "misc" }; > int err, nvqs; > > /* > * We expect two virtqueues: inflate and deflate, and > * optionally stat. > */ > - nvqs = virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_STATS_VQ) ? 3 : 2; > + if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_MISC_VQ)) > + nvqs = 4; Does misc vq depend on stats vq feature then? if yes please validate that. > + else > + nvqs = virtio_has_feature(vb->vdev, > + VIRTIO_BALLOON_F_STATS_VQ) ? 3 : 2; Replace that ? with else too pls. > err = vb->vdev->config->find_vqs(vb->vdev, nvqs, vqs, callbacks, names); > if (err) > return err; > @@ -543,6 +622,16 @@ static int init_vqs(struct virtio_balloon *vb) > BUG(); > virtqueue_kick(vb->stats_vq); > } > + if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_MISC_VQ)) { > + struct scatterlist sg_in; > + > + vb->misc_vq = vqs[3]; > + sg_init_one(&sg_in, &vb->req_hdr, sizeof(vb->req_hdr)); > + if (virtqueue_add_inbuf(vb->misc_vq, &sg_in, 1, > + &vb->req_hdr, GFP_KERNEL) < 0) > + BUG(); > + virtqueue_kick(vb->misc_vq); > + } > return 0; > } > > @@ -639,8 +728,10 @@ static int virtballoon_probe(struct virtio_device *vdev) > vb->bmap_hdr = kzalloc(hdr_len + vb->bmap_len, GFP_KERNEL); > > /* Clear the feature bit if memory allocation fails */ > - if (!vb->bmap_hdr) > + if (!vb->bmap_hdr) { > __virtio_clear_bit(vdev, VIRTIO_BALLOON_F_PAGE_BITMAP); > + __virtio_clear_bit(vdev, VIRTIO_BALLOON_F_MISC_VQ); > + } > else > vb->page_bitmap = vb->bmap_hdr + hdr_len; > mutex_init(&vb->balloon_lock); > @@ -743,6 +834,7 @@ static unsigned int features[] = { > VIRTIO_BALLOON_F_STATS_VQ, > VIRTIO_BALLOON_F_DEFLATE_ON_OOM, > VIRTIO_BALLOON_F_PAGE_BITMAP, > + VIRTIO_BALLOON_F_MISC_VQ, > }; > > static struct virtio_driver virtio_balloon_driver = { > -- > 1.9.1 From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Michael S. Tsirkin" Subject: Re: [PATCH v2 repost 7/7] virtio-balloon: tell host vm's free page info Date: Thu, 28 Jul 2016 01:00:19 +0300 Message-ID: <20160728004606-mutt-send-email-mst@kernel.org> References: <1469582616-5729-1-git-send-email-liang.z.li@intel.com> <1469582616-5729-8-git-send-email-liang.z.li@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Cc: virtio-dev@lists.oasis-open.org, kvm@vger.kernel.org, Amit Shah , qemu-devel@nongnu.org, linux-kernel@vger.kernel.org, linux-mm@kvack.org, Vlastimil Babka , Paolo Bonzini , Andrew Morton , virtualization@lists.linux-foundation.org, Mel Gorman , dgilbert@redhat.com To: Liang Li Return-path: Content-Disposition: inline In-Reply-To: <1469582616-5729-8-git-send-email-liang.z.li@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: virtualization-bounces@lists.linux-foundation.org Errors-To: virtualization-bounces@lists.linux-foundation.org List-Id: kvm.vger.kernel.org On Wed, Jul 27, 2016 at 09:23:36AM +0800, Liang Li wrote: > Support the request for vm's free page information, response with > a page bitmap. QEMU can make use of this free page bitmap to speed > up live migration process by skipping process the free pages. > > Signed-off-by: Liang Li > Cc: Michael S. Tsirkin > Cc: Andrew Morton > Cc: Vlastimil Babka > Cc: Mel Gorman > Cc: Paolo Bonzini > Cc: Cornelia Huck > Cc: Amit Shah > --- > drivers/virtio/virtio_balloon.c | 104 +++++++++++++++++++++++++++++++++++++--- > 1 file changed, 98 insertions(+), 6 deletions(-) > > diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c > index 2d18ff6..5ca4ad3 100644 > --- a/drivers/virtio/virtio_balloon.c > +++ b/drivers/virtio/virtio_balloon.c > @@ -62,10 +62,13 @@ module_param(oom_pages, int, S_IRUSR | S_IWUSR); > MODULE_PARM_DESC(oom_pages, "pages to free on OOM"); > > extern unsigned long get_max_pfn(void); > +extern int get_free_pages(unsigned long start_pfn, unsigned long end_pfn, > + unsigned long *bitmap, unsigned long len); > + > > struct virtio_balloon { > struct virtio_device *vdev; > - struct virtqueue *inflate_vq, *deflate_vq, *stats_vq; > + struct virtqueue *inflate_vq, *deflate_vq, *stats_vq, *misc_vq; > > /* The balloon servicing is delegated to a freezable workqueue. */ > struct work_struct update_balloon_stats_work; > @@ -89,6 +92,8 @@ struct virtio_balloon { > unsigned long pfn_limit; > /* Used to record the processed pfn range */ > unsigned long min_pfn, max_pfn, start_pfn, end_pfn; > + /* Request header */ > + struct balloon_req_hdr req_hdr; > /* > * The pages we've told the Host we're not using are enqueued > * at vb_dev_info->pages list. > @@ -373,6 +378,49 @@ static void update_balloon_stats(struct virtio_balloon *vb) > pages_to_bytes(available)); > } > > +static void update_free_pages_stats(struct virtio_balloon *vb, why _stats? > + unsigned long req_id) > +{ > + struct scatterlist sg_in, sg_out; > + unsigned long pfn = 0, bmap_len, max_pfn; > + struct virtqueue *vq = vb->misc_vq; > + struct balloon_bmap_hdr *hdr = vb->bmap_hdr; > + int ret = 1; > + > + max_pfn = get_max_pfn(); > + mutex_lock(&vb->balloon_lock); > + while (pfn < max_pfn) { > + memset(vb->page_bitmap, 0, vb->bmap_len); > + ret = get_free_pages(pfn, pfn + vb->pfn_limit, > + vb->page_bitmap, vb->bmap_len * BITS_PER_BYTE); > + hdr->cmd = cpu_to_virtio16(vb->vdev, BALLOON_GET_FREE_PAGES); > + hdr->page_shift = cpu_to_virtio16(vb->vdev, PAGE_SHIFT); > + hdr->req_id = cpu_to_virtio64(vb->vdev, req_id); > + hdr->start_pfn = cpu_to_virtio64(vb->vdev, pfn); > + bmap_len = vb->pfn_limit / BITS_PER_BYTE; > + if (!ret) { > + hdr->flag = cpu_to_virtio16(vb->vdev, > + BALLOON_FLAG_DONE); > + if (pfn + vb->pfn_limit > max_pfn) > + bmap_len = (max_pfn - pfn) / BITS_PER_BYTE; > + } else > + hdr->flag = cpu_to_virtio16(vb->vdev, > + BALLOON_FLAG_CONT); > + hdr->bmap_len = cpu_to_virtio64(vb->vdev, bmap_len); > + sg_init_one(&sg_out, hdr, > + sizeof(struct balloon_bmap_hdr) + bmap_len); Wait a second. This adds the same buffer multiple times in a loop. We will overwrite the buffer without waiting for hypervisor to process it. What did I miss? > + > + virtqueue_add_outbuf(vq, &sg_out, 1, vb, GFP_KERNEL); this can fail. you want to maybe make sure vq has enough space before you use it or check error and wait. > + virtqueue_kick(vq); why kick here within loop? wait until done. in fact kick outside lock is better for smp. > + pfn += vb->pfn_limit; > + } > + > + sg_init_one(&sg_in, &vb->req_hdr, sizeof(vb->req_hdr)); > + virtqueue_add_inbuf(vq, &sg_in, 1, &vb->req_hdr, GFP_KERNEL); > + virtqueue_kick(vq); > + mutex_unlock(&vb->balloon_lock); > +} > + > /* > * While most virtqueues communicate guest-initiated requests to the hypervisor, > * the stats queue operates in reverse. The driver initializes the virtqueue > @@ -511,18 +559,49 @@ static void update_balloon_size_func(struct work_struct *work) > queue_work(system_freezable_wq, work); > } > > +static void misc_handle_rq(struct virtio_balloon *vb) > +{ > + struct balloon_req_hdr *ptr_hdr; > + unsigned int len; > + > + ptr_hdr = virtqueue_get_buf(vb->misc_vq, &len); > + if (!ptr_hdr || len != sizeof(vb->req_hdr)) > + return; > + > + switch (ptr_hdr->cmd) { > + case BALLOON_GET_FREE_PAGES: > + update_free_pages_stats(vb, ptr_hdr->param); > + break; > + default: > + break; > + } > +} > + > +static void misc_request(struct virtqueue *vq) > +{ > + struct virtio_balloon *vb = vq->vdev->priv; > + > + misc_handle_rq(vb); > +} > + > static int init_vqs(struct virtio_balloon *vb) > { > - struct virtqueue *vqs[3]; > - vq_callback_t *callbacks[] = { balloon_ack, balloon_ack, stats_request }; > - static const char * const names[] = { "inflate", "deflate", "stats" }; > + struct virtqueue *vqs[4]; > + vq_callback_t *callbacks[] = { balloon_ack, balloon_ack, > + stats_request, misc_request }; > + static const char * const names[] = { "inflate", "deflate", "stats", > + "misc" }; > int err, nvqs; > > /* > * We expect two virtqueues: inflate and deflate, and > * optionally stat. > */ > - nvqs = virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_STATS_VQ) ? 3 : 2; > + if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_MISC_VQ)) > + nvqs = 4; Does misc vq depend on stats vq feature then? if yes please validate that. > + else > + nvqs = virtio_has_feature(vb->vdev, > + VIRTIO_BALLOON_F_STATS_VQ) ? 3 : 2; Replace that ? with else too pls. > err = vb->vdev->config->find_vqs(vb->vdev, nvqs, vqs, callbacks, names); > if (err) > return err; > @@ -543,6 +622,16 @@ static int init_vqs(struct virtio_balloon *vb) > BUG(); > virtqueue_kick(vb->stats_vq); > } > + if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_MISC_VQ)) { > + struct scatterlist sg_in; > + > + vb->misc_vq = vqs[3]; > + sg_init_one(&sg_in, &vb->req_hdr, sizeof(vb->req_hdr)); > + if (virtqueue_add_inbuf(vb->misc_vq, &sg_in, 1, > + &vb->req_hdr, GFP_KERNEL) < 0) > + BUG(); > + virtqueue_kick(vb->misc_vq); > + } > return 0; > } > > @@ -639,8 +728,10 @@ static int virtballoon_probe(struct virtio_device *vdev) > vb->bmap_hdr = kzalloc(hdr_len + vb->bmap_len, GFP_KERNEL); > > /* Clear the feature bit if memory allocation fails */ > - if (!vb->bmap_hdr) > + if (!vb->bmap_hdr) { > __virtio_clear_bit(vdev, VIRTIO_BALLOON_F_PAGE_BITMAP); > + __virtio_clear_bit(vdev, VIRTIO_BALLOON_F_MISC_VQ); > + } > else > vb->page_bitmap = vb->bmap_hdr + hdr_len; > mutex_init(&vb->balloon_lock); > @@ -743,6 +834,7 @@ static unsigned int features[] = { > VIRTIO_BALLOON_F_STATS_VQ, > VIRTIO_BALLOON_F_DEFLATE_ON_OOM, > VIRTIO_BALLOON_F_PAGE_BITMAP, > + VIRTIO_BALLOON_F_MISC_VQ, > }; > > static struct virtio_driver virtio_balloon_driver = { > -- > 1.9.1 From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-io0-f199.google.com (mail-io0-f199.google.com [209.85.223.199]) by kanga.kvack.org (Postfix) with ESMTP id D8D946B0253 for ; Wed, 27 Jul 2016 18:00:27 -0400 (EDT) Received: by mail-io0-f199.google.com with SMTP id g14so20032587ioj.3 for ; Wed, 27 Jul 2016 15:00:27 -0700 (PDT) Received: from mx1.redhat.com (mx1.redhat.com. [209.132.183.28]) by mx.google.com with ESMTPS id o68si9135839iod.157.2016.07.27.15.00.27 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 27 Jul 2016 15:00:27 -0700 (PDT) Date: Thu, 28 Jul 2016 01:00:19 +0300 From: "Michael S. Tsirkin" Subject: Re: [PATCH v2 repost 7/7] virtio-balloon: tell host vm's free page info Message-ID: <20160728004606-mutt-send-email-mst@kernel.org> References: <1469582616-5729-1-git-send-email-liang.z.li@intel.com> <1469582616-5729-8-git-send-email-liang.z.li@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1469582616-5729-8-git-send-email-liang.z.li@intel.com> Sender: owner-linux-mm@kvack.org List-ID: To: Liang Li 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 On Wed, Jul 27, 2016 at 09:23:36AM +0800, Liang Li wrote: > Support the request for vm's free page information, response with > a page bitmap. QEMU can make use of this free page bitmap to speed > up live migration process by skipping process the free pages. > > Signed-off-by: Liang Li > Cc: Michael S. Tsirkin > Cc: Andrew Morton > Cc: Vlastimil Babka > Cc: Mel Gorman > Cc: Paolo Bonzini > Cc: Cornelia Huck > Cc: Amit Shah > --- > drivers/virtio/virtio_balloon.c | 104 +++++++++++++++++++++++++++++++++++++--- > 1 file changed, 98 insertions(+), 6 deletions(-) > > diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c > index 2d18ff6..5ca4ad3 100644 > --- a/drivers/virtio/virtio_balloon.c > +++ b/drivers/virtio/virtio_balloon.c > @@ -62,10 +62,13 @@ module_param(oom_pages, int, S_IRUSR | S_IWUSR); > MODULE_PARM_DESC(oom_pages, "pages to free on OOM"); > > extern unsigned long get_max_pfn(void); > +extern int get_free_pages(unsigned long start_pfn, unsigned long end_pfn, > + unsigned long *bitmap, unsigned long len); > + > > struct virtio_balloon { > struct virtio_device *vdev; > - struct virtqueue *inflate_vq, *deflate_vq, *stats_vq; > + struct virtqueue *inflate_vq, *deflate_vq, *stats_vq, *misc_vq; > > /* The balloon servicing is delegated to a freezable workqueue. */ > struct work_struct update_balloon_stats_work; > @@ -89,6 +92,8 @@ struct virtio_balloon { > unsigned long pfn_limit; > /* Used to record the processed pfn range */ > unsigned long min_pfn, max_pfn, start_pfn, end_pfn; > + /* Request header */ > + struct balloon_req_hdr req_hdr; > /* > * The pages we've told the Host we're not using are enqueued > * at vb_dev_info->pages list. > @@ -373,6 +378,49 @@ static void update_balloon_stats(struct virtio_balloon *vb) > pages_to_bytes(available)); > } > > +static void update_free_pages_stats(struct virtio_balloon *vb, why _stats? > + unsigned long req_id) > +{ > + struct scatterlist sg_in, sg_out; > + unsigned long pfn = 0, bmap_len, max_pfn; > + struct virtqueue *vq = vb->misc_vq; > + struct balloon_bmap_hdr *hdr = vb->bmap_hdr; > + int ret = 1; > + > + max_pfn = get_max_pfn(); > + mutex_lock(&vb->balloon_lock); > + while (pfn < max_pfn) { > + memset(vb->page_bitmap, 0, vb->bmap_len); > + ret = get_free_pages(pfn, pfn + vb->pfn_limit, > + vb->page_bitmap, vb->bmap_len * BITS_PER_BYTE); > + hdr->cmd = cpu_to_virtio16(vb->vdev, BALLOON_GET_FREE_PAGES); > + hdr->page_shift = cpu_to_virtio16(vb->vdev, PAGE_SHIFT); > + hdr->req_id = cpu_to_virtio64(vb->vdev, req_id); > + hdr->start_pfn = cpu_to_virtio64(vb->vdev, pfn); > + bmap_len = vb->pfn_limit / BITS_PER_BYTE; > + if (!ret) { > + hdr->flag = cpu_to_virtio16(vb->vdev, > + BALLOON_FLAG_DONE); > + if (pfn + vb->pfn_limit > max_pfn) > + bmap_len = (max_pfn - pfn) / BITS_PER_BYTE; > + } else > + hdr->flag = cpu_to_virtio16(vb->vdev, > + BALLOON_FLAG_CONT); > + hdr->bmap_len = cpu_to_virtio64(vb->vdev, bmap_len); > + sg_init_one(&sg_out, hdr, > + sizeof(struct balloon_bmap_hdr) + bmap_len); Wait a second. This adds the same buffer multiple times in a loop. We will overwrite the buffer without waiting for hypervisor to process it. What did I miss? > + > + virtqueue_add_outbuf(vq, &sg_out, 1, vb, GFP_KERNEL); this can fail. you want to maybe make sure vq has enough space before you use it or check error and wait. > + virtqueue_kick(vq); why kick here within loop? wait until done. in fact kick outside lock is better for smp. > + pfn += vb->pfn_limit; > + } > + > + sg_init_one(&sg_in, &vb->req_hdr, sizeof(vb->req_hdr)); > + virtqueue_add_inbuf(vq, &sg_in, 1, &vb->req_hdr, GFP_KERNEL); > + virtqueue_kick(vq); > + mutex_unlock(&vb->balloon_lock); > +} > + > /* > * While most virtqueues communicate guest-initiated requests to the hypervisor, > * the stats queue operates in reverse. The driver initializes the virtqueue > @@ -511,18 +559,49 @@ static void update_balloon_size_func(struct work_struct *work) > queue_work(system_freezable_wq, work); > } > > +static void misc_handle_rq(struct virtio_balloon *vb) > +{ > + struct balloon_req_hdr *ptr_hdr; > + unsigned int len; > + > + ptr_hdr = virtqueue_get_buf(vb->misc_vq, &len); > + if (!ptr_hdr || len != sizeof(vb->req_hdr)) > + return; > + > + switch (ptr_hdr->cmd) { > + case BALLOON_GET_FREE_PAGES: > + update_free_pages_stats(vb, ptr_hdr->param); > + break; > + default: > + break; > + } > +} > + > +static void misc_request(struct virtqueue *vq) > +{ > + struct virtio_balloon *vb = vq->vdev->priv; > + > + misc_handle_rq(vb); > +} > + > static int init_vqs(struct virtio_balloon *vb) > { > - struct virtqueue *vqs[3]; > - vq_callback_t *callbacks[] = { balloon_ack, balloon_ack, stats_request }; > - static const char * const names[] = { "inflate", "deflate", "stats" }; > + struct virtqueue *vqs[4]; > + vq_callback_t *callbacks[] = { balloon_ack, balloon_ack, > + stats_request, misc_request }; > + static const char * const names[] = { "inflate", "deflate", "stats", > + "misc" }; > int err, nvqs; > > /* > * We expect two virtqueues: inflate and deflate, and > * optionally stat. > */ > - nvqs = virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_STATS_VQ) ? 3 : 2; > + if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_MISC_VQ)) > + nvqs = 4; Does misc vq depend on stats vq feature then? if yes please validate that. > + else > + nvqs = virtio_has_feature(vb->vdev, > + VIRTIO_BALLOON_F_STATS_VQ) ? 3 : 2; Replace that ? with else too pls. > err = vb->vdev->config->find_vqs(vb->vdev, nvqs, vqs, callbacks, names); > if (err) > return err; > @@ -543,6 +622,16 @@ static int init_vqs(struct virtio_balloon *vb) > BUG(); > virtqueue_kick(vb->stats_vq); > } > + if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_MISC_VQ)) { > + struct scatterlist sg_in; > + > + vb->misc_vq = vqs[3]; > + sg_init_one(&sg_in, &vb->req_hdr, sizeof(vb->req_hdr)); > + if (virtqueue_add_inbuf(vb->misc_vq, &sg_in, 1, > + &vb->req_hdr, GFP_KERNEL) < 0) > + BUG(); > + virtqueue_kick(vb->misc_vq); > + } > return 0; > } > > @@ -639,8 +728,10 @@ static int virtballoon_probe(struct virtio_device *vdev) > vb->bmap_hdr = kzalloc(hdr_len + vb->bmap_len, GFP_KERNEL); > > /* Clear the feature bit if memory allocation fails */ > - if (!vb->bmap_hdr) > + if (!vb->bmap_hdr) { > __virtio_clear_bit(vdev, VIRTIO_BALLOON_F_PAGE_BITMAP); > + __virtio_clear_bit(vdev, VIRTIO_BALLOON_F_MISC_VQ); > + } > else > vb->page_bitmap = vb->bmap_hdr + hdr_len; > mutex_init(&vb->balloon_lock); > @@ -743,6 +834,7 @@ static unsigned int features[] = { > VIRTIO_BALLOON_F_STATS_VQ, > VIRTIO_BALLOON_F_DEFLATE_ON_OOM, > VIRTIO_BALLOON_F_PAGE_BITMAP, > + VIRTIO_BALLOON_F_MISC_VQ, > }; > > static struct virtio_driver virtio_balloon_driver = { > -- > 1.9.1 -- 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: email@kvack.org From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47105) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bSWsR-000450-Di for qemu-devel@nongnu.org; Wed, 27 Jul 2016 18:00:32 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bSWsN-0008NH-Oe for qemu-devel@nongnu.org; Wed, 27 Jul 2016 18:00:30 -0400 Received: from mx1.redhat.com ([209.132.183.28]:47266) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bSWsN-0008ND-Gv for qemu-devel@nongnu.org; Wed, 27 Jul 2016 18:00:27 -0400 Date: Thu, 28 Jul 2016 01:00:19 +0300 From: "Michael S. Tsirkin" Message-ID: <20160728004606-mutt-send-email-mst@kernel.org> References: <1469582616-5729-1-git-send-email-liang.z.li@intel.com> <1469582616-5729-8-git-send-email-liang.z.li@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1469582616-5729-8-git-send-email-liang.z.li@intel.com> Subject: Re: [Qemu-devel] [PATCH v2 repost 7/7] virtio-balloon: tell host vm's free page info List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Liang Li 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 On Wed, Jul 27, 2016 at 09:23:36AM +0800, Liang Li wrote: > Support the request for vm's free page information, response with > a page bitmap. QEMU can make use of this free page bitmap to speed > up live migration process by skipping process the free pages. > > Signed-off-by: Liang Li > Cc: Michael S. Tsirkin > Cc: Andrew Morton > Cc: Vlastimil Babka > Cc: Mel Gorman > Cc: Paolo Bonzini > Cc: Cornelia Huck > Cc: Amit Shah > --- > drivers/virtio/virtio_balloon.c | 104 +++++++++++++++++++++++++++++++++++++--- > 1 file changed, 98 insertions(+), 6 deletions(-) > > diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c > index 2d18ff6..5ca4ad3 100644 > --- a/drivers/virtio/virtio_balloon.c > +++ b/drivers/virtio/virtio_balloon.c > @@ -62,10 +62,13 @@ module_param(oom_pages, int, S_IRUSR | S_IWUSR); > MODULE_PARM_DESC(oom_pages, "pages to free on OOM"); > > extern unsigned long get_max_pfn(void); > +extern int get_free_pages(unsigned long start_pfn, unsigned long end_pfn, > + unsigned long *bitmap, unsigned long len); > + > > struct virtio_balloon { > struct virtio_device *vdev; > - struct virtqueue *inflate_vq, *deflate_vq, *stats_vq; > + struct virtqueue *inflate_vq, *deflate_vq, *stats_vq, *misc_vq; > > /* The balloon servicing is delegated to a freezable workqueue. */ > struct work_struct update_balloon_stats_work; > @@ -89,6 +92,8 @@ struct virtio_balloon { > unsigned long pfn_limit; > /* Used to record the processed pfn range */ > unsigned long min_pfn, max_pfn, start_pfn, end_pfn; > + /* Request header */ > + struct balloon_req_hdr req_hdr; > /* > * The pages we've told the Host we're not using are enqueued > * at vb_dev_info->pages list. > @@ -373,6 +378,49 @@ static void update_balloon_stats(struct virtio_balloon *vb) > pages_to_bytes(available)); > } > > +static void update_free_pages_stats(struct virtio_balloon *vb, why _stats? > + unsigned long req_id) > +{ > + struct scatterlist sg_in, sg_out; > + unsigned long pfn = 0, bmap_len, max_pfn; > + struct virtqueue *vq = vb->misc_vq; > + struct balloon_bmap_hdr *hdr = vb->bmap_hdr; > + int ret = 1; > + > + max_pfn = get_max_pfn(); > + mutex_lock(&vb->balloon_lock); > + while (pfn < max_pfn) { > + memset(vb->page_bitmap, 0, vb->bmap_len); > + ret = get_free_pages(pfn, pfn + vb->pfn_limit, > + vb->page_bitmap, vb->bmap_len * BITS_PER_BYTE); > + hdr->cmd = cpu_to_virtio16(vb->vdev, BALLOON_GET_FREE_PAGES); > + hdr->page_shift = cpu_to_virtio16(vb->vdev, PAGE_SHIFT); > + hdr->req_id = cpu_to_virtio64(vb->vdev, req_id); > + hdr->start_pfn = cpu_to_virtio64(vb->vdev, pfn); > + bmap_len = vb->pfn_limit / BITS_PER_BYTE; > + if (!ret) { > + hdr->flag = cpu_to_virtio16(vb->vdev, > + BALLOON_FLAG_DONE); > + if (pfn + vb->pfn_limit > max_pfn) > + bmap_len = (max_pfn - pfn) / BITS_PER_BYTE; > + } else > + hdr->flag = cpu_to_virtio16(vb->vdev, > + BALLOON_FLAG_CONT); > + hdr->bmap_len = cpu_to_virtio64(vb->vdev, bmap_len); > + sg_init_one(&sg_out, hdr, > + sizeof(struct balloon_bmap_hdr) + bmap_len); Wait a second. This adds the same buffer multiple times in a loop. We will overwrite the buffer without waiting for hypervisor to process it. What did I miss? > + > + virtqueue_add_outbuf(vq, &sg_out, 1, vb, GFP_KERNEL); this can fail. you want to maybe make sure vq has enough space before you use it or check error and wait. > + virtqueue_kick(vq); why kick here within loop? wait until done. in fact kick outside lock is better for smp. > + pfn += vb->pfn_limit; > + } > + > + sg_init_one(&sg_in, &vb->req_hdr, sizeof(vb->req_hdr)); > + virtqueue_add_inbuf(vq, &sg_in, 1, &vb->req_hdr, GFP_KERNEL); > + virtqueue_kick(vq); > + mutex_unlock(&vb->balloon_lock); > +} > + > /* > * While most virtqueues communicate guest-initiated requests to the hypervisor, > * the stats queue operates in reverse. The driver initializes the virtqueue > @@ -511,18 +559,49 @@ static void update_balloon_size_func(struct work_struct *work) > queue_work(system_freezable_wq, work); > } > > +static void misc_handle_rq(struct virtio_balloon *vb) > +{ > + struct balloon_req_hdr *ptr_hdr; > + unsigned int len; > + > + ptr_hdr = virtqueue_get_buf(vb->misc_vq, &len); > + if (!ptr_hdr || len != sizeof(vb->req_hdr)) > + return; > + > + switch (ptr_hdr->cmd) { > + case BALLOON_GET_FREE_PAGES: > + update_free_pages_stats(vb, ptr_hdr->param); > + break; > + default: > + break; > + } > +} > + > +static void misc_request(struct virtqueue *vq) > +{ > + struct virtio_balloon *vb = vq->vdev->priv; > + > + misc_handle_rq(vb); > +} > + > static int init_vqs(struct virtio_balloon *vb) > { > - struct virtqueue *vqs[3]; > - vq_callback_t *callbacks[] = { balloon_ack, balloon_ack, stats_request }; > - static const char * const names[] = { "inflate", "deflate", "stats" }; > + struct virtqueue *vqs[4]; > + vq_callback_t *callbacks[] = { balloon_ack, balloon_ack, > + stats_request, misc_request }; > + static const char * const names[] = { "inflate", "deflate", "stats", > + "misc" }; > int err, nvqs; > > /* > * We expect two virtqueues: inflate and deflate, and > * optionally stat. > */ > - nvqs = virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_STATS_VQ) ? 3 : 2; > + if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_MISC_VQ)) > + nvqs = 4; Does misc vq depend on stats vq feature then? if yes please validate that. > + else > + nvqs = virtio_has_feature(vb->vdev, > + VIRTIO_BALLOON_F_STATS_VQ) ? 3 : 2; Replace that ? with else too pls. > err = vb->vdev->config->find_vqs(vb->vdev, nvqs, vqs, callbacks, names); > if (err) > return err; > @@ -543,6 +622,16 @@ static int init_vqs(struct virtio_balloon *vb) > BUG(); > virtqueue_kick(vb->stats_vq); > } > + if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_MISC_VQ)) { > + struct scatterlist sg_in; > + > + vb->misc_vq = vqs[3]; > + sg_init_one(&sg_in, &vb->req_hdr, sizeof(vb->req_hdr)); > + if (virtqueue_add_inbuf(vb->misc_vq, &sg_in, 1, > + &vb->req_hdr, GFP_KERNEL) < 0) > + BUG(); > + virtqueue_kick(vb->misc_vq); > + } > return 0; > } > > @@ -639,8 +728,10 @@ static int virtballoon_probe(struct virtio_device *vdev) > vb->bmap_hdr = kzalloc(hdr_len + vb->bmap_len, GFP_KERNEL); > > /* Clear the feature bit if memory allocation fails */ > - if (!vb->bmap_hdr) > + if (!vb->bmap_hdr) { > __virtio_clear_bit(vdev, VIRTIO_BALLOON_F_PAGE_BITMAP); > + __virtio_clear_bit(vdev, VIRTIO_BALLOON_F_MISC_VQ); > + } > else > vb->page_bitmap = vb->bmap_hdr + hdr_len; > mutex_init(&vb->balloon_lock); > @@ -743,6 +834,7 @@ static unsigned int features[] = { > VIRTIO_BALLOON_F_STATS_VQ, > VIRTIO_BALLOON_F_DEFLATE_ON_OOM, > VIRTIO_BALLOON_F_PAGE_BITMAP, > + VIRTIO_BALLOON_F_MISC_VQ, > }; > > static struct virtio_driver virtio_balloon_driver = { > -- > 1.9.1