Re: [virtio] New virtio balloon...

[Rusty Russell <rusty@au1.ibm.com>]

Daniel Kiper <daniel.kiper@oracle.com> writes:
> On Tue, Feb 04, 2014 at 12:24:48PM +1030, Rusty Russell wrote:
>> "Michael S. Tsirkin" <mst@redhat.com> writes:
>> > On Mon, Feb 03, 2014 at 02:29:36PM +0100, Daniel Kiper wrote:
>> >> On Sun, Feb 02, 2014 at 06:21:14PM +0200, Michael S. Tsirkin wrote:
>> >> > On Fri, Jan 31, 2014 at 04:01:39PM +1030, Rusty Russell wrote:
>> >> > > That's very hard to define across guests.  Should we be using stats for
>> >> > > that instead?  In fact, should we allow gratuitous stats sending,
>> >> > > instead of a simple NEED_MEM flag?
>> >>
>> >> I think that it should be simple as possible. Guest just set new target and host
>> >> fulfill request or not. Guest slow down requests from balloon if requests cannot
>> >> be fulfilled some time. That is all.
>> >
>> > Hmm that's exactly the reverse of what Rusty suggests.
>>
>> Indeed.  The current balloon is entirely host-led.  There's no way for
>
> Maybe I am missing something but I am curious why we would like to avoid
> symmetric solution. I mean that guest and host could control the target.
> Such solution is used in Xen balloon driver.

Interesting reading.

Am I understanding this correctly?  One part is simple (balloon.c),
where the balloon_process tries to meet the target.

The target comes from three places: guest sysfs, host xenstore, or
xen-selfballoon.c.  What does the host do if the guest is
self-ballooning?  It seems like they'll just fight over target values
if they both try to act.

>> But even if we had such a thing, the guest has NO IDEA how bad its
>> problems are, because bad is relative.  So it has to have a way of
>
> You mean in comparison to other guests?

And the host itself.

>> reporting its pain level to the host, who *can* balance things.
>>
>> I chose to use stats + "need_mem" flag reporting for that method.  But
>> if the host doesn't increase the target, the guest should not disobey.
>
> So if I understand correctly we are going in such direction that only
> host could control the target and guest may only gently ask for more
> memory. Right? However, as I can see we do not have a mechanism to
> directly reject guest requests for more memory. So how guest would
> know that its needs would not be fulfilled at all or they will be
> fulfilled partially? How long guest should wait for target change?
> If it does not have such information directly from a host then it
> does not have a chance to quickly limit impact of memory pressure
> in other way. So I think that the host should have a chance to reject
> guests request directly or inform that a given request will be fulfilled
> partially. This way guest will have a chance to make relevant actions
> (if it use such information).

Excellent point... hmmm, what if we put a writable 'le64 target' at the
end of the (now increasingly badly named) VIRTIO_BALLOON_GCMD_STATS?

The device MUST update this with the new target.  This means that the
driver will know as soon as the stats are digested (which should be
pretty fast).

Patch below against previous... renames to VIRTIO_BALLOON_GCMD_UPDATE.

Cheers,
Rusty.

diff --git a/drivers/virtio/virtio_balloon2.c b/drivers/virtio/virtio_balloon2.c
index cbe552802f43..01ffecb0463d 100644
--- a/drivers/virtio/virtio_balloon2.c
+++ b/drivers/virtio/virtio_balloon2.c
@@ -45,10 +45,11 @@ struct gcmd_exchange_pages {
 	__le64 to_balloon;
 };
 
-struct gcmd_stats {
-	__le64 type; /* VIRTIO_BALLOON_GCMD_STATS */
+struct gcmd_update {
+	__le64 type; /* VIRTIO_BALLOON_GCMD_UPDATE */
 	__le64 need_more;
 	struct virtio_balloon_statistic stats[VIRTIO_BALLOON_S_NR];
+	__le64 target;
 };
 
 struct hcmd_set_balloon {
@@ -94,7 +95,7 @@ struct virtio_balloon {
 		struct gcmd_get_pages get_pages;
 		struct gcmd_give_pages give_pages;
 		struct gcmd_exchange_pages exchange_pages;
-		struct gcmd_stats stats;
+		struct gcmd_update update;
 	} gcmd;
 
 	union hcmd {
@@ -207,17 +208,17 @@ static void take_from_balloon(struct virtio_balloon *vb, u64 num)
 	mutex_unlock(&vb->lock);
 }
 
-static inline void set_stat(struct gcmd_stats_reply *stats, int idx,
+static inline void set_stat(struct gcmd_update *update, int idx,
 			    u64 tag, u64 val)
 {
-	BUG_ON(idx >= ARRAY_SIZE(stats->stats));
-	stats->stats[idx].tag = cpu_to_le64(tag);
-	stats->stats[idx].val = cpu_to_le64(val);
+	BUG_ON(idx >= ARRAY_SIZE(update->stats));
+	update->stats[idx].tag = cpu_to_le64(tag);
+	update->stats[idx].val = cpu_to_le64(val);
 }
 
 #define pages_to_bytes(x) ((u64)(x) << PAGE_SHIFT)
 
-static void get_stats(struct gcmd_stats_reply *stats)
+static void get_stats(struct gcmd_update *update)
 {
 	unsigned long events[NR_VM_EVENT_ITEMS];
 	struct sysinfo i;
@@ -226,18 +227,19 @@ static void get_stats(struct gcmd_stats_reply *stats)
 	all_vm_events(events);
 	si_meminfo(&i);
 
-	stats->type = cpu_to_le64(VIRTIO_BALLOON_GCMD_STATS_REPLY);
-	set_stat(stats, idx++, VIRTIO_BALLOON_S_SWAP_IN,
+	update->type = cpu_to_le64(VIRTIO_BALLOON_GCMD_UPDATE);
+	update->need_more = cpu_to_le64(0);
+	set_stat(update, idx++, VIRTIO_BALLOON_S_SWAP_IN,
 		 pages_to_bytes(events[PSWPIN]));
-	set_stat(stats, idx++, VIRTIO_BALLOON_S_SWAP_OUT,
+	set_stat(update, idx++, VIRTIO_BALLOON_S_SWAP_OUT,
 		 pages_to_bytes(events[PSWPOUT]));
-	set_stat(stats, idx++, VIRTIO_BALLOON_S_MAJFLT,
+	set_stat(update, idx++, VIRTIO_BALLOON_S_MAJFLT,
 		 events[PGMAJFAULT]);
-	set_stat(stats, idx++, VIRTIO_BALLOON_S_MINFLT,
+	set_stat(update, idx++, VIRTIO_BALLOON_S_MINFLT,
 		 events[PGFAULT]);
-	set_stat(stats, idx++, VIRTIO_BALLOON_S_MEMFREE,
+	set_stat(update, idx++, VIRTIO_BALLOON_S_MEMFREE,
 		 pages_to_bytes(i.freeram));
-	set_stat(stats, idx++, VIRTIO_BALLOON_S_MEMTOT,
+	set_stat(update, idx++, VIRTIO_BALLOON_S_MEMTOT,
 		 pages_to_bytes(i.totalram));
 }
 
@@ -281,8 +283,9 @@ static bool process_hcmd(struct virtio_balloon *vb)
 		vb->target = le64_to_cpu(hcmd->set_balloon.target);
 		break;
 	case cpu_to_le64(VIRTIO_BALLOON_HCMD_GET_STATS):
-		get_stats(&vb->gcmd.stats_reply);
-		send_gcmd(vb, sizeof(vb->gcmd.stats_reply));
+		get_stats(&vb->gcmd.update);
+		send_gcmd(vb, sizeof(vb->gcmd.update));
+		vb->target = le64_to_cpu(vb->gcmd.update.target);
 		break;
 	default:
 		dev_err_ratelimited(&vb->vdev->dev, "Unknown hcmd %llu\n",
diff --git a/include/uapi/linux/virtio_balloon.h b/include/uapi/linux/virtio_balloon.h
index 925d79ad5c90..dcb6494f138c 100644
--- a/include/uapi/linux/virtio_balloon.h
+++ b/include/uapi/linux/virtio_balloon.h
@@ -69,8 +69,9 @@ struct virtio_balloon_statistic {
  * Stats, and optional request for memory.
  * __le64: 0 if we don't want target increased, 1 if we do.
  * Followed by 0 or more struct virtio_balloon_statistic structs.
+ * Ended by a writable __le64 with updated target.
  */
-#define VIRTIO_BALLOON_GCMD_STATS	((__le64)3)
+#define VIRTIO_BALLOON_GCMD_UPDATE	((__le64)3)
 
 /* Host->guest command queue. */