Wei Wang wrote:

quoted

quoted

@@ -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?

quoted

quoted

  
  	/* 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:

quoted

Michael S. Tsirkin wrote:

quoted

quoted

-	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>