Re: [PATCH v2 1/3] x86: Fix Zen3/Zen4 ERMS selection (BZ 30994)

[Noah Goldstein <goldstein.w.n@gmail.com>]

On Wed, Feb 7, 2024 at 6:10 PM Adhemerval Zanella Netto
<adhemerval.zanella@linaro.org> wrote:
>
>
>
> On 07/02/24 15:06, Adhemerval Zanella Netto wrote:
> >
> >
> > On 07/02/24 14:39, Noah Goldstein wrote:
> >> On Wed, Feb 7, 2024 at 12:10 PM Adhemerval Zanella Netto
> >> <adhemerval.zanella@linaro.org> wrote:
> >>>
> >>>
> >>>
> >>> On 06/02/24 15:36, Noah Goldstein wrote:
> >>>> On Tue, Feb 6, 2024 at 5:43 PM Adhemerval Zanella
> >>>> <adhemerval.zanella@linaro.org> wrote:
> >>>>>
> >>>>> The REP MOVSB usage on memcpy/memmove does not show much performance
> >>>>> improvement on Zen3/Zen4 cores compared to the vectorized loops.  Also,
> >>>>> as from BZ 30994, if the source is aligned and the destination is not
> >>>>> the performance can be 20x slower.
> >>>>>
> >>>>> The performance difference is noticeable with small buffer sizes, closer
> >>>>> to the lower bounds limits when memcpy/memmove starts to use ERMS.  The
> >>>>> performance of REP MOVSB is similar to vectorized instruction on the
> >>>>> size limit (the L2 cache).  Also, there is no drawback to multiple cores
> >>>>> sharing the cache.
> >>>>>
> >>>>> A new tunable, glibc.cpu.x86_rep_movsb_stop_threshold, allows to set up
> >>>>> the higher bound size to use 'rep movsb'.
> >>>>>
> >>>>> Checked on x86_64-linux-gnu on Zen3.
> >>>>> ---
> >>>>>  manual/tunables.texi         |  9 +++++++
> >>>>>  sysdeps/x86/dl-cacheinfo.h   | 50 +++++++++++++++++++++---------------
> >>>>>  sysdeps/x86/dl-tunables.list | 10 ++++++++
> >>>>>  3 files changed, 48 insertions(+), 21 deletions(-)
> >>>>>
> >>>>> diff --git a/manual/tunables.texi b/manual/tunables.texi
> >>>>> index be97190d67..ee5d90b91b 100644
> >>>>> --- a/manual/tunables.texi
> >>>>> +++ b/manual/tunables.texi
> >>>>> @@ -569,6 +569,15 @@ greater than zero, and currently defaults to 2048 bytes.
> >>>>>  This tunable is specific to i386 and x86-64.
> >>>>>  @end deftp
> >>>>>
> >>>>> +@deftp Tunable glibc.cpu.x86_rep_movsb_stop_threshold
> >>>>> +The @code{glibc.cpu.x86_rep_movsb_threshold} tunable allows the user to
> >>>>> +set the threshold in bytes to stop using "rep movsb".  The value must be
> >>>>> +greater than zero, and currently, the default depends on the CPU and the
> >>>>> +cache size.
> >>>>> +
> >>>>> +This tunable is specific to i386 and x86-64.
> >>>>> +@end deftp
> >>>>> +
> >>>>>  @deftp Tunable glibc.cpu.x86_rep_stosb_threshold
> >>>>>  The @code{glibc.cpu.x86_rep_stosb_threshold} tunable allows the user to
> >>>>>  set threshold in bytes to start using "rep stosb".  The value must be
> >>>>> diff --git a/sysdeps/x86/dl-cacheinfo.h b/sysdeps/x86/dl-cacheinfo.h
> >>>>> index d5101615e3..74b804c5e6 100644
> >>>>> --- a/sysdeps/x86/dl-cacheinfo.h
> >>>>> +++ b/sysdeps/x86/dl-cacheinfo.h
> >>>>> @@ -791,7 +791,6 @@ dl_init_cacheinfo (struct cpu_features *cpu_features)
> >>>>>    long int data = -1;
> >>>>>    long int shared = -1;
> >>>>>    long int shared_per_thread = -1;
> >>>>> -  long int core = -1;
> >>>>>    unsigned int threads = 0;
> >>>>>    unsigned long int level1_icache_size = -1;
> >>>>>    unsigned long int level1_icache_linesize = -1;
> >>>>> @@ -809,7 +808,6 @@ dl_init_cacheinfo (struct cpu_features *cpu_features)
> >>>>>    if (cpu_features->basic.kind == arch_kind_intel)
> >>>>>      {
> >>>>>        data = handle_intel (_SC_LEVEL1_DCACHE_SIZE, cpu_features);
> >>>>> -      core = handle_intel (_SC_LEVEL2_CACHE_SIZE, cpu_features);
> >>>>>        shared = handle_intel (_SC_LEVEL3_CACHE_SIZE, cpu_features);
> >>>>>        shared_per_thread = shared;
> >>>>>
> >>>>> @@ -822,7 +820,8 @@ dl_init_cacheinfo (struct cpu_features *cpu_features)
> >>>>>         = handle_intel (_SC_LEVEL1_DCACHE_ASSOC, cpu_features);
> >>>>>        level1_dcache_linesize
> >>>>>         = handle_intel (_SC_LEVEL1_DCACHE_LINESIZE, cpu_features);
> >>>>> -      level2_cache_size = core;
> >>>>> +      level2_cache_size
> >>>>> +       = handle_intel (_SC_LEVEL2_CACHE_SIZE, cpu_features);
> >>>>>        level2_cache_assoc
> >>>>>         = handle_intel (_SC_LEVEL2_CACHE_ASSOC, cpu_features);
> >>>>>        level2_cache_linesize
> >>>>> @@ -835,12 +834,12 @@ dl_init_cacheinfo (struct cpu_features *cpu_features)
> >>>>>        level4_cache_size
> >>>>>         = handle_intel (_SC_LEVEL4_CACHE_SIZE, cpu_features);
> >>>>>
> >>>>> -      get_common_cache_info (&shared, &shared_per_thread, &threads, core);
> >>>>> +      get_common_cache_info (&shared, &shared_per_thread, &threads,
> >>>>> +                            level2_cache_size);
> >>>>>      }
> >>>>>    else if (cpu_features->basic.kind == arch_kind_zhaoxin)
> >>>>>      {
> >>>>>        data = handle_zhaoxin (_SC_LEVEL1_DCACHE_SIZE);
> >>>>> -      core = handle_zhaoxin (_SC_LEVEL2_CACHE_SIZE);
> >>>>>        shared = handle_zhaoxin (_SC_LEVEL3_CACHE_SIZE);
> >>>>>        shared_per_thread = shared;
> >>>>>
> >>>>> @@ -849,19 +848,19 @@ dl_init_cacheinfo (struct cpu_features *cpu_features)
> >>>>>        level1_dcache_size = data;
> >>>>>        level1_dcache_assoc = handle_zhaoxin (_SC_LEVEL1_DCACHE_ASSOC);
> >>>>>        level1_dcache_linesize = handle_zhaoxin (_SC_LEVEL1_DCACHE_LINESIZE);
> >>>>> -      level2_cache_size = core;
> >>>>> +      level2_cache_size = handle_zhaoxin (_SC_LEVEL2_CACHE_SIZE);
> >>>>>        level2_cache_assoc = handle_zhaoxin (_SC_LEVEL2_CACHE_ASSOC);
> >>>>>        level2_cache_linesize = handle_zhaoxin (_SC_LEVEL2_CACHE_LINESIZE);
> >>>>>        level3_cache_size = shared;
> >>>>>        level3_cache_assoc = handle_zhaoxin (_SC_LEVEL3_CACHE_ASSOC);
> >>>>>        level3_cache_linesize = handle_zhaoxin (_SC_LEVEL3_CACHE_LINESIZE);
> >>>>>
> >>>>> -      get_common_cache_info (&shared, &shared_per_thread, &threads, core);
> >>>>> +      get_common_cache_info (&shared, &shared_per_thread, &threads,
> >>>>> +                            level2_cache_size);
> >>>>>      }
> >>>>>    else if (cpu_features->basic.kind == arch_kind_amd)
> >>>>>      {
> >>>>>        data = handle_amd (_SC_LEVEL1_DCACHE_SIZE);
> >>>>> -      core = handle_amd (_SC_LEVEL2_CACHE_SIZE);
> >>>>>        shared = handle_amd (_SC_LEVEL3_CACHE_SIZE);
> >>>>>
> >>>>>        level1_icache_size = handle_amd (_SC_LEVEL1_ICACHE_SIZE);
> >>>>> @@ -869,7 +868,7 @@ dl_init_cacheinfo (struct cpu_features *cpu_features)
> >>>>>        level1_dcache_size = data;
> >>>>>        level1_dcache_assoc = handle_amd (_SC_LEVEL1_DCACHE_ASSOC);
> >>>>>        level1_dcache_linesize = handle_amd (_SC_LEVEL1_DCACHE_LINESIZE);
> >>>>> -      level2_cache_size = core;
> >>>>> +      level2_cache_size = handle_amd (_SC_LEVEL2_CACHE_SIZE);;
> >>>>>        level2_cache_assoc = handle_amd (_SC_LEVEL2_CACHE_ASSOC);
> >>>>>        level2_cache_linesize = handle_amd (_SC_LEVEL2_CACHE_LINESIZE);
> >>>>>        level3_cache_size = shared;
> >>>>> @@ -880,12 +879,12 @@ dl_init_cacheinfo (struct cpu_features *cpu_features)
> >>>>>        if (shared <= 0)
> >>>>>          {
> >>>>>             /* No shared L3 cache.  All we have is the L2 cache.  */
> >>>>> -           shared = core;
> >>>>> +           shared = level2_cache_size;
> >>>>>          }
> >>>>>        else if (cpu_features->basic.family < 0x17)
> >>>>>          {
> >>>>>             /* Account for exclusive L2 and L3 caches.  */
> >>>>> -           shared += core;
> >>>>> +           shared += level2_cache_size;
> >>>>>          }
> >>>>>
> >>>>>        shared_per_thread = shared;
> >>>>> @@ -1028,16 +1027,25 @@ dl_init_cacheinfo (struct cpu_features *cpu_features)
> >>>>>                            SIZE_MAX);
> >>>>>
> >>>>>    unsigned long int rep_movsb_stop_threshold;
> >>>>> -  /* ERMS feature is implemented from AMD Zen3 architecture and it is
> >>>>> -     performing poorly for data above L2 cache size. Henceforth, adding
> >>>>> -     an upper bound threshold parameter to limit the usage of Enhanced
> >>>>> -     REP MOVSB operations and setting its value to L2 cache size.  */
> >>>>> -  if (cpu_features->basic.kind == arch_kind_amd)
> >>>>> -    rep_movsb_stop_threshold = core;
> >>>>> -  /* Setting the upper bound of ERMS to the computed value of
> >>>>> -     non-temporal threshold for architectures other than AMD.  */
> >>>>> -  else
> >>>>> -    rep_movsb_stop_threshold = non_temporal_threshold;
> >>>>> +  /* If the tunable is set and with a valid value (larger than the minimal
> >>>>> +     threshold to use ERMS) use it instead of default values.  */
> >>>>> +  rep_movsb_stop_threshold = TUNABLE_GET (x86_rep_movsb_stop_threshold,
> >>>>> +                                         long int, NULL);
> >>>>> +  if (!TUNABLE_IS_INITIALIZED (x86_rep_movsb_stop_threshold)
> >>>>> +      || rep_movsb_stop_threshold <= rep_movsb_threshold)
> >>>>> +    {
> >>>>> +      /* For AMD CPUs that support ERMS (Zen3+), REP MOVSB is in a lot of
> >>>>> +        cases slower than the vectorized path (and for some alignments,
> >>>>> +        it is really slow, check BZ #30994).  */
> >>>>> +      if (cpu_features->basic.kind == arch_kind_amd)
> >>>>> +       rep_movsb_stop_threshold = 0;
> >>>> note that if `size >= rep_movsb_threshold && size >= rep_movsb_stop_threshold`
> >>>> we will use NT stores, not temporal stores.
> >>>>
> >>>> Id think you would want this to be setting the
> >>>> `rep_movsb_threshold` -> `non_temporal_threshold`
> >>>> which would essentially disable `rep movsb` but continue to
> >>>> use the other tunables for temporal/non-temporal decisions.
> >>>
> >>> My understanding is it will keep using non temporal stores, for instance
> >>> with a size equal to 25165824 (x86.cpu_features.non_temporal_threshold)
> >>> the code will:
> >>>
> >>> sysdeps/x86_64/multiarch/memmove-vec-unaligned-erms.S
> >>>
> >>> 384 #if defined USE_MULTIARCH && IS_IN (libc)
> >>> 385 L(movsb_more_2x_vec):
> >>> 386         cmp     __x86_rep_movsb_threshold(%rip), %RDX_LP
> >>> 387         ja      L(movsb)
> >>>
> >>>
> >>> And then:
> >>>
> >>> 613         /* If above __x86_rep_movsb_stop_threshold most likely is
> >>> 614            candidate for NT moves as well.  */
> >>> 615         cmp     __x86_rep_movsb_stop_threshold(%rip), %RDX_LP
> >>> 616         jae     L(large_memcpy_2x_check)
> >>>
> >>> And then skipping 'rep movsb' altogether.  And it will check whether
> >>> to use temporal stores:
> >>>
> >>> 683 L(large_memcpy_2x):
> >>> 684         mov     __x86_shared_non_temporal_threshold(%rip), %R11_LP
> >>> 685         cmp     %R11_LP, %RDX_LP
> >>> 686         jb      L(more_8x_vec_check)
> >>>
> >>
> >> Ah you're right, forgot about that code!
> >>
> >>
> >>>
> >>> Maybe one options would to set rep_movsb_stop_threshold to rep_movsb_threshold,
> >>> it slight clear that the range to use ERMS is a 0 size internal.
> >> So never use `rep movsb`?
> >> If that where the case, I would just set `rep_movsb_threshold` to
> >> `non_temporal_threshold`
> >> as the default.
> >
> > It works as well, it is essentially the same as setting rep_movsb_threshold to
> > rep_movsb_stop_threshold.
> >
>
> And I think by just setting rep_movsb_threshold, it makes the rep_movsb_stop_threshold
> tunable less appealing (it would be a matter to adjust the existing x86_rep_movsb_threshold
> tunable to enable ERMS).

Would think that is a good thing? Less configs to juggle.