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

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

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.

>> +The driver MUST negotiate only one of the page size features (4k to
>> +16G pages).  If it cannot negotiate any page size feature, the driver
>> +MUST not set FEATURES_OK, and it MAY set the FAILED status bit.
>
> How will this negotiation work?  Does the device offer a range of
> valid page sizes, and the driver accepts one?  In what order is
> negotiation done?

Yep.  That's how normal feature negotiation works; in this case the
caveat is that it can't accept more than one page size feature.

>> +The driver chooses when to add or remove pages based on unspecified
>> +internal heuristics, which can be overridden by the
>> +VIRTIO_MEMBALLOON_HCMD_MIN_BALLOON command from the driver via the
>> +fromdevq.
>
> from the *device*

Thanks, fixed!

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


>> +\begin{description}
>> +\item[VIRTIO_MEMBALLOON_GCMD_GET_PAGES (0)] Get one or more pages out of the balloon (todevq).
>
> "Attempt to get one or more..."?

Agreed.

>> +\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!)

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

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

>> +\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:

>> +\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.

>> +\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.

>> +\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.

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

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.

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.

>> +The device MUST write the number of pages it has given in the final field
>> +of the VIRTIO_MEMBALLOON_GCMD_GET_PAGES buffer.  This number MAY be 0.
>> +
>> +The device MUST NOT use a negative value for the MEMBALLOON_HCMD_MIN_BALLOON
>> +command unless VIRTIO_MEMBALLOON_F_EXTRA_MEM was negotiated.
>> +
>> +\drivernormative{\subsubsection}{Device Operation}{Device Types / Memory Balloon Device / Device Operation}
>> +
>> +The driver SHOULD give pages to the balloon when it has excess pages
>> +whose loss will have minimal effect on system performance.
>> +
>> +The driver SHOULD give pages to the balloon when the balloon is
>> +below the minimum specified by VIRTIO_MEMBALLOON_HCMD_MIN_BALLOON
>
> "...when the balloon _size_ is below..."?

Thanks.

>> +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.
>
>> +The driver MUST NOT give pages to the balloon twice.
>> +
>> +The driver MUST NOT use ask for pages which are already available to
>> +it outside the balloon.
>
> drop "use".

Thanks.

>> +If VIRTIO_MEMBALLOON_F_EXTRA_MEM is negotiated, the driver MAY ask the
>> +balloon for pages outside its current memory.  Otherwise the driver
>> +MUST NOT ask for pages which it did not place into the balloon.
>> 
>> +
>>  \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
>>  
>>  Currently there are three device-independent feature bits defined:
>> diff --git a/conformance.tex b/conformance.tex
>> index 033481f..d18521d 100644
>> --- a/conformance.tex
>> +++ b/conformance.tex
>> @@ -15,13 +15,13 @@ Conformance targets:
>>    \begin{itemize}
>>      \item Clause \ref{sec:Conformance / Driver Conformance},
>>      \item One of clauses \ref{sec:Conformance / Driver Conformance / PCI Driver Conformance}, \ref{sec:Conformance / Driver Conformance / MMIO Driver Conformance} or \ref{sec:Conformance / Driver Conformance / Channel I/O Driver Conformance}.
>> -    \item One of clauses \ref{sec:Conformance / Driver Conformance / Network Driver Conformance}, \ref{sec:Conformance / Driver Conformance / Block Driver Conformance}, \ref{sec:Conformance / Driver Conformance / Console Driver Conformance}, \ref{sec:Conformance / Driver Conformance / Entropy Driver Conformance} or \ref{sec:Conformance / Driver Conformance / SCSI Host Driver Conformance}.
>> +    \item One of clauses \ref{sec:Conformance / Driver Conformance / Network Driver Conformance}, \ref{sec:Conformance / Driver Conformance / Block Driver Conformance}, \ref{sec:Conformance / Driver Conformance / Console Driver Conformance}, \ref{sec:Conformance / Driver Conformance / Entropy Driver Conformance}, \ref{sec:Conformance / Driver Conformance / SCSI Host Driver Conformance} or \ref{sec:Conformance / Driver Conformance / Memory Balloon Driver Conformance}.
>>    \end{itemize}
>>  \item[Device] A device MUST conform to three conformance clauses:
>>    \begin{itemize}
>>      \item Clause \ref{sec:Conformance / Device Conformance},
>>      \item One of clauses \ref{sec:Conformance / Device Conformance / PCI Device Conformance}, \ref{sec:Conformance / Device Conformance / MMIO Device Conformance} or \ref{sec:Conformance / Device Conformance / Channel I/O Device Conformance}.
>> -    \item One of clauses \ref{sec:Conformance / Device Conformance / Network Device Conformance}, \ref{sec:Conformance / Device Conformance / Block Device Conformance}, \ref{sec:Conformance / Device Conformance / Console Device Conformance}, \ref{sec:Conformance / Device Conformance / Entropy Device Conformance} or \ref{sec:Conformance / Device Conformance / SCSI Host Device Conformance}.
>> +    \item One of clauses \ref{sec:Conformance / Device Conformance / Network Device Conformance}, \ref{sec:Conformance / Device Conformance / Block Device Conformance}, \ref{sec:Conformance / Device Conformance / Console Device Conformance}, \ref{sec:Conformance / Device Conformance / Entropy Device Conformance}, \ref{sec:Conformance / Device Conformance / SCSI Host Device Conformance} or \ref{sec:Conformance / Device Conformance / Memory Balloon Device Conformance}.
>>    \end{itemize}
>>  \end{description}
>>  
>> @@ -132,6 +132,15 @@ An SCSI host driver MUST conform to the following normative statements:
>>  \item \ref{drivernormative:Device Types / SCSI Host Device / Device Operation / Device Operation: eventq}
>>  \end{itemize}
>>  
>> +\subsection{Memory Balloon Driver Conformance}\label{sec:Conformance / Driver Conformance / Memory Balloon Driver Conformance}
>> +
>> +An memory balloon driver MUST conform to the following normative statements:
>
> s/An/A

Thanks, and in one more place.

Cheers,
Rusty.
PS.  Now available as VIRTIO-28-new branch on github:
	https://github.com/rustyrussell/virtio-wip/tree/VIRTIO-28-new