Re: [PATCH 1/1] vfio: Change default dirty pages tracking behavior during migration

[Alex Williamson <alex.williamson@redhat.com>]

On Thu, 19 Nov 2020 09:48:01 +0530
Kirti Wankhede <kwankhede@nvidia.com> wrote:

> By default dirty pages tracking is enabled during iterative phase
> (pre-copy phase).
> Added per device opt-out option 'pre-copy-dirty-page-tracking' to
> disable dirty pages tracking during iterative phase. If the option
> 'pre-copy-dirty-page-tracking=off' is set for any VFIO device, dirty
> pages tracking during iterative phase will be disabled.
> 
> Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com>
> ---
>  hw/vfio/common.c              | 11 +++++++----
>  hw/vfio/pci.c                 |  3 +++
>  include/hw/vfio/vfio-common.h |  1 +
>  3 files changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index c1fdbf17f2e6..6ff1daa763f8 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -311,7 +311,7 @@ bool vfio_mig_active(void)
>      return true;
>  }
>  
> -static bool vfio_devices_all_stopped_and_saving(VFIOContainer *container)
> +static bool vfio_devices_all_saving(VFIOContainer *container)
>  {
>      VFIOGroup *group;
>      VFIODevice *vbasedev;
> @@ -329,8 +329,11 @@ static bool vfio_devices_all_stopped_and_saving(VFIOContainer *container)
>                  return false;
>              }
>  
> -            if ((migration->device_state & VFIO_DEVICE_STATE_SAVING) &&
> -                !(migration->device_state & VFIO_DEVICE_STATE_RUNNING)) {
> +            if (migration->device_state & VFIO_DEVICE_STATE_SAVING) {
> +                if ((vbasedev->pre_copy_dirty_page_tracking == ON_OFF_AUTO_OFF)
> +                    && (migration->device_state & VFIO_DEVICE_STATE_RUNNING)) {
> +                        return false;
> +                }
>                  continue;
>              } else {
>                  return false;
> @@ -1125,7 +1128,7 @@ static void vfio_listerner_log_sync(MemoryListener *listener,
>          return;
>      }
>  
> -    if (vfio_devices_all_stopped_and_saving(container)) {
> +    if (vfio_devices_all_saving(container)) {
>          vfio_sync_dirty_bitmap(container, section);
>      }
>  }
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 58c0ce8971e3..5bea4b3e71f5 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -3182,6 +3182,9 @@ static void vfio_instance_init(Object *obj)
>  static Property vfio_pci_dev_properties[] = {
>      DEFINE_PROP_PCI_HOST_DEVADDR("host", VFIOPCIDevice, host),
>      DEFINE_PROP_STRING("sysfsdev", VFIOPCIDevice, vbasedev.sysfsdev),
> +    DEFINE_PROP_ON_OFF_AUTO("pre-copy-dirty-page-tracking", VFIOPCIDevice,
> +                            vbasedev.pre_copy_dirty_page_tracking,
> +                            ON_OFF_AUTO_ON),


Shouldn't this be an experimental option, ie.
x-pre-copy-dirty-page-tracking?  We can always make it a supported
option at a later time, but creating it as a supported option and later
changing our mind would require a deprecation process.  As I see it,
this option limits the device's ability to fully participate in
migration, leading to potentially erroneous downtime estimates and
should therefore, at best, make it an experimental option.  We also
have no data yet as to scenarios and configurations when this option
might be recommended.  Thanks,

Alex

>      DEFINE_PROP_ON_OFF_AUTO("display", VFIOPCIDevice,
>                              display, ON_OFF_AUTO_OFF),
>      DEFINE_PROP_UINT32("xres", VFIOPCIDevice, display_xres, 0),
> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> index baeb4dcff102..267cf854bbba 100644
> --- a/include/hw/vfio/vfio-common.h
> +++ b/include/hw/vfio/vfio-common.h
> @@ -129,6 +129,7 @@ typedef struct VFIODevice {
>      unsigned int flags;
>      VFIOMigration *migration;
>      Error *migration_blocker;
> +    OnOffAuto pre_copy_dirty_page_tracking;
>  } VFIODevice;
>  
>  struct VFIODeviceOps {