Re: [PATCH 1/1] Add AVX512 support for xbzrle_encode_buffer function

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

On Thu, Jul 21, 2022 at 06:31:47PM +0800, ling xu wrote:
> This commit adds AVX512 implementation of xbzrle_encode_buffer function to
> accelerate xbzrle encoding speed. Compared with C version of xbzrle_encode_buffer function,
> AVX512 version can achieve almost 60%-70% performance improvement on unit test provided by qemu.
> In addition, we provide one more unit test called "test_encode_decode_random", in which
> dirty data are randomly located in 4K page, and this case can achieve almost 140% performance gain.
> 
> Signed-off-by: ling xu <ling1.xu@intel.com>
> Co-authored-by: Zhou Zhao <zhou.zhao@intel.com>
> Co-authored-by: Jun Jin <jun.i.jin@intel.com>
> ---
>  configure                | 434 ++++++++++++++++++++++++++++++++++++++-
>  migration/ram.c          |   6 +
>  migration/xbzrle.c       | 177 ++++++++++++++++
>  migration/xbzrle.h       |   4 +
>  tests/unit/test-xbzrle.c | 307 +++++++++++++++++++++++++--
>  5 files changed, 908 insertions(+), 20 deletions(-)

> diff --git a/migration/ram.c b/migration/ram.c
> index 01f9cc1d72..3b931c325f 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -747,9 +747,15 @@ static int save_xbzrle_page(RAMState *rs, uint8_t **current_data,
>      memcpy(XBZRLE.current_buf, *current_data, TARGET_PAGE_SIZE);
>  
>      /* XBZRLE encoding (if there is no overflow) */
> +    #if defined(__x86_64__) && defined(__AVX512BW__)
> +    encoded_len = xbzrle_encode_buffer_512(prev_cached_page, XBZRLE.current_buf,
> +                                       TARGET_PAGE_SIZE, XBZRLE.encoded_buf,
> +                                       TARGET_PAGE_SIZE);
> +    #else
>      encoded_len = xbzrle_encode_buffer(prev_cached_page, XBZRLE.current_buf,
>                                         TARGET_PAGE_SIZE, XBZRLE.encoded_buf,
>                                         TARGET_PAGE_SIZE);
> +    #endif

Shouldn't we be deciding which impl using a runtime check of
the current CPUID, rather than a compile time check ? I'm
thinking along the lines of what util/bufferiszero.c does to
select different optimized versions based on CPUID. The build
host CPU features can't be expected to match the runtime host
CPU features.


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 :|