Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758506AbcJYJtT convert rfc822-to-8bit (ORCPT ); Tue, 25 Oct 2016 05:49:19 -0400 Received: from mga01.intel.com ([192.55.52.88]:28228 "EHLO mga01.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756998AbcJYJtQ (ORCPT ); Tue, 25 Oct 2016 05:49:16 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.31,545,1473145200"; d="scan'208";a="1058753686" 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" , "quintela@redhat.com" , "dgilbert@redhat.com" , "Hansen, Dave" , "pbonzini@redhat.com" , "cornelia.huck@de.ibm.com" , "amit.shah@redhat.com" Subject: RE: [RESEND PATCH v3 kernel 4/7] virtio-balloon: speed up inflate/deflate process Thread-Topic: [RESEND PATCH v3 kernel 4/7] virtio-balloon: speed up inflate/deflate process Thread-Index: AQHSLoocmxWiVMMnP0i/Bph0haq46aC45YxQ Date: Tue, 25 Oct 2016 09:46:33 +0000 Message-ID: References: <1477031080-12616-1-git-send-email-liang.z.li@intel.com> <1477031080-12616-5-git-send-email-liang.z.li@intel.com> <20161025091821-mutt-send-email-mst@kernel.org> In-Reply-To: <20161025091821-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: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiZmQ4NTBiMWItYWU2Zi00ZWUxLTg2NGItMmE2OTE3YzQ5OTI4IiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE1LjkuNi42IiwiVHJ1c3RlZExhYmVsSGFzaCI6InpzbHFISkpiejhBVzE5SWVVUjIyWmhvTiswRU9zZXNKcUNVU3M4bFVoQUE9In0= 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: 6550 Lines: 224 > > +static inline void init_pfn_range(struct virtio_balloon *vb) { > > + vb->min_pfn = ULONG_MAX; > > + vb->max_pfn = 0; > > +} > > + > > +static inline void update_pfn_range(struct virtio_balloon *vb, > > + struct page *page) > > +{ > > + unsigned long balloon_pfn = page_to_balloon_pfn(page); > > + > > + if (balloon_pfn < vb->min_pfn) > > + vb->min_pfn = balloon_pfn; > > + if (balloon_pfn > vb->max_pfn) > > + vb->max_pfn = balloon_pfn; > > +} > > + > > rename to hint these are all bitmap related. Will change in v4. > > > > static void tell_host(struct virtio_balloon *vb, struct virtqueue > > *vq) { > > - struct scatterlist sg; > > - unsigned int len; > > + struct scatterlist sg, sg2[BALLOON_BMAP_COUNT + 1]; > > + unsigned int len, i; > > + > > + if (virtio_has_feature(vb->vdev, > VIRTIO_BALLOON_F_PAGE_BITMAP)) { > > + struct balloon_bmap_hdr *hdr = vb->bmap_hdr; > > + unsigned long bmap_len; > > + int nr_pfn, nr_used_bmap, nr_buf; > > + > > + nr_pfn = vb->end_pfn - vb->start_pfn + 1; > > + nr_pfn = roundup(nr_pfn, BITS_PER_LONG); > > + nr_used_bmap = nr_pfn / PFNS_PER_BMAP; > > + bmap_len = nr_pfn / BITS_PER_BYTE; > > + nr_buf = nr_used_bmap + 1; > > + > > + /* cmd, reserved and req_id are init to 0, unused here */ > > + hdr->page_shift = cpu_to_virtio16(vb->vdev, PAGE_SHIFT); > > + hdr->start_pfn = cpu_to_virtio64(vb->vdev, vb->start_pfn); > > + hdr->bmap_len = cpu_to_virtio64(vb->vdev, bmap_len); > > + sg_init_table(sg2, nr_buf); > > + sg_set_buf(&sg2[0], hdr, sizeof(struct balloon_bmap_hdr)); > > + for (i = 0; i < nr_used_bmap; i++) { > > + unsigned int buf_len = BALLOON_BMAP_SIZE; > > + > > + if (i + 1 == nr_used_bmap) > > + buf_len = bmap_len - BALLOON_BMAP_SIZE > * i; > > + sg_set_buf(&sg2[i + 1], vb->page_bitmap[i], > buf_len); > > + } > > > > - sg_init_one(&sg, vb->pfns, sizeof(vb->pfns[0]) * vb->num_pfns); > > + while (vq->num_free < nr_buf) > > + msleep(2); > > > What's going on here? Who is expected to update num_free? > I just want to wait until the vq have enough space to write the bitmap, I thought qemu side will update the vq->num_free, is it wrong? > > > > + if (virtqueue_add_outbuf(vq, sg2, nr_buf, vb, GFP_KERNEL) > == 0) > > + virtqueue_kick(vq); > > > > - /* We should always be able to add one buffer to an empty queue. > */ > > - virtqueue_add_outbuf(vq, &sg, 1, vb, GFP_KERNEL); > > - virtqueue_kick(vq); > > + } else { > > + sg_init_one(&sg, vb->pfns, sizeof(vb->pfns[0]) * vb- > >num_pfns); > > + > > + /* We should always be able to add one buffer to an empty > > + * queue. */ > > Pls use a multiple comment style consistent with kernel coding style. Will change in next version. > > > + virtqueue_add_outbuf(vq, &sg, 1, vb, GFP_KERNEL); > > + virtqueue_kick(vq); > > + } > > > > /* When host has read buffer, this completes via balloon_ack */ > > wait_event(vb->acked, virtqueue_get_buf(vq, &len)); @@ -138,13 > > +199,93 @@ static void set_page_pfns(struct virtio_balloon *vb, > > page_to_balloon_pfn(page) + i); } > > > > -static unsigned fill_balloon(struct virtio_balloon *vb, size_t num) > > +static void extend_page_bitmap(struct virtio_balloon *vb) { > > + int i; > > + unsigned long bmap_len, bmap_count; > > + > > + bmap_len = ALIGN(get_max_pfn(), BITS_PER_LONG) / > BITS_PER_BYTE; > > + bmap_count = bmap_len / BALLOON_BMAP_SIZE; > > + if (bmap_len % BALLOON_BMAP_SIZE) > > + bmap_count++; > > + if (bmap_count > BALLOON_BMAP_COUNT) > > + bmap_count = BALLOON_BMAP_COUNT; > > + > > This is doing simple things in tricky ways. > Please use macros such as ALIGN and max instead of if. > Will change. > > > + for (i = 1; i < bmap_count; i++) { > > why 1? In probe stage, already allocated one bitmap. > > > + vb->page_bitmap[i] = kmalloc(BALLOON_BMAP_SIZE, > GFP_ATOMIC); > > why GFP_ATOMIC? Yes, GFP_ATOMIC is not necessary. > and what will free the previous buffer? The previous buffer will not be freed. > > > > + if (vb->page_bitmap[i]) > > + vb->nr_page_bmap++; > > + else > > + break; > > and what will happen then? I plan to use the previous allocated buffer to save the bitmap, need more code for kmalloc failure? > > -static unsigned leak_balloon(struct virtio_balloon *vb, size_t num) > > +static unsigned int leak_balloon(struct virtio_balloon *vb, size_t num, > > + bool use_bmap) > > this is just a feature bit - why not get it internally? Indeed. > > @@ -218,8 +374,14 @@ static unsigned leak_balloon(struct virtio_balloon > *vb, size_t num) > > * virtio_has_feature(vdev, VIRTIO_BALLOON_F_MUST_TELL_HOST); > > * is true, we *have* to do it in this order > > */ > > - if (vb->num_pfns != 0) > > - tell_host(vb, vb->deflate_vq); > > + if (vb->num_pfns != 0) { > > + if (use_bmap) > > + set_page_bitmap(vb, &pages, vb->deflate_vq); > > + else > > + tell_host(vb, vb->deflate_vq); > > + > > + release_pages_balloon(vb, &pages); > > + } > > release_pages_balloon(vb, &pages); > > mutex_unlock(&vb->balloon_lock); > > return num_freed_pages; > > @@ -354,13 +516,15 @@ static int virtballoon_oom_notify(struct > notifier_block *self, > > struct virtio_balloon *vb; > > unsigned long *freed; > > unsigned num_freed_pages; > > + bool use_bmap; > > > > vb = container_of(self, struct virtio_balloon, nb); > > if (!virtio_has_feature(vb->vdev, > VIRTIO_BALLOON_F_DEFLATE_ON_OOM)) > > return NOTIFY_OK; > > > > freed = parm; > > - num_freed_pages = leak_balloon(vb, oom_pages); > > + use_bmap = virtio_has_feature(vb->vdev, > VIRTIO_BALLOON_F_PAGE_BITMAP); > > + num_freed_pages = leak_balloon(vb, oom_pages, use_bmap); > > update_balloon_size(vb); > > *freed += num_freed_pages; > > > > @@ -380,15 +544,19 @@ static void update_balloon_size_func(struct > > work_struct *work) { > > struct virtio_balloon *vb; > > s64 diff; > > + bool use_bmap; > > > > vb = container_of(work, struct virtio_balloon, > > update_balloon_size_work); > > diff = towards_target(vb); > > + use_bmap = virtio_has_feature(vb->vdev, > VIRTIO_BALLOON_F_PAGE_BITMAP); > > + if (use_bmap && diff && vb->nr_page_bmap == 1) > > + extend_page_bitmap(vb); > > So you allocate it on first use, then keep it around until device remove? > Seems ugly. Yes, this version behave like this. > Needs comments explaining the motivation for this. > Can't we free it immediately when it becomes unused? > Yes, it can be freed immediately, will change in v4. Thanks for your time and your valuable comments! I will send out the v4 soon. Liang