Add ifunc memcpy and memmove for aarch64

  On Wed, 2017-02-08 at 11:15 +0530, Siddhesh Poyarekar wrote:

> Looks OK with a couple of nits below.

Here is a de-nitted version with the ChangeLog using 'Likewise'
instead of 'Ditto' and with a one line description at the top
of libc-start.c.

Steve Ellcey
sellcey@caviium.com



2017-02-08  Steve Ellcey  <sellcey@caviumnetworks.com>
	    Adhemerval Zanella  <adhemerval.zanella@linaro.org>

	* sysdeps/aarch64/dl-machine.h: Include cpu-features.c.
	(DL_PLATFORM_INIT): New define.
	(dl_platform_init): New function.
	* sysdeps/aarch64/ldsodefs.h: Include cpu-features.h.
	* sysdeps/unix/sysv/linux/aarch64/cpu-features.c: New file.
	* sysdeps/unix/sysv/linux/aarch64/cpu-features.h: Likewise.
	* sysdeps/unix/sysv/linux/aarch64/dl-procinfo.c: Likewise.
	* sysdeps/unix/sysv/linux/aarch64/libc-start.c: Likewise.
  

On 09/02/17 00:02, Steve Ellcey wrote:
> +static inline void __attribute__ ((unused))
> +dl_platform_init (void)
> +{
> +  if (GLRO(dl_platform) != NULL && *GLRO(dl_platform) == '\0')
> +    /* Avoid an empty string which would disturb us.  */
> +    GLRO(dl_platform) = NULL;
> +
> +#ifdef SHARED
> +  /* init_cpu_features has been called early from __libc_start_main in
> +     static executable.  */
> +  init_cpu_features (&GLRO(dl_aarch64_cpu_features));
> +#endif
> +}
...
> +static inline void
> +init_cpu_features (struct cpu_features *cpu_features)
> +{
> +  if (GLRO(dl_hwcap) & HWCAP_CPUID)
> +    {
> +      register uint64_t id = 0;
> +      asm volatile ("mrs %0, midr_el1" : "=r"(id));
> +      cpu_features->midr_el1 = id;

this is a trap into the kernel at every process startup

since this is called very early (dynamic linking case
above, static linking case below) i wonder if there
could be a way for the user to request midr_el1==0
unconditionally (avoiding the overhead and making
sure the most generic implementation is used)

is there something like that on other targets?

> +    }
> +  else
> +    {
> +      cpu_features->midr_el1 = 0;
> +    }
> +}
...
> +#ifdef SHARED
> +# include <csu/libc-start.c>
> +# else
> +/* The main work is done in the generic function.  */
> +# define LIBC_START_DISABLE_INLINE
> +# define LIBC_START_MAIN generic_start_main
> +# include <csu/libc-start.c>
> +# include <cpu-features.c>
> +
> +extern struct cpu_features _dl_aarch64_cpu_features;
> +
> +int
> +__libc_start_main (int (*main) (int, char **, char ** MAIN_AUXVEC_DECL),
> +		   int argc, char **argv,
> +		   __typeof (main) init,
> +		   void (*fini) (void),
> +		   void (*rtld_fini) (void), void *stack_end)
> +{
> +  init_cpu_features (&_dl_aarch64_cpu_features);
> +  return generic_start_main (main, argc, argv, init, fini, rtld_fini,
> +			     stack_end);
> +}
> +#endif
>
  

On Thursday 09 February 2017 04:20 PM, Szabolcs Nagy wrote:
> this is a trap into the kernel at every process startup
> 
> since this is called very early (dynamic linking case
> above, static linking case below) i wonder if there
> could be a way for the user to request midr_el1==0
> unconditionally (avoiding the overhead and making
> sure the most generic implementation is used)

Well you could use tunables to avoid it, but then if a single trap is a
problem for you then the tunables infra is going to be just as expensive.

Why is a single trap at startup such a concern though?

> is there something like that on other targets?

H.J. Lu had a patch to override IFUNCs using (what will now be) a
tunable but that patch did not make progress.  I hope it will now since
I too am interested in overriding IFUNC selection using tunables.  But
then this would be an orthogonal discussion to avoiding the trap since
the goals of both are different.

Siddhesh
  

On 09/02/17 11:04, Siddhesh Poyarekar wrote:
> On Thursday 09 February 2017 04:20 PM, Szabolcs Nagy wrote:
>> this is a trap into the kernel at every process startup
>>
>> since this is called very early (dynamic linking case
>> above, static linking case below) i wonder if there
>> could be a way for the user to request midr_el1==0
>> unconditionally (avoiding the overhead and making
>> sure the most generic implementation is used)
> 
> Well you could use tunables to avoid it, but then if a single trap is a
> problem for you then the tunables infra is going to be just as expensive.
> 
> Why is a single trap at startup such a concern though?

ok, it is probably not worth worrying about
(should be around 0.1% of the minimal startup time now)

but doing it just to control ifunc selection is still
useful (at least for development)

(if eventually there will be widespread use of ifunc
based function multi-versioning then the trap will
not be per process startup but per module load which
is a bit more relevant, but also not something we can
control from the libc)

>> is there something like that on other targets?
> 
> H.J. Lu had a patch to override IFUNCs using (what will now be) a
> tunable but that patch did not make progress.  I hope it will now since
> I too am interested in overriding IFUNC selection using tunables.  But
> then this would be an orthogonal discussion to avoiding the trap since
> the goals of both are different.

i see.
  

On Thursday 09 February 2017 05:10 PM, Szabolcs Nagy wrote:
> ok, it is probably not worth worrying about
> (should be around 0.1% of the minimal startup time now)
> 
> but doing it just to control ifunc selection is still
> useful (at least for development)

A simple tunable that disables CPU selection would be nice:

glibc.tune.cpu = default|thunderx|a57

However then you would have to think harder about positioning the cpu
structure initialization in static binaries to have them be controlled
by tunables and not just blindly put them right at the top of
__libc_start as the easy way out.  It is something that should be done
anyway.

However, couldn't we do that as an add-on to this patch?  I'd really
like this framework to go in early and be exercised a bit because I am
interested in pushing (in the coming weeks hopefully) some optimal
string routines for aarch64.

> (if eventually there will be widespread use of ifunc
> based function multi-versioning then the trap will
> not be per process startup but per module load which
> is a bit more relevant, but also not something we can
> control from the libc)

Right we cannot control that.

Siddhesh
  

On Thu, Feb 9, 2017 at 2:50 AM, Szabolcs Nagy <szabolcs.nagy@arm.com> wrote:
> On 09/02/17 00:02, Steve Ellcey wrote:
>> +static inline void __attribute__ ((unused))
>> +dl_platform_init (void)
>> +{
>> +  if (GLRO(dl_platform) != NULL && *GLRO(dl_platform) == '\0')
>> +    /* Avoid an empty string which would disturb us.  */
>> +    GLRO(dl_platform) = NULL;
>> +
>> +#ifdef SHARED
>> +  /* init_cpu_features has been called early from __libc_start_main in
>> +     static executable.  */
>> +  init_cpu_features (&GLRO(dl_aarch64_cpu_features));
>> +#endif
>> +}
> ...
>> +static inline void
>> +init_cpu_features (struct cpu_features *cpu_features)
>> +{
>> +  if (GLRO(dl_hwcap) & HWCAP_CPUID)
>> +    {
>> +      register uint64_t id = 0;
>> +      asm volatile ("mrs %0, midr_el1" : "=r"(id));
>> +      cpu_features->midr_el1 = id;
>
> this is a trap into the kernel at every process startup
>
> since this is called very early (dynamic linking case
> above, static linking case below) i wonder if there
> could be a way for the user to request midr_el1==0
> unconditionally (avoiding the overhead and making
> sure the most generic implementation is used)

Well the easy way to do this would be use LD_HWCAP_MASK and mask off
the HWCAP_CPUID bit.

>
> is there something like that on other targets?

Some targets like PowerPC use the hwcap to say which processor they
are being run on so you use the same method as above.

Thanks,
Andrew

>
>> +    }
>> +  else
>> +    {
>> +      cpu_features->midr_el1 = 0;
>> +    }
>> +}
> ...
>> +#ifdef SHARED
>> +# include <csu/libc-start.c>
>> +# else
>> +/* The main work is done in the generic function.  */
>> +# define LIBC_START_DISABLE_INLINE
>> +# define LIBC_START_MAIN generic_start_main
>> +# include <csu/libc-start.c>
>> +# include <cpu-features.c>
>> +
>> +extern struct cpu_features _dl_aarch64_cpu_features;
>> +
>> +int
>> +__libc_start_main (int (*main) (int, char **, char ** MAIN_AUXVEC_DECL),
>> +                int argc, char **argv,
>> +                __typeof (main) init,
>> +                void (*fini) (void),
>> +                void (*rtld_fini) (void), void *stack_end)
>> +{
>> +  init_cpu_features (&_dl_aarch64_cpu_features);
>> +  return generic_start_main (main, argc, argv, init, fini, rtld_fini,
>> +                          stack_end);
>> +}
>> +#endif
>>
>
  

On Thu, 2017-02-09 at 07:54 -0800, Andrew Pinski wrote:
> On Thu, Feb 9, 2017 at 2:50 AM, Szabolcs Nagy <szabolcs.nagy@arm.com> wrote:> 
> > since this is called very early (dynamic linking case
> > above, static linking case below) i wonder if there
> > could be a way for the user to request midr_el1==0
> > unconditionally (avoiding the overhead and making
> > sure the most generic implementation is used)
> Well the easy way to do this would be use LD_HWCAP_MASK and mask off
> the HWCAP_CPUID bit.
> 
> > is there something like that on other targets?
> Some targets like PowerPC use the hwcap to say which processor they
> are being run on so you use the same method as above.
> 
> Thanks,
> Andrew

Do you know if LD_HWCAP_MASK actually works on PowerPC to do this?  I
see where PowerPC is reading GLRO(dl_hwcap) but I don't see them
reading GLRO(dl_hwcap_mask).  I don't think that the generic parts of
libc apply LD_HWCAP_MASK to HWCAP automatically but maybe I missed it
somewhere.

I changed init_cpu_features in my patch from:

	if (GLRO(dl_hwcap) & HWCAP_CPUID)
to
	if (GLRO(dl_hwcap) & GLRO(dl_hwcap_mask) & HWCAP_CPUID)

to see what would happen.  Initially this returned false because
we were using the default dl-procinfo.h and HWCAP_IMPORTANT (used
to initialize dl_hwcap_mask) was 0.  I made a copy of dl-procinfo.h
and set HWCAP_IMPORTANT to HPWCAP_CPUID and I got true here and the
thunderx ifuncs were run.

But if I ran things after setting LD_HWCAP_MASK to 0 it didn't seem to
have any affect and I still ran thunderx ifuncs.  I am not sure but it
seemed like this code in init_cpu_features may have been getting run
before LD_HWCAP_MASK was getting read and before GLRO(dl_hwcap_mask)
was reset from its initial value.

Steve Ellcey