Re: PowerPC: gettimeofday optimization by using IFUNC

[Adhemerval Zanella <azanella at linux dot vnet dot ibm dot com>]

On 14-03-2013 14:47, Richard Henderson wrote:
> How about something like:
>
> /* This macro is needed for PPC64 to return a skeleton OPD entry of a vDSO
>    symbol.  This works because _dl_vdso_vsym always return the function
>    address, and no vDSO symbols use the TOC or chain pointers from the OPD
>    so we can allow them to be garbage.  */
>
> and then name the macro "VDSO_IFUNC_RET" to make it clear we're not talking
> about generic pointers as "PTR_IFUNC_RET" might suggest.

Fair enough, your comment is clear and I adjusted the patch to use it.


> Future work might include cleaning all this up so that it's much more obvious
> what's going on.  Consider:
>
> struct __vdso_opd {
>   void *code;
> #ifndef USE_PPC64_OVERLAPPING_OPD
>   void *toc;
>   void *chain;
> #endif
> };
>
> typedef struct __vdso_opd __vdso_ret;
>
> Change the return type of _dl_vdso_vsym generically to __vdso_ret, where the
> default version is a typedef to void*.
>
> I'm not sure that you can adjust DL_SYMBOL_ADDRESS, as that's also used by the
> regular part of the dynamic linker.  You could either invent a new macro to be
> used by the generic dl-vdso.c, or make a private copy for ppc64.  If a new
> macro, you could fairly easily compute the value as (__vdso_ret){ ... } to fill
> in only the first entry of the structure (letting C rules fill the rest w/0).
>
> Adjust VDSO_IFUNC_RET so that the type of the object passed is validated.

Seems reasonable, I'll consider this in possible futures cleanups.

I believe I adjusted the patch to your comments, is it good to commit?

Thanks for the review Richard.

--

2013-03-14  Adhemerval Zanella  <azanella@linux.vnet.ibm.com>

	* sysdeps/unix/sysv/linux/powerpc/bits/libc-vdso.h: Add macro to
	return vdso values correctly in IFUNC implementations.
	* sysdeps/unix/sysv/linux/powerpc/gettimeofday.c (__gettimeofday):
	Optimization by using IFUNC.


diff --git a/sysdeps/unix/sysv/linux/powerpc/bits/libc-vdso.h b/sysdeps/unix/sysv/linux/powerpc/bits/libc-vdso.h
index 545fda4..5f5fc1e 100644
--- a/sysdeps/unix/sysv/linux/powerpc/bits/libc-vdso.h
+++ b/sysdeps/unix/sysv/linux/powerpc/bits/libc-vdso.h
@@ -32,6 +32,16 @@ extern void *__vdso_get_tbfreq;
 
 extern void *__vdso_getcpu;
 
+/* This macro is needed for PPC64 to return a skeleton OPD entry of a vDSO
+   symbol.  This works because _dl_vdso_vsym always return the function
+   address, and no vDSO symbols use the TOC or chain pointers from the OPD
+   so we can allow them to be garbage.  */
+#if defined(__PPC64__) || defined(__powerpc64__)
+#define VDSO_IFUNC_RET(value)  &value
+#else
+#define VDSO_IFUNC_RET(value)  value
+#endif
+
 #endif
 
 #endif /* _LIBC_VDSO_H */
diff --git a/sysdeps/unix/sysv/linux/powerpc/gettimeofday.c b/sysdeps/unix/sysv/linux/powerpc/gettimeofday.c
index f607485..6506d75 100644
--- a/sysdeps/unix/sysv/linux/powerpc/gettimeofday.c
+++ b/sysdeps/unix/sysv/linux/powerpc/gettimeofday.c
@@ -15,25 +15,49 @@
    License along with the GNU C Library; if not, see
    <http://www.gnu.org/licenses/>.  */
 
-#include <sysdep.h>
-#include <stddef.h>
+
 #include <sys/time.h>
-#include <time.h>
-#include <hp-timing.h>
 
-#include <bits/libc-vdso.h>
+#ifdef SHARED
+
+# include <dl-vdso.h>
+# include <bits/libc-vdso.h>
+
+void *gettimeofday_ifunc (void) __asm__ ("__gettimeofday");
+
+static int
+__gettimeofday_syscall (struct timeval *tv, struct timezone *tz)
+{
+  return INLINE_SYSCALL (gettimeofday, 2, tv, tz);
+}
+
+void *
+gettimeofday_ifunc (void)
+{
+  /* If the vDSO is not available we fall back syscall.  */
+  return (__vdso_gettimeofday ? VDSO_IFUNC_RET (__vdso_gettimeofday)
+	  : __gettimeofday_syscall);
+}
+asm (".type __gettimeofday, %gnu_indirect_function");
+
+/* This is doing "libc_hidden_def (__gettimeofday)" but the compiler won't
+   let us do it in C because it doesn't know we're defining __gettimeofday
+   here in this file.  */
+asm (".globl __GI___gettimeofday\n"
+     "__GI___gettimeofday = __gettimeofday");
+
+#else
 
-/* Get the current time of day and timezone information,
-   putting it into *TV and *TZ.  If TZ is NULL, *TZ is not filled.
-   Returns 0 on success, -1 on errors.  */
+# include <sysdep.h>
+# include <errno.h>
 
 int
-__gettimeofday (tv, tz)
-     struct timeval *tv;
-     struct timezone *tz;
+__gettimeofday (struct timeval *tv, struct timezone *tz)
 {
-  return INLINE_VSYSCALL (gettimeofday, 2, tv, tz);
+  return INLINE_SYSCALL (gettimeofday, 2, tv, tz);
 }
 libc_hidden_def (__gettimeofday)
+
+#endif
 weak_alias (__gettimeofday, gettimeofday)
 libc_hidden_weak (gettimeofday)
-- 
1.7.1