Re: [Qemu-devel] [PATCH] docs: Convert migration.txt to rst

[Dr. David Alan Gilbert]

* Kashyap Chamarthy (address@hidden) wrote:
> On Tue, Dec 12, 2017 at 01:56:00PM +0000, Dr. David Alan Gilbert (git) wrote:
> > From: "Dr. David Alan Gilbert" <address@hidden>
> > 
> > Mostly just manual conversion with very minor fixes.
> > 
> > Signed-off-by: Dr. David Alan Gilbert <address@hidden>
> > ---
> >  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 / address@hidden / Manchester, UK