Re: [virtio] New virtio balloon...

[Daniel Kiper <daniel.kiper@oracle.com>]

On Wed, Feb 05, 2014 at 01:45:38PM +1030, Rusty Russell wrote:
> 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.

Yep.

> The target comes from three places: guest sysfs, host xenstore, or
> xen-selfballoon.c.  What does the host do if the guest is

Yep.

> self-ballooning?  It seems like they'll just fight over target values
> if they both try to act.

Sadly, yep. I know about that issue. Do you mean that we would like to avoid
such situation in new VIRTIO balloon driver? Any other reasons?

[...]

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

Good idea but...

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

Great...

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

Hmmm... Update what? Maybe VIRTIO_BALLOON_GCMD_UPDATE_STATS
or VIRTIO_BALLOON_GCMD_UPDATE_STATE? Both are not perfect
but are better then simple update...

>  	__le64 need_more;

I do not think that it is needed if we have a target here. I hope that
host is smart and can make simple comparison with current target.

>  	struct virtio_balloon_statistic stats[VIRTIO_BALLOON_S_NR];
> +	__le64 target;

Nice but:
  - what to do if guest puts here target below current target;
    I think that a host should do what is asked for (if it is possible)
    - just lower target; Otherwise host should reject target us usual,
  - 0 (zero) should be a special value; it should mean that guest
    asks for nothing or does not have any preference.

Daniel