Re: [PATCH] AArch64: Check kernel version for SVE ifuncs

[Szabolcs Nagy <szabolcs.nagy@arm.com>]

The 03/13/2024 16:55, Adhemerval Zanella Netto wrote:
> On 13/03/24 16:25, Szabolcs Nagy wrote:
> > The 03/13/2024 15:12, Adhemerval Zanella Netto wrote:
> >> On 13/03/24 11:31, Wilco Dijkstra wrote:
> >>>
> >>> Older Linux kernels may disable SVE after certain syscalls.  Calling the
> >>> SVE-optimized memcpy afterwards will then cause a trap to reenable SVE.
> >>> As a result, applications with a high use of syscalls may run slower with
> >>> the SVE memcpy.  Avoid this by checking the kernel version and enable the
> >>> SVE-optimized memcpy/memmove ifuncs only on Linux kernel 6.2 or newer.
> >>>
> >>> Passes regress, OK for commit?
> >>
> >> How does it handle backports? Parsing kernel version is really not ideal
> >> (and that's why we removed it on previous version), can't kernel provide
> >> this information through any means (as powerpc does for HTM without
> >> suspend, PPC_FEATURE2_HTM_NO_SUSPEND) ?
> > 
> > the check is for performance only, not correctness.
> > 
> > if we detect a bad kernel as good, that can cause significant
> > slowdown due to the traps depending on the workload.
> > (at least this is the claim, i haven't verified, but plausible)
> > 
> > if we detect a good kernel as bad (e.g. because of kernel
> > backports), that is acceptable amount of slowdown (the fallback
> > memcpy is reasonably fast).
> > 
> > i think the kernel version check is ugly, but in this case it
> > makes sense: direct detection of the behaviour is hard, the
> > version check should reliably avoid detecting a bad kernel as
> > good, and it is better than always assuming a bad kernel.
> 
> Yes, I understand this. My point it won't be possible to backport this 
> kernel fix to get the performance improvement of SVE routines without 
> hacking the glibc as well.  It is not really a blocker, but I would
> expect kernel to do proper advertise for such performance change that
> might interfere with ifunc selection. Maybe we can add a tunable to
> force SVE selection, but I don't have a strong opinion.

right.

i can ask linux devs, if they are happy introducing an easy
check in future kernels or if the change is easy to backport
at all or we don't want to encourage that.

> 
> And I don't think __ASSUME_FAST_SVE would work well here, it means it
> would always detect a good kernel even when running on a older one

why? if it is set based on min supported kernel version then
running on older kernel is a bug. if it can be overriden by
a tunable then it is a user error if the tunable is wrong.

> (I am not sure how usual this is).  The minimum supported kernel 
> version can work to ensure that this check won't be necessary, but in
> this case we won't really need this test anyway.

you mean after min version is increased the check can be removed?
i expect all __ASSUME* based on min linux version works like that
and it's useful to have the __ASSUME exactly to be able to find
which code can be removed after a min version increase.

i don't know if distros actually adjust the min version in their
glibc, i guess that would be risky if it should work in containers,
so 6.2 min version is probably far in the future.

> 
> The _dl_discover_osversion (removed by b46d250656794e63a2946c481fda)
> used to check the PT_NOTE, and fallback to uname or /proc/sys/kernel/osrelease.
> I think we will need at least the osrelease fallback in the case of
> uname syscall filtering. 

i didn't know about the vdso PT_NOTE.

we could check osrelease there i guess. fallback is not critical
as slightly slower memcpy is not the end of the world, and there
is a balance how complicated code we want to maintain here.



> 
> > 
> >>
> >>>
> >>> ---
> >>>
> >>> diff --git a/sysdeps/aarch64/cpu-features.h b/sysdeps/aarch64/cpu-features.h
> >>> index 77a782422af1b6e4b2af32bfebfda37874111510..5f2da91ebbd0adafb0d84ec503b0f902f566da5a 100644
> >>> --- a/sysdeps/aarch64/cpu-features.h
> >>> +++ b/sysdeps/aarch64/cpu-features.h
> >>> @@ -71,6 +71,7 @@ struct cpu_features
> >>>    /* Currently, the GLIBC memory tagging tunable only defines 8 bits.  */
> >>>    uint8_t mte_state;
> >>>    bool sve;
> >>> +  bool prefer_sve_ifuncs;
> >>>    bool mops;
> >>>  };
> >>>  
> >>> diff --git a/sysdeps/aarch64/multiarch/init-arch.h b/sysdeps/aarch64/multiarch/init-arch.h
> >>> index c52860efb22d70eb4bdf356781f51c7de8ec67dc..61dc40088f4d9e5e06b57bdc7d54bde1e2a686a4 100644
> >>> --- a/sysdeps/aarch64/multiarch/init-arch.h
> >>> +++ b/sysdeps/aarch64/multiarch/init-arch.h
> >>> @@ -36,5 +36,7 @@
> >>>      MTE_ENABLED ();							      \
> >>>    bool __attribute__((unused)) sve =					      \
> >>>      GLRO(dl_aarch64_cpu_features).sve;					      \
> >>> +  bool __attribute__((unused)) prefer_sve_ifuncs =			      \
> >>> +    GLRO(dl_aarch64_cpu_features).prefer_sve_ifuncs;			      \
> >>>    bool __attribute__((unused)) mops =					      \
> >>>      GLRO(dl_aarch64_cpu_features).mops;
> >>> diff --git a/sysdeps/aarch64/multiarch/memcpy.c b/sysdeps/aarch64/multiarch/memcpy.c
> >>> index d12eccfca51f4bcfef6ccf5aa286edb301e361ac..ce53567dab33c2f00b89b4069235abd4651811a6 100644
> >>> --- a/sysdeps/aarch64/multiarch/memcpy.c
> >>> +++ b/sysdeps/aarch64/multiarch/memcpy.c
> >>> @@ -47,7 +47,7 @@ select_memcpy_ifunc (void)
> >>>      {
> >>>        if (IS_A64FX (midr))
> >>>  	return __memcpy_a64fx;
> >>> -      return __memcpy_sve;
> >>> +      return prefer_sve_ifuncs ? __memcpy_sve : __memcpy_generic;
> >>>      }
> >>>  
> >>>    if (IS_THUNDERX (midr))
> >>> diff --git a/sysdeps/aarch64/multiarch/memmove.c b/sysdeps/aarch64/multiarch/memmove.c
> >>> index 2081eeb4d40e0240e67a7b26b64576f44eaf18e3..fe95037be391896c7670ef606bf4d3ba7dfb6a00 100644
> >>> --- a/sysdeps/aarch64/multiarch/memmove.c
> >>> +++ b/sysdeps/aarch64/multiarch/memmove.c
> >>> @@ -47,7 +47,7 @@ select_memmove_ifunc (void)
> >>>      {
> >>>        if (IS_A64FX (midr))
> >>>  	return __memmove_a64fx;
> >>> -      return __memmove_sve;
> >>> +      return prefer_sve_ifuncs ? __memmove_sve : __memmove_generic;
> >>>      }
> >>>  
> >>>    if (IS_THUNDERX (midr))
> >>> diff --git a/sysdeps/unix/sysv/linux/aarch64/cpu-features.c b/sysdeps/unix/sysv/linux/aarch64/cpu-features.c
> >>> index b1a3f673f067280bdacfddd92723a81e418023e5..13b02c45df80b493516b3c9d4acbbbffaa47af92 100644
> >>> --- a/sysdeps/unix/sysv/linux/aarch64/cpu-features.c
> >>> +++ b/sysdeps/unix/sysv/linux/aarch64/cpu-features.c
> >>> @@ -21,6 +21,7 @@
> >>>  #include <sys/auxv.h>
> >>>  #include <elf/dl-hwcaps.h>
> >>>  #include <sys/prctl.h>
> >>> +#include <sys/utsname.h>
> >>>  #include <dl-tunables-parse.h>
> >>>  
> >>>  #define DCZID_DZP_MASK (1 << 4)
> >>> @@ -62,6 +63,41 @@ get_midr_from_mcpu (const struct tunable_str_t *mcpu)
> >>>    return UINT64_MAX;
> >>>  }
> >>>  
> >>> +/* Parse kernel version without calling any library functions.
> >>> +   Allow 2 digits for kernel version and 3 digits for major version,
> >>> +   separated by '.': "kk.mmm.".
> >>> +   Return kernel version * 1000 + major version, or -1 on failure.  */
> >>> +
> >>> +static inline int
> >>> +kernel_version (void)
> >>> +{
> >>> +  struct utsname buf;
> >>> +  const char *p = &buf.release[0];
> >>> +  int kernel = 0;
> >>> +  int major = 0;
> >>> +
> >>> +  if (__uname (&buf) < 0)
> >>> +    return -1;
> >>> +
> >>> +  if (*p >= '0' && *p <= '9')
> >>> +    kernel = (kernel * 10) + *p++ - '0';
> >>> +  if (*p >= '0' && *p <= '9')
> >>> +    kernel = (kernel * 10) + *p++ - '0';
> >>> +  if (*p != '.')
> >>> +    return -1;
> >>> +  p++;
> >>> +  if (*p >= '0' && *p <= '9')
> >>> +    major = (major * 10) + *p++ - '0';
> >>> +  if (*p >= '0' && *p <= '9')
> >>> +    major = (major * 10) + *p++ - '0';
> >>> +  if (*p >= '0' && *p <= '9')
> >>> +    major = (major * 10) + *p++ - '0';
> >>> +  if (*p != '.' && *p != '\0')
> >>> +    return -1;
> >>> +
> >>> +  return kernel * 1000 + major;
> >>> +}
> >>> +
> >>>  static inline void
> >>>  init_cpu_features (struct cpu_features *cpu_features)
> >>>  {
> >>> @@ -126,6 +162,10 @@ init_cpu_features (struct cpu_features *cpu_features)
> >>>    /* Check if SVE is supported.  */
> >>>    cpu_features->sve = GLRO (dl_hwcap) & HWCAP_SVE;
> >>>  
> >>> +  /* Prefer using SVE in string ifuncs from Linux 6.2 onwards.  */
> >>> +  cpu_features->prefer_sve_ifuncs =
> >>> +    cpu_features->sve && kernel_version () >= 6002;
> >>> +
> >>>    /* Check if MOPS is supported.  */
> >>>    cpu_features->mops = GLRO (dl_hwcap2) & HWCAP2_MOPS;
> >>>  }
> >>>