Re: [virtio-dev] Re: Straw linux/lguest implementation of new "memballoon"

[Amit Shah <amit.shah@redhat.com>]

On (Wed) 28 May 2014 [17:09:16], Rusty Russell wrote:
> Amit Shah <amit.shah@redhat.com> writes:
> > On (Thu) 15 May 2014 [13:08:20], Rusty Russell wrote:
> >> And here's the spec change.  It adds one thing that the implementation
> >> didn't have: a way for the host to refuse to release pages (using a simple
> >> "how many pages did I return" counter).
> >> 
> >> I would *really* like feedback on this approach!
> ...
> >> +The virtio memory balloon device allows the guest to voluntarily
> >> +return memory to the host.  The device can also be used to communicate
> >
> > return _some_ memory to the host?
> 
> Sure.
> 
> >> +guest memory statistics to the host.
> >
> > I don't see changes related to stats here.
> 
> You're right.  I've deleted that sentence; we need to have a discussion
> on whether stats are actually valuable as they are today.

What would a guest admin allow a host admin to know?
1. How much memory the guest was started with
2. Did the guest ask for extra pages (negative balloon)
3. What is the size of the current balloon

In addition, a host admin would like to know (well, it'd like to know
a lot more, but being realistic):
4. Did the guest refuse to give it memory when asked?  How many times?
5. Did the guest voluntarily give up memory when not asked for (min
   going beyond the min balloon)

Anything else?

None of these require anything specific from the spec.

> >> +The driver tracks which pages are in the balloon, so it can ask for
> >> +them back, and so it knows the balloon size for handling
> >> +VIRTIO_MEMBALLOON_HCMD_MIN_BALLOON.
> >
> > "ask for them back" reads awkwardly, how about "ask for them back from
> > the device"?  Or some other wording altogether?
> 
> Agreed.  Hows this:
> 
> The driver is responsible for tracking which pages it has given to the
> balloon (it needs that for the VIRTIO_MEMBALLOON_GCMD_GET_PAGES and
> VIRTIO_MEMBALLOON_GCMD_EXCHANGE_PAGES commands).  It also needs to know
> the balloon size for handling VIRTIO_MEMBALLOON_HCMD_MIN_BALLOON.

+1.

> >> +\begin{enumerate}
> >> +\item The driver contructs a buffer of le64 values.  The first is
> >
> > typo in constructs
> 
> Err, would you believe contruct is a portmanteau of ... OK, I got
> nothing.  Replaced them all (it shows that I cut and paste!)

Heh.

> >> +\item When the device has finished with the buffer, the final 64-bit value
> >> +  indicates the number of pages which were successfully obtained.
> >> +\end{enumerate}
> >
> > How does the driver communicate addresses of pages it didn't have
> > (EXTRA_MEM)?  And how does the device give them to the driver?
> 
> The driver explicitly tells the device "I want these pages" in
> VIRTIO_MEMBALLOON_GCMD_GET_PAGES.  EXTRA_MEM means those page addresses
> could be outside normal memory:
> 
>   Pages are taken from the balloon using the
>   VIRTIO_MEMBALLOON_GCMD_GET_PAGES command (and if
>   VIRTIO_MEMBALLOON_F_EXTRA_MEM is negotiated, the driver can take pages
>   it didn't put in).

Yep, clearer, thanks.

> > Also, if a device gives control of 2 out of 5 requested pages, which
> > pages were the ones the device gave back?
> 
> The first two.

Should this be explicit?  I think so.

> >> +\subsubsection{Exchanging Pages With The Balloon}\label{sec:Device Types / Memory Balloon Device / Device Operation / Exchanging Pages With The Balloon}
> >> +
> >> +Since VIRTIO_MEMBALLOON_GCMD_GET_PAGES can fail, we provide an explicit
> >> +zero-sum exhange operation which can't:
> >
> > typo in exchange
> 
> Thanks.
> 
> > "cannot" instead of "can't"?
> >
> > "we provide": is such usage fine for a spec?
> 
> How's this:
> 
>    Since VIRTIO_MEMBALLOON_GCMD_GET_PAGES can fail,
>    VIRTIO_MEMBALLOON_GCMD_EXCHANGE_PAGES provides an explicit zero-sum
>    exchange operation which cannot:

+1

> >> +\begin{enumerate}
> >> +\item The driver contructs a buffer of le64 values.  The first is
> >> +  device-readable VIRTIO_MEMBALLOON_GCMD_EXCHANGE_PAGES, followed by one
> >> +  or more device-readable 64-bit page addresses of pages it wants,
> >> +  followed by the same number of device-readable 64-bit page addresses of pages
> >> +  to enter into the balloon.
> >> +\item The driver places the buffer into the todevq and notifies the device.
> >> +\end{enumerate}
> >
> > Leave the math to the driver and the device?  Or be explicit and
> > announce how many addresses follow?  (Applies to all the commands.)
> 
> Since we have an explicit descriptor length, we might as well use it.
> The entropy device works the same way.  I thought about pairing the
> pages adjacent, though, which might be slightly easier to implement.
> 
> How's this:
> 
> \item The driver constructs a buffer of le64 values.  The first is
>   device-readable VIRTIO_MEMBALLOON_GCMD_EXCHANGE_PAGES, followed by
>   one or more pairs of device-readable 64-bit page addresses.  The
>   first address is the page the driver wants from the balloon, the
>   second is the page to add to the balloon.

Yes, I like this better.

> >> +\item The driver contructs a buffer of two device-writable le64 values,
> >> +  places it in the fromdevq queue and notifies the device.
> >
> > When does this happen?  How does a driver know it's supposed to
> > construct the buffer and notify the device?
> 
> It keeps the fromdevq populated with at least one buffer (same pattern
> as other device-notification queues):
> 
> \item The driver adds one empty buffer of at least 16 bytes to the
>   fromdevq virtqueue and notifies the device.
> 
> I've added this requirement:
> 
> The driver SHOULD ensure one device-writable buffer of at least 16
> bytes is in the fromdevq virtqueue at all times.

Yes, better.

> >> +\item When (if) it wishes to set the minimum, the device fills the
> >> +  first value with VIRTIO_MEMBALLOON_HCMD_MIN_BALLOON, and the
> >> +  second with a (signed) number of bytes which is the new minimum and
> >> +  sends an interrupt to the driver.
> >
> > Reading ahead, I see that the signed value is to specify there were
> > extra pages given to the driver by the device.
> >
> > This means the value is always absolute: i.e. if the initial minimum
> > was 2G, and then host went out of memory pressure, and revised the new
> > minimum to 1G, it'll just communicate 1G to the guest, not -1G, right?
> >
> > How will -ve values work with extra pages given to the guest?
> >
> > I don't really understand the extra pages thing well.
> 
> It starts at 0 (no minimum balloon size).  Normally, it will be
> positive, but with EXTRA_MEM it could be negative (should be, in fact).
> If the device wants to let the driver have 1G of extra memory, it will
> set the minimum to -1G.

Got it.

> >> +\item The driver refills the buffer for the next notification.
> >> +\item If the balloon is below the minimum, the driver adds pages to
> >> +  the balloon.
> >
> > driver MAY add pages to the balloon?

I mean, the driver has an option to not add pages here..

> The actual requirements are below, this is just the operational
> description.  Here is the matching requirements:
> 
>   The driver SHOULD give pages to the balloon when the balloon is
>   below the minimum specified by VIRTIO_MEMBALLOON_HCMD_MIN_BALLOON
> 
> >> +The device MUST have no pages in the balloon after reset.
> >
> > after driver system reset?  Also, when the driver is unloaded from the
> > guest?
> 
> Device reset.

Ah, right.

> When the driver is unloaded is a very interesting question.  Unloading
> is hard, since the driver must track.  It can ask for all its pages back
> from the balloon (as our current driver does), or track them some other
> way, or leak memory.

Right, none of these belong to the spec, so ignore.

> >> +The driver MUST specify one or more pages in
> >> +VIRTIO_MEMBALLOON_GCMD_GIVE_PAGES and VIRTIO_MEMBALLOON_GCMD_GET_PAGES
> >> +commands, and one or more pair of pages in the
> >> +VIRTIO_MEMBALLOON_GCMD_EXCHANGE_PAGES command.
> >> +
> >> +The driver MUST NOT access pages which have been given to the balloon.
> >
> > Not just the driver, but the OS itself must not use those pages?
> 
> Yes, but we tend to talk about the driver.  How about:
> 
>         The driver MUST NOT allow any access to pages which have been given to
>         the balloon.

Yea.

I forgot to mention the first time around: overall I like the new
design; it also adds atomicity to the give- and get- pages operations,
where the previous balloon did not have that.

Thanks,

		Amit