From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> To: wei.w.wang@intel.com, mst@redhat.com Cc: mhocko@kernel.org, linux-kernel@vger.kernel.org, linux-mm@kvack.org, virtualization@lists.linux-foundation.org Subject: Re: [PATCH v1 1/3] virtio-balloon: replace the coarse-grained balloon_lock Date: Sun, 22 Oct 2017 20:50:44 +0900 [thread overview] Message-ID: <201710222050.GIF35945.FHOMQFOVSFLtOJ@I-love.SAKURA.ne.jp> (raw) In-Reply-To: <59EC7FF5.6070906@intel.com> Wei Wang wrote: > >> @@ -162,20 +160,20 @@ static unsigned fill_balloon(struct virtio_balloon *vb, size_t num) > >> msleep(200); > >> break; > >> } > >> - set_page_pfns(vb, vb->pfns + vb->num_pfns, page); > >> - vb->num_pages += VIRTIO_BALLOON_PAGES_PER_PAGE; > >> + set_page_pfns(vb, pfns + num_pfns, page); > >> if (!virtio_has_feature(vb->vdev, > >> VIRTIO_BALLOON_F_DEFLATE_ON_OOM)) > >> adjust_managed_page_count(page, -1); > >> } > >> > >> - num_allocated_pages = vb->num_pfns; > >> + mutex_lock(&vb->inflate_lock); > >> /* Did we get any? */ > >> - if (vb->num_pfns != 0) > >> - tell_host(vb, vb->inflate_vq); > >> - mutex_unlock(&vb->balloon_lock); > >> + if (num_pfns != 0) > >> + tell_host(vb, vb->inflate_vq, pfns, num_pfns); > >> + mutex_unlock(&vb->inflate_lock); > >> + atomic64_add(num_pfns, &vb->num_pages); > > Isn't this addition too late? If leak_balloon() is called due to > > out_of_memory(), it will fail to find up to dated vb->num_pages value. > > Not really. I think the old way of implementation above: > "vb->num_pages += VIRTIO_BALLOON_PAGES_PER_PAGE" > isn't quite accurate, because "vb->num_page" should reflect the number of > pages that have already been inflated, which means those pages have > already been given to the host via "tell_host()". > > If we update "vb->num_page" earlier before tell_host(), then it will > include the pages > that haven't been given to the host, which I think shouldn't be counted > as inflated pages. > > On the other hand, OOM will use leak_balloon() to release the pages that > should > have already been inflated. But leak_balloon() finds max inflated pages from vb->num_pages, doesn't it? > > >> > >> /* We can only do one array worth at a time. */ > >> - num = min(num, ARRAY_SIZE(vb->pfns)); > >> + num = min_t(size_t, num, VIRTIO_BALLOON_ARRAY_PFNS_MAX); > >> > >> - mutex_lock(&vb->balloon_lock); > >> /* We can't release more pages than taken */ > >> - num = min(num, (size_t)vb->num_pages); > >> - for (vb->num_pfns = 0; vb->num_pfns < num; > >> - vb->num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE) { > >> + num = min_t(size_t, num, atomic64_read(&vb->num_pages)); > >> + for (num_pfns = 0; num_pfns < num; > >> + num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE) { > >> page = balloon_page_dequeue(vb_dev_info); > > If balloon_page_dequeue() can be concurrently called by both host's request > > and guest's OOM event, is (!dequeued_page) test in balloon_page_dequeue() safe? > > > I'm not sure about the question. The "dequeue_page" is a local variable > in the function, why would it be unsafe for two invocations (the shared > b_dev_info->pages are operated under a lock)? I'm not MM person nor virtio person. I'm commenting from point of view of safe programming. My question is, isn't there possibility of hitting if (unlikely(list_empty(&b_dev_info->pages) && !b_dev_info->isolated_pages)) BUG(); when things run concurrently. Wei Wang wrote: > On 10/22/2017 12:11 PM, Tetsuo Handa wrote: > > Michael S. Tsirkin wrote: > >>> - num_freed_pages = leak_balloon(vb, oom_pages); > >>> + > >>> + /* Don't deflate more than the number of inflated pages */ > >>> + while (npages && atomic64_read(&vb->num_pages)) > >>> + npages -= leak_balloon(vb, npages); > > don't we need to abort if leak_balloon() returned 0 for some reason? > > I don't think so. Returning 0 should be a normal case when the host tries > to give back some pages to the guest, but there is no pages that have ever > been inflated. For example, right after booting the guest, the host sends a > deflating request to give the guest 1G memory, leak_balloon should return 0, > and guest wouldn't get 1 more G memory. > My question is, isn't there possibility of leak_balloon() returning 0 for reasons other than vb->num_pages == 0 ? If yes, this can cause infinite loop (i.e. lockups) when things run concurrently.
WARNING: multiple messages have this Message-ID (diff)
From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> To: wei.w.wang@intel.com, mst@redhat.com Cc: mhocko@kernel.org, linux-kernel@vger.kernel.org, linux-mm@kvack.org, virtualization@lists.linux-foundation.org Subject: Re: [PATCH v1 1/3] virtio-balloon: replace the coarse-grained balloon_lock Date: Sun, 22 Oct 2017 20:50:44 +0900 [thread overview] Message-ID: <201710222050.GIF35945.FHOMQFOVSFLtOJ@I-love.SAKURA.ne.jp> (raw) In-Reply-To: <59EC7FF5.6070906@intel.com> Wei Wang wrote: > >> @@ -162,20 +160,20 @@ static unsigned fill_balloon(struct virtio_balloon *vb, size_t num) > >> msleep(200); > >> break; > >> } > >> - set_page_pfns(vb, vb->pfns + vb->num_pfns, page); > >> - vb->num_pages += VIRTIO_BALLOON_PAGES_PER_PAGE; > >> + set_page_pfns(vb, pfns + num_pfns, page); > >> if (!virtio_has_feature(vb->vdev, > >> VIRTIO_BALLOON_F_DEFLATE_ON_OOM)) > >> adjust_managed_page_count(page, -1); > >> } > >> > >> - num_allocated_pages = vb->num_pfns; > >> + mutex_lock(&vb->inflate_lock); > >> /* Did we get any? */ > >> - if (vb->num_pfns != 0) > >> - tell_host(vb, vb->inflate_vq); > >> - mutex_unlock(&vb->balloon_lock); > >> + if (num_pfns != 0) > >> + tell_host(vb, vb->inflate_vq, pfns, num_pfns); > >> + mutex_unlock(&vb->inflate_lock); > >> + atomic64_add(num_pfns, &vb->num_pages); > > Isn't this addition too late? If leak_balloon() is called due to > > out_of_memory(), it will fail to find up to dated vb->num_pages value. > > Not really. I think the old way of implementation above: > "vb->num_pages += VIRTIO_BALLOON_PAGES_PER_PAGE" > isn't quite accurate, because "vb->num_page" should reflect the number of > pages that have already been inflated, which means those pages have > already been given to the host via "tell_host()". > > If we update "vb->num_page" earlier before tell_host(), then it will > include the pages > that haven't been given to the host, which I think shouldn't be counted > as inflated pages. > > On the other hand, OOM will use leak_balloon() to release the pages that > should > have already been inflated. But leak_balloon() finds max inflated pages from vb->num_pages, doesn't it? > > >> > >> /* We can only do one array worth at a time. */ > >> - num = min(num, ARRAY_SIZE(vb->pfns)); > >> + num = min_t(size_t, num, VIRTIO_BALLOON_ARRAY_PFNS_MAX); > >> > >> - mutex_lock(&vb->balloon_lock); > >> /* We can't release more pages than taken */ > >> - num = min(num, (size_t)vb->num_pages); > >> - for (vb->num_pfns = 0; vb->num_pfns < num; > >> - vb->num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE) { > >> + num = min_t(size_t, num, atomic64_read(&vb->num_pages)); > >> + for (num_pfns = 0; num_pfns < num; > >> + num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE) { > >> page = balloon_page_dequeue(vb_dev_info); > > If balloon_page_dequeue() can be concurrently called by both host's request > > and guest's OOM event, is (!dequeued_page) test in balloon_page_dequeue() safe? > > > I'm not sure about the question. The "dequeue_page" is a local variable > in the function, why would it be unsafe for two invocations (the shared > b_dev_info->pages are operated under a lock)? I'm not MM person nor virtio person. I'm commenting from point of view of safe programming. My question is, isn't there possibility of hitting if (unlikely(list_empty(&b_dev_info->pages) && !b_dev_info->isolated_pages)) BUG(); when things run concurrently. Wei Wang wrote: > On 10/22/2017 12:11 PM, Tetsuo Handa wrote: > > Michael S. Tsirkin wrote: > >>> - num_freed_pages = leak_balloon(vb, oom_pages); > >>> + > >>> + /* Don't deflate more than the number of inflated pages */ > >>> + while (npages && atomic64_read(&vb->num_pages)) > >>> + npages -= leak_balloon(vb, npages); > > don't we need to abort if leak_balloon() returned 0 for some reason? > > I don't think so. Returning 0 should be a normal case when the host tries > to give back some pages to the guest, but there is no pages that have ever > been inflated. For example, right after booting the guest, the host sends a > deflating request to give the guest 1G memory, leak_balloon should return 0, > and guest wouldn't get 1 more G memory. > My question is, isn't there possibility of leak_balloon() returning 0 for reasons other than vb->num_pages == 0 ? If yes, this can cause infinite loop (i.e. lockups) when things run concurrently. -- 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: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
next prev parent reply other threads:[~2017-10-22 11:51 UTC|newest] Thread overview: 47+ messages / expand[flat|nested] mbox.gz Atom feed top 2017-10-20 11:54 [PATCH v1 0/3] Virtio-balloon Improvement Wei Wang 2017-10-20 11:54 ` Wei Wang 2017-10-20 11:54 ` [PATCH v1 1/3] virtio-balloon: replace the coarse-grained balloon_lock Wei Wang 2017-10-20 11:54 ` Wei Wang 2017-10-22 5:20 ` Tetsuo Handa 2017-10-22 5:20 ` Tetsuo Handa 2017-10-22 5:20 ` Tetsuo Handa 2017-10-22 11:24 ` Wei Wang 2017-10-22 11:24 ` Wei Wang 2017-10-22 11:24 ` Wei Wang 2017-10-22 11:50 ` Tetsuo Handa [this message] 2017-10-22 11:50 ` Tetsuo Handa 2017-10-24 1:46 ` Wei Wang 2017-10-24 1:46 ` Wei Wang 2017-10-24 1:46 ` Wei Wang 2017-10-22 11:50 ` Tetsuo Handa 2017-10-20 11:54 ` Wei Wang 2017-10-20 11:54 ` [PATCH v1 2/3] virtio-balloon: deflate up to oom_pages on OOM Wei Wang 2017-10-20 11:54 ` Wei Wang 2017-10-20 11:54 ` Wei Wang 2017-10-22 3:21 ` Michael S. Tsirkin 2017-10-22 3:21 ` Michael S. Tsirkin 2017-10-22 3:21 ` Michael S. Tsirkin 2017-10-22 4:11 ` Tetsuo Handa 2017-10-22 4:11 ` Tetsuo Handa 2017-10-22 4:11 ` Tetsuo Handa 2017-10-22 11:31 ` Wei Wang 2017-10-22 11:31 ` Wei Wang 2017-10-22 11:31 ` Wei Wang 2017-10-20 11:54 ` [PATCH v1 3/3] virtio-balloon: stop inflating when OOM occurs Wei Wang 2017-10-20 11:54 ` Wei Wang 2017-10-22 17:13 ` Michael S. Tsirkin 2017-10-22 17:13 ` Michael S. Tsirkin 2017-10-22 17:13 ` Michael S. Tsirkin 2017-10-24 1:58 ` Wei Wang 2017-10-24 1:58 ` Wei Wang 2017-10-24 1:58 ` Wei Wang 2017-10-20 11:54 ` Wei Wang 2017-10-22 3:19 ` [PATCH v1 0/3] Virtio-balloon Improvement Michael S. Tsirkin 2017-10-22 3:19 ` Michael S. Tsirkin 2017-10-22 3:19 ` Michael S. Tsirkin 2017-10-22 11:19 ` Wei Wang 2017-10-22 11:19 ` Wei Wang 2017-10-22 11:19 ` Wei Wang 2017-11-03 8:35 ` Wei Wang 2017-11-03 8:35 ` Wei Wang 2017-11-03 8:35 ` Wei Wang
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=201710222050.GIF35945.FHOMQFOVSFLtOJ@I-love.SAKURA.ne.jp \ --to=penguin-kernel@i-love.sakura.ne.jp \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-mm@kvack.org \ --cc=mhocko@kernel.org \ --cc=mst@redhat.com \ --cc=virtualization@lists.linux-foundation.org \ --cc=wei.w.wang@intel.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.