Re: [PATCH v4 08/14] migration/multifd: Add new migration option for multifd DSA offloading.

["Daniel P. Berrangé" <berrange@redhat.com>]

On Thu, Apr 25, 2024 at 02:21:11AM +0000, Hao Xiang wrote:
> Intel DSA offloading is an optional feature that turns on if
> proper hardware and software stack is available. To turn on
> DSA offloading in multifd live migration:
> 
> multifd-dsa-accel="[dsa_dev_path1] [dsa_dev_path2] ... [dsa_dev_pathX]"
> 
> This feature is turned off by default.
> 
> Signed-off-by: Hao Xiang <hao.xiang@linux.dev>
> ---
>  migration/migration-hmp-cmds.c |  8 ++++++++
>  migration/options.c            | 30 ++++++++++++++++++++++++++++++
>  migration/options.h            |  1 +
>  qapi/migration.json            | 26 +++++++++++++++++++++++---
>  4 files changed, 62 insertions(+), 3 deletions(-)

> diff --git a/qapi/migration.json b/qapi/migration.json
> index 8c65b90328..934fa8839e 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -914,6 +914,12 @@
>  #     See description in @ZeroPageDetection.  Default is 'multifd'.
>  #     (since 9.0)
>  #
> +# @multifd-dsa-accel: If enabled, use DSA accelerator offloading for
> +#     certain memory operations. Enable DSA accelerator offloading by
> +#     setting this string to a list of DSA device path separated by space
> +#     characters. Setting this string to an empty string means disabling
> +#     DSA accelerator offloading. Defaults to an empty string. (since 9.2)

Passing a list of paths as a single space separate string is a
design anti-pattern. This needs to use a list type at the QAPI
level.

Also I don't think we need add 'multifd' on the name - all
new features are for multifd.

Overall it should be called 'dsa-accel-path' I thjink

> @@ -1122,6 +1128,12 @@
>  #     See description in @ZeroPageDetection.  Default is 'multifd'.
>  #     (since 9.0)
>  #
> +# @multifd-dsa-accel: If enabled, use DSA accelerator offloading for
> +#     certain memory operations. Enable DSA accelerator offloading by
> +#     setting this string to a list of DSA device path separated by space
> +#     characters. Setting this string to an empty string means disabling
> +#     DSA accelerator offloading. Defaults to an empty string. (since 9.2)
> +#
>  # Features:
>  #
>  # @deprecated: Member @block-incremental is deprecated.  Use
> @@ -1176,7 +1188,8 @@
>                                              'features': [ 'unstable' ] },
>              '*vcpu-dirty-limit': 'uint64',
>              '*mode': 'MigMode',
> -            '*zero-page-detection': 'ZeroPageDetection'} }
> +            '*zero-page-detection': 'ZeroPageDetection',
> +            '*multifd-dsa-accel': 'StrOrNull'} }

This needs to be

  ['str']   not 'StrOrNull'

>  
>  ##
>  # @migrate-set-parameters:
> @@ -1354,6 +1367,12 @@
>  #     See description in @ZeroPageDetection.  Default is 'multifd'.
>  #     (since 9.0)
>  #
> +# @multifd-dsa-accel: If enabled, use DSA accelerator offloading for
> +#     certain memory operations. Enable DSA accelerator offloading by
> +#     setting this string to a list of DSA device path separated by space
> +#     characters. Setting this string to an empty string means disabling
> +#     DSA accelerator offloading. Defaults to an empty string. (since 9.2)
> +#
>  # Features:
>  #
>  # @deprecated: Member @block-incremental is deprecated.  Use
> @@ -1405,7 +1424,8 @@
>                                              'features': [ 'unstable' ] },
>              '*vcpu-dirty-limit': 'uint64',
>              '*mode': 'MigMode',
> -            '*zero-page-detection': 'ZeroPageDetection'} }
> +            '*zero-page-detection': 'ZeroPageDetection',
> +            '*multifd-dsa-accel': 'str'} }

Liekewise needs to be

   ['str']


Having mgmt apps pass in the path every time though, feels like
overkill. Surely there's a standard path that QEMU should use
by default, and should only require flag to turn on its usage.

IOW, why not extend the ZeroPageDetection enum, to have a further
entry for 'dsa' to request ue of dsa accel. Passing paths could
be optional.


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|