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

[Zhao, Zhou]

Hi dainel:
  Cause our code depend on intel intrinsics lib implement. And this lib depend 
on macro like  " AVX512BW ". This macro need compile time check to enable some 
machine options . if you only use that utility to do runtime check ,you will 
met compile issue. And also if we want to save cpu time , we'd better check it 
in compile time.

-----Original Message-----
From: Daniel P. Berrangé <berrange@redhat.com> 
Sent: Thursday, July 21, 2022 11:11 PM
To: Xu, Ling1 <ling1.xu@intel.com>
Cc: qemu-devel@nongnu.org; quintela@redhat.com; dgilbert@redhat.com; Zhao, Zhou 
<zhou.zhao@intel.com>; Jin, Jun I <jun.i.jin@intel.com>
Subject: Re: [PATCH 1/1] Add AVX512 support for xbzrle_encode_buffer function

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