qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
From: Cornelia Huck <cohuck@redhat.com>
To: Tarun Gupta <targupta@nvidia.com>
Cc: kevin.tian@intel.com, cjia@nvidia.com, quintela@redhat.com,
	alex.williamson@redhat.com, qemu-devel@nongnu.org,
	yan.y.zhao@intel.com, lushenming@huawei.com,
	kwankhede@nvidia.com, dnigam@nvidia.com, berrange@redhat.com,
	philmd@redhat.com, dgilbert@redhat.com
Subject: Re: [PATCH v3 1/1] docs/devel: Add VFIO device migration documentation
Date: Thu, 1 Apr 2021 13:05:22 +0200	[thread overview]
Message-ID: <20210401130522.1e9c2871.cohuck@redhat.com> (raw)
In-Reply-To: <20210326131850.149337-1-targupta@nvidia.com>

On Fri, 26 Mar 2021 18:48:50 +0530
Tarun Gupta <targupta@nvidia.com> wrote:

> Document interfaces used for VFIO device migration. Added flow of state changes
> during live migration with VFIO device. Tested by building docs with the new
> vfio-migration.rst file.

I don't think you want to include the test state in the patch
description; that should go into a --- section that is stripped off by
git am.

> 
> v3:
> - Add introductory line about VM migration in general.
> - Remove occurcences of vfio_pin_pages() to describe pinning.
> - Incorporated comments from v2
> 
> v2:
> - Included the new vfio-migration.rst file in index.rst
> - Updated dirty page tracking section, also added details about
>   'pre-copy-dirty-page-tracking' opt-out option.
> - Incorporated comments around wording of doc.

Same for the changelog; this is interesting for review, but not for the
final git log.

> 
> Signed-off-by: Tarun Gupta <targupta@nvidia.com>
> Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com>

This S-o-b chain does not look correct. Your address should be the last
one in the chain, signing off on all of the previous ones. (Maybe Kirti
also needs to be listed in a Co-developed-by: statement?)

> ---
>  MAINTAINERS                   |   1 +
>  docs/devel/index.rst          |   1 +
>  docs/devel/vfio-migration.rst | 143 ++++++++++++++++++++++++++++++++++
>  3 files changed, 145 insertions(+)
>  create mode 100644 docs/devel/vfio-migration.rst

> diff --git a/docs/devel/vfio-migration.rst b/docs/devel/vfio-migration.rst
> new file mode 100644
> index 0000000000..24cb55991a
> --- /dev/null
> +++ b/docs/devel/vfio-migration.rst

(...)

> +VFIO device hooks for iterative approach:

"VFIO implements the device hooks for the iterative approach as
follows:"

?

> +
> +* A ``save_setup`` function that sets up the migration region, sets _SAVING
> +  flag in the VFIO device state and informs the VFIO IOMMU module to start
> +  dirty page tracking.
> +
> +* A ``load_setup`` function that sets up the migration region on the
> +  destination and sets _RESUMING flag in the VFIO device state.
> +
> +* A ``save_live_pending`` function that reads pending_bytes from the vendor
> +  driver, which indicates the amount of data that the vendor driver has yet to
> +  save for the VFIO device.
> +
> +* A ``save_live_iterate`` function that reads the VFIO device's data from the
> +  vendor driver through the migration region during iterative phase.
> +
> +* A ``save_live_complete_precopy`` function that resets _RUNNING flag from the
> +  VFIO device state, saves the device config space, if any, and iteratively
> +  copies the remaining data for the VFIO device until the vendor driver
> +  indicates that no data remains (pending bytes is zero).
> +
> +* A ``load_state`` function that loads the config section and the data
> +  sections that are generated by the save functions above
> +
> +* ``cleanup`` functions for both save and load that perform any migration
> +  related cleanup, including unmapping the migration region
> +
> +A VM state change handler is registered to change the VFIO device state when
> +the VM state changes.

This sentence is not very informative. What about:

"The VFIO migration code uses a VM state change handler to change the
VFIO device state when the VM state changes from running to
not-running, and vice versa."

> +
> +Similarly, a migration state change notifier is registered to get a
> +notification on migration state change. These states are translated to the
> +corresponding VFIO device state and conveyed to the vendor driver.

"Similarly, a migration state change handler is used to transition the
VFIO device state back to _RUNNING in case a migration failed or was
canceled."


> +
> +System memory dirty pages tracking
> +----------------------------------
> +
> +A ``log_sync`` memory listener callback marks those system memory pages
> +as dirty which are used for DMA by the VFIO device. The dirty pages bitmap is
> +queried per container. All pages pinned by the vendor driver through external
> +APIs have to be marked as dirty during migration. When there are CPU writes,
> +CPU dirty page tracking can identify dirtied pages, but any page pinned by the
> +vendor driver can also be written by device. There is currently no device or

s/by/by the/

> +IOMMU support for dirty page tracking in hardware.
> +
> +By default, dirty pages are tracked when the device is in pre-copy as well as
> +stop-and-copy phase. So, a page pinned by vendor driver will be copied to

s/by/by the/
s/to/to the/

> +destination in both the phases. Copying dirty pages in pre-copy phase helps

s/both the/both/ ?

> +QEMU to predict if it can achieve its downtime tolerances. If QEMU during
> +pre-copy phase keeps finding dirty pages continuously, then it understands
> +that even in stop-and-copy phase, it is likely to find dirty pages and can
> +predict the downtime accordingly
> +
> +QEMU also provides per device opt-out option ``pre-copy-dirty-page-tracking``

s/provides/provides a/

> +which disables querying dirty bitmap during pre-copy phase. If it is set to

s/querying/querying the/

> +off, all dirty pages will be copied to destination in stop-and-copy phase only

s/to/to the/

(...)



  parent reply	other threads:[~2021-04-01 11:06 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-03-26 13:18 Tarun Gupta
2021-03-27  6:04 ` Shenming Lu
2021-04-01  6:58   ` Tarun Gupta (SW-GPU)
2021-04-01 11:05 ` Cornelia Huck [this message]
2021-04-05 17:02   ` Tarun Gupta (SW-GPU)
2021-04-07 10:23     ` Cornelia Huck
2021-04-07 11:33       ` Tarun Gupta (SW-GPU)

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210401130522.1e9c2871.cohuck@redhat.com \
    --to=cohuck@redhat.com \
    --cc=alex.williamson@redhat.com \
    --cc=berrange@redhat.com \
    --cc=cjia@nvidia.com \
    --cc=dgilbert@redhat.com \
    --cc=dnigam@nvidia.com \
    --cc=kevin.tian@intel.com \
    --cc=kwankhede@nvidia.com \
    --cc=lushenming@huawei.com \
    --cc=philmd@redhat.com \
    --cc=qemu-devel@nongnu.org \
    --cc=quintela@redhat.com \
    --cc=targupta@nvidia.com \
    --cc=yan.y.zhao@intel.com \
    --subject='Re: [PATCH v3 1/1] docs/devel: Add VFIO device migration documentation' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).