Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752678AbeAQJ2A (ORCPT + 1 other); Wed, 17 Jan 2018 04:28:00 -0500 Received: from mx1.redhat.com ([209.132.183.28]:42902 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752660AbeAQJ1w (ORCPT ); Wed, 17 Jan 2018 04:27:52 -0500 Date: Wed, 17 Jan 2018 04:27:51 -0500 (EST) From: Pankaj Gupta To: Wei Wang Cc: virtio-dev@lists.oasis-open.org, linux-kernel@vger.kernel.org, virtualization@lists.linux-foundation.org, kvm@vger.kernel.org, linux-mm@kvack.org, mst@redhat.com, mhocko@kernel.org, akpm@linux-foundation.org, pbonzini@redhat.com, liliang opensource , yang zhang wz , quan xu0 , nilal@redhat.com, riel@redhat.com Message-ID: <1239524301.1023371.1516181271621.JavaMail.zimbra@redhat.com> In-Reply-To: <5A5F109B.7090200@intel.com> References: <1516165812-3995-1-git-send-email-wei.w.wang@intel.com> <1516165812-3995-3-git-send-email-wei.w.wang@intel.com> <1003745745.1007975.1516177271163.JavaMail.zimbra@redhat.com> <5A5F109B.7090200@intel.com> Subject: Re: [PATCH v22 2/3] virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_VQ MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-Originating-IP: [10.67.116.141, 10.4.195.25] Thread-Topic: virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_VQ Thread-Index: JIHwmq5He5GplFQ5dwnXirR4fvrGjw== X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.32]); Wed, 17 Jan 2018 09:27:52 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Return-Path: > On 01/17/2018 04:21 PM, Pankaj Gupta wrote: > >> Negotiation of the VIRTIO_BALLOON_F_FREE_PAGE_VQ feature indicates the > >> support of reporting hints of guest free pages to host via virtio-balloon. > >> > >> Host requests the guest to report free pages by sending a new cmd > >> id to the guest via the free_page_report_cmd_id configuration register. > >> > >> When the guest starts to report, the first element added to the free page > >> vq is the cmd id given by host. When the guest finishes the reporting > >> of all the free pages, VIRTIO_BALLOON_FREE_PAGE_REPORT_STOP_ID is added > >> to the vq to tell host that the reporting is done. Host may also requests > >> the guest to stop the reporting in advance by sending the stop cmd id to > >> the guest via the configuration register. > >> > >> Signed-off-by: Wei Wang > >> Signed-off-by: Liang Li > >> Cc: Michael S. Tsirkin > >> Cc: Michal Hocko > >> --- > >> drivers/virtio/virtio_balloon.c | 242 > >> +++++++++++++++++++++++++++++++----- > >> include/uapi/linux/virtio_balloon.h | 4 + > >> 2 files changed, 214 insertions(+), 32 deletions(-) > >> > >> diff --git a/drivers/virtio/virtio_balloon.c > >> b/drivers/virtio/virtio_balloon.c > >> index a1fb52c..b9561a5 100644 > >> --- a/drivers/virtio/virtio_balloon.c > >> +++ b/drivers/virtio/virtio_balloon.c > >> @@ -53,7 +53,12 @@ static struct vfsmount *balloon_mnt; > >> > >> struct virtio_balloon { > >> struct virtio_device *vdev; > >> - struct virtqueue *inflate_vq, *deflate_vq, *stats_vq; > >> + struct virtqueue *inflate_vq, *deflate_vq, *stats_vq, *free_page_vq; > >> + > >> + /* Balloon's own wq for cpu-intensive work items */ > >> + struct workqueue_struct *balloon_wq; > >> + /* The free page reporting work item submitted to the balloon wq */ > >> + struct work_struct report_free_page_work; > >> > >> /* The balloon servicing is delegated to a freezable workqueue. */ > >> struct work_struct update_balloon_stats_work; > >> @@ -63,6 +68,13 @@ struct virtio_balloon { > >> spinlock_t stop_update_lock; > >> bool stop_update; > >> > >> + /* Start to report free pages */ > >> + bool report_free_page; > >> + /* Stores the cmd id given by host to start the free page reporting */ > >> + uint32_t start_cmd_id; > >> + /* Stores STOP_ID as a sign to tell host that the reporting is done */ > >> + uint32_t stop_cmd_id; > >> + > >> /* Waiting for host to ack the pages we released. */ > >> wait_queue_head_t acked; > >> > >> @@ -281,6 +293,71 @@ static unsigned int update_balloon_stats(struct > >> virtio_balloon *vb) > >> return idx; > >> } > >> > >> +static void add_one_sg(struct virtqueue *vq, unsigned long pfn, uint32_t > >> len) > >> +{ > >> + struct scatterlist sg; > >> + unsigned int unused; > >> + int err; > >> + > >> + sg_init_table(&sg, 1); > >> + sg_set_page(&sg, pfn_to_page(pfn), len, 0); > >> + > >> + /* Detach all the used buffers from the vq */ > >> + while (virtqueue_get_buf(vq, &unused)) > >> + ; > >> + > >> + /* > >> + * Since this is an optimization feature, losing a couple of free > >> + * pages to report isn't important. We simply resturn without adding > >> + * the page if the vq is full. We are adding one entry each time, > >> + * which essentially results in no memory allocation, so the > >> + * GFP_KERNEL flag below can be ignored. > >> + */ > >> + if (vq->num_free) { > >> + err = virtqueue_add_inbuf(vq, &sg, 1, vq, GFP_KERNEL); > >> + /* > >> + * This is expected to never fail, because there is always an > >> + * entry available on the vq. > >> + */ > >> + BUG_ON(err); > >> + } > >> +} > >> + > >> +static void batch_free_page_sg(struct virtqueue *vq, > >> + unsigned long pfn, > >> + uint32_t len) > >> +{ > >> + add_one_sg(vq, pfn, len); > >> + > >> + /* Batch till the vq is full */ > >> + if (!vq->num_free) > >> + virtqueue_kick(vq); > >> +} > >> + > >> +static void send_cmd_id(struct virtqueue *vq, void *addr) > >> +{ > >> + struct scatterlist sg; > >> + unsigned int unused; > >> + int err; > >> + > >> + sg_init_one(&sg, addr, sizeof(uint32_t)); > >> + > >> + /* > >> + * This handles the cornercase that the vq happens to be full when > >> + * adding a cmd id. Rarely happen in practice. > >> + */ > >> + while (!vq->num_free) > >> + virtqueue_get_buf(vq, &unused); > >> + > >> + err = virtqueue_add_outbuf(vq, &sg, 1, vq, GFP_KERNEL); > >> + /* > >> + * This is expected to never fail, because there is always an > >> + * entry available on the vq. > >> + */ > >> + BUG_ON(err); > >> + virtqueue_kick(vq); > >> +} > >> + > >> /* > >> * While most virtqueues communicate guest-initiated requests to the > >> hypervisor, > >> * the stats queue operates in reverse. The driver initializes the > >> virtqueue > >> @@ -316,17 +393,6 @@ static void stats_handle_request(struct > >> virtio_balloon > >> *vb) > >> virtqueue_kick(vq); > >> } > >> > >> -static void virtballoon_changed(struct virtio_device *vdev) > >> -{ > >> - struct virtio_balloon *vb = vdev->priv; > >> - unsigned long flags; > >> - > >> - spin_lock_irqsave(&vb->stop_update_lock, flags); > >> - if (!vb->stop_update) > >> - queue_work(system_freezable_wq, &vb->update_balloon_size_work); > >> - spin_unlock_irqrestore(&vb->stop_update_lock, flags); > >> -} > >> - > >> static inline s64 towards_target(struct virtio_balloon *vb) > >> { > >> s64 target; > >> @@ -343,6 +409,36 @@ static inline s64 towards_target(struct > >> virtio_balloon > >> *vb) > >> return target - vb->num_pages; > >> } > >> > >> +static void virtballoon_changed(struct virtio_device *vdev) > >> +{ > >> + struct virtio_balloon *vb = vdev->priv; > >> + unsigned long flags; > >> + __u32 cmd_id; > >> + s64 diff = towards_target(vb); > >> + > >> + if (diff) { > >> + spin_lock_irqsave(&vb->stop_update_lock, flags); > >> + if (!vb->stop_update) > >> + queue_work(system_freezable_wq, > >> + &vb->update_balloon_size_work); > >> + spin_unlock_irqrestore(&vb->stop_update_lock, flags); > >> + } > >> + > >> + virtio_cread(vb->vdev, struct virtio_balloon_config, > >> + free_page_report_cmd_id, &cmd_id); > >> + if (cmd_id == VIRTIO_BALLOON_FREE_PAGE_REPORT_STOP_ID) { > >> + WRITE_ONCE(vb->report_free_page, false); > >> + } else if (cmd_id != vb->start_cmd_id) { > >> + /* > >> + * Host requests to start the reporting by sending a new cmd > >> + * id. > >> + */ > >> + WRITE_ONCE(vb->report_free_page, true); > >> + vb->start_cmd_id = cmd_id; > >> + queue_work(vb->balloon_wq, &vb->report_free_page_work); > >> + } > >> +} > >> + > >> static void update_balloon_size(struct virtio_balloon *vb) > >> { > >> u32 actual = vb->num_pages; > >> @@ -417,40 +513,113 @@ static void update_balloon_size_func(struct > >> work_struct *work) > >> > >> 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" }; > >> - int err, nvqs; > >> + struct virtqueue **vqs; > >> + vq_callback_t **callbacks; > >> + const char **names; > >> + struct scatterlist sg; > >> + int i, nvqs, err = -ENOMEM; > >> + > >> + /* Inflateq and deflateq are used unconditionally */ > >> + nvqs = 2; > >> + if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_STATS_VQ)) > >> + nvqs++; > >> + if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_FREE_PAGE_VQ)) > >> + nvqs++; > >> + > >> + /* Allocate space for find_vqs parameters */ > >> + vqs = kcalloc(nvqs, sizeof(*vqs), GFP_KERNEL); > >> + if (!vqs) > >> + goto err_vq; > >> + callbacks = kmalloc_array(nvqs, sizeof(*callbacks), GFP_KERNEL); > >> + if (!callbacks) > >> + goto err_callback; > >> + names = kmalloc_array(nvqs, sizeof(*names), GFP_KERNEL); > >> + if (!names) > >> + goto err_names; > >> + > >> + callbacks[0] = balloon_ack; > >> + names[0] = "inflate"; > >> + callbacks[1] = balloon_ack; > >> + names[1] = "deflate"; > >> + > >> + i = 2; > >> + if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_STATS_VQ)) { > >> + callbacks[i] = stats_request; > >> + names[i] = "stats"; > >> + i++; > >> + } > >> > >> - /* > >> - * We expect two virtqueues: inflate and deflate, and > >> - * optionally stat. > >> - */ > >> - nvqs = virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_STATS_VQ) ? 3 : 2; > >> - err = virtio_find_vqs(vb->vdev, nvqs, vqs, callbacks, names, NULL); > >> + if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_FREE_PAGE_VQ)) { > >> + callbacks[i] = NULL; > >> + names[i] = "free_page_vq"; > >> + } > >> + > >> + err = vb->vdev->config->find_vqs(vb->vdev, nvqs, vqs, callbacks, names, > >> + NULL, NULL); > >> if (err) > >> - return err; > >> + goto err_find; > >> > >> vb->inflate_vq = vqs[0]; > >> vb->deflate_vq = vqs[1]; > >> + i = 2; > >> if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_STATS_VQ)) { > >> - struct scatterlist sg; > >> - unsigned int num_stats; > >> - vb->stats_vq = vqs[2]; > >> - > >> + vb->stats_vq = vqs[i++]; > >> /* > >> * Prime this virtqueue with one buffer so the hypervisor can > >> * use it to signal us later (it can't be broken yet!). > >> */ > >> - num_stats = update_balloon_stats(vb); > >> - > >> - sg_init_one(&sg, vb->stats, sizeof(vb->stats[0]) * num_stats); > >> + sg_init_one(&sg, vb->stats, sizeof(vb->stats)); > >> if (virtqueue_add_outbuf(vb->stats_vq, &sg, 1, vb, GFP_KERNEL) > >> - < 0) > >> - BUG(); > >> + < 0) { > >> + dev_warn(&vb->vdev->dev, "%s: add stat_vq failed\n", > >> + __func__); > >> + goto err_find; > >> + } > >> virtqueue_kick(vb->stats_vq); > >> } > >> + > >> + if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_FREE_PAGE_VQ)) > >> + vb->free_page_vq = vqs[i]; > >> + > >> + kfree(names); > >> + kfree(callbacks); > >> + kfree(vqs); > >> return 0; > > > > We can assign err=0 and remove above duplicate code? > > > > Where do you want to assign err=0? Could you show it using code? o.k you have initialize "err = -ENOMEM;" Remove these four lines. - kfree(names); - kfree(callbacks); - kfree(vqs); - return 0; + err = 0; // if executed without any error Below code is already there, so for error, err is already 'ENOMEM' and a jump to any label. + +err_find: + kfree(names); +err_names: + kfree(callbacks); +err_callback: + kfree(vqs); > +err_vq: > + return err; > +} Thanks, Pankaj > > > Best, > Wei > > >