virtio_balloon: disable oom killer when fill balloon

Balloon could cause guest memory oom killing and panic. If we disable the oom killer it will be better at least avoid guest panic.

If alloc failed we can just adjust the balloon target to be equal to current number by call vdev->config->set

But during test I found the config->set num_pages does not change the config actually, Should I do hacks in userspace as well? If so where should I start to hack?

Thanks

drivers/virtio/virtio_balloon.c +40 -10

@@ -25,6 +25,7 @@
  #include<linux/freezer.h>
  #include<linux/delay.h>
  #include<linux/slab.h>
+#include<linux/oom.h>
 
  struct virtio_balloon
  {

@@ -97,8 +98,22 @@ static void tell_host(struct virtio_ball
  	wait_for_completion(&vb->acked);
  }
 
-static void fill_balloon(struct virtio_balloon *vb, size_t num)
+static int cblimit(int times)
  {
+	static int t;
+
+	if (t<  times)
+		t++;
+	else
+		t = 0;
+
+	return !t;
+}
+
+static int fill_balloon(struct virtio_balloon *vb, size_t num)
+{
+	int ret = 0;
+
  	/* We can only do one array worth at a time. */
  	num = min(num, ARRAY_SIZE(vb->pfns));
 

@@ -106,10 +121,13 @@ static void fill_balloon(struct virtio_b
  		struct page *page = alloc_page(GFP_HIGHUSER | __GFP_NORETRY |
  					__GFP_NOMEMALLOC | __GFP_NOWARN);
  		if (!page) {
-			if (printk_ratelimit())
+			if (cblimit(5)) {
  				dev_printk(KERN_INFO,&vb->vdev->dev,
  					   "Out of puff! Can't get %zu pages\n",
  					   num);
+				ret = -ENOMEM;
+				goto out;
+			}
  			/* Sleep for at least 1/5 of a second before retry. */
  			msleep(200);
  			break;

@@ -120,11 +138,11 @@ static void fill_balloon(struct virtio_b
  		list_add(&page->lru,&vb->pages);
  	}
 
-	/* Didn't get any?  Oh well. */
-	if (vb->num_pfns == 0)
-		return;
+out:
+	if (vb->num_pfns)
+		tell_host(vb, vb->inflate_vq);
 
-	tell_host(vb, vb->inflate_vq);
+	return ret;
  }
 
  static void release_pages_by_pfn(const u32 pfns[], unsigned int num)

@@ -251,6 +269,14 @@ static void update_balloon_size(struct v
  			&actual, sizeof(actual));
  }
 
+static void update_balloon_target(struct virtio_balloon *vb)
+{
+	__le32 num_pages = cpu_to_le32(vb->num_pages);
+	vb->vdev->config->set(vb->vdev,
+			      offsetof(struct virtio_balloon_config, num_pages),
+			&num_pages, sizeof(num_pages));
+}
+
  static int balloon(void *_vballoon)
  {
  	struct virtio_balloon *vb = _vballoon;

@@ -267,9 +293,14 @@ static int balloon(void *_vballoon)
  					 || freezing(current));
  		if (vb->need_stats_update)
  			stats_handle_request(vb);
-		if (diff>  0)
-			fill_balloon(vb, diff);
-		else if (diff<  0)
+		if (diff>  0) {
+			int oom;
+			oom_killer_disable();
+			oom = fill_balloon(vb, diff);
+			oom_killer_enable();
+			if (oom)
+				update_balloon_target(vb);
+		} else if (diff<  0)
  			leak_balloon(vb, -diff);
  		update_balloon_size(vb);
  	}
--

To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 4 lines · Dave Young

Balloon could cause guest memory oom killing and panic. If we disable the oom killer it will be better at least avoid guest panic.

Quoted 7 lines

If alloc failed we can just adjust the balloon target to be equal to current number by call vdev->config->set

But during test I found the config->set num_pages does not change the config actually, Should I do hacks in userspace as well? If so where should I start to hack?

The guest is not supposed to set the target field in it's config.  This is a host read/write, guest read-only field.

The problem with your approach generally speaking is that it's unclear whether this is the right policy.  For instance, what if another application held a very large allocation which caused the fill request to stop but then shortly afterwards, the aforementioned application released that allocation. If instead of just stopping, we paused and tried again later, we could potentially succeed.

I think a better approach might be a graceful back off.  For instance, when you hit this condition, deflate the balloon by 10% based on the assumption that you probably already gone too far.  Before you attempt to allocate to the target again, set a timeout that increases in duration exponentially until you reach some maximum (say 10s).

This isn't the only approach, but hopefully it conveys the idea of gracefully backing off without really giving up.

Regards,

Anthony Liguori

Quoted 111 lines

Thanks

--- linux-2.6.orig/drivers/virtio/virtio_balloon.c      2010-09-25 20:58:14.190000001 +0800
+++ linux-2.6/drivers/virtio/virtio_balloon.c   2010-09-28 21:05:42.203333675 +0800

(quoted diff) +1 -0

@@ -25,6 +25,7 @@
  #include<linux/freezer.h>
  #include<linux/delay.h>
  #include<linux/slab.h>
+#include<linux/oom.h>
 

 struct virtio_balloon
 {

drivers/virtio/virtio_balloon.c +15 -1

@@ -97,8 +98,22 @@ static void tell_host(struct virtio_ball
        wait_for_completion(&vb->acked);
  }
 
-static void fill_balloon(struct virtio_balloon *vb, size_t num)
+static int cblimit(int times)
  {
+       static int t;
+
+       if (t<  times)
+               t++;
+       else
+               t = 0;
+
+       return !t;
+}
+
+static int fill_balloon(struct virtio_balloon *vb, size_t num)
+{
+       int ret = 0;
+
        /* We can only do one array worth at a time. */
        num = min(num, ARRAY_SIZE(vb->pfns));
 

@@ -106,10 +121,13 @@ static void fill_balloon(struct virtio_b

               struct page *page = alloc_page(GFP_HIGHUSER | __GFP_NORETRY |
                                       __GFP_NOMEMALLOC | __GFP_NOWARN); if (!page) {
-                       if (printk_ratelimit())
+                       if (cblimit(5)) { dev_printk(KERN_INFO,&vb->vdev->dev, "Out of puff! Can't get %zu pages\n",
                                          num);
+                               ret = -ENOMEM;
+                               goto out;
+                       }
                       /* Sleep for at least 1/5 of a second before retry. */
                       msleep(200);
                       break;

drivers/virtio/virtio_balloon.c +12 -4

@@ -120,11 +138,11 @@ static void fill_balloon(struct virtio_b
                list_add(&page->lru,&vb->pages);
        }
 
-       /* Didn't get any?  Oh well. */
-       if (vb->num_pfns == 0)
-               return;
+out:
+       if (vb->num_pfns)
+               tell_host(vb, vb->inflate_vq);
 
-       tell_host(vb, vb->inflate_vq);
+       return ret;
  }
 
  static void release_pages_by_pfn(const u32 pfns[], unsigned int num)

@@ -251,6 +269,14 @@ static void update_balloon_size(struct v
                        &actual, sizeof(actual));
  }
 
+static void update_balloon_target(struct virtio_balloon *vb)
+{
+       __le32 num_pages = cpu_to_le32(vb->num_pages);
+       vb->vdev->config->set(vb->vdev,
+                             offsetof(struct virtio_balloon_config,
 num_pages),
+                       &num_pages, sizeof(num_pages));
+}
+
  static int balloon(void *_vballoon)
  {

       struct virtio_balloon *vb = _vballoon;

drivers/virtio/virtio_balloon.c +8 -4

@@ -267,9 +293,14 @@ static int balloon(void *_vballoon)
                                         || freezing(current));
                if (vb->need_stats_update)
                        stats_handle_request(vb);
-               if (diff>  0)
-                       fill_balloon(vb, diff);
-               else if (diff<  0)
+               if (diff>  0) {
+                       int oom;
+                       oom_killer_disable();
+                       oom = fill_balloon(vb, diff);
+                       oom_killer_enable();
+                       if (oom)
+                               update_balloon_target(vb);
+               } else if (diff<  0)
                        leak_balloon(vb, -diff);
                update_balloon_size(vb);
        }
--

To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Quoted 5 lines · Anthony Liguori

Quoted 4 lines · Dave Young

Balloon could cause guest memory oom killing and panic. If we disable the oom killer it will be better at least avoid guest panic.

Quoted 8 lines

Quoted 7 lines

If alloc failed we can just adjust the balloon target to be equal to current number by call vdev->config->set

But during test I found the config->set num_pages does not change the config actually, Should I do hacks in userspace as well? If so where should I start to hack?

Hi,

Thanks your comments.

Quoted 2 lines

The guest is not supposed to set the target field in it's config.  This is a host read/write, guest read-only field.

Could you tell where to set it? If so, IMHO set config api should fail, isn't it?

Quoted 8 lines

The problem with your approach generally speaking is that it's unclear whether this is the right policy.  For instance, what if another application held a very large allocation which caused the fill request to stop but then shortly afterwards, the aforementioned application released that allocation. If instead of just stopping, we paused and tried again later, we could potentially succeed.

Yes, it is possible. But maybe better to do balloon from qemu monitor later?

Quoted 6 lines

I think a better approach might be a graceful back off.  For instance, when you hit this condition, deflate the balloon by 10% based on the assumption that you probably already gone too far.  Before you attempt to allocate to the target again, set a timeout that increases in duration exponentially until you reach some maximum (say 10s).

I'm afraid most times it will keep doing inflate/deflate circularly.

Quoted 111 lines

This isn't the only approach, but hopefully it conveys the idea of gracefully backing off without really giving up.

Regards,

Anthony Liguori

Quoted 103 lines

Thanks

--- linux-2.6.orig/drivers/virtio/virtio_balloon.c      2010-09-25 20:58:14.190000001 +0800
+++ linux-2.6/drivers/virtio/virtio_balloon.c   2010-09-28 21:05:42.203333675 +0800

(quoted diff) +1 -0

@@ -25,6 +25,7 @@
  #include<linux/freezer.h>
  #include<linux/delay.h>
  #include<linux/slab.h>
+#include<linux/oom.h>
 

 struct virtio_balloon
 {

drivers/virtio/virtio_balloon.c +15 -1

@@ -97,8 +98,22 @@ static void tell_host(struct virtio_ball
        wait_for_completion(&vb->acked);
  }
 
-static void fill_balloon(struct virtio_balloon *vb, size_t num)
+static int cblimit(int times)
  {
+       static int t;
+
+       if (t<  times)
+               t++;
+       else
+               t = 0;
+
+       return !t;
+}
+
+static int fill_balloon(struct virtio_balloon *vb, size_t num)
+{
+       int ret = 0;
+
        /* We can only do one array worth at a time. */
        num = min(num, ARRAY_SIZE(vb->pfns));
 

@@ -106,10 +121,13 @@ static void fill_balloon(struct virtio_b

               struct page *page = alloc_page(GFP_HIGHUSER | __GFP_NORETRY |
                                       __GFP_NOMEMALLOC | __GFP_NOWARN); if (!page) {
-                       if (printk_ratelimit())
+                       if (cblimit(5)) { dev_printk(KERN_INFO,&vb->vdev->dev, "Out of puff! Can't get %zu pages\n",
                                          num);
+                               ret = -ENOMEM;
+                               goto out;
+                       }
                       /* Sleep for at least 1/5 of a second before retry. */
                       msleep(200);
                       break;

drivers/virtio/virtio_balloon.c +12 -4

@@ -120,11 +138,11 @@ static void fill_balloon(struct virtio_b
                list_add(&page->lru,&vb->pages);
        }
 
-       /* Didn't get any?  Oh well. */
-       if (vb->num_pfns == 0)
-               return;
+out:
+       if (vb->num_pfns)
+               tell_host(vb, vb->inflate_vq);
 
-       tell_host(vb, vb->inflate_vq);
+       return ret;
  }
 
  static void release_pages_by_pfn(const u32 pfns[], unsigned int num)

@@ -251,6 +269,14 @@ static void update_balloon_size(struct v
                        &actual, sizeof(actual));
  }
 
+static void update_balloon_target(struct virtio_balloon *vb)
+{
+       __le32 num_pages = cpu_to_le32(vb->num_pages);
+       vb->vdev->config->set(vb->vdev,
+                             offsetof(struct virtio_balloon_config,
 num_pages),
+                       &num_pages, sizeof(num_pages));
+}
+
  static int balloon(void *_vballoon)
  {

       struct virtio_balloon *vb = _vballoon;

drivers/virtio/virtio_balloon.c +8 -3

@@ -267,9 +293,14 @@ static int balloon(void *_vballoon)
                                         || freezing(current));
                if (vb->need_stats_update)
                        stats_handle_request(vb);
-               if (diff>  0)
-                       fill_balloon(vb, diff);
-               else if (diff<  0)
+               if (diff>  0) {
+                       int oom;
+                       oom_killer_disable();
+                       oom = fill_balloon(vb, diff);
+                       oom_killer_enable();
+                       if (oom)
+                               update_balloon_target(vb);
+               } else if (diff<  0)

Quoted 10 lines

Quoted 8 lines

drivers/virtio/virtio_balloon.c +0 -1

                        leak_balloon(vb, -diff);
                update_balloon_size(vb);
        }
--

To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
Regards
dave

Quoted 5 lines · Anthony Liguori

Quoted 3 lines · Dave Young

Balloon could cause guest memory oom killing and panic. If we disable the oom killer it will be better at least avoid guest panic.

Quoted 9 lines

Quoted 8 lines

If alloc failed we can just adjust the balloon target to be equal to current number by call vdev->config->set

But during test I found the config->set num_pages does not change the config actually, Should I do hacks in userspace as well? If so where should I start to hack?

Hi,

Thanks your comments.

Quoted 3 lines

The guest is not supposed to set the target field in it's config. This is a host read/write, guest read-only field.

Could you tell where to set it? If so, IMHO set config api should fail, isn't it?

Quoted 7 lines

The problem with your approach generally speaking is that it's unclear whether this is the right policy. For instance, what if another application held a very large allocation which caused the fill request to stop but then shortly afterwards, the aforementioned application released that allocation.
If instead of just stopping, we paused and tried again later, we could
potentially succeed.

Yes, it is possible. But maybe better to do balloon from qemu monitor later?

Quoted 6 lines

I think a better approach might be a graceful back off. For instance, when you hit this condition, deflate the balloon by 10% based on the assumption that you probably already gone too far. Before you attempt to allocate to the target again, set a timeout that increases in duration exponentially until you reach some maximum (say 10s).

I'm afraid most times it will keep doing inflate/deflate circularly.

Quoted 123 lines

This isn't the only approach, but hopefully it conveys the idea of gracefully backing off without really giving up.

Regards,

Anthony Liguori

Quoted 112 lines

Thanks

--- linux-2.6.orig/drivers/virtio/virtio_balloon.c 2010-09-25
20:58:14.190000001 +0800
+++ linux-2.6/drivers/virtio/virtio_balloon.c 2010-09-28
21:05:42.203333675 +0800

drivers/virtio/virtio_balloon.c +16 -1

@@ -25,6 +25,7 @@
  #include<linux/freezer.h>
  #include<linux/delay.h>
  #include<linux/slab.h>
+#include<linux/oom.h>
 
  struct virtio_balloon
  {

@@ -97,8 +98,22 @@ static void tell_host(struct virtio_ball
        wait_for_completion(&vb->acked);
  }
 
-static void fill_balloon(struct virtio_balloon *vb, size_t num)
+static int cblimit(int times)
  {
+       static int t;
+
+       if (t<    times)
+               t++;
+       else
+               t = 0;
+
+       return !t;
+}
+
+static int fill_balloon(struct virtio_balloon *vb, size_t num)
+{
+       int ret = 0;
+
        /* We can only do one array worth at a time. */
        num = min(num, ARRAY_SIZE(vb->pfns));
 

@@ -106,10 +121,13 @@ static void fill_balloon(struct virtio_b
                struct page *page = alloc_page(GFP_HIGHUSER | __GFP_NORETRY

|

                                        __GFP_NOMEMALLOC | __GFP_NOWARN);
                if (!page) {

drivers/virtio/virtio_balloon.c +1 -1

-                       if (printk_ratelimit())
+                       if (cblimit(5)) {
                                dev_printk(KERN_INFO,&vb->vdev->dev,
                                           "Out of puff! Can't get %zu

pages\n",

                                           num);

drivers/virtio/virtio_balloon.c +3 -0

+                               ret = -ENOMEM;
+                               goto out;
+                       }
                        /* Sleep for at least 1/5 of a second before retry.

*/

                        msleep(200);
                        break;

drivers/virtio/virtio_balloon.c +12 -4

@@ -120,11 +138,11 @@ static void fill_balloon(struct virtio_b
                list_add(&page->lru,&vb->pages);
        }
 
-       /* Didn't get any?  Oh well. */
-       if (vb->num_pfns == 0)
-               return;
+out:
+       if (vb->num_pfns)
+               tell_host(vb, vb->inflate_vq);
 
-       tell_host(vb, vb->inflate_vq);
+       return ret;
  }
 
  static void release_pages_by_pfn(const u32 pfns[], unsigned int num)

@@ -251,6 +269,14 @@ static void update_balloon_size(struct v
                        &actual, sizeof(actual));
  }
 
+static void update_balloon_target(struct virtio_balloon *vb)
+{
+       __le32 num_pages = cpu_to_le32(vb->num_pages);
+       vb->vdev->config->set(vb->vdev,
+                             offsetof(struct virtio_balloon_config,
 num_pages),
+&num_pages, sizeof(num_pages));
+}
+
  static int balloon(void *_vballoon)
  {

        struct virtio_balloon *vb = _vballoon;

drivers/virtio/virtio_balloon.c +8 -4

@@ -267,9 +293,14 @@ static int balloon(void *_vballoon)
                                         || freezing(current));
                if (vb->need_stats_update)
                        stats_handle_request(vb);
-               if (diff>    0)
-                       fill_balloon(vb, diff);
-               else if (diff<    0)
+               if (diff>    0) {
+                       int oom;
+                       oom_killer_disable();
+                       oom = fill_balloon(vb, diff);
+                       oom_killer_enable();
+                       if (oom)
+                               update_balloon_target(vb);
+               } else if (diff<    0)
                        leak_balloon(vb, -diff);
                update_balloon_size(vb);
        }
--

To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 51 lines · Dave Young

Quoted 11 lines · Anthony Liguori

Quoted 8 lines · Dave Young

Balloon could cause guest memory oom killing and panic. If we disable the
oom killer it will be better at least avoid guest panic.

If alloc failed we can just adjust the balloon target to be equal to current number by call vdev->config->set

Quoted 8 lines

Quoted 6 lines

But during test I found the config->set num_pages does not change the config actually, Should I do hacks in userspace as well? If so where should
I start to hack?

Hi,

Thanks your comments.

Quoted 5 lines

The guest is not supposed to set the target field in it's config.  This is a
host read/write, guest read-only field.

Could you tell where to set it? If so, IMHO set config api should fail, isn't it?

Quoted 11 lines

The problem with your approach generally speaking is that it's unclear whether this is the right policy.  For instance, what if another application
held a very large allocation which caused the fill request to stop but then
shortly afterwards, the aforementioned application released that allocation.
 If instead of just stopping, we paused and tried again later, we could potentially succeed.

Quoted 3 lines

Yes, it is possible. But maybe better to do balloon from qemu monitor later?

It's part of the specification, not something that's enforced or even visible within the APIs.

Quoted 10 lines

Quoted 9 lines

I think a better approach might be a graceful back off.  For instance, when
you hit this condition, deflate the balloon by 10% based on the assumption
that you probably already gone too far.  Before you attempt to allocate to
the target again, set a timeout that increases in duration exponentially until you reach some maximum (say 10s).

Quoted 2 lines

I'm afraid most times it will keep doing inflate/deflate circularly.

With sufficiently large timeouts, does it matter?

The other side of the argument is that the host should be more careful about doing balloon requests to the guest.  Alternatively, you can argue that the guest should be able to balloon itself and that's where the logic should be.

But I think split policy across the guest and host would prove to be prohibitively complex to deal with.

Regards,

Anthony Liguori

Quoted 130 lines

Quoted 127 lines

This isn't the only approach, but hopefully it conveys the idea of gracefully backing off without really giving up.

Regards,

Anthony Liguori

Quoted 115 lines

Thanks

--- linux-2.6.orig/drivers/virtio/virtio_balloon.c      2010-09-25 20:58:14.190000001 +0800
+++ linux-2.6/drivers/virtio/virtio_balloon.c   2010-09-28 21:05:42.203333675 +0800

(quoted diff) +1 -0

@@ -25,6 +25,7 @@
  #include<linux/freezer.h>
  #include<linux/delay.h>
  #include<linux/slab.h>
+#include<linux/oom.h>
 

 struct virtio_balloon
 {

drivers/virtio/virtio_balloon.c +15 -1

@@ -97,8 +98,22 @@ static void tell_host(struct virtio_ball
        wait_for_completion(&vb->acked);
  }
 
-static void fill_balloon(struct virtio_balloon *vb, size_t num)
+static int cblimit(int times)
  {
+       static int t;
+
+       if (t<    times)
+               t++;
+       else
+               t = 0;
+
+       return !t;
+}
+
+static int fill_balloon(struct virtio_balloon *vb, size_t num)
+{
+       int ret = 0;
+
        /* We can only do one array worth at a time. */
        num = min(num, ARRAY_SIZE(vb->pfns));
 

@@ -106,10 +121,13 @@ static void fill_balloon(struct virtio_b

               struct page *page = alloc_page(GFP_HIGHUSER | __GFP_NORETRY
|
                                       __GFP_NOMEMALLOC | __GFP_NOWARN); if (!page) {
-                       if (printk_ratelimit())
+                       if (cblimit(5)) { dev_printk(KERN_INFO,&vb->vdev->dev, "Out of puff! Can't get %zu pages\n",
                                          num);
+                               ret = -ENOMEM;
+                               goto out;
+                       }
                       /* Sleep for at least 1/5 of a second before retry.
*/
                       msleep(200);
                       break;

drivers/virtio/virtio_balloon.c +12 -4

@@ -120,11 +138,11 @@ static void fill_balloon(struct virtio_b
                list_add(&page->lru,&vb->pages);
        }
 
-       /* Didn't get any?  Oh well. */
-       if (vb->num_pfns == 0)
-               return;
+out:
+       if (vb->num_pfns)
+               tell_host(vb, vb->inflate_vq);
 
-       tell_host(vb, vb->inflate_vq);
+       return ret;
  }
 
  static void release_pages_by_pfn(const u32 pfns[], unsigned int num)

@@ -251,6 +269,14 @@ static void update_balloon_size(struct v
                        &actual, sizeof(actual));
  }
 
+static void update_balloon_target(struct virtio_balloon *vb)
+{
+       __le32 num_pages = cpu_to_le32(vb->num_pages);
+       vb->vdev->config->set(vb->vdev,
+                             offsetof(struct virtio_balloon_config,
 num_pages),
+&num_pages, sizeof(num_pages));
+}
+
  static int balloon(void *_vballoon)
  {

       struct virtio_balloon *vb = _vballoon;

drivers/virtio/virtio_balloon.c +8 -4

@@ -267,9 +293,14 @@ static int balloon(void *_vballoon)
                                         || freezing(current));
                if (vb->need_stats_update)
                        stats_handle_request(vb);
-               if (diff>    0)
-                       fill_balloon(vb, diff);
-               else if (diff<    0)
+               if (diff>    0) {
+                       int oom;
+                       oom_killer_disable();
+                       oom = fill_balloon(vb, diff);
+                       oom_killer_enable();
+                       if (oom)
+                               update_balloon_target(vb);
+               } else if (diff<    0)
                        leak_balloon(vb, -diff);
                update_balloon_size(vb);
        }
--

To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html