[virtio-dev] Re: [PATCH RFC 1/3] content: Document balloon feature free page hints
Thanks for the review. For anything below where I didn't comment I am
just going with your suggestion.
On Thu, May 7, 2020 at 8:35 AM Cornelia Huck wrote:
>
> On Wed, 29 Apr 2020 11:57:45 -0700
> Alexander Duyck wrote:
>
> > From: Alexander Duyck
> >
> > 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
> > ---
> > content.tex | 113
> > +--
> > 1 file changed, 110 insertions(+), 3 deletions(-)
> >
> > diff --git a/content.tex b/content.tex
> > index b91a132df146..796901e83a71 100644
> > --- a/content.tex
> > +++ b/content.tex
> > @@ -5006,9 +5006,12 @@ \subsection{Virtqueues}\label{sec:Device Types /
> > Memory Balloon Device / Virtque
> > \item[0] inflateq
> > \item[1] deflateq
> > \item[2] statsq.
> > +\item[3] free_page_vq.
>
> I'd lose the trailing period (and probably also remove it from statsq;
> it just looks weird.)
>
> > \end{description}
> >
> > - Virtqueue 2 only exists if VIRTIO_BALLOON_F_STATS_VQ set.
> > + statsq only exists if VIRTIO_BALLOON_F_STATS_VQ set.
>
> As you are touching this line anyway, s/set/is set/
>
> > +
> > + free_page_vq only exists if VIRTIO_BALLOON_F_FREE_PAGE_HINT set.
>
> s/set/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) ] 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,15 @@ \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.
> > + The first two fields of this configuration are always present. The
> > + availability of the others all depend on various feature bits as
> > + indicated above.
>
> Maybe make this more explicit?
>
> "
> \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}
> >
> > @@ -5075,6 +5084,9 @@ \subsection{Device Initialization}\label{sec:Device
> > Types / Memory Balloon Devic
> >\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.
>
> This startup sequence now seems wrong (identifying a vq after
> DRIVER_OK). Should probably be:
>
> - id inflate/deflate vqs
> - if STATS_VQ:
> - id stats vq
> - put buffer on stats vq
> - if FREE_PAGE_HINT:
> - id free page vq
> - DRIVER_OK is set
> - if STATS_VQ:
> - notify about buffer
>
> > \end{enumerate}
> >
> > \subsection{Device Operation}\label{sec:Device Types / Memory Balloon
> > Device / Device Operation}
> > @@ -5345,6 +5357,101 @@ \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 used during migration to determine what pages within
>
> s/used/designed to be used/ ?
>
> Or maybe
>
> s/used during migration/can be used/ ?
I'll go with the "designed to be used". Part of the issue is that free
page hinting seems to have assumptions all over the place about how it
is expected to be used so I want to make sure that people understand
that this is the primary use case.
> > +the guest are current unused so that they can be skipped over when it comes
>
> s/current/currently/
>
> > +time for migration. The device will indicate that it is ready to start
>
> s/when it comes time for migration/while migrating the guest/ ?
>
> > +performing hinting by setting the \field{free_page_hint_cmd_id}
[virtio-dev] Re: [PATCH RFC 1/3] content: Document balloon feature free page hints
On Wed, 29 Apr 2020 11:57:45 -0700
Alexander Duyck wrote:
> From: Alexander Duyck
>
> 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
> ---
> content.tex | 113
> +--
> 1 file changed, 110 insertions(+), 3 deletions(-)
>
> diff --git a/content.tex b/content.tex
> index b91a132df146..796901e83a71 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -5006,9 +5006,12 @@ \subsection{Virtqueues}\label{sec:Device Types /
> Memory Balloon Device / Virtque
> \item[0] inflateq
> \item[1] deflateq
> \item[2] statsq.
> +\item[3] free_page_vq.
I'd lose the trailing period (and probably also remove it from statsq;
it just looks weird.)
> \end{description}
>
> - Virtqueue 2 only exists if VIRTIO_BALLOON_F_STATS_VQ set.
> + statsq only exists if VIRTIO_BALLOON_F_STATS_VQ set.
As you are touching this line anyway, s/set/is set/
> +
> + free_page_vq only exists if VIRTIO_BALLOON_F_FREE_PAGE_HINT set.
s/set/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) ] 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,15 @@ \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.
> + The first two fields of this configuration are always present. The
> + availability of the others all depend on various feature bits as
> + indicated above.
Maybe make this more explicit?
"
\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}
>
> @@ -5075,6 +5084,9 @@ \subsection{Device Initialization}\label{sec:Device
> Types / Memory Balloon Devic
>\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.
This startup sequence now seems wrong (identifying a vq after
DRIVER_OK). Should probably be:
- id inflate/deflate vqs
- if STATS_VQ:
- id stats vq
- put buffer on stats vq
- if FREE_PAGE_HINT:
- id free page vq
- DRIVER_OK is set
- if STATS_VQ:
- notify about buffer
> \end{enumerate}
>
> \subsection{Device Operation}\label{sec:Device Types / Memory Balloon Device
> / Device Operation}
> @@ -5345,6 +5357,101 @@ \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 used during migration to determine what pages within
s/used/designed to be used/ ?
Or maybe
s/used during migration/can be used/ ?
> +the guest are current unused so that they can be skipped over when it comes
s/current/currently/
> +time for migration. The device will indicate that it is ready to start
s/when it comes time for migration/while migrating the guest/ ?
> +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:
s/command ID:/command ID. The following values are reserved:/
> +
> +\begin{description}
> +\item[VIRTIO_BALLOON_CMD_ID_STOP (0)] Any previous command ID is invalid.
"Any command ID previously supplied by the device is invalid." ?
> + All hinting SHOULD halt until a new command ID is supplied.
"The driver should halt any hinting until..." ? Or what is actually
halting?
(I think we don't want to use 'SHOULD' outside of normative section,
although we probably haven't followed that rule everywhere.)
> +
> +\item[VIRTIO_BALLOON_CMD_ID_DONE (1)] Any previous
