[virtio-comment] [PATCH v2 0/3] virtio-spec: Add documentation for recently added balloon features

[virtio-comment] [PATCH v2 0/3] virtio-spec: Add documentation for recently added balloon features

[Alexander Duyck]

This patch set is meant to add documentation for balloon features that have
been recently added to the Linux kernel[1,2] and that we are currently
working on adding to QEMU[3].

Changes since RFC:
Incorporated suggestions from Cornelia Huck
Fixed a few additional spelling errors

Changes since v1:
Incorporated additional suggestions from Cornelia Huck
Dropped documentation referring to free page reporting from page poison patch

[1]: https://lore.kernel.org/lkml/20200211224416.29318.44077.stgit@localhost.localdomain/
[2]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=b0c504f154718904ae49349147e3b7e6ae91ffdc
[3]: https://lists.oasis-open.org/archives/virtio-dev/202004/msg00180.html

---

Alexander Duyck (3):
      content: Document balloon feature free page hints
      content: Document balloon feature page poison
      content: Document balloon feature free page reporting


 content.tex |  253 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 247 insertions(+), 6 deletions(-)

--


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

[virtio-comment] [PATCH v2 1/3] content: Document balloon feature free page hints

[Alexander Duyck]

From: Alexander Duyck <alexander.h.duyck@linux.intel.com>

Free page hints allow the balloon driver to provide information on what
pages are not currently in use so that we can avoid the cost of copying
them in migration scenarios. Add a feature description for free page hints
describing basic functioning and requirements.

Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
---
 content.tex |  128 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 122 insertions(+), 6 deletions(-)

diff --git a/content.tex b/content.tex
index 91735e3eb018..ec0abf177526 100644
--- a/content.tex
+++ b/content.tex
@@ -5005,10 +5005,13 @@ \subsection{Virtqueues}\label{sec:Device Types / Memory Balloon Device / Virtque
 \begin{description}
 \item[0] inflateq
 \item[1] deflateq
-\item[2] statsq.
+\item[2] statsq
+\item[3] free_page_vq
 \end{description}
 
-  Virtqueue 2 only exists if VIRTIO_BALLOON_F_STATS_VQ set.
+  statsq only exists if VIRTIO_BALLOON_F_STATS_VQ is set.
+
+  free_page_vq only exists if VIRTIO_BALLOON_F_FREE_PAGE_HINT is set.
 
 \subsection{Feature bits}\label{sec:Device Types / Memory Balloon Device / Feature bits}
 \begin{description}
@@ -5019,6 +5022,10 @@ \subsection{Feature bits}\label{sec:Device Types / Memory Balloon Device / Featu
     memory statistics is present.
 \item[VIRTIO_BALLOON_F_DEFLATE_ON_OOM (2) ] Deflate balloon on
     guest out of memory condition.
+\item[ VIRTIO_BALLOON_F_FREE_PAGE_HINT(3) ] The device has support for free
+    page hinting. A virtqueue for providing hints as to what memory is
+    currently free is present. Configuration field free_page_hint_cmd_id
+    is valid.
 
 \end{description}
 
@@ -5042,13 +5049,17 @@ \subsection{Feature bits}\label{sec:Device Types / Memory Balloon Device / Featu
 VIRTIO_BALLOON_F_MUST_TELL_HOST is not negotiated.
 
 \subsection{Device configuration layout}\label{sec:Device Types / Memory Balloon Device / Device configuration layout}
-  Both fields of this configuration
-  are always available.
+  \field{num_pages} and \field{actual} are always available.
+
+  \field{free_page_hint_cmd_id} is available if
+    VIRTIO_BALLOON_F_FREE_PAGE_HINT has been negotiated and is read-only by
+    the driver.
 
 \begin{lstlisting}
 struct virtio_balloon_config {
         le32 num_pages;
         le32 actual;
+        le32 free_page_hint_cmd_id;
 };
 \end{lstlisting}
 
@@ -5072,9 +5083,15 @@ \subsection{Device Initialization}\label{sec:Device Types / Memory Balloon Devic
   \begin{enumerate}
   \item Identify the stats virtqueue.
   \item Add one empty buffer to the stats virtqueue.
-  \item DRIVER_OK is set: device operation begins.
-  \item Notify the device about the stats virtqueue buffer.
   \end{enumerate}
+
+\item If the VIRTIO_BALLOON_F_FREE_PAGE_HINT feature bit is negotiated, the
+  free_page_vq is identified.
+
+\item DRIVER_OK is set: device operation begins.
+
+\item If the VIRTIO_BALLOON_F_STATS_VQ feature bit is negotiated, then
+  notify the device about the stats virtqueue buffer.
 \end{enumerate}
 
 \subsection{Device Operation}\label{sec:Device Types / Memory Balloon Device / Device Operation}
@@ -5345,6 +5362,105 @@ \subsubsection{Memory Statistics Tags}\label{sec:Device Types / Memory Balloon D
   allocations in the guest.
 \end{description}
 
+\subsubsection{Free Page Hinting}\label{sec:Device Types / Memory Balloon Device / Device Operation / Free Page Hinting}
+
+Free page hinting is designed to be used during migration to determine what
+pages within the guest are currently unused so that they can be skipped over
+while migrating the guest. The device will indicate that it is ready to start
+performing hinting by setting the \field{free_page_hint_cmd_id} to one of the
+non-reserved values that can be used as a command ID. The following values
+are reserved:
+
+\begin{description}
+\item[VIRTIO_BALLOON_CMD_ID_STOP (0)] Any command ID previously supplied by
+  the device is invalid. The driver should halt all hinting until a new
+  command ID is supplied.
+
+\item[VIRTIO_BALLOON_CMD_ID_DONE (1)] Any command ID previously supplied by
+  the device is invalid. The driver should halt all hinting and the hinting
+  balloon can now be deflated returning all pages to the guest.
+\end{description}
+
+A request for free page hinting proceeds as follows:
+
+\begin{enumerate}
+
+\item The driver examines the \field{free_page_hint_cmd_id} configuration field.
+  If it contains a non-reserved value then inflation of the balloon will begin.
+
+\item To supply memory to the hinting balloon:
+  \begin{enumerate}
+  \item The driver constructs an output descriptor containing the new value
+    from the \field{free_page_hint_cmd_id} configuration field and adds it to
+    the free_page_hint_vq.
+  \item The driver maps a series of pages and adds them to the
+    free_page_hint_vq as individual scatter-gather input descriptor entries.
+  \item When the driver is no longer able to fetch additional pages to add
+    to the free_page_hint_vq, it will construct an output descriptor
+    containing the command ID VIRTIO_BALLOON_CMD_ID_STOP.
+  \end{enumerate}
+
+\item A round of hinting ends either when the driver is no longer able to
+  add more pages to the balloon as described above, or when the device
+  updates \field{free_page_hint_cmd_id} configuration field contain either
+  VIRTIO_BALLOON_CMD_ID_STOP or VIRTIO_BALLOON_CMD_ID_DONE.
+
+\item The device may follow VIRTIO_BALLOON_CMD_ID_STOP with a new
+  non-reserved value for the \field{free_page_hint_cmd_id} configuration
+  field in which case it will resume supplying the hinting balloon.
+
+\item Otherwise, if the device provides VIRTIO_BALLOON_CMD_ID_DONE then
+  hinting is complete and the guest may begin to re-use pages previously
+  given to the balloon.
+
+\end{enumerate}
+
+\drivernormative{\paragraph}{Free Page Hinting}{Device Types / Memory Balloon Device / Device Operation / Free Page Hinting}
+
+Normative statements in this section apply if the
+VIRTIO_BALLOON_F_FREE_PAGE_HINT feature has been negotiated.
+
+The driver SHOULD supply pages to the hinting balloon when
+\field{free_page_hint_cmd_id} reports a value of 2 or greater.
+
+The driver MUST start hinting by providing an output descriptor
+containing the current command ID for the given block of pages.
+
+The driver SHOULD stop supplying pages to the hinting balloon when
+\field{free_page_hint_cmd_id} reports a value of VIRTIO_BALLOON_CMD_ID_STOP.
+
+If the driver is unable to supply pages, it MUST complete hinting by adding
+an output descriptor containing the command ID VIRTIO_BALLOON_CMD_ID_STOP.
+
+The driver MAY use pages from the balloon after adding them to the balloon,
+including when the device has not yet used the descriptor containing the
+hinting request.
+
+The driver SHOULD return pages for use once \field{free_page_hint_cmd_id}
+reports a value of VIRTIO_BALLOON_CMD_ID_DONE.
+
+\devicenormative{\paragraph}{Free Page Hinting}{Device Types / Memory Balloon Device / Device Operation / Free Page Hinting}
+
+Normative statements in this section apply if the
+VIRTIO_BALLOON_F_FREE_PAGE_HINT feature has been negotiated.
+
+The device MUST set \field{free_page_hint_cmd_id} to
+VIRTIO_BALLOON_CMD_ID_STOP any time that the dirty pages for the given
+guest are being recorded.
+
+The device MUST NOT reuse a command ID until it has received an output
+descriptor containing VIRTIO_BALLOON_CMD_ID_STOP from the driver.
+
+The device MUST ignore pages that are provided with a command ID that does
+not match the current value in \field{free_page_hint_cmd_id}.
+
+The device MAY modify the contents of the page in the balloon if the page
+has not been modified by the guest since the \field{free_page_hint_cmd_id}
+associated with the hint was issued by the device.
+
+The device MAY NOT modify the contents of the balloon after
+\field{free_page_hint_cmd_id} is set to VIRTIO_BALLOON_CMD_ID_DONE.
+
 \section{SCSI Host Device}\label{sec:Device Types / SCSI Host Device}
 
 The virtio SCSI host device groups together one or more virtual



This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

[virtio-comment] [PATCH v2 2/3] content: Document balloon feature page poison

[Alexander Duyck]

From: Alexander Duyck <alexander.h.duyck@linux.intel.com>

Page poison provides a way for the guest to notify the host of the content
expected to be found in pages when they are added back to the guest after
being discarded. The feature currently doesn't apply to the existing
balloon features, however it will apply to an upcoming feature, free page
reporting. Add documentation for the page poison feature describing the
basic functionality and requirements.

Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
---
 content.tex |   43 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/content.tex b/content.tex
index ec0abf177526..3d30fd5bb6fa 100644
--- a/content.tex
+++ b/content.tex
@@ -5026,6 +5026,9 @@ \subsection{Feature bits}\label{sec:Device Types / Memory Balloon Device / Featu
     page hinting. A virtqueue for providing hints as to what memory is
     currently free is present. Configuration field free_page_hint_cmd_id
     is valid.
+\item[ VIRTIO_BALLOON_F_PAGE_POISON(4) ] The device has to be notified if
+    the driver is expecting balloon pages to contain a certain value when
+    returned. Configuration field poison_val is valid.
 
 \end{description}
 
@@ -5033,6 +5036,9 @@ \subsection{Feature bits}\label{sec:Device Types / Memory Balloon Device / Featu
 The driver SHOULD accept the VIRTIO_BALLOON_F_MUST_TELL_HOST
 feature if offered by the device.
 
+The driver SHOULD clear the VIRTIO_BALLOON_F_PAGE_POISON flag if it is not
+expecting any specific value to be stored in the page.
+
 \devicenormative{\subsubsection}{Feature bits}{Device Types / Memory Balloon Device / Feature bits}
 If the device offers the VIRTIO_BALLOON_F_MUST_TELL_HOST feature
 bit, and if the driver did not accept this feature bit, the
@@ -5055,11 +5061,15 @@ \subsection{Device configuration layout}\label{sec:Device Types / Memory Balloon
     VIRTIO_BALLOON_F_FREE_PAGE_HINT has been negotiated and is read-only by
     the driver.
 
+  \field{poison_val} is available if VIRTIO_BALLOON_F_PAGE_POISON has been
+    negotiated.
+
 \begin{lstlisting}
 struct virtio_balloon_config {
         le32 num_pages;
         le32 actual;
         le32 free_page_hint_cmd_id;
+        le32 poison_val;
 };
 \end{lstlisting}
 
@@ -5088,6 +5098,9 @@ \subsection{Device Initialization}\label{sec:Device Types / Memory Balloon Devic
 \item If the VIRTIO_BALLOON_F_FREE_PAGE_HINT feature bit is negotiated, the
   free_page_vq is identified.
 
+\item If the VIRTIO_BALLOON_F_PAGE_POISON feature bit is negotiated, the
+  driver updates the \field{poison_val} configuration field.
+
 \item DRIVER_OK is set: device operation begins.
 
 \item If the VIRTIO_BALLOON_F_STATS_VQ feature bit is negotiated, then
@@ -5461,6 +5474,36 @@ \subsubsection{Free Page Hinting}\label{sec:Device Types / Memory Balloon Device
 The device MAY NOT modify the contents of the balloon after
 \field{free_page_hint_cmd_id} is set to VIRTIO_BALLOON_CMD_ID_DONE.
 
+\subsubsection{Page Poison}\label{sec:Device Types / Memory Balloon Device / Device Operation / Page Poison}
+
+Page Poison provides a way to notify the host of the contents that are
+currently in the balloon pages, and those that are expected to be in the
+pages when they are pulled from the balloon.
+
+If VIRTIO_BALLOON_F_PAGE_POISON feature has been negotiated, the guest
+will place the expected poison value into the \field{poison_val}
+configuration field data.
+
+\drivernormative{\paragraph}{Page Poison}{Device Types / Memory Balloon Device / Device Operation / Page Poison}
+
+Normative statements in this section apply if the
+VIRTIO_BALLOON_F_PAGE_POISON feature has been negotiated.
+
+The driver MUST populate the \field{poison_val} configuration data if it is
+expecting the page to contain some fixed value when free.
+
+The driver MAY opt to reject the feature if it will take care of
+re-initializing pages when first accessing them.
+
+\devicenormative{\paragraph}{Page Poison}{Device Types / Memory Balloon Device / Device Operation / Page Poison}
+
+Normative statements in this section apply if the
+VIRTIO_BALLOON_F_PAGE_POISON feature has been negotiated.
+
+As page poisoning is optional for normal balloon operations and free
+page hinting, the device MAY ignore the content of \field{poison_val}
+for those operations.
+
 \section{SCSI Host Device}\label{sec:Device Types / SCSI Host Device}
 
 The virtio SCSI host device groups together one or more virtual



This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

[virtio-comment] [PATCH v2 3/3] content: Document balloon feature free page reporting

[Alexander Duyck]

From: Alexander Duyck <alexander.h.duyck@linux.intel.com>

Free page reporting is a feature that allows the guest to proactively
report unused pages to the host. By making use of this feature is is
possible to reduce the overall memory footprint of the guest in cases where
some significant portion of the memory is idle. Add documentation for the
free page reporting feature describing the functionality and requirements.

Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
---
 content.tex |   84 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 83 insertions(+), 1 deletion(-)

diff --git a/content.tex b/content.tex
index 3d30fd5bb6fa..3cb38105f794 100644
--- a/content.tex
+++ b/content.tex
@@ -5007,12 +5007,15 @@ \subsection{Virtqueues}\label{sec:Device Types / Memory Balloon Device / Virtque
 \item[1] deflateq
 \item[2] statsq
 \item[3] free_page_vq
+\item[4] reporting_vq
 \end{description}
 
   statsq only exists if VIRTIO_BALLOON_F_STATS_VQ is set.
 
   free_page_vq only exists if VIRTIO_BALLOON_F_FREE_PAGE_HINT is set.
 
+  reporting_vq only exists if VIRTIO_BALLOON_F_PAGE_REPORTING is set.
+
 \subsection{Feature bits}\label{sec:Device Types / Memory Balloon Device / Feature bits}
 \begin{description}
 \item[VIRTIO_BALLOON_F_MUST_TELL_HOST (0)] Host has to be told before
@@ -5029,6 +5032,8 @@ \subsection{Feature bits}\label{sec:Device Types / Memory Balloon Device / Featu
 \item[ VIRTIO_BALLOON_F_PAGE_POISON(4) ] The device has to be notified if
     the driver is expecting balloon pages to contain a certain value when
     returned. Configuration field poison_val is valid.
+\item[ VIRTIO_BALLOON_F_PAGE_REPORTING(5) ] The device has support for free
+    page reporting. A virtqueue for reporting free guest memory is present.
 
 \end{description}
 
@@ -5039,6 +5044,10 @@ \subsection{Feature bits}\label{sec:Device Types / Memory Balloon Device / Featu
 The driver SHOULD clear the VIRTIO_BALLOON_F_PAGE_POISON flag if it is not
 expecting any specific value to be stored in the page.
 
+If the driver is expecting the pages to retain some initialized value,
+it MUST NOT accept VIRTIO_BALLOON_F_PAGE_REPORTING unless it also
+negotiates VIRTIO_BALLOON_F_PAGE_POISON.
+
 \devicenormative{\subsubsection}{Feature bits}{Device Types / Memory Balloon Device / Feature bits}
 If the device offers the VIRTIO_BALLOON_F_MUST_TELL_HOST feature
 bit, and if the driver did not accept this feature bit, the
@@ -5101,10 +5110,16 @@ \subsection{Device Initialization}\label{sec:Device Types / Memory Balloon Devic
 \item If the VIRTIO_BALLOON_F_PAGE_POISON feature bit is negotiated, the
   driver updates the \field{poison_val} configuration field.
 
+\item If the VIRTIO_BALLOON_F_PAGE_REPORTING feature bit is negotiated the
+  reporting_vq is identified.
+
 \item DRIVER_OK is set: device operation begins.
 
 \item If the VIRTIO_BALLOON_F_STATS_VQ feature bit is negotiated, then
   notify the device about the stats virtqueue buffer.
+
+\item If the VIRTIO_BALLOON_F_PAGE_REPORTING feature bit is negotiated then
+  begin reporting free pages to device.
 \end{enumerate}
 
 \subsection{Device Operation}\label{sec:Device Types / Memory Balloon Device / Device Operation}
@@ -5478,7 +5493,9 @@ \subsubsection{Page Poison}\label{sec:Device Types / Memory Balloon Device / Dev
 
 Page Poison provides a way to notify the host of the contents that are
 currently in the balloon pages, and those that are expected to be in the
-pages when they are pulled from the balloon.
+pages when they are pulled from the balloon. It is used for in-place
+reporting of pages without needing to pull them from the memory allocator
+of the guest.
 
 If VIRTIO_BALLOON_F_PAGE_POISON feature has been negotiated, the guest
 will place the expected poison value into the \field{poison_val}
@@ -5504,6 +5521,71 @@ \subsubsection{Page Poison}\label{sec:Device Types / Memory Balloon Device / Dev
 page hinting, the device MAY ignore the content of \field{poison_val}
 for those operations.
 
+\subsubsection{Free Page Reporting}\label{sec:Device Types / Memory Balloon Device / Device Operation / Free Page Reporting}
+
+Free Page Reporting provides a mechanism similar to balloon inflation,
+however it does not provide a deflation queue. The expectation is that the
+device will have a means by which it can detect the guest page access and
+fault in such pages with some initial value, likely a zero page.
+
+The driver will respond to to memory conditions and begin reporting free
+pages when some number of pages are available.
+
+\begin{enumerate}
+
+\item The driver determines it has enough pages available to begin
+  reporting pages.
+
+\item The driver gathers pages into a scatter-gather list and adds them to
+  the reporting_vq.
+
+\item The device acknowledges the reporting request by using the
+  reporting_vq descriptor.
+
+\item Once the device has acknowledged the report, the pages can be
+  returned to the location from which they were pulled.
+
+\item The driver can then continue to gather and report pages until it
+  has determined it has reported a sufficient quantity of pages.
+
+\end{enumerate}
+
+\drivernormative{\paragraph}{Free Page Reporting}{Device Types / Memory Balloon Device / Device Operation / Free Page Reporting}
+
+Normative statements in this section apply if the
+VIRTIO_BALLOON_F_PAGE_REPORTING feature has been negotiated.
+
+If the driver is expecting the free page to contain some initial value it
+MUST NOT negotiate this feature without negotiating the
+VIRTIO_BALLOON_F_PAGE_POISON feature as well and supply this value via
+\field{poison_val}.
+
+If the driver is expecting the pages to retain some initialized value,
+it MUST NOT accept VIRTIO_BALLOON_F_PAGE_REPORTING unless it also
+negotiates the VIRTIO_BALLOON_F_PAGE_POISON feature and supplies this
+value via \field{poison_val}.
+
+The driver MUST NOT use the reported pages until the device has
+acknowledged the reporting request.
+
+The driver MAY report free pages any time after DRIVER_OK is set.
+
+It is RECOMMENDED that the driver avoid unnecessary reads or writes to the
+page contents as this could reduce the performance for free page reporting.
+
+\devicenormative{\paragraph}{Free Page Reporting}{Device Types / Memory Balloon Device / Device Operation / Free Page Reporting}
+
+Normative statements in this section apply if the
+VIRTIO_BALLOON_F_PAGE_REPORTING feature has been negotiated.
+
+The device MAY modify the contents of any page supplied in a report
+request even before acknowledging that request by using the
+reporting_vq descriptor.
+
+If the VIRTIO_BALLOON_F_PAGE_POISON feature has been negotiated, the device
+SHALL NOT modify the the page if this will result in the page containing a
+value other than \field{poison_val}.
+
 \section{SCSI Host Device}\label{sec:Device Types / SCSI Host Device}
 
 The virtio SCSI host device groups together one or more virtual



This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

Re: [virtio-comment] [PATCH v2 1/3] content: Document balloon feature free page hints

[David Hildenbrand]

On 15.05.20 19:33, Alexander Duyck wrote:
> From: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> 
> Free page hints allow the balloon driver to provide information on what
> pages are not currently in use so that we can avoid the cost of copying
> them in migration scenarios. Add a feature description for free page hints
> describing basic functioning and requirements.
> 
> Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> ---
>  content.tex |  128 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 122 insertions(+), 6 deletions(-)
> 
> diff --git a/content.tex b/content.tex
> index 91735e3eb018..ec0abf177526 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -5005,10 +5005,13 @@ \subsection{Virtqueues}\label{sec:Device Types / Memory Balloon Device / Virtque
>  \begin{description}
>  \item[0] inflateq
>  \item[1] deflateq
> -\item[2] statsq.
> +\item[2] statsq
> +\item[3] free_page_vq
>  \end{description}
>  
> -  Virtqueue 2 only exists if VIRTIO_BALLOON_F_STATS_VQ set.
> +  statsq only exists if VIRTIO_BALLOON_F_STATS_VQ is set.
> +
> +  free_page_vq only exists if VIRTIO_BALLOON_F_FREE_PAGE_HINT is set.
>  
>  \subsection{Feature bits}\label{sec:Device Types / Memory Balloon Device / Feature bits}
>  \begin{description}
> @@ -5019,6 +5022,10 @@ \subsection{Feature bits}\label{sec:Device Types / Memory Balloon Device / Featu
>      memory statistics is present.
>  \item[VIRTIO_BALLOON_F_DEFLATE_ON_OOM (2) ] Deflate balloon on
>      guest out of memory condition.
> +\item[ VIRTIO_BALLOON_F_FREE_PAGE_HINT(3) ] The device has support for free
> +    page hinting. A virtqueue for providing hints as to what memory is
> +    currently free is present. Configuration field free_page_hint_cmd_id
> +    is valid.

\field{free_page_hint_cmd_id} ?

>  
>  \end{description}
>  
> @@ -5042,13 +5049,17 @@ \subsection{Feature bits}\label{sec:Device Types / Memory Balloon Device / Featu
>  VIRTIO_BALLOON_F_MUST_TELL_HOST is not negotiated.
>  
>  \subsection{Device configuration layout}\label{sec:Device Types / Memory Balloon Device / Device configuration layout}
> -  Both fields of this configuration
> -  are always available.
> +  \field{num_pages} and \field{actual} are always available.
> +
> +  \field{free_page_hint_cmd_id} is available if
> +    VIRTIO_BALLOON_F_FREE_PAGE_HINT has been negotiated and is read-only by
> +    the driver.
>  
>  \begin{lstlisting}
>  struct virtio_balloon_config {
>          le32 num_pages;
>          le32 actual;
> +        le32 free_page_hint_cmd_id;
>  };
>  \end{lstlisting}
>  
> @@ -5072,9 +5083,15 @@ \subsection{Device Initialization}\label{sec:Device Types / Memory Balloon Devic
>    \begin{enumerate}
>    \item Identify the stats virtqueue.
>    \item Add one empty buffer to the stats virtqueue.
> -  \item DRIVER_OK is set: device operation begins.
> -  \item Notify the device about the stats virtqueue buffer.
>    \end{enumerate}
> +
> +\item If the VIRTIO_BALLOON_F_FREE_PAGE_HINT feature bit is negotiated, the
> +  free_page_vq is identified.
> +
> +\item DRIVER_OK is set: device operation begins.
> +
> +\item If the VIRTIO_BALLOON_F_STATS_VQ feature bit is negotiated, then
> +  notify the device about the stats virtqueue buffer.
>  \end{enumerate}
>  
>  \subsection{Device Operation}\label{sec:Device Types / Memory Balloon Device / Device Operation}
> @@ -5345,6 +5362,105 @@ \subsubsection{Memory Statistics Tags}\label{sec:Device Types / Memory Balloon D
>    allocations in the guest.
>  \end{description}
>  
> +\subsubsection{Free Page Hinting}\label{sec:Device Types / Memory Balloon Device / Device Operation / Free Page Hinting}
> +
> +Free page hinting is designed to be used during migration to determine what
> +pages within the guest are currently unused so that they can be skipped over
> +while migrating the guest. The device will indicate that it is ready to start
> +performing hinting by setting the \field{free_page_hint_cmd_id} to one of the
> +non-reserved values that can be used as a command ID. The following values
> +are reserved:
> +
> +\begin{description}
> +\item[VIRTIO_BALLOON_CMD_ID_STOP (0)] Any command ID previously supplied by
> +  the device is invalid. The driver should halt all hinting until a new
> +  command ID is supplied.

Maybe "The driver should stop hinting free pages, but should not reuse
all previously hinted pages."

> +
> +\item[VIRTIO_BALLOON_CMD_ID_DONE (1)] Any command ID previously supplied by
> +  the device is invalid. The driver should halt all hinting and the hinting
> +  balloon can now be deflated returning all pages to the guest.

I would avoid the terminology "hinting balloon" and "deflation".

"The driver should stop hinting free pages and should reuse all
previously hinted pages.".

> +\end{description}
> +
> +A request for free page hinting proceeds as follows:
> +
> +\begin{enumerate}
> +
> +\item The driver examines the \field{free_page_hint_cmd_id} configuration field.
> +  If it contains a non-reserved value then inflation of the balloon will begin.
> +
> +\item To supply memory to the hinting balloon:
> +  \begin{enumerate}
> +  \item The driver constructs an output descriptor containing the new value
> +    from the \field{free_page_hint_cmd_id} configuration field and adds it to
> +    the free_page_hint_vq.
> +  \item The driver maps a series of pages and adds them to the
> +    free_page_hint_vq as individual scatter-gather input descriptor entries.
> +  \item When the driver is no longer able to fetch additional pages to add
> +    to the free_page_hint_vq, it will construct an output descriptor
> +    containing the command ID VIRTIO_BALLOON_CMD_ID_STOP.
> +  \end{enumerate}
> +
> +\item A round of hinting ends either when the driver is no longer able to
> +  add more pages to the balloon as described above, or when the device
> +  updates \field{free_page_hint_cmd_id} configuration field contain either
> +  VIRTIO_BALLOON_CMD_ID_STOP or VIRTIO_BALLOON_CMD_ID_DONE.
> +
> +\item The device may follow VIRTIO_BALLOON_CMD_ID_STOP with a new
> +  non-reserved value for the \field{free_page_hint_cmd_id} configuration
> +  field in which case it will resume supplying the hinting balloon.

Again, I would avoid any kind of "hinting balloon" terminology.

> +
> +\item Otherwise, if the device provides VIRTIO_BALLOON_CMD_ID_DONE then
> +  hinting is complete and the guest may begin to re-use pages previously
> +  given to the balloon.

See my rephrase attempt above.

> +
> +\end{enumerate}
> +
> +\drivernormative{\paragraph}{Free Page Hinting}{Device Types / Memory Balloon Device / Device Operation / Free Page Hinting}
> +
> +Normative statements in this section apply if the
> +VIRTIO_BALLOON_F_FREE_PAGE_HINT feature has been negotiated.
> +
> +The driver SHOULD supply pages to the hinting balloon when
> +\field{free_page_hint_cmd_id} reports a value of 2 or greater.
> +
> +The driver MUST start hinting by providing an output descriptor
> +containing the current command ID for the given block of pages.
> +
> +The driver SHOULD stop supplying pages to the hinting balloon when
> +\field{free_page_hint_cmd_id} reports a value of VIRTIO_BALLOON_CMD_ID_STOP.
> +
> +If the driver is unable to supply pages, it MUST complete hinting by adding
> +an output descriptor containing the command ID VIRTIO_BALLOON_CMD_ID_STOP.
> +
> +The driver MAY use pages from the balloon after adding them to the balloon,
> +including when the device has not yet used the descriptor containing the
> +hinting request.
> +
> +The driver SHOULD return pages for use once \field{free_page_hint_cmd_id}
> +reports a value of VIRTIO_BALLOON_CMD_ID_DONE.

"return pages" -> "start to reuse all previously hinted pages".


Also,

"The driver MUST reinitialize hinted pages before reusing them."

> +
> +\devicenormative{\paragraph}{Free Page Hinting}{Device Types / Memory Balloon Device / Device Operation / Free Page Hinting}
> +
> +Normative statements in this section apply if the
> +VIRTIO_BALLOON_F_FREE_PAGE_HINT feature has been negotiated.
> +
> +The device MUST set \field{free_page_hint_cmd_id} to
> +VIRTIO_BALLOON_CMD_ID_STOP any time that the dirty pages for the given
> +guest are being recorded.
> +
> +The device MUST NOT reuse a command ID until it has received an output
> +descriptor containing VIRTIO_BALLOON_CMD_ID_STOP from the driver.
> +
> +The device MUST ignore pages that are provided with a command ID that does
> +not match the current value in \field{free_page_hint_cmd_id}.
> +
> +The device MAY modify the contents of the page in the balloon if the page
> +has not been modified by the guest since the \field{free_page_hint_cmd_id}
> +associated with the hint was issued by the device.

"page in the balloon" -> "previously hinted pages"

But it's not that easy in respect to the guest reusing the pages.

"previously hinted pages and not reused pages" ?

Also, something like

"The device MUST NOT modify the contents of previously hinted pages in
case they are reused by the devices, even if they are reused by the
driver before the hinting request is processed."

?

> +
> +The device MAY NOT modify the contents of the balloon after
> +\field{free_page_hint_cmd_id} is set to VIRTIO_BALLOON_CMD_ID_DONE.

"contents of the balloon" is misleading. What exactly did you want to
say here? Modifying the content of previously hinted pages?

"MAY NOT" does not exist in RFC2119. Was this meant to be "MUST NOT".

> +
>  \section{SCSI Host Device}\label{sec:Device Types / SCSI Host Device}
>  
>  The virtio SCSI host device groups together one or more virtual
> 
> 
> 

Whenever you add new normative sections/paragraphs, you also have to
link to them from conformance.tex (both, for devices and drivers).


In addition, we should document that VIRTIO_BALLOON_F_MUST_TELL_HOST
does not affect VIRTIO_BALLOON_F_FREE_PAGE_HINT.

-- 
Thanks,

David / dhildenb


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

Re: [virtio-comment] [PATCH v2 1/3] content: Document balloon feature free page hints

[David Hildenbrand]

>> +
>> +\devicenormative{\paragraph}{Free Page Hinting}{Device Types / Memory Balloon Device / Device Operation / Free Page Hinting}
>> +
>> +Normative statements in this section apply if the
>> +VIRTIO_BALLOON_F_FREE_PAGE_HINT feature has been negotiated.
>> +
>> +The device MUST set \field{free_page_hint_cmd_id} to
>> +VIRTIO_BALLOON_CMD_ID_STOP any time that the dirty pages for the given
>> +guest are being recorded.
>> +
>> +The device MUST NOT reuse a command ID until it has received an output
>> +descriptor containing VIRTIO_BALLOON_CMD_ID_STOP from the driver.
>> +
>> +The device MUST ignore pages that are provided with a command ID that does
>> +not match the current value in \field{free_page_hint_cmd_id}.
>> +
>> +The device MAY modify the contents of the page in the balloon if the page
>> +has not been modified by the guest since the \field{free_page_hint_cmd_id}
>> +associated with the hint was issued by the device.
> 
> "page in the balloon" -> "previously hinted pages"
> 
> But it's not that easy in respect to the guest reusing the pages.
> 
> "previously hinted pages and not reused pages" ?
> 
> Also, something like
> 
> "The device MUST NOT modify the contents of previously hinted pages in
> case they are reused by the devices, even if they are reused by the
> driver before the hinting request is processed."

"reused by the driver" in the first instance of course.


-- 
Thanks,

David / dhildenb


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

Re: [virtio-comment] [PATCH v2 1/3] content: Document balloon feature free page hints

[Alexander Duyck]

On Mon, May 18, 2020 at 6:18 AM David Hildenbrand <david@redhat.com> wrote:
>
> On 15.05.20 19:33, Alexander Duyck wrote:
> > From: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> >
> > Free page hints allow the balloon driver to provide information on what
> > pages are not currently in use so that we can avoid the cost of copying
> > them in migration scenarios. Add a feature description for free page hints
> > describing basic functioning and requirements.
> >
> > Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> > ---
> >  content.tex |  128 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 122 insertions(+), 6 deletions(-)
> >
> > diff --git a/content.tex b/content.tex
> > index 91735e3eb018..ec0abf177526 100644
> > --- a/content.tex
> > +++ b/content.tex
> > @@ -5005,10 +5005,13 @@ \subsection{Virtqueues}\label{sec:Device Types / Memory Balloon Device / Virtque
> >  \begin{description}
> >  \item[0] inflateq
> >  \item[1] deflateq
> > -\item[2] statsq.
> > +\item[2] statsq
> > +\item[3] free_page_vq
> >  \end{description}
> >
> > -  Virtqueue 2 only exists if VIRTIO_BALLOON_F_STATS_VQ set.
> > +  statsq only exists if VIRTIO_BALLOON_F_STATS_VQ is set.
> > +
> > +  free_page_vq only exists if VIRTIO_BALLOON_F_FREE_PAGE_HINT is set.
> >
> >  \subsection{Feature bits}\label{sec:Device Types / Memory Balloon Device / Feature bits}
> >  \begin{description}
> > @@ -5019,6 +5022,10 @@ \subsection{Feature bits}\label{sec:Device Types / Memory Balloon Device / Featu
> >      memory statistics is present.
> >  \item[VIRTIO_BALLOON_F_DEFLATE_ON_OOM (2) ] Deflate balloon on
> >      guest out of memory condition.
> > +\item[ VIRTIO_BALLOON_F_FREE_PAGE_HINT(3) ] The device has support for free
> > +    page hinting. A virtqueue for providing hints as to what memory is
> > +    currently free is present. Configuration field free_page_hint_cmd_id
> > +    is valid.
>
> \field{free_page_hint_cmd_id} ?
>
> >
> >  \end{description}
> >
> > @@ -5042,13 +5049,17 @@ \subsection{Feature bits}\label{sec:Device Types / Memory Balloon Device / Featu
> >  VIRTIO_BALLOON_F_MUST_TELL_HOST is not negotiated.
> >
> >  \subsection{Device configuration layout}\label{sec:Device Types / Memory Balloon Device / Device configuration layout}
> > -  Both fields of this configuration
> > -  are always available.
> > +  \field{num_pages} and \field{actual} are always available.
> > +
> > +  \field{free_page_hint_cmd_id} is available if
> > +    VIRTIO_BALLOON_F_FREE_PAGE_HINT has been negotiated and is read-only by
> > +    the driver.
> >
> >  \begin{lstlisting}
> >  struct virtio_balloon_config {
> >          le32 num_pages;
> >          le32 actual;
> > +        le32 free_page_hint_cmd_id;
> >  };
> >  \end{lstlisting}
> >
> > @@ -5072,9 +5083,15 @@ \subsection{Device Initialization}\label{sec:Device Types / Memory Balloon Devic
> >    \begin{enumerate}
> >    \item Identify the stats virtqueue.
> >    \item Add one empty buffer to the stats virtqueue.
> > -  \item DRIVER_OK is set: device operation begins.
> > -  \item Notify the device about the stats virtqueue buffer.
> >    \end{enumerate}
> > +
> > +\item If the VIRTIO_BALLOON_F_FREE_PAGE_HINT feature bit is negotiated, the
> > +  free_page_vq is identified.
> > +
> > +\item DRIVER_OK is set: device operation begins.
> > +
> > +\item If the VIRTIO_BALLOON_F_STATS_VQ feature bit is negotiated, then
> > +  notify the device about the stats virtqueue buffer.
> >  \end{enumerate}
> >
> >  \subsection{Device Operation}\label{sec:Device Types / Memory Balloon Device / Device Operation}
> > @@ -5345,6 +5362,105 @@ \subsubsection{Memory Statistics Tags}\label{sec:Device Types / Memory Balloon D
> >    allocations in the guest.
> >  \end{description}
> >
> > +\subsubsection{Free Page Hinting}\label{sec:Device Types / Memory Balloon Device / Device Operation / Free Page Hinting}
> > +
> > +Free page hinting is designed to be used during migration to determine what
> > +pages within the guest are currently unused so that they can be skipped over
> > +while migrating the guest. The device will indicate that it is ready to start
> > +performing hinting by setting the \field{free_page_hint_cmd_id} to one of the
> > +non-reserved values that can be used as a command ID. The following values
> > +are reserved:
> > +
> > +\begin{description}
> > +\item[VIRTIO_BALLOON_CMD_ID_STOP (0)] Any command ID previously supplied by
> > +  the device is invalid. The driver should halt all hinting until a new
> > +  command ID is supplied.
>
> Maybe "The driver should stop hinting free pages, but should not reuse
> all previously hinted pages."

The "reuse all previously hinted pages" seems rather unclear to me. I
would like to make clear that in this case the "use" is the guest
making use of the memory, not the driver doing something like
recycling hints. So in the spots where you reference "the driver
reusing pages" I think I might prefer to go with something along the
lines of "releasing pages for use by the guest". The problem is that
when you have a balloon we were referencing using pages from the
balloon. Since we cannot reference the balloon I figure I will go with
language where we "supply" and "release" hinted pages. That way we
acknowledge that the driver is holding onto pages and not freeing them
for use by the guest.

I'll probably go with something like:
  The driver should stop hinting free pages, but
  should not release any hinted pages for use by the guest.

> > +
> > +\item[VIRTIO_BALLOON_CMD_ID_DONE (1)] Any command ID previously supplied by
> > +  the device is invalid. The driver should halt all hinting and the hinting
> > +  balloon can now be deflated returning all pages to the guest.
>
> I would avoid the terminology "hinting balloon" and "deflation".

So I will clean up the spots you pointed out and also work on the
workflow down below that includes "hinting balloon" in the
terminology.

> "The driver should stop hinting free pages and should reuse all
> previously hinted pages.".

Similar to the comment above:
  The driver should stop hinting free pages, and
  should release all hinted pages for use by the guest.

> > +\end{description}
> > +
> > +A request for free page hinting proceeds as follows:
> > +
> > +\begin{enumerate}
> > +
> > +\item The driver examines the \field{free_page_hint_cmd_id} configuration field.
> > +  If it contains a non-reserved value then inflation of the balloon will begin.
> > +
> > +\item To supply memory to the hinting balloon:
> > +  \begin{enumerate}
> > +  \item The driver constructs an output descriptor containing the new value
> > +    from the \field{free_page_hint_cmd_id} configuration field and adds it to
> > +    the free_page_hint_vq.
> > +  \item The driver maps a series of pages and adds them to the
> > +    free_page_hint_vq as individual scatter-gather input descriptor entries.
> > +  \item When the driver is no longer able to fetch additional pages to add
> > +    to the free_page_hint_vq, it will construct an output descriptor
> > +    containing the command ID VIRTIO_BALLOON_CMD_ID_STOP.
> > +  \end{enumerate}
> > +
> > +\item A round of hinting ends either when the driver is no longer able to
> > +  add more pages to the balloon as described above, or when the device
> > +  updates \field{free_page_hint_cmd_id} configuration field contain either
> > +  VIRTIO_BALLOON_CMD_ID_STOP or VIRTIO_BALLOON_CMD_ID_DONE.
> > +
> > +\item The device may follow VIRTIO_BALLOON_CMD_ID_STOP with a new
> > +  non-reserved value for the \field{free_page_hint_cmd_id} configuration
> > +  field in which case it will resume supplying the hinting balloon.
>
> Again, I would avoid any kind of "hinting balloon" terminology.

I'll probably just go with "resume supplying free page hints", as well
as updating title for the list to "To supply free page hints:".

> > +
> > +\item Otherwise, if the device provides VIRTIO_BALLOON_CMD_ID_DONE then
> > +  hinting is complete and the guest may begin to re-use pages previously
> > +  given to the balloon.
>
> See my rephrase attempt above.

\item Otherwise, if the device provides VIRTIO_BALLOON_CMD_ID_DONE then
  hinting is complete and the driver may release all previously hinted
  pages for use by the guest.

> > +
> > +\end{enumerate}
> > +
> > +\drivernormative{\paragraph}{Free Page Hinting}{Device Types / Memory Balloon Device / Device Operation / Free Page Hinting}
> > +
> > +Normative statements in this section apply if the
> > +VIRTIO_BALLOON_F_FREE_PAGE_HINT feature has been negotiated.
> > +
> > +The driver SHOULD supply pages to the hinting balloon when
> > +\field{free_page_hint_cmd_id} reports a value of 2 or greater.
> > +
> > +The driver MUST start hinting by providing an output descriptor
> > +containing the current command ID for the given block of pages.
> > +
> > +The driver SHOULD stop supplying pages to the hinting balloon when
> > +\field{free_page_hint_cmd_id} reports a value of VIRTIO_BALLOON_CMD_ID_STOP.
> > +
> > +If the driver is unable to supply pages, it MUST complete hinting by adding
> > +an output descriptor containing the command ID VIRTIO_BALLOON_CMD_ID_STOP.
> > +
> > +The driver MAY use pages from the balloon after adding them to the balloon,
> > +including when the device has not yet used the descriptor containing the
> > +hinting request.

Another balloon reference here I am getting rid of:
  The driver MAY release hinted pages for use by the guest including when the
  device has not yet used the descriptor containing the hinting request.

> > +
> > +The driver SHOULD return pages for use once \field{free_page_hint_cmd_id}
> > +reports a value of VIRTIO_BALLOON_CMD_ID_DONE.
>
> "return pages" -> "start to reuse all previously hinted pages".

The driver SHOULD release all hinted pages for use by the guest once
\field{free_page_hint_cmd_id} reports a value of VIRTIO_BALLOON_CMD_ID_DONE.

> Also,
>
> "The driver MUST reinitialize hinted pages before reusing them."

That isn't quite correct though. It is only necessary to initialize
the pages if the guest depends on them being initialized.

Maybe something like:
  The driver MUST treat the content of all hinted pages as uninitialized memory.

> > +
> > +\devicenormative{\paragraph}{Free Page Hinting}{Device Types / Memory Balloon Device / Device Operation / Free Page Hinting}
> > +
> > +Normative statements in this section apply if the
> > +VIRTIO_BALLOON_F_FREE_PAGE_HINT feature has been negotiated.
> > +
> > +The device MUST set \field{free_page_hint_cmd_id} to
> > +VIRTIO_BALLOON_CMD_ID_STOP any time that the dirty pages for the given
> > +guest are being recorded.
> > +
> > +The device MUST NOT reuse a command ID until it has received an output
> > +descriptor containing VIRTIO_BALLOON_CMD_ID_STOP from the driver.
> > +
> > +The device MUST ignore pages that are provided with a command ID that does
> > +not match the current value in \field{free_page_hint_cmd_id}.
> > +
> > +The device MAY modify the contents of the page in the balloon if the page
> > +has not been modified by the guest since the \field{free_page_hint_cmd_id}
> > +associated with the hint was issued by the device.
>
> "page in the balloon" -> "previously hinted pages"
>
> But it's not that easy in respect to the guest reusing the pages.
>
> "previously hinted pages and not reused pages" ?
>
> Also, something like
>
> "The device MUST NOT modify the contents of previously hinted pages in
> case they are reused by the devices, even if they are reused by the
> driver before the hinting request is processed."

That is not quite true.

The driver can end up releasing the pages back to the buddy allocator
and if they are not poisoned/init_on_free then they will go there and
can still potentially change until such time as the guest writes to
the page modifying it or the balloon driver switches the cmd ID to
VIRTIO_BALLOON_CMD_ID_DONE. That was one of the reasons for trying to
frame it the way I did. So what I can do is reword the two statements
as follows:

  If the content of a previously hinted page has not been modified by the
  guest since the device issued the \field{free_page_hint_cmd_id} associated
  with the hint, the device MAY modify the contents of the page.

  The device MUST NOT modify the content of a previously hinted page
after
  \field{free_page_hint_cmd_id} is set to VIRTIO_BALLOON_CMD_ID_DONE.

The first statement describes the start of the modification window and
the second describes the end of it. If the page content is modified
after the hint command ID was issued then the page can no longer be
modified. That could even occur before the hint is generated so it is
possible the page may not be modified at all. The standard case is
that the hints will be generated and nothing will happen until the
migration actually happens and VIRTIO_BALLOON_CMD_ID_DONE is issued.
Then at that point from the guests view the contents of the pages it
had not modified will suddenly revert to either a zero page or an
older version.

> > +
> > +The device MAY NOT modify the contents of the balloon after
> > +\field{free_page_hint_cmd_id} is set to VIRTIO_BALLOON_CMD_ID_DONE.
>
> "contents of the balloon" is misleading. What exactly did you want to
> say here? Modifying the content of previously hinted pages?
>
> "MAY NOT" does not exist in RFC2119. Was this meant to be "MUST NOT".

Thanks for pointing that out.

> > +
> >  \section{SCSI Host Device}\label{sec:Device Types / SCSI Host Device}
> >
> >  The virtio SCSI host device groups together one or more virtual
> >
> >
> >
>
> Whenever you add new normative sections/paragraphs, you also have to
> link to them from conformance.tex (both, for devices and drivers).

I will go through and update that.

>
> In addition, we should document that VIRTIO_BALLOON_F_MUST_TELL_HOST
> does not affect VIRTIO_BALLOON_F_FREE_PAGE_HINT.

I'm not sure that really makes sense though. With the "balloon" and
"deflate" wording being stripped I'm not sure it makes sense to
indicate that we don't need to deflate the hinted pages since we don't
describe doing that anywhere.

This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

Re: [virtio-comment] [PATCH v2 1/3] content: Document balloon feature free page hints

[Cornelia Huck]

On Mon, 18 May 2020 13:15:50 -0700
Alexander Duyck <alexander.duyck@gmail.com> wrote:

> On Mon, May 18, 2020 at 6:18 AM David Hildenbrand <david@redhat.com> wrote:
> >
> > On 15.05.20 19:33, Alexander Duyck wrote:  

> > > +\subsubsection{Free Page Hinting}\label{sec:Device Types / Memory Balloon Device / Device Operation / Free Page Hinting}
> > > +
> > > +Free page hinting is designed to be used during migration to determine what
> > > +pages within the guest are currently unused so that they can be skipped over
> > > +while migrating the guest. The device will indicate that it is ready to start
> > > +performing hinting by setting the \field{free_page_hint_cmd_id} to one of the
> > > +non-reserved values that can be used as a command ID. The following values
> > > +are reserved:
> > > +
> > > +\begin{description}
> > > +\item[VIRTIO_BALLOON_CMD_ID_STOP (0)] Any command ID previously supplied by
> > > +  the device is invalid. The driver should halt all hinting until a new
> > > +  command ID is supplied.  
> >
> > Maybe "The driver should stop hinting free pages, but should not reuse
> > all previously hinted pages."  
> 
> The "reuse all previously hinted pages" seems rather unclear to me. I
> would like to make clear that in this case the "use" is the guest
> making use of the memory, not the driver doing something like
> recycling hints. So in the spots where you reference "the driver
> reusing pages" I think I might prefer to go with something along the
> lines of "releasing pages for use by the guest". The problem is that
> when you have a balloon we were referencing using pages from the
> balloon. Since we cannot reference the balloon I figure I will go with
> language where we "supply" and "release" hinted pages. That way we
> acknowledge that the driver is holding onto pages and not freeing them
> for use by the guest.
> 
> I'll probably go with something like:
>   The driver should stop hinting free pages, but
>   should not release any hinted pages for use by the guest.

"The driver should stop hinting free pages until a new command ID is
supplied, but should not release..."

would sound good to me.

> 
> > > +
> > > +\item[VIRTIO_BALLOON_CMD_ID_DONE (1)] Any command ID previously supplied by
> > > +  the device is invalid. The driver should halt all hinting and the hinting
> > > +  balloon can now be deflated returning all pages to the guest.  
> >
> > I would avoid the terminology "hinting balloon" and "deflation".  
> 
> So I will clean up the spots you pointed out and also work on the
> workflow down below that includes "hinting balloon" in the
> terminology.
> 
> > "The driver should stop hinting free pages and should reuse all
> > previously hinted pages.".  
> 
> Similar to the comment above:
>   The driver should stop hinting free pages, and
>   should release all hinted pages for use by the guest.

Also sounds good to me.


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

Re: [virtio-comment] [PATCH v2 1/3] content: Document balloon feature free page hints

[David Hildenbrand]

[...]
>>> +\begin{description}
>>> +\item[VIRTIO_BALLOON_CMD_ID_STOP (0)] Any command ID previously supplied by
>>> +  the device is invalid. The driver should halt all hinting until a new
>>> +  command ID is supplied.
>>
>> Maybe "The driver should stop hinting free pages, but should not reuse
>> all previously hinted pages."
> 
> The "reuse all previously hinted pages" seems rather unclear to me. I
> would like to make clear that in this case the "use" is the guest
> making use of the memory, not the driver doing something like
> recycling hints. So in the spots where you reference "the driver

IMHO, In term of use/reuse, I think it does not matter. From spec POV,
whatever happens in the guest in respect to hinting is under driver
control. The driver just has to find a way that the memory won't be reused.

> reusing pages" I think I might prefer to go with something along the
> lines of "releasing pages for use by the guest". The problem is that
> when you have a balloon we were referencing using pages from the
> balloon. Since we cannot reference the balloon I figure I will go with
> language where we "supply" and "release" hinted pages. That way we
> acknowledge that the driver is holding onto pages and not freeing them
> for use by the guest.
> 
> I'll probably go with something like:
>   The driver should stop hinting free pages, but
>   should not release any hinted pages for use by the guest.
> 

I'd say "release any hinted pages" is an implementation detail in the
guest to make sure the pages won't be reused. But I don't have a strong
opinion here as long as it helps to describe what has to be done :)

[...]

>>> +
>>> +The driver SHOULD return pages for use once \field{free_page_hint_cmd_id}
>>> +reports a value of VIRTIO_BALLOON_CMD_ID_DONE.
>>
>> "return pages" -> "start to reuse all previously hinted pages".
> 
> The driver SHOULD release all hinted pages for use by the guest once
> \field{free_page_hint_cmd_id} reports a value of VIRTIO_BALLOON_CMD_ID_DONE.
> 
>> Also,
>>
>> "The driver MUST reinitialize hinted pages before reusing them."
> 
> That isn't quite correct though. It is only necessary to initialize
> the pages if the guest depends on them being initialized.
> 
> Maybe something like:
>   The driver MUST treat the content of all hinted pages as uninitialized memory.
> 

Makes sense.

>>> +
>>> +\devicenormative{\paragraph}{Free Page Hinting}{Device Types / Memory Balloon Device / Device Operation / Free Page Hinting}
>>> +
>>> +Normative statements in this section apply if the
>>> +VIRTIO_BALLOON_F_FREE_PAGE_HINT feature has been negotiated.
>>> +
>>> +The device MUST set \field{free_page_hint_cmd_id} to
>>> +VIRTIO_BALLOON_CMD_ID_STOP any time that the dirty pages for the given
>>> +guest are being recorded.
>>> +
>>> +The device MUST NOT reuse a command ID until it has received an output
>>> +descriptor containing VIRTIO_BALLOON_CMD_ID_STOP from the driver.
>>> +
>>> +The device MUST ignore pages that are provided with a command ID that does
>>> +not match the current value in \field{free_page_hint_cmd_id}.
>>> +
>>> +The device MAY modify the contents of the page in the balloon if the page
>>> +has not been modified by the guest since the \field{free_page_hint_cmd_id}
>>> +associated with the hint was issued by the device.
>>
>> "page in the balloon" -> "previously hinted pages"
>>
>> But it's not that easy in respect to the guest reusing the pages.
>>
>> "previously hinted pages and not reused pages" ?
>>
>> Also, something like
>>
>> "The device MUST NOT modify the contents of previously hinted pages in
>> case they are reused by the devices, even if they are reused by the
>> driver before the hinting request is processed."
> 
> That is not quite true.

I proposed that the driver MUST reinitialize the pages when reusing
(which is what Linux does), so then this is true. Reuse implies
initializing, implies modification. It's somewhat simpler than what you
propose, leaving the case open where the driver would reuse pages by
only reading them (I don't really see a use case for that ...). But I
don't care as long as it's consistent and correct :)

> 
> The driver can end up releasing the pages back to the buddy allocator
> and if they are not poisoned/init_on_free then they will go there and
> can still potentially change until such time as the guest writes to
> the page modifying it or the balloon driver switches the cmd ID to
> VIRTIO_BALLOON_CMD_ID_DONE. That was one of the reasons for trying to
> frame it the way I did. So what I can do is reword the two statements
> as follows:
> 
>   If the content of a previously hinted page has not been modified by the
>   guest since the device issued the \field{free_page_hint_cmd_id} associated
>   with the hint, the device MAY modify the contents of the page.
> 
>   The device MUST NOT modify the content of a previously hinted page
> after
>   \field{free_page_hint_cmd_id} is set to VIRTIO_BALLOON_CMD_ID_DONE.
Is it really only "DONE" that closes the current window? I think a
"STOP" from the device will also close the window. DONE is only set at
the very last iteration during memory migration.

(virtio_balloon_free_page_report_notify() in QEMU)

I consider one window == one iteration == one value of
\field{free_page_hint_cmd_id} until either DONE or STOP

[...]

Let's think this through, what about this scenario:

The device sets \field{free_page_hint_cmd_id} = X
The driver starts reporting free pages (and reports all pages it has)
1. Sends X to start the windows
2. Sends all page hints (\field{free_page_hint_cmd_id} stays X)
3. Sends VIRTIO_BALLOON_CMD_ID_STOP to end the window
The driver sets \field{free_page_hint_cmd_id} = DONE or STOP

The guest can reuse the pages any time (triggered by the shrinker),
especially, during 2, before the hypervisor even processed a hint
request. It can happen that the guest reuses a page before the
hypervisor processes the request and before
\field{free_page_hint_cmd_id} changes.

In QEMU, the double-bitmap magic makes sure that this is guaranteed to
work IIRC.

In that case, the page has to be migrated in that windows, the
hypervisor must not modify the content.


>>> +
>>> +The device MAY NOT modify the contents of the balloon after
>>> +\field{free_page_hint_cmd_id} is set to VIRTIO_BALLOON_CMD_ID_DONE.
>>
>> "contents of the balloon" is misleading. What exactly did you want to
>> say here? Modifying the content of previously hinted pages?
>>
>> "MAY NOT" does not exist in RFC2119. Was this meant to be "MUST NOT".
> 
> Thanks for pointing that out.
> 
>>> +
>>>  \section{SCSI Host Device}\label{sec:Device Types / SCSI Host Device}
>>>
>>>  The virtio SCSI host device groups together one or more virtual
>>>
>>>
>>>
>>
>> Whenever you add new normative sections/paragraphs, you also have to
>> link to them from conformance.tex (both, for devices and drivers).
> 
> I will go through and update that.
> 
>>
>> In addition, we should document that VIRTIO_BALLOON_F_MUST_TELL_HOST
>> does not affect VIRTIO_BALLOON_F_FREE_PAGE_HINT.
> 
> I'm not sure that really makes sense though. With the "balloon" and
> "deflate" wording being stripped I'm not sure it makes sense to
> indicate that we don't need to deflate the hinted pages since we don't
> describe doing that anywhere.

Agreed, if we rip out any trace of "balloon" "inflate" and "deflate",
this should work.

-- 
Thanks,

David / dhildenb


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

Re: [virtio-comment] [PATCH v2 3/3] content: Document balloon feature free page reporting

[David Hildenbrand]

On 15.05.20 19:33, Alexander Duyck wrote:
> From: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> 
> Free page reporting is a feature that allows the guest to proactively
> report unused pages to the host. By making use of this feature is is
> possible to reduce the overall memory footprint of the guest in cases where
> some significant portion of the memory is idle. Add documentation for the
> free page reporting feature describing the functionality and requirements.
> 
> Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> ---
>  content.tex |   84 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 83 insertions(+), 1 deletion(-)
> 
> diff --git a/content.tex b/content.tex
> index 3d30fd5bb6fa..3cb38105f794 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -5007,12 +5007,15 @@ \subsection{Virtqueues}\label{sec:Device Types / Memory Balloon Device / Virtque
>  \item[1] deflateq
>  \item[2] statsq
>  \item[3] free_page_vq
> +\item[4] reporting_vq
>  \end{description}
>  
>    statsq only exists if VIRTIO_BALLOON_F_STATS_VQ is set.
>  
>    free_page_vq only exists if VIRTIO_BALLOON_F_FREE_PAGE_HINT is set.
>  
> +  reporting_vq only exists if VIRTIO_BALLOON_F_PAGE_REPORTING is set.
> +
>  \subsection{Feature bits}\label{sec:Device Types / Memory Balloon Device / Feature bits}
>  \begin{description}
>  \item[VIRTIO_BALLOON_F_MUST_TELL_HOST (0)] Host has to be told before
> @@ -5029,6 +5032,8 @@ \subsection{Feature bits}\label{sec:Device Types / Memory Balloon Device / Featu
>  \item[ VIRTIO_BALLOON_F_PAGE_POISON(4) ] The device has to be notified if
>      the driver is expecting balloon pages to contain a certain value when
>      returned. Configuration field poison_val is valid.
> +\item[ VIRTIO_BALLOON_F_PAGE_REPORTING(5) ] The device has support for free
> +    page reporting. A virtqueue for reporting free guest memory is present.
>  
>  \end{description}
>  
> @@ -5039,6 +5044,10 @@ \subsection{Feature bits}\label{sec:Device Types / Memory Balloon Device / Featu
>  The driver SHOULD clear the VIRTIO_BALLOON_F_PAGE_POISON flag if it is not
>  expecting any specific value to be stored in the page.
>  
> +If the driver is expecting the pages to retain some initialized value,
> +it MUST NOT accept VIRTIO_BALLOON_F_PAGE_REPORTING unless it also
> +negotiates VIRTIO_BALLOON_F_PAGE_POISON.
> +

Is "accept" really the right word here? Below you use "negotiate", which
makes more sense.

>  \devicenormative{\subsubsection}{Feature bits}{Device Types / Memory Balloon Device / Feature bits}
>  If the device offers the VIRTIO_BALLOON_F_MUST_TELL_HOST feature
>  bit, and if the driver did not accept this feature bit, the
> @@ -5101,10 +5110,16 @@ \subsection{Device Initialization}\label{sec:Device Types / Memory Balloon Devic
>  \item If the VIRTIO_BALLOON_F_PAGE_POISON feature bit is negotiated, the
>    driver updates the \field{poison_val} configuration field.
>  
> +\item If the VIRTIO_BALLOON_F_PAGE_REPORTING feature bit is negotiated the
> +  reporting_vq is identified.
> +
>  \item DRIVER_OK is set: device operation begins.
>  
>  \item If the VIRTIO_BALLOON_F_STATS_VQ feature bit is negotiated, then
>    notify the device about the stats virtqueue buffer.
> +
> +\item If the VIRTIO_BALLOON_F_PAGE_REPORTING feature bit is negotiated then
> +  begin reporting free pages to device.
>  \end{enumerate}
>  
>  \subsection{Device Operation}\label{sec:Device Types / Memory Balloon Device / Device Operation}
> @@ -5478,7 +5493,9 @@ \subsubsection{Page Poison}\label{sec:Device Types / Memory Balloon Device / Dev
>  
>  Page Poison provides a way to notify the host of the contents that are
>  currently in the balloon pages, and those that are expected to be in the
> -pages when they are pulled from the balloon.
> +pages when they are pulled from the balloon. It is used for in-place
> +reporting of pages without needing to pull them from the memory allocator
> +of the guest.

Let's see how that looks like after you modify patch #2.

>  
>  If VIRTIO_BALLOON_F_PAGE_POISON feature has been negotiated, the guest
>  will place the expected poison value into the \field{poison_val}
> @@ -5504,6 +5521,71 @@ \subsubsection{Page Poison}\label{sec:Device Types / Memory Balloon Device / Dev
>  page hinting, the device MAY ignore the content of \field{poison_val}
>  for those operations.
>  
> +\subsubsection{Free Page Reporting}\label{sec:Device Types / Memory Balloon Device / Device Operation / Free Page Reporting}
> +
> +Free Page Reporting provides a mechanism similar to balloon inflation,
> +however it does not provide a deflation queue. The expectation is that the
> +device will have a means by which it can detect the guest page access and
> +fault in such pages with some initial value, likely a zero page.
> +
> +The driver will respond to to memory conditions and begin reporting free

"to to memory conditions" I don't understand what you are trying to say.
The driver will simply report some free pages (e.g., of a guest-specific
minimum size) when it feels like the right time has come.

This (and below) is too implementation specific. You could just
implement a driver that hints a single page every time it is getting
freed. Nothing wrong about that. There is just the option do to a bulk
report whenever the driver feels like doing it.

> +pages when some number of pages are available.
> +
> +\begin{enumerate}
> +
> +\item The driver determines it has enough pages available to begin
> +  reporting pages.
> +
> +\item The driver gathers pages into a scatter-gather list and adds them to
> +  the reporting_vq.
> +
> +\item The device acknowledges the reporting request by using the
> +  reporting_vq descriptor.
> +
> +\item Once the device has acknowledged the report, the pages can be
> +  returned to the location from which they were pulled.
> +
> +\item The driver can then continue to gather and report pages until it
> +  has determined it has reported a sufficient quantity of pages.
> +
> +\end{enumerate}
> +
> +\drivernormative{\paragraph}{Free Page Reporting}{Device Types / Memory Balloon Device / Device Operation / Free Page Reporting}
> +
> +Normative statements in this section apply if the
> +VIRTIO_BALLOON_F_PAGE_REPORTING feature has been negotiated.
> +
> +If the driver is expecting the free page to contain some initial value it
> +MUST NOT negotiate this feature without negotiating the
> +VIRTIO_BALLOON_F_PAGE_POISON feature as well and supply this value via
> +\field{poison_val}.
> +
> +If the driver is expecting the pages to retain some initialized value,
> +it MUST NOT accept VIRTIO_BALLOON_F_PAGE_REPORTING unless it also
> +negotiates the VIRTIO_BALLOON_F_PAGE_POISON feature and supplies this
> +value via \field{poison_val}.

I'd probably do something like

The driver MUST NOT negotiate VIRTIO_BALLOON_F_PAGE_REPORTING without
VIRTIO_BALLOON_F_PAGE_POISON in case it relies on reported pages to not
change their content.

The driver MUST negotiate VIRTIO_BALLOON_F_PAGE_REPORTING and
VIRTIO_BALLOON_F_PAGE_POISON in case it relies on reported pages to be
filled with \field{poison_val} when reusing them.

But I'll have to double check your QEMU patches :)

> +
> +The driver MUST NOT use the reported pages until the device has
> +acknowledged the reporting request.
> +
> +The driver MAY report free pages any time after DRIVER_OK is set.

Now, this is the right thing to document. Maybe also something like "The
driver MAY decide to not report some free pages, especially of smaller
granularity."

> +
> +It is RECOMMENDED that the driver avoid unnecessary reads or writes to the

s/avoid/avoids/

> +page contents as this could reduce the performance for free page reporting.
> +
You're the second user of RECOMMENDED.

I'd do: "The driver SHOULD avoid unnecessary reads ..."

> +\devicenormative{\paragraph}{Free Page Reporting}{Device Types / Memory Balloon Device / Device Operation / Free Page Reporting}
> +
> +Normative statements in this section apply if the
> +VIRTIO_BALLOON_F_PAGE_REPORTING feature has been negotiated.
> +
> +The device MAY modify the contents of any page supplied in a report
> +request even before acknowledging that request by using the
> +reporting_vq descriptor.
> +
> +If the VIRTIO_BALLOON_F_PAGE_POISON feature has been negotiated, the device
> +SHALL NOT modify the the page if this will result in the page containing a
> +value other than \field{poison_val}.

You're the first user of "SHALL NOT" -> "MUST NOT" ?

s/the the page/the content of a reported page/

"If the VIRTIO_BALLOON_F_PAGE_POISON feature has been negotiated, the
device MUST NOT modify the content of a reported page to something other
than than \field{poison_val}." ?

> +
>  \section{SCSI Host Device}\label{sec:Device Types / SCSI Host Device}
>  


-- 
Thanks,

David / dhildenb


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

Re: [virtio-comment] [PATCH v2 1/3] content: Document balloon feature free page hints

[Alexander Duyck]

On Tue, May 19, 2020 at 2:40 AM David Hildenbrand <david@redhat.com> wrote:
>
> [...]
> >>> +\begin{description}
> >>> +\item[VIRTIO_BALLOON_CMD_ID_STOP (0)] Any command ID previously supplied by
> >>> +  the device is invalid. The driver should halt all hinting until a new
> >>> +  command ID is supplied.
> >>
> >> Maybe "The driver should stop hinting free pages, but should not reuse
> >> all previously hinted pages."
> >
> > The "reuse all previously hinted pages" seems rather unclear to me. I
> > would like to make clear that in this case the "use" is the guest
> > making use of the memory, not the driver doing something like
> > recycling hints. So in the spots where you reference "the driver
>
> IMHO, In term of use/reuse, I think it does not matter. From spec POV,
> whatever happens in the guest in respect to hinting is under driver
> control. The driver just has to find a way that the memory won't be reused.

My only complaint about the use/reuse term is that when it was
previously employed it was to describe things in relation to a
balloon. So you would "use/reuse a page from the balloon" which
implied removing it. Without the balloon terminology it becomes much
more vague as "use" could mean many different things when the page
might be left on the guest already.

> > reusing pages" I think I might prefer to go with something along the
> > lines of "releasing pages for use by the guest". The problem is that
> > when you have a balloon we were referencing using pages from the
> > balloon. Since we cannot reference the balloon I figure I will go with
> > language where we "supply" and "release" hinted pages. That way we
> > acknowledge that the driver is holding onto pages and not freeing them
> > for use by the guest.
> >
> > I'll probably go with something like:
> >   The driver should stop hinting free pages, but
> >   should not release any hinted pages for use by the guest.
> >
>
> I'd say "release any hinted pages" is an implementation detail in the
> guest to make sure the pages won't be reused. But I don't have a strong
> opinion here as long as it helps to describe what has to be done :)

Well the wording here is "should not" which implies that not releasing
the pages is optional but recommended. The problem with releasing the
pages is that without something like the page reporting code I did for
the Linux kernel the guest would end up having to hint all the pages
again to get to where it was before.

>
> >>> +
> >>> +The driver SHOULD return pages for use once \field{free_page_hint_cmd_id}
> >>> +reports a value of VIRTIO_BALLOON_CMD_ID_DONE.
> >>
> >> "return pages" -> "start to reuse all previously hinted pages".
> >
> > The driver SHOULD release all hinted pages for use by the guest once
> > \field{free_page_hint_cmd_id} reports a value of VIRTIO_BALLOON_CMD_ID_DONE.
> >
> >> Also,
> >>
> >> "The driver MUST reinitialize hinted pages before reusing them."
> >
> > That isn't quite correct though. It is only necessary to initialize
> > the pages if the guest depends on them being initialized.
> >
> > Maybe something like:
> >   The driver MUST treat the content of all hinted pages as uninitialized memory.
> >
>
> Makes sense.
>
> >>> +
> >>> +\devicenormative{\paragraph}{Free Page Hinting}{Device Types / Memory Balloon Device / Device Operation / Free Page Hinting}
> >>> +
> >>> +Normative statements in this section apply if the
> >>> +VIRTIO_BALLOON_F_FREE_PAGE_HINT feature has been negotiated.
> >>> +
> >>> +The device MUST set \field{free_page_hint_cmd_id} to
> >>> +VIRTIO_BALLOON_CMD_ID_STOP any time that the dirty pages for the given
> >>> +guest are being recorded.
> >>> +
> >>> +The device MUST NOT reuse a command ID until it has received an output
> >>> +descriptor containing VIRTIO_BALLOON_CMD_ID_STOP from the driver.
> >>> +
> >>> +The device MUST ignore pages that are provided with a command ID that does
> >>> +not match the current value in \field{free_page_hint_cmd_id}.
> >>> +
> >>> +The device MAY modify the contents of the page in the balloon if the page
> >>> +has not been modified by the guest since the \field{free_page_hint_cmd_id}
> >>> +associated with the hint was issued by the device.
> >>
> >> "page in the balloon" -> "previously hinted pages"
> >>
> >> But it's not that easy in respect to the guest reusing the pages.
> >>
> >> "previously hinted pages and not reused pages" ?
> >>
> >> Also, something like
> >>
> >> "The device MUST NOT modify the contents of previously hinted pages in
> >> case they are reused by the devices, even if they are reused by the
> >> driver before the hinting request is processed."
> >
> > That is not quite true.
>
> I proposed that the driver MUST reinitialize the pages when reusing
> (which is what Linux does), so then this is true. Reuse implies
> initializing, implies modification. It's somewhat simpler than what you
> propose, leaving the case open where the driver would reuse pages by
> only reading them (I don't really see a use case for that ...). But I
> don't care as long as it's consistent and correct :)

Linux does not reinitialize the pages when it frees them. That only
happens if poison or init_on_free are enabled which are rare cases.
When it does reinitialize the pages then I agree that the device
cannot modify the contents.

The current implementation is assuming QEMU live-migration with the
Linux guest as the only use case. As such I want to make sure we
correctly capture all of the behaviors that are expected based on
those assumptions, but I want to avoid inserting behaviors we would
like to see occur but aren't really a part of this.

> >
> > The driver can end up releasing the pages back to the buddy allocator
> > and if they are not poisoned/init_on_free then they will go there and
> > can still potentially change until such time as the guest writes to
> > the page modifying it or the balloon driver switches the cmd ID to
> > VIRTIO_BALLOON_CMD_ID_DONE. That was one of the reasons for trying to
> > frame it the way I did. So what I can do is reword the two statements
> > as follows:
> >
> >   If the content of a previously hinted page has not been modified by the
> >   guest since the device issued the \field{free_page_hint_cmd_id} associated
> >   with the hint, the device MAY modify the contents of the page.
> >
> >   The device MUST NOT modify the content of a previously hinted page
> > after
> >   \field{free_page_hint_cmd_id} is set to VIRTIO_BALLOON_CMD_ID_DONE.
> Is it really only "DONE" that closes the current window? I think a
> "STOP" from the device will also close the window. DONE is only set at
> the very last iteration during memory migration.

So the CMD_ID_DONE is issued when the migration has occurred. The
migration is what is actually modifying the memory.

> (virtio_balloon_free_page_report_notify() in QEMU)
>
> I consider one window == one iteration == one value of
> \field{free_page_hint_cmd_id} until either DONE or STOP

CMD_ID_STOP will close the current window for providing hints, but the
migration hasn't happened yet. We are still accumulating the hints. We
don't receive CMD_ID_DONE from the device until the migration has
occurred. It is the migration that will alter the content of the pages
by leaving them behind on the previous VM.

> [...]
>
> Let's think this through, what about this scenario:
>
> The device sets \field{free_page_hint_cmd_id} = X
> The driver starts reporting free pages (and reports all pages it has)
> 1. Sends X to start the windows
> 2. Sends all page hints (\field{free_page_hint_cmd_id} stays X)
> 3. Sends VIRTIO_BALLOON_CMD_ID_STOP to end the window
> The driver sets \field{free_page_hint_cmd_id} = DONE or STOP
>
> The guest can reuse the pages any time (triggered by the shrinker),
> especially, during 2, before the hypervisor even processed a hint
> request. It can happen that the guest reuses a page before the
> hypervisor processes the request and before
> \field{free_page_hint_cmd_id} changes.
>
> In QEMU, the double-bitmap magic makes sure that this is guaranteed to
> work IIRC.
>
> In that case, the page has to be migrated in that windows, the
> hypervisor must not modify the content.

If by "reuse" you mean write to or reinitialize then that is correct.
All that is really happening is that any pages that are hinted have
the potential to be left behind with the original VM and not migrated
to the new one. We get the notification that the migration happened
when CMD_ID_DONE is passed to us. At that point the hinting is
complete and the device has no use for additional data.

Instead of CMD_ID_STOP it probably would have made more sense to call
it something like CMD_ID_PAUSE or CMD_ID_HOLD as that is what it is
really doing. It is just temporarily holding the hints off while the
hypervisor synchronizes the dirty bits from the host.

> >>> +
> >>> +The device MAY NOT modify the contents of the balloon after
> >>> +\field{free_page_hint_cmd_id} is set to VIRTIO_BALLOON_CMD_ID_DONE.
> >>
> >> "contents of the balloon" is misleading. What exactly did you want to
> >> say here? Modifying the content of previously hinted pages?
> >>
> >> "MAY NOT" does not exist in RFC2119. Was this meant to be "MUST NOT".
> >
> > Thanks for pointing that out.
> >
> >>> +
> >>>  \section{SCSI Host Device}\label{sec:Device Types / SCSI Host Device}
> >>>
> >>>  The virtio SCSI host device groups together one or more virtual
> >>>
> >>>
> >>>
> >>
> >> Whenever you add new normative sections/paragraphs, you also have to
> >> link to them from conformance.tex (both, for devices and drivers).
> >
> > I will go through and update that.
> >
> >>
> >> In addition, we should document that VIRTIO_BALLOON_F_MUST_TELL_HOST
> >> does not affect VIRTIO_BALLOON_F_FREE_PAGE_HINT.
> >
> > I'm not sure that really makes sense though. With the "balloon" and
> > "deflate" wording being stripped I'm not sure it makes sense to
> > indicate that we don't need to deflate the hinted pages since we don't
> > describe doing that anywhere.
>
> Agreed, if we rip out any trace of "balloon" "inflate" and "deflate",
> this should work.

Sounds good. Thanks.

- Alex

This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

Re: [virtio-comment] [PATCH v2 1/3] content: Document balloon feature free page hints

[Alexander Duyck]

On Mon, May 18, 2020 at 11:54 PM Cornelia Huck <cohuck@redhat.com> wrote:
>
> On Mon, 18 May 2020 13:15:50 -0700
> Alexander Duyck <alexander.duyck@gmail.com> wrote:
>
> > On Mon, May 18, 2020 at 6:18 AM David Hildenbrand <david@redhat.com> wrote:
> > >
> > > On 15.05.20 19:33, Alexander Duyck wrote:
>
> > > > +\subsubsection{Free Page Hinting}\label{sec:Device Types / Memory Balloon Device / Device Operation / Free Page Hinting}
> > > > +
> > > > +Free page hinting is designed to be used during migration to determine what
> > > > +pages within the guest are currently unused so that they can be skipped over
> > > > +while migrating the guest. The device will indicate that it is ready to start
> > > > +performing hinting by setting the \field{free_page_hint_cmd_id} to one of the
> > > > +non-reserved values that can be used as a command ID. The following values
> > > > +are reserved:
> > > > +
> > > > +\begin{description}
> > > > +\item[VIRTIO_BALLOON_CMD_ID_STOP (0)] Any command ID previously supplied by
> > > > +  the device is invalid. The driver should halt all hinting until a new
> > > > +  command ID is supplied.
> > >
> > > Maybe "The driver should stop hinting free pages, but should not reuse
> > > all previously hinted pages."
> >
> > The "reuse all previously hinted pages" seems rather unclear to me. I
> > would like to make clear that in this case the "use" is the guest
> > making use of the memory, not the driver doing something like
> > recycling hints. So in the spots where you reference "the driver
> > reusing pages" I think I might prefer to go with something along the
> > lines of "releasing pages for use by the guest". The problem is that
> > when you have a balloon we were referencing using pages from the
> > balloon. Since we cannot reference the balloon I figure I will go with
> > language where we "supply" and "release" hinted pages. That way we
> > acknowledge that the driver is holding onto pages and not freeing them
> > for use by the guest.
> >
> > I'll probably go with something like:
> >   The driver should stop hinting free pages, but
> >   should not release any hinted pages for use by the guest.
>
> "The driver should stop hinting free pages until a new command ID is
> supplied, but should not release..."
>
> would sound good to me.

Thanks, I will make that tweak.

This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

Re: [virtio-comment] [PATCH v2 1/3] content: Document balloon feature free page hints

[David Hildenbrand]

>> I proposed that the driver MUST reinitialize the pages when reusing
>> (which is what Linux does), so then this is true. Reuse implies
>> initializing, implies modification. It's somewhat simpler than what you
>> propose, leaving the case open where the driver would reuse pages by
>> only reading them (I don't really see a use case for that ...). But I
>> don't care as long as it's consistent and correct :)
> 
> Linux does not reinitialize the pages when it frees them. That only

Whoever uses the pages has to initialize. Again, I don't think we should
make difference between the guest and the driver. From spec POV, they
are one piece. Everything else is implementation detail.

> happens if poison or init_on_free are enabled which are rare cases.
> When it does reinitialize the pages then I agree that the device
> cannot modify the contents.

What about a user who relies on the content of uninitialized pages?
Like, read it, if it has the value, don't set it to the value. Unlikely
but possible, no? We could have data corruption.

We should document that in some way, because this is what could happen
with the *current* QEMU implementation

> 
> The current implementation is assuming QEMU live-migration with the
> Linux guest as the only use case. As such I want to make sure we
> correctly capture all of the behaviors that are expected based on
> those assumptions, but I want to avoid inserting behaviors we would
> like to see occur but aren't really a part of this.

Exactly that's why I bring this ^ up.

> 
>>>
>>> The driver can end up releasing the pages back to the buddy allocator
>>> and if they are not poisoned/init_on_free then they will go there and
>>> can still potentially change until such time as the guest writes to
>>> the page modifying it or the balloon driver switches the cmd ID to
>>> VIRTIO_BALLOON_CMD_ID_DONE. That was one of the reasons for trying to
>>> frame it the way I did. So what I can do is reword the two statements
>>> as follows:
>>>
>>>   If the content of a previously hinted page has not been modified by the
>>>   guest since the device issued the \field{free_page_hint_cmd_id} associated
>>>   with the hint, the device MAY modify the contents of the page.
>>>
>>>   The device MUST NOT modify the content of a previously hinted page
>>> after
>>>   \field{free_page_hint_cmd_id} is set to VIRTIO_BALLOON_CMD_ID_DONE.
>> Is it really only "DONE" that closes the current window? I think a
>> "STOP" from the device will also close the window. DONE is only set at
>> the very last iteration during memory migration.
> 
> So the CMD_ID_DONE is issued when the migration has occurred. The
> migration is what is actually modifying the memory.
> 
>> (virtio_balloon_free_page_report_notify() in QEMU)
>>
>> I consider one window == one iteration == one value of
>> \field{free_page_hint_cmd_id} until either DONE or STOP
> 
> CMD_ID_STOP will close the current window for providing hints, but the
> migration hasn't happened yet. We are still accumulating the hints. We
> don't receive CMD_ID_DONE from the device until the migration has
> occurred. It is the migration that will alter the content of the pages
> by leaving them behind on the previous VM.

I'll have to think about again if your statements reflect the reality
today. I'll have to dive once again into QEMU code :( Complicated stuff.

> 
>> [...]
>>
>> Let's think this through, what about this scenario:
>>
>> The device sets \field{free_page_hint_cmd_id} = X
>> The driver starts reporting free pages (and reports all pages it has)
>> 1. Sends X to start the windows
>> 2. Sends all page hints (\field{free_page_hint_cmd_id} stays X)
>> 3. Sends VIRTIO_BALLOON_CMD_ID_STOP to end the window
>> The driver sets \field{free_page_hint_cmd_id} = DONE or STOP
>>
>> The guest can reuse the pages any time (triggered by the shrinker),
>> especially, during 2, before the hypervisor even processed a hint
>> request. It can happen that the guest reuses a page before the
>> hypervisor processes the request and before
>> \field{free_page_hint_cmd_id} changes.
>>
>> In QEMU, the double-bitmap magic makes sure that this is guaranteed to
>> work IIRC.
>>
>> In that case, the page has to be migrated in that windows, the
>> hypervisor must not modify the content.
> 
> If by "reuse" you mean write to or reinitialize then that is correct.
> All that is really happening is that any pages that are hinted have
> the potential to be left behind with the original VM and not migrated
> to the new one. We get the notification that the migration happened
> when CMD_ID_DONE is passed to us. At that point the hinting is
> complete and the device has no use for additional data.
> 
> Instead of CMD_ID_STOP it probably would have made more sense to call
> it something like CMD_ID_PAUSE or CMD_ID_HOLD as that is what it is
> really doing. It is just temporarily holding the hints off while the
> hypervisor synchronizes the dirty bits from the host.

I think if migration fails, it will be left set to STOP. Guess we should
specify that possibility somehow as well.

-- 
Thanks,

David / dhildenb


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

Re: [virtio-comment] [PATCH v2 3/3] content: Document balloon feature free page reporting

[Alexander Duyck]

On Tue, May 19, 2020 at 3:27 AM David Hildenbrand <david@redhat.com> wrote:
>
> On 15.05.20 19:33, Alexander Duyck wrote:
> > From: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> >
> > Free page reporting is a feature that allows the guest to proactively
> > report unused pages to the host. By making use of this feature is is
> > possible to reduce the overall memory footprint of the guest in cases where
> > some significant portion of the memory is idle. Add documentation for the
> > free page reporting feature describing the functionality and requirements.
> >
> > Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> > ---
> >  content.tex |   84 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 83 insertions(+), 1 deletion(-)
> >
> > diff --git a/content.tex b/content.tex
> > index 3d30fd5bb6fa..3cb38105f794 100644
> > --- a/content.tex
> > +++ b/content.tex
> > @@ -5007,12 +5007,15 @@ \subsection{Virtqueues}\label{sec:Device Types / Memory Balloon Device / Virtque
> >  \item[1] deflateq
> >  \item[2] statsq
> >  \item[3] free_page_vq
> > +\item[4] reporting_vq
> >  \end{description}
> >
> >    statsq only exists if VIRTIO_BALLOON_F_STATS_VQ is set.
> >
> >    free_page_vq only exists if VIRTIO_BALLOON_F_FREE_PAGE_HINT is set.
> >
> > +  reporting_vq only exists if VIRTIO_BALLOON_F_PAGE_REPORTING is set.
> > +
> >  \subsection{Feature bits}\label{sec:Device Types / Memory Balloon Device / Feature bits}
> >  \begin{description}
> >  \item[VIRTIO_BALLOON_F_MUST_TELL_HOST (0)] Host has to be told before
> > @@ -5029,6 +5032,8 @@ \subsection{Feature bits}\label{sec:Device Types / Memory Balloon Device / Featu
> >  \item[ VIRTIO_BALLOON_F_PAGE_POISON(4) ] The device has to be notified if
> >      the driver is expecting balloon pages to contain a certain value when
> >      returned. Configuration field poison_val is valid.
> > +\item[ VIRTIO_BALLOON_F_PAGE_REPORTING(5) ] The device has support for free
> > +    page reporting. A virtqueue for reporting free guest memory is present.
> >
> >  \end{description}
> >
> > @@ -5039,6 +5044,10 @@ \subsection{Feature bits}\label{sec:Device Types / Memory Balloon Device / Featu
> >  The driver SHOULD clear the VIRTIO_BALLOON_F_PAGE_POISON flag if it is not
> >  expecting any specific value to be stored in the page.
> >
> > +If the driver is expecting the pages to retain some initialized value,
> > +it MUST NOT accept VIRTIO_BALLOON_F_PAGE_REPORTING unless it also
> > +negotiates VIRTIO_BALLOON_F_PAGE_POISON.
> > +
>
> Is "accept" really the right word here? Below you use "negotiate", which
> makes more sense.

Okay, I'll change that.

> >  \devicenormative{\subsubsection}{Feature bits}{Device Types / Memory Balloon Device / Feature bits}
> >  If the device offers the VIRTIO_BALLOON_F_MUST_TELL_HOST feature
> >  bit, and if the driver did not accept this feature bit, the
> > @@ -5101,10 +5110,16 @@ \subsection{Device Initialization}\label{sec:Device Types / Memory Balloon Devic
> >  \item If the VIRTIO_BALLOON_F_PAGE_POISON feature bit is negotiated, the
> >    driver updates the \field{poison_val} configuration field.
> >
> > +\item If the VIRTIO_BALLOON_F_PAGE_REPORTING feature bit is negotiated the
> > +  reporting_vq is identified.
> > +
> >  \item DRIVER_OK is set: device operation begins.
> >
> >  \item If the VIRTIO_BALLOON_F_STATS_VQ feature bit is negotiated, then
> >    notify the device about the stats virtqueue buffer.
> > +
> > +\item If the VIRTIO_BALLOON_F_PAGE_REPORTING feature bit is negotiated then
> > +  begin reporting free pages to device.
> >  \end{enumerate}
> >
> >  \subsection{Device Operation}\label{sec:Device Types / Memory Balloon Device / Device Operation}
> > @@ -5478,7 +5493,9 @@ \subsubsection{Page Poison}\label{sec:Device Types / Memory Balloon Device / Dev
> >
> >  Page Poison provides a way to notify the host of the contents that are
> >  currently in the balloon pages, and those that are expected to be in the
> > -pages when they are pulled from the balloon.
> > +pages when they are pulled from the balloon. It is used for in-place
> > +reporting of pages without needing to pull them from the memory allocator
> > +of the guest.
>
> Let's see how that looks like after you modify patch #2.

What I currently have is:
  Page Poison provides a way to notify the host that the guest is initializing
  and/or poisoning free pages. When the feature is enabled pages that are
  deflated will be immediately written to by the guest, and pages indicated by
  free page reporting will contain the value indicated by \field{poison_val}.

> >
> >  If VIRTIO_BALLOON_F_PAGE_POISON feature has been negotiated, the guest
> >  will place the expected poison value into the \field{poison_val}
> > @@ -5504,6 +5521,71 @@ \subsubsection{Page Poison}\label{sec:Device Types / Memory Balloon Device / Dev
> >  page hinting, the device MAY ignore the content of \field{poison_val}
> >  for those operations.
> >
> > +\subsubsection{Free Page Reporting}\label{sec:Device Types / Memory Balloon Device / Device Operation / Free Page Reporting}
> > +
> > +Free Page Reporting provides a mechanism similar to balloon inflation,
> > +however it does not provide a deflation queue. The expectation is that the
> > +device will have a means by which it can detect the guest page access and
> > +fault in such pages with some initial value, likely a zero page.
> > +
> > +The driver will respond to to memory conditions and begin reporting free
>
> "to to memory conditions" I don't understand what you are trying to say.
> The driver will simply report some free pages (e.g., of a guest-specific
> minimum size) when it feels like the right time has come.
>
> This (and below) is too implementation specific. You could just
> implement a driver that hints a single page every time it is getting
> freed. Nothing wrong about that. There is just the option do to a bulk
> report whenever the driver feels like doing it.
>
> > +pages when some number of pages are available.
> > +

I don't really think it is all that specific. The full wording is:
  The driver will respond to memory conditions and begin reporting free
  pages when some number of pages are available.

So in this case "memory conditions" could be freeing a page and "some
number" could be 1 if that is what you want to go for. I am not saying
it has to bulk, but it could. There are a number of ways this could be
interpreted. Basically there is "some condition" that will trigger us
reporting pages. If that is 1 free page then I think that is described
by that sentence, but so is the case where we wait until there are a
large number of free pages.

> > +\begin{enumerate}
> > +
> > +\item The driver determines it has enough pages available to begin
> > +  reporting pages.
> > +
> > +\item The driver gathers pages into a scatter-gather list and adds them to
> > +  the reporting_vq.
> > +
> > +\item The device acknowledges the reporting request by using the
> > +  reporting_vq descriptor.
> > +
> > +\item Once the device has acknowledged the report, the pages can be
> > +  returned to the location from which they were pulled.
> > +
> > +\item The driver can then continue to gather and report pages until it
> > +  has determined it has reported a sufficient quantity of pages.
> > +
> > +\end{enumerate}
> > +
> > +\drivernormative{\paragraph}{Free Page Reporting}{Device Types / Memory Balloon Device / Device Operation / Free Page Reporting}
> > +
> > +Normative statements in this section apply if the
> > +VIRTIO_BALLOON_F_PAGE_REPORTING feature has been negotiated.
> > +
> > +If the driver is expecting the free page to contain some initial value it
> > +MUST NOT negotiate this feature without negotiating the
> > +VIRTIO_BALLOON_F_PAGE_POISON feature as well and supply this value via
> > +\field{poison_val}.
> > +
> > +If the driver is expecting the pages to retain some initialized value,
> > +it MUST NOT accept VIRTIO_BALLOON_F_PAGE_REPORTING unless it also
> > +negotiates the VIRTIO_BALLOON_F_PAGE_POISON feature and supplies this
> > +value via \field{poison_val}.
>
> I'd probably do something like
>
> The driver MUST NOT negotiate VIRTIO_BALLOON_F_PAGE_REPORTING without
> VIRTIO_BALLOON_F_PAGE_POISON in case it relies on reported pages to not
> change their content.

So the problem is VIRTIO_BALLOON_F_PAGE_POISON doesn't protect the
content. It guarantees that the page will be filled with the
poison_val if they are modified.

Actually maybe I can go back through and revise this based on the
updates I did for the Free Page Hinting. I can just state that the
driver must treat reported pages as uninitialized if PAGE_POISON is
not defined.

> The driver MUST negotiate VIRTIO_BALLOON_F_PAGE_REPORTING and
> VIRTIO_BALLOON_F_PAGE_POISON in case it relies on reported pages to be
> filled with \field{poison_val} when reusing them.

Again I want to be careful about the wording here. The device doesn't
have to modify the pages. It can just do nothing in response to a
reporting request. So it is the responsibility of the driver to fill
the page with poison_val and then the device cannot modify the page in
any way that might cause it to stray from that.

So I could probably replace these with something like:
  If the VIRTIO_BALLOON_F_PAGE_POISON feature has not been negotiated, then
  the driver MUST treat all reported pages as uninitialized memory.

  If the VIRTIO_BALLOON_F_PAGE_POISON feature has been negotiated, the driver
  MUST guarantee that the page has been filled with no value other than
  \field{poison_val}.

Then when we complete it when we combine that with the comment in the
device section:
  If the VIRTIO_BALLOON_F_PAGE_POISON feature has been negotiated, the device
  MUST NOT modify the the content of a reported page to a value other than
  \field{poison_val}.

> But I'll have to double check your QEMU patches :)

So I still have the patch that enables this bit outstanding. Basically
it is disabling free page reporting because we cannot handle the case
where we have to treat a reported page a uninitialized memory if the
page poison feature is not present.

> > +
> > +The driver MUST NOT use the reported pages until the device has
> > +acknowledged the reporting request.
> > +
> > +The driver MAY report free pages any time after DRIVER_OK is set.
>
> Now, this is the right thing to document. Maybe also something like "The
> driver MAY decide to not report some free pages, especially of smaller
> granularity."

So I probably wouldn't want to use MAY here, but may want to phrase
this as more of a SHOULD.

So maybe something like:
  The driver SHOULD attempt to report large pages rather than smaller ones
  as this could improve performance for free page reporting.

> > +
> > +It is RECOMMENDED that the driver avoid unnecessary reads or writes to the
>
> s/avoid/avoids/
>
> > +page contents as this could reduce the performance for free page reporting.
> > +
> You're the second user of RECOMMENDED.
>
> I'd do: "The driver SHOULD avoid unnecessary reads ..."

I'll update it to the following:
  The driver SHOULD avoid unnecessary reads or writes to the reported page
  contents as this could reduce the performance for free page reporting.

> > +\devicenormative{\paragraph}{Free Page Reporting}{Device Types / Memory Balloon Device / Device Operation / Free Page Reporting}
> > +
> > +Normative statements in this section apply if the
> > +VIRTIO_BALLOON_F_PAGE_REPORTING feature has been negotiated.
> > +
> > +The device MAY modify the contents of any page supplied in a report
> > +request even before acknowledging that request by using the
> > +reporting_vq descriptor.
> > +
> > +If the VIRTIO_BALLOON_F_PAGE_POISON feature has been negotiated, the device
> > +SHALL NOT modify the the page if this will result in the page containing a
> > +value other than \field{poison_val}.
>
> You're the first user of "SHALL NOT" -> "MUST NOT" ?
>
> s/the the page/the content of a reported page/
>
> "If the VIRTIO_BALLOON_F_PAGE_POISON feature has been negotiated, the
> device MUST NOT modify the content of a reported page to something other
> than than \field{poison_val}." ?

I will tweak the wording to match what you have here.

So with this I think we have gone through the full set. I will see
about cleaning the last bits up and resubmitting the set later today.

Thanks.

- Alex

This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

Re: [virtio-comment] [PATCH v2 3/3] content: Document balloon feature free page reporting

[David Hildenbrand]

> 
>>>  \devicenormative{\subsubsection}{Feature bits}{Device Types / Memory Balloon Device / Feature bits}
>>>  If the device offers the VIRTIO_BALLOON_F_MUST_TELL_HOST feature
>>>  bit, and if the driver did not accept this feature bit, the
>>> @@ -5101,10 +5110,16 @@ \subsection{Device Initialization}\label{sec:Device Types / Memory Balloon Devic
>>>  \item If the VIRTIO_BALLOON_F_PAGE_POISON feature bit is negotiated, the
>>>    driver updates the \field{poison_val} configuration field.
>>>
>>> +\item If the VIRTIO_BALLOON_F_PAGE_REPORTING feature bit is negotiated the
>>> +  reporting_vq is identified.
>>> +
>>>  \item DRIVER_OK is set: device operation begins.
>>>
>>>  \item If the VIRTIO_BALLOON_F_STATS_VQ feature bit is negotiated, then
>>>    notify the device about the stats virtqueue buffer.
>>> +
>>> +\item If the VIRTIO_BALLOON_F_PAGE_REPORTING feature bit is negotiated then
>>> +  begin reporting free pages to device.
>>>  \end{enumerate}
>>>
>>>  \subsection{Device Operation}\label{sec:Device Types / Memory Balloon Device / Device Operation}
>>> @@ -5478,7 +5493,9 @@ \subsubsection{Page Poison}\label{sec:Device Types / Memory Balloon Device / Dev
>>>
>>>  Page Poison provides a way to notify the host of the contents that are
>>>  currently in the balloon pages, and those that are expected to be in the
>>> -pages when they are pulled from the balloon.
>>> +pages when they are pulled from the balloon. It is used for in-place
>>> +reporting of pages without needing to pull them from the memory allocator
>>> +of the guest.
>>
>> Let's see how that looks like after you modify patch #2.
> 
> What I currently have is:
>   Page Poison provides a way to notify the host that the guest is initializing
>   and/or poisoning free pages. When the feature is enabled pages that are
>   deflated will be immediately written to by the guest, and pages indicated by
>   free page reporting will contain the value indicated by \field{poison_val}.

Sounds good! Wonder if "will be immediately" -> "might be immediately".

> 
>>>
>>>  If VIRTIO_BALLOON_F_PAGE_POISON feature has been negotiated, the guest
>>>  will place the expected poison value into the \field{poison_val}
>>> @@ -5504,6 +5521,71 @@ \subsubsection{Page Poison}\label{sec:Device Types / Memory Balloon Device / Dev
>>>  page hinting, the device MAY ignore the content of \field{poison_val}
>>>  for those operations.
>>>
>>> +\subsubsection{Free Page Reporting}\label{sec:Device Types / Memory Balloon Device / Device Operation / Free Page Reporting}
>>> +
>>> +Free Page Reporting provides a mechanism similar to balloon inflation,
>>> +however it does not provide a deflation queue. The expectation is that the
>>> +device will have a means by which it can detect the guest page access and
>>> +fault in such pages with some initial value, likely a zero page.
>>> +
>>> +The driver will respond to to memory conditions and begin reporting free
>>
>> "to to memory conditions" I don't understand what you are trying to say.
>> The driver will simply report some free pages (e.g., of a guest-specific
>> minimum size) when it feels like the right time has come.
>>
>> This (and below) is too implementation specific. You could just
>> implement a driver that hints a single page every time it is getting
>> freed. Nothing wrong about that. There is just the option do to a bulk
>> report whenever the driver feels like doing it.
>>
>>> +pages when some number of pages are available.
>>> +
> 
> I don't really think it is all that specific. The full wording is:
>   The driver will respond to memory conditions and begin reporting free
>   pages when some number of pages are available.
> 
> So in this case "memory conditions" could be freeing a page and "some
> number" could be 1 if that is what you want to go for. I am not saying
> it has to bulk, but it could. There are a number of ways this could be
> interpreted. Basically there is "some condition" that will trigger us
> reporting pages. If that is 1 free page then I think that is described
> by that sentence, but so is the case where we wait until there are a
> large number of free pages.

If it's not all that specific, why not simplify to

" The driver will begin reporting free pages. When exactly and which
free pages are reported is up to the driver."

?

> 
>>> +\begin{enumerate}
>>> +
>>> +\item The driver determines it has enough pages available to begin
>>> +  reporting pages.
>>> +
>>> +\item The driver gathers pages into a scatter-gather list and adds them to
>>> +  the reporting_vq.
>>> +
>>> +\item The device acknowledges the reporting request by using the
>>> +  reporting_vq descriptor.
>>> +
>>> +\item Once the device has acknowledged the report, the pages can be
>>> +  returned to the location from which they were pulled.
>>> +
>>> +\item The driver can then continue to gather and report pages until it
>>> +  has determined it has reported a sufficient quantity of pages.
>>> +
>>> +\end{enumerate}
>>> +
>>> +\drivernormative{\paragraph}{Free Page Reporting}{Device Types / Memory Balloon Device / Device Operation / Free Page Reporting}
>>> +
>>> +Normative statements in this section apply if the
>>> +VIRTIO_BALLOON_F_PAGE_REPORTING feature has been negotiated.
>>> +
>>> +If the driver is expecting the free page to contain some initial value it
>>> +MUST NOT negotiate this feature without negotiating the
>>> +VIRTIO_BALLOON_F_PAGE_POISON feature as well and supply this value via
>>> +\field{poison_val}.
>>> +
>>> +If the driver is expecting the pages to retain some initialized value,
>>> +it MUST NOT accept VIRTIO_BALLOON_F_PAGE_REPORTING unless it also
>>> +negotiates the VIRTIO_BALLOON_F_PAGE_POISON feature and supplies this
>>> +value via \field{poison_val}.
>>
>> I'd probably do something like
>>
>> The driver MUST NOT negotiate VIRTIO_BALLOON_F_PAGE_REPORTING without
>> VIRTIO_BALLOON_F_PAGE_POISON in case it relies on reported pages to not
>> change their content.
> 
> So the problem is VIRTIO_BALLOON_F_PAGE_POISON doesn't protect the
> content. It guarantees that the page will be filled with the
> poison_val if they are modified.

Right

> 
> Actually maybe I can go back through and revise this based on the
> updates I did for the Free Page Hinting. I can just state that the
> driver must treat reported pages as uninitialized if PAGE_POISON is
> not defined.

I think that makes sense.

> 
>> The driver MUST negotiate VIRTIO_BALLOON_F_PAGE_REPORTING and
>> VIRTIO_BALLOON_F_PAGE_POISON in case it relies on reported pages to be
>> filled with \field{poison_val} when reusing them.
> 
> Again I want to be careful about the wording here. The device doesn't
> have to modify the pages. It can just do nothing in response to a
> reporting request. So it is the responsibility of the driver to fill
> the page with poison_val and then the device cannot modify the page in
> any way that might cause it to stray from that.
> 
> So I could probably replace these with something like:
>   If the VIRTIO_BALLOON_F_PAGE_POISON feature has not been negotiated, then
>   the driver MUST treat all reported pages as uninitialized memory.
> 
>   If the VIRTIO_BALLOON_F_PAGE_POISON feature has been negotiated, the driver
>   MUST guarantee that the page has been filled with no value other than
>   \field{poison_val}.

Sounds better!

> 
> Then when we complete it when we combine that with the comment in the
> device section:
>   If the VIRTIO_BALLOON_F_PAGE_POISON feature has been negotiated, the device
>   MUST NOT modify the the content of a reported page to a value other than
>   \field{poison_val}.
> 
>> But I'll have to double check your QEMU patches :)
> 
> So I still have the patch that enables this bit outstanding. Basically
> it is disabling free page reporting because we cannot handle the case
> where we have to treat a reported page a uninitialized memory if the
> page poison feature is not present.
> 
>>> +
>>> +The driver MUST NOT use the reported pages until the device has
>>> +acknowledged the reporting request.
>>> +
>>> +The driver MAY report free pages any time after DRIVER_OK is set.
>>
>> Now, this is the right thing to document. Maybe also something like "The
>> driver MAY decide to not report some free pages, especially of smaller
>> granularity."
> 
> So I probably wouldn't want to use MAY here, but may want to phrase
> this as more of a SHOULD.
> 
> So maybe something like:
>   The driver SHOULD attempt to report large pages rather than smaller ones
>   as this could improve performance for free page reporting.

Makes sense!

> 
>>> +
>>> +It is RECOMMENDED that the driver avoid unnecessary reads or writes to the
>>
>> s/avoid/avoids/
>>
>>> +page contents as this could reduce the performance for free page reporting.
>>> +
>> You're the second user of RECOMMENDED.
>>
>> I'd do: "The driver SHOULD avoid unnecessary reads ..."
> 
> I'll update it to the following:
>   The driver SHOULD avoid unnecessary reads or writes to the reported page
>   contents as this could reduce the performance for free page reporting.
> 
>>> +\devicenormative{\paragraph}{Free Page Reporting}{Device Types / Memory Balloon Device / Device Operation / Free Page Reporting}
>>> +
>>> +Normative statements in this section apply if the
>>> +VIRTIO_BALLOON_F_PAGE_REPORTING feature has been negotiated.
>>> +
>>> +The device MAY modify the contents of any page supplied in a report
>>> +request even before acknowledging that request by using the
>>> +reporting_vq descriptor.
>>> +
>>> +If the VIRTIO_BALLOON_F_PAGE_POISON feature has been negotiated, the device
>>> +SHALL NOT modify the the page if this will result in the page containing a
>>> +value other than \field{poison_val}.
>>
>> You're the first user of "SHALL NOT" -> "MUST NOT" ?
>>
>> s/the the page/the content of a reported page/
>>
>> "If the VIRTIO_BALLOON_F_PAGE_POISON feature has been negotiated, the
>> device MUST NOT modify the content of a reported page to something other
>> than than \field{poison_val}." ?
> 
> I will tweak the wording to match what you have here.
> 
> So with this I think we have gone through the full set. I will see
> about cleaning the last bits up and resubmitting the set later today.

Perfect, thanks!


-- 
Thanks,

David / dhildenb


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

Re: [virtio-comment] [PATCH v2 1/3] content: Document balloon feature free page hints

[Alexander Duyck]

On Tue, May 19, 2020 at 9:09 AM David Hildenbrand <david@redhat.com> wrote:
>
> >> I proposed that the driver MUST reinitialize the pages when reusing
> >> (which is what Linux does), so then this is true. Reuse implies
> >> initializing, implies modification. It's somewhat simpler than what you
> >> propose, leaving the case open where the driver would reuse pages by
> >> only reading them (I don't really see a use case for that ...). But I
> >> don't care as long as it's consistent and correct :)
> >
> > Linux does not reinitialize the pages when it frees them. That only
>
> Whoever uses the pages has to initialize. Again, I don't think we should
> make difference between the guest and the driver. From spec POV, they
> are one piece. Everything else is implementation detail.

Right, but the problem is "use". In the case of balloon it was pages
being pulled out of the balloon. In the case of free pages nobody is
really using them. They are "free" already. Part of the issue here is
that unlike the balloon or page reporting we don't really have a good
definition for where they are. Getting back to the wording I have been
using for free page hinting I am looking at something like:
  The driver MUST reinitialize the contents of any previously hinted page
  released before receiving the command ID VIRTIO_BALLOON_CMD_ID_DONE.

I might reference that as well as the earlier comment about treating
the hinted pages as uninitialized memory.

> > happens if poison or init_on_free are enabled which are rare cases.
> > When it does reinitialize the pages then I agree that the device
> > cannot modify the contents.
>
> What about a user who relies on the content of uninitialized pages?
> Like, read it, if it has the value, don't set it to the value. Unlikely
> but possible, no? We could have data corruption.
>
> We should document that in some way, because this is what could happen
> with the *current* QEMU implementation

Agreed. This is a problem with the current QEMU/Linux driver
implementation. What worries me is that I wonder if this might not be
more possible then we realize. For example I wonder if something like
KSM could read the page and try merging it with others just for the
value to eventually change.

So I was documenting the driver side mostly as-is for the
specification. What we probably do need to do is update both the
driver and the specification to address this since if we are pulling
the page out before we get "DONE" we probably should reinitialize it
so that the state if fixed going forward and it cannot change.

> >
> > The current implementation is assuming QEMU live-migration with the
> > Linux guest as the only use case. As such I want to make sure we
> > correctly capture all of the behaviors that are expected based on
> > those assumptions, but I want to avoid inserting behaviors we would
> > like to see occur but aren't really a part of this.
>
> Exactly that's why I bring this ^ up.
>
> >
> >>>
> >>> The driver can end up releasing the pages back to the buddy allocator
> >>> and if they are not poisoned/init_on_free then they will go there and
> >>> can still potentially change until such time as the guest writes to
> >>> the page modifying it or the balloon driver switches the cmd ID to
> >>> VIRTIO_BALLOON_CMD_ID_DONE. That was one of the reasons for trying to
> >>> frame it the way I did. So what I can do is reword the two statements
> >>> as follows:
> >>>
> >>>   If the content of a previously hinted page has not been modified by the
> >>>   guest since the device issued the \field{free_page_hint_cmd_id} associated
> >>>   with the hint, the device MAY modify the contents of the page.
> >>>
> >>>   The device MUST NOT modify the content of a previously hinted page
> >>> after
> >>>   \field{free_page_hint_cmd_id} is set to VIRTIO_BALLOON_CMD_ID_DONE.
> >> Is it really only "DONE" that closes the current window? I think a
> >> "STOP" from the device will also close the window. DONE is only set at
> >> the very last iteration during memory migration.
> >
> > So the CMD_ID_DONE is issued when the migration has occurred. The
> > migration is what is actually modifying the memory.
> >
> >> (virtio_balloon_free_page_report_notify() in QEMU)
> >>
> >> I consider one window == one iteration == one value of
> >> \field{free_page_hint_cmd_id} until either DONE or STOP
> >
> > CMD_ID_STOP will close the current window for providing hints, but the
> > migration hasn't happened yet. We are still accumulating the hints. We
> > don't receive CMD_ID_DONE from the device until the migration has
> > occurred. It is the migration that will alter the content of the pages
> > by leaving them behind on the previous VM.
>
> I'll have to think about again if your statements reflect the reality
> today. I'll have to dive once again into QEMU code :( Complicated stuff.

So I am operating on the assumption that the memory isn't going to
change until the migration occurs. If you take a look at
virtio_balloon_free_page_hint_notify you will see that it changes the
status to FREE_PAGE_HINT_S_DONE after the bitmap is synced with the VM
not running. I am assuming that is the stop-and-copy phase of the
migration so when the VM does come back up it should report that the
CMD_ID_DONE in the free page hinting command ID.

> >
> >> [...]
> >>
> >> Let's think this through, what about this scenario:
> >>
> >> The device sets \field{free_page_hint_cmd_id} = X
> >> The driver starts reporting free pages (and reports all pages it has)
> >> 1. Sends X to start the windows
> >> 2. Sends all page hints (\field{free_page_hint_cmd_id} stays X)
> >> 3. Sends VIRTIO_BALLOON_CMD_ID_STOP to end the window
> >> The driver sets \field{free_page_hint_cmd_id} = DONE or STOP
> >>
> >> The guest can reuse the pages any time (triggered by the shrinker),
> >> especially, during 2, before the hypervisor even processed a hint
> >> request. It can happen that the guest reuses a page before the
> >> hypervisor processes the request and before
> >> \field{free_page_hint_cmd_id} changes.
> >>
> >> In QEMU, the double-bitmap magic makes sure that this is guaranteed to
> >> work IIRC.
> >>
> >> In that case, the page has to be migrated in that windows, the
> >> hypervisor must not modify the content.
> >
> > If by "reuse" you mean write to or reinitialize then that is correct.
> > All that is really happening is that any pages that are hinted have
> > the potential to be left behind with the original VM and not migrated
> > to the new one. We get the notification that the migration happened
> > when CMD_ID_DONE is passed to us. At that point the hinting is
> > complete and the device has no use for additional data.
> >
> > Instead of CMD_ID_STOP it probably would have made more sense to call
> > it something like CMD_ID_PAUSE or CMD_ID_HOLD as that is what it is
> > really doing. It is just temporarily holding the hints off while the
> > hypervisor synchronizes the dirty bits from the host.
>
> I think if migration fails, it will be left set to STOP. Guess we should
> specify that possibility somehow as well.

Actually I think that is a bug in the QEMU implementation. We cannot
let that happen otherwise the guest is never going to let the memory
go. If we have to abort the migration we should be calling
virtio_balloon_free_page_done(), probably in response to something
like PRECOPY_NOTIFY_CLEANUP?

The problem is I am not that familiar with the migration process
itself within QEMU so I am not sure which labels mean what in the
notifier, but we should be signaling DONE if we are either going to
abort the migration or if we completed it and are now on the new
system.

This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

Re: [virtio-comment] [PATCH v2 1/3] content: Document balloon feature free page hints

[David Hildenbrand]

On 19.05.20 23:00, Alexander Duyck wrote:
> On Tue, May 19, 2020 at 9:09 AM David Hildenbrand <david@redhat.com> wrote:
>>
>>>> I proposed that the driver MUST reinitialize the pages when reusing
>>>> (which is what Linux does), so then this is true. Reuse implies
>>>> initializing, implies modification. It's somewhat simpler than what you
>>>> propose, leaving the case open where the driver would reuse pages by
>>>> only reading them (I don't really see a use case for that ...). But I
>>>> don't care as long as it's consistent and correct :)
>>>
>>> Linux does not reinitialize the pages when it frees them. That only
>>
>> Whoever uses the pages has to initialize. Again, I don't think we should
>> make difference between the guest and the driver. From spec POV, they
>> are one piece. Everything else is implementation detail.
> 
> Right, but the problem is "use". In the case of balloon it was pages
> being pulled out of the balloon. In the case of free pages nobody is
> really using them. They are "free" already. Part of the issue here is
> that unlike the balloon or page reporting we don't really have a good
> definition for where they are. Getting back to the wording I have been
> using for free page hinting I am looking at something like:
>   The driver MUST reinitialize the contents of any previously hinted page
>   released before receiving the command ID VIRTIO_BALLOON_CMD_ID_DONE.
> 
> I might reference that as well as the earlier comment about treating
> the hinted pages as uninitialized memory.
> 
>>> happens if poison or init_on_free are enabled which are rare cases.
>>> When it does reinitialize the pages then I agree that the device
>>> cannot modify the contents.
>>
>> What about a user who relies on the content of uninitialized pages?
>> Like, read it, if it has the value, don't set it to the value. Unlikely
>> but possible, no? We could have data corruption.
>>
>> We should document that in some way, because this is what could happen
>> with the *current* QEMU implementation
> 
> Agreed. This is a problem with the current QEMU/Linux driver
> implementation. What worries me is that I wonder if this might not be
> more possible then we realize. For example I wonder if something like
> KSM could read the page and try merging it with others just for the
> value to eventually change.
> 
> So I was documenting the driver side mostly as-is for the
> specification. What we probably do need to do is update both the
> driver and the specification to address this since if we are pulling
> the page out before we get "DONE" we probably should reinitialize it
> so that the state if fixed going forward and it cannot change.
> 
>>>
>>> The current implementation is assuming QEMU live-migration with the
>>> Linux guest as the only use case. As such I want to make sure we
>>> correctly capture all of the behaviors that are expected based on
>>> those assumptions, but I want to avoid inserting behaviors we would
>>> like to see occur but aren't really a part of this.
>>
>> Exactly that's why I bring this ^ up.
>>
>>>
>>>>>
>>>>> The driver can end up releasing the pages back to the buddy allocator
>>>>> and if they are not poisoned/init_on_free then they will go there and
>>>>> can still potentially change until such time as the guest writes to
>>>>> the page modifying it or the balloon driver switches the cmd ID to
>>>>> VIRTIO_BALLOON_CMD_ID_DONE. That was one of the reasons for trying to
>>>>> frame it the way I did. So what I can do is reword the two statements
>>>>> as follows:
>>>>>
>>>>>   If the content of a previously hinted page has not been modified by the
>>>>>   guest since the device issued the \field{free_page_hint_cmd_id} associated
>>>>>   with the hint, the device MAY modify the contents of the page.
>>>>>
>>>>>   The device MUST NOT modify the content of a previously hinted page
>>>>> after
>>>>>   \field{free_page_hint_cmd_id} is set to VIRTIO_BALLOON_CMD_ID_DONE.
>>>> Is it really only "DONE" that closes the current window? I think a
>>>> "STOP" from the device will also close the window. DONE is only set at
>>>> the very last iteration during memory migration.
>>>
>>> So the CMD_ID_DONE is issued when the migration has occurred. The
>>> migration is what is actually modifying the memory.
>>>
>>>> (virtio_balloon_free_page_report_notify() in QEMU)
>>>>
>>>> I consider one window == one iteration == one value of
>>>> \field{free_page_hint_cmd_id} until either DONE or STOP
>>>
>>> CMD_ID_STOP will close the current window for providing hints, but the
>>> migration hasn't happened yet. We are still accumulating the hints. We
>>> don't receive CMD_ID_DONE from the device until the migration has
>>> occurred. It is the migration that will alter the content of the pages
>>> by leaving them behind on the previous VM.
>>
>> I'll have to think about again if your statements reflect the reality
>> today. I'll have to dive once again into QEMU code :( Complicated stuff.
> 
> So I am operating on the assumption that the memory isn't going to
> change until the migration occurs. If you take a look at
> virtio_balloon_free_page_hint_notify you will see that it changes the
> status to FREE_PAGE_HINT_S_DONE after the bitmap is synced with the VM
> not running. I am assuming that is the stop-and-copy phase of the
> migration so when the VM does come back up it should report that the
> CMD_ID_DONE in the free page hinting command ID.
> 
>>>
>>>> [...]
>>>>
>>>> Let's think this through, what about this scenario:
>>>>
>>>> The device sets \field{free_page_hint_cmd_id} = X
>>>> The driver starts reporting free pages (and reports all pages it has)
>>>> 1. Sends X to start the windows
>>>> 2. Sends all page hints (\field{free_page_hint_cmd_id} stays X)
>>>> 3. Sends VIRTIO_BALLOON_CMD_ID_STOP to end the window
>>>> The driver sets \field{free_page_hint_cmd_id} = DONE or STOP
>>>>
>>>> The guest can reuse the pages any time (triggered by the shrinker),
>>>> especially, during 2, before the hypervisor even processed a hint
>>>> request. It can happen that the guest reuses a page before the
>>>> hypervisor processes the request and before
>>>> \field{free_page_hint_cmd_id} changes.
>>>>
>>>> In QEMU, the double-bitmap magic makes sure that this is guaranteed to
>>>> work IIRC.
>>>>
>>>> In that case, the page has to be migrated in that windows, the
>>>> hypervisor must not modify the content.
>>>
>>> If by "reuse" you mean write to or reinitialize then that is correct.
>>> All that is really happening is that any pages that are hinted have
>>> the potential to be left behind with the original VM and not migrated
>>> to the new one. We get the notification that the migration happened
>>> when CMD_ID_DONE is passed to us. At that point the hinting is
>>> complete and the device has no use for additional data.
>>>
>>> Instead of CMD_ID_STOP it probably would have made more sense to call
>>> it something like CMD_ID_PAUSE or CMD_ID_HOLD as that is what it is
>>> really doing. It is just temporarily holding the hints off while the
>>> hypervisor synchronizes the dirty bits from the host.
>>
>> I think if migration fails, it will be left set to STOP. Guess we should
>> specify that possibility somehow as well.
> 
> Actually I think that is a bug in the QEMU implementation. We cannot
> let that happen otherwise the guest is never going to let the memory
> go. If we have to abort the migration we should be calling
> virtio_balloon_free_page_done(), probably in response to something
> like PRECOPY_NOTIFY_CLEANUP?
> 
> The problem is I am not that familiar with the migration process
> itself within QEMU so I am not sure which labels mean what in the
> notifier, but we should be signaling DONE if we are either going to
> abort the migration or if we completed it and are now on the new
> system.
> 

We are starting to see way too many issues in QEMU code that are hard to
tackle.

1. The issue with hinted pages not being 0 or the old content when
re-accessed, due to the way they are skipped during migration. MST
called that a memory corruption and that this would be broken. Very hard
to fix, maybe impossible.

2. Some cases (migration failing while we are only sending memory) not
setting the status to DONE. The handling is just a mess.

3. Take a look at that asynchronous iothread handling.
virtio_ballloon_get_free_page_hints(). I think you can trick it into
doing nasty things while you are e.g., resetting the device. Also,
virtio_ballloon_get_free_page_hints() will just busy-loop while waiting
for requests. Just ugly. Also that block_iothread, just hard to grasp.

Besides all the other issues I keep finding.

At this point I'd just love to rip out the QEMU implement and let
whoever wants to really have it try again. The Linux side seems to be in
a better state.

This feature is in an unmaintainable state in QEMU.

@MST, is there something that speaks against ripping out the QEMU and
Linux parts, documenting in the spec that this feature should not be used?

-- 
Thanks,

David / dhildenb


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

Re: [virtio-comment] [PATCH v2 1/3] content: Document balloon feature free page hints

[Alexander Duyck]

On Wed, May 20, 2020 at 1:24 AM David Hildenbrand <david@redhat.com> wrote:
>
> On 19.05.20 23:00, Alexander Duyck wrote:
> > On Tue, May 19, 2020 at 9:09 AM David Hildenbrand <david@redhat.com> wrote:

[...]

> >>>>
> >>>> Let's think this through, what about this scenario:
> >>>>
> >>>> The device sets \field{free_page_hint_cmd_id} = X
> >>>> The driver starts reporting free pages (and reports all pages it has)
> >>>> 1. Sends X to start the windows
> >>>> 2. Sends all page hints (\field{free_page_hint_cmd_id} stays X)
> >>>> 3. Sends VIRTIO_BALLOON_CMD_ID_STOP to end the window
> >>>> The driver sets \field{free_page_hint_cmd_id} = DONE or STOP
> >>>>
> >>>> The guest can reuse the pages any time (triggered by the shrinker),
> >>>> especially, during 2, before the hypervisor even processed a hint
> >>>> request. It can happen that the guest reuses a page before the
> >>>> hypervisor processes the request and before
> >>>> \field{free_page_hint_cmd_id} changes.
> >>>>
> >>>> In QEMU, the double-bitmap magic makes sure that this is guaranteed to
> >>>> work IIRC.
> >>>>
> >>>> In that case, the page has to be migrated in that windows, the
> >>>> hypervisor must not modify the content.
> >>>
> >>> If by "reuse" you mean write to or reinitialize then that is correct.
> >>> All that is really happening is that any pages that are hinted have
> >>> the potential to be left behind with the original VM and not migrated
> >>> to the new one. We get the notification that the migration happened
> >>> when CMD_ID_DONE is passed to us. At that point the hinting is
> >>> complete and the device has no use for additional data.
> >>>
> >>> Instead of CMD_ID_STOP it probably would have made more sense to call
> >>> it something like CMD_ID_PAUSE or CMD_ID_HOLD as that is what it is
> >>> really doing. It is just temporarily holding the hints off while the
> >>> hypervisor synchronizes the dirty bits from the host.
> >>
> >> I think if migration fails, it will be left set to STOP. Guess we should
> >> specify that possibility somehow as well.
> >
> > Actually I think that is a bug in the QEMU implementation. We cannot
> > let that happen otherwise the guest is never going to let the memory
> > go. If we have to abort the migration we should be calling
> > virtio_balloon_free_page_done(), probably in response to something
> > like PRECOPY_NOTIFY_CLEANUP?
> >
> > The problem is I am not that familiar with the migration process
> > itself within QEMU so I am not sure which labels mean what in the
> > notifier, but we should be signaling DONE if we are either going to
> > abort the migration or if we completed it and are now on the new
> > system.
> >
>
> We are starting to see way too many issues in QEMU code that are hard to
> tackle.
>
> 1. The issue with hinted pages not being 0 or the old content when
> re-accessed, due to the way they are skipped during migration. MST
> called that a memory corruption and that this would be broken. Very hard
> to fix, maybe impossible.
>
> 2. Some cases (migration failing while we are only sending memory) not
> setting the status to DONE. The handling is just a mess.
>
> 3. Take a look at that asynchronous iothread handling.
> virtio_ballloon_get_free_page_hints(). I think you can trick it into
> doing nasty things while you are e.g., resetting the device. Also,
> virtio_ballloon_get_free_page_hints() will just busy-loop while waiting
> for requests. Just ugly. Also that block_iothread, just hard to grasp.
>
> Besides all the other issues I keep finding.
>
> At this point I'd just love to rip out the QEMU implement and let
> whoever wants to really have it try again. The Linux side seems to be in
> a better state.
>
> This feature is in an unmaintainable state in QEMU.
>
> @MST, is there something that speaks against ripping out the QEMU and
> Linux parts, documenting in the spec that this feature should not be used?

Have we mad a decision on this? I plan to resubmit my QEMU patch set
as I had a bug in patch 3 that needs to be addressed. I'm just
wondering if I should rebase on David's patches that fix free page
hinting, or if I should just replace the patch with one that removes
free page hinting?

Thanks.

- Alex

This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/