* Kashyap Chamarthy (kcham...@redhat.com) wrote:
> On Tue, Dec 12, 2017 at 01:56:00PM +0000, Dr. David Alan Gilbert (git) wrote:
> > From: "Dr. David Alan Gilbert" <dgilb...@redhat.com>
> >
> > Mostly just manual conversion with very minor fixes.
> >
> > Signed-off-by: Dr. David Alan Gilbert <dgilb...@redhat.com>
> > ---
> > docs/devel/{migration.txt => migration.rst} | 326
> > +++++++++++++++-------------
> > 1 file changed, 176 insertions(+), 150 deletions(-)
> > rename docs/devel/{migration.txt => migration.rst} (74%)
>
> Hey Dave,
>
> A good chunk of changes I suggest are pre-existing, so take it with a
> grain of salt :-). I'm ambivalent on whether to keep the conversion to
> rST and the other rST syntax changes separate. Maybe we can keep in one
> change, as this doesn't impact backports in terms of code /
> functionality.
I'd like to fix the non-rST fixes some other time; there's plenty wrong
with the actual contents of this file.
(I need to pull in a fix from Jay Zhou before resending this actually).
> [...]
>
> > +State Live Migration
> > +====================
> >
> > This is used for RAM and block devices. It is not yet ported to vmstate.
> > <Fill more information here>
>
> Not this patch's problem, but do we have the more information that is to
> be filled in above?
Oh yes, plenty!
> [...]
>
> > You can use any internal state that you need using the opaque void *
> > pointer that is passed to all functions.
> > @@ -83,7 +96,8 @@ pointer that is passed to all functions.
> > The important functions for us are put_buffer()/get_buffer() that
>
> Might want to highlight the fuctions (as you call them as important)
> with back ticks (and some spacing): `put_buffer()` / `get_buffer()`.
OK, I've done a bunch of these highlights you suggest; one or two
questions below.
> > allow to write/read a buffer into the QEMUFile.
> >
> > -=== How to save the state of one device ===
> > +Saving the state of one device
> > +==============================
> >
> > The state of a device is saved using intermediate buffers. There are
> > some helper functions to assist this saving.
> > @@ -97,30 +111,34 @@ associated with a series of fields saved. The
> > save_state always saves
>
> Here too: s/save_state/``save_state``/
>
> (And other occurrences throughout the doc.)
>
> > the state as the newer version. But load_state sometimes is able to
>
> s/load_state/``load_state``/
>
> (And other occurrences throughout the doc.)
>
> > load state from an older version.
> >
> > -=== Legacy way ===
> > +Legacy way
> > +----------
> >
> > This way is going to disappear as soon as all current users are ported to
> > VMSTATE.
> >
> > Each device has to register two functions, one to save the state and
> > another to load the state back.
>
> [...]
>
> > The important functions for the device state format are the save_state
> > and load_state. Notice that load_state receives a version_id
> > parameter to know what state format is receiving. save_state doesn't
> > have a version_id parameter because it always uses the latest version.
>
> In the above paragrah, too (``save_state``, ``load_state``).
>
> Also s/version_id/'version_id'/g
>
> > -=== VMState ===
> > +VMState
> > +-------
> >
>
> [...]
>
> > We are declaring the state with name "pckbd".
> > The version_id is 3, and the fields are 4 uint8_t in a KBDState structure.
> > We registered this with:
> >
> > +.. code:: c
> > +
> > vmstate_register(NULL, 0, &vmstate_kbd, s);
> >
> > Note: talk about how vmstate <-> qdev interact, and what the instance ids
> > mean.
>
> Not introduced in this patch, but: s/ids/IDs/
>
> Two points:
>
> - I think the above is a TODO item; so you can use:
>
> .. TODO (dgilbert):: talk about how vmstate <-> qdev interact, and
> what the instance ids mean.
>
> As that'll keep it only in the source rST; but not in the rendered
> version.
I'd rather leave the items rendered, otherwise they have even less
chance of being fixed.
> - Also, other minor thing (again, pre-existing): capitalize all the
> things like s/tcp/TCP; s/unix/UNIX/ where appropriate.
>
> > @@ -159,7 +181,8 @@ Note: talk about how vmstate <-> qdev interact, and
> > what the instance ids mean.
> > You can search for VMSTATE_* macros for lots of types used in QEMU in
>
> Maybe: s/VMSTATE_*/``VMSTATE_*``/
>
> > include/hw/hw.h.
> >
> > -=== More about versions ===
> > +More about versions
> > +-------------------
> >
> > Version numbers are intended for major incompatible changes to the
> > migration of a device, and using them breaks backwards-migration
> > @@ -183,7 +206,8 @@ function is deprecated and will be removed when no more
> > users are left.
> > Saving state will always create a section with the 'version_id' value
> > and thus can't be loaded by any older QEMU.
>
> Also, not in this patch, but keeping up with the theme of highlighting
> function names:
>
> s/load_state_old()/``load_state_old()``/
>
> And other fields and strings (you can use a single quote, or a back tick
> for italics; whatever you choose, might want to keep it consistent):
>
> s/version_id/'version_id'/
>
> > -=== Massaging functions ===
> > +Massaging functions
> > +-------------------
>
> [...]
>
> > -=== Subsections ===
> > +Subsections
> > +-----------
> >
> > The use of version_id allows to be able to migrate from older versions
>
> s/version_id/'version_id'/ (or `version_id`, if you prefer italics)
>
> Again, not in this patch, but noticed in the rendered version locally:
>
> s/post_load()/``post_load()``/
>
> [...]
>
> > Here we have a subsection for the pio state. We only need to
> > save/send this state when we are in the middle of a pio operation
>
> s/ide_drive_pio_state_needed()/``ide_drive_pio_state_needed()``/
>
> > @@ -305,14 +331,11 @@ to send a subsection allows backwards migration
> > compatibility when
>
> [...]
>
> > +Not sending existing elements
> > +-----------------------------
> >
> > Sometimes members of the VMState are no longer needed;
>
> Pre-existing: s/;/:/
>
> [...]
>
> > +Return path
> > +-----------
>
> [...]
>
> Pre-existing, and not this patch's problem. Noticed in my HTML render:
> in this section, you might to highlight (double back ticks) the
> function:
>
> ``qemu_file_get_return_path(QEMUFile* fwdpath)``
>
> [...]
>
> > +Postcopy
> > +========
>
> [...]
>
> > -=== Enabling postcopy ===
> > +Enabling postcopy
> > +-----------------
>
> [...]
>
> Not in this patch, but in this section, 'migrate_set_speed' is
> mentioned; since it's a QMP command, you might want to highlith it:
> ``migrate_set_speed``
>
> Also, from this section, you might want to use adminitions like:
>
> .. note::
> During the postcopy phase, the bandwidth limits set using
> migrate_set_speed is ignored (to avoid delaying requested pages
> that the destination is waiting for).
>
> Use this wherever you see fit in the document.
>
> [...]
>
> > Source behaviour
> > +----------------
> >
> > Until postcopy is entered the migration stream is identical to normal
> > precopy, except for the addition of a 'postcopy advise' command at
> > @@ -423,11 +453,10 @@ the beginning, to tell the destination that postcopy
> > might happen.
> > When postcopy starts the source sends the page discard data and then
> > forms the 'package' containing:
> >
> > - Command: 'postcopy listen'
> > - The device state
>
> In my local Sphinx-based HTML rendering, the above "The device state"
> ended up being bold somehow.
Any idea why? It's fine both in vim and rst2html-3's output.
> > - A series of sections, identical to the precopy streams device state
> > stream
> > - containing everything except postcopiable devices (i.e. RAM)
> > - Command: 'postcopy run'
> > + - Command: 'postcopy listen'
> > + - The device state
> > + A series of sections, identical to the precopy streams device state
> > stream containing everything except postcopiable devices (i.e. RAM)
>
> Might want to wrap this long line (and other places); as it's causing an
> extra new line in my local rendering.
Yes, done (as per Peter's comments); it took me a while to figure out
how to wrap them in lists.
> > + - Command: 'postcopy run'
> >
> > The 'package' is sent as the data part of a Command: 'CMD_PACKAGED', and
> > the
>
> s/CMD_PACKAGED/``CMD_PACKAGED``/g ?
>
>
> [...]
>
> > +- On receipt of CMD_PACKAGED (1)
> > + All the data associated with the package - the ( ... ) section in the
> > diagram - is read into memory, and the main thread recurses into
> > qemu_loadvm_state_main to process the contents of the package (2) which
> > contains commands (3,6) and devices (4...)
> > +
> > +- On receipt of 'postcopy listen' - 3 -(i.e. the 1st command in the
> > package)
> > + a new thread (a) is started that takes over servicing the migration
> > stream, while the main thread carries on loading the package. It loads
> > normal background page data (b) but if during a device load a fault happens
> > (5) the returned page (c) is loaded by the listen thread allowing the main
> > threads device load to carry on.
> > +
> > +- The last thing in the CMD_PACKAGED is a 'RUN' command (6)
> > + letting the destination CPUs start running. At the end of the
> > CMD_PACKAGED (7) the main thread returns to normal running behaviour and is
> > no longer used by migration, while the listen thread carries on servicing
> > page data until the end of migration.
>
> Super long lines, needs wrapping. Here too something seem to cause
> needless "bold" text in my Sphinx-based HTML rendering.
>
> > +
> > +Postcopy states
> > +---------------
>
> [...]
>
> There's a list in this document. You might want to 'listify' it (using
> numbers, alphabets, etc), like you did it in other places.
>
> [...]
>
> > -=== Postcopy with hugepages ===
> > +Postcopy with hugepages
> > +-----------------------
>
> Here too, noticed after reading the rendered version; you might want to
> highlight the QEMU command-line parameters & file system paths using the
> verbatim (``):
>
> -mem-path
> /dev/hugepages
> -mem-prealloc
>
> [...]
>
> --
> /kashyap
Thanks,
Dave
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
