Re: [PATCH] PowerPC - Add a faster way to read the Time Base register

[Tulio Magno Quites Machado Filho <tuliom at linux dot vnet dot ibm dot com>]

On Tue, Feb 14, 2012 at 10:46:58AM -0600, Ryan S. Arnold wrote:
> On Tue, Feb 14, 2012 at 9:30 AM, Tulio Magno Quites Machado Filho
> <tuliom@linux.vnet.ibm.com> wrote:
> > Add macro __PPC_GETTIMEBASE() to directly read the Time Base register.
> > This is required for applications that measure time at high frequencies
> > with high precision that can't afford a syscall.
> >
> > My FSF copyright assignment is on file.
> >
> > 2012-02-06 ?Tulio Magno Quites Machado Filho ?<tuliom@linux.vnet.ibm.com>
> >
> > ? ? ? ?* sysdeps/unix/sysv/linux/powerpc/sys/user.h: New macro definition.
> 
> This should be:
>         * sysdeps/unix/sysv/linux/powerpc/sys/user.h (__PPC_GETTIMEBASE): New
>         macro definition.
> 
> > ? ? ? ?* sysdeps/unix/sysv/linux/powerpc/Makefile (tests): Add
> > ? ? ? ?test-gettimebase.
> > ? ? ? ?* sysdeps/unix/sysv/linux/powerpc/test-gettimebase.c: Test for
> > ? ? ? ?__PPC_GETTIMEBASE() to catch future ISA opcode/insn changes.
> > ---
> > ?sysdeps/unix/sysv/linux/powerpc/Makefile ? ? ? ? ? | ? ?2 +
> > ?sysdeps/unix/sysv/linux/powerpc/sys/user.h ? ? ? ? | ? 40 +++++++++++++++++++-
> > ?sysdeps/unix/sysv/linux/powerpc/test-gettimebase.c | ? 35 +++++++++++++++++
> > ?3 files changed, 76 insertions(+), 1 deletions(-)
> > ?create mode 100644 sysdeps/unix/sysv/linux/powerpc/test-gettimebase.c
> >
> > diff --git a/sysdeps/unix/sysv/linux/powerpc/Makefile b/sysdeps/unix/sysv/linux/powerpc/Makefile
> > index 55311a4..a2ab09a 100644
> > --- a/sysdeps/unix/sysv/linux/powerpc/Makefile
> > +++ b/sysdeps/unix/sysv/linux/powerpc/Makefile
> > @@ -15,3 +15,5 @@ endif
> > ?ifeq ($(subdir),elf)
> > ?sysdep_routines += dl-vdso
> > ?endif
> > +
> > +tests += test-gettimebase
> > diff --git a/sysdeps/unix/sysv/linux/powerpc/sys/user.h b/sysdeps/unix/sysv/linux/powerpc/sys/user.h
> > index 5fa3745..301e64a 100644
> > --- a/sysdeps/unix/sysv/linux/powerpc/sys/user.h
> > +++ b/sysdeps/unix/sysv/linux/powerpc/sys/user.h
> > @@ -1,4 +1,4 @@
> > -/* Copyright (C) 1998 Free Software Foundation, Inc.
> > +/* Copyright (C) 1998, 2012 Free Software Foundation, Inc.
> > ? ?This file is part of the GNU C Library.
> >
> > ? ?The GNU C Library is free software; you can redistribute it and/or
> > @@ -36,4 +36,42 @@ struct user {
> > ? ? ? ?char ? ? ? ? ? ?u_comm[32]; ? ? ? ? ? ? /* user command name */
> > ?};
> >
> > +typedef unsigned long long int __ppc_timebase;
> > +
> 
> Since the timebase is only held in bits 0:59 you need to add a #define
> for __PPC_TIMEBASE_MAX__ that is equivalent to when the timebase rolls
> over to zero, i.e., 2^59 - 1 (per the Power ISA 2.06), since
> __LONG_LONG_MAX__ is too large.  Don't forget the LL suffix on the
> value.
> 
> > +#define __STRINGIFY(...) #__VA_ARGS__
> > +#define STRINGIFY(...) __STRINGIFY(__VA_ARGS__)
> > +
> > +#define SPRN_TBRL 268 /* Time Base Read Lower Register */
> 
> Please indicate with a comment, where in the Power ISA the timebase
> register is mentioned (ISA version, section, etc.) and how this is to
> be used.
> 
> > +#ifdef __powerpc64__
> > +# define __PPC_GETTIMEBASE() ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? \
> > + ?({ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? \
> > + ? ?__ppc_timebase __tb; ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? \
> > + ? ?__asm__ volatile ( ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? \
> > + ? ? ? ? ? ? ? ? ? ? "mfspr %[tb], " STRINGIFY(SPRN_TBRL) "\n" \
> > + ? ? ? ? ? ? ? ? ? ? : [tb]"=r" (__tb) ? ? ? ? ? ? ? ? ? ? ? ? \
> > + ? ? ? ? ? ? ? ? ? ? : ); ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?\
> > + ? ?__tb; ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?\
> > + ?})
> > +#else ?/* __powerpc64__ */
> > +# define __PPC_GETTIMEBASE() ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? \
> > + ?({ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? \
> > + ? ?__ppc_timebase __tb; ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? \
> > + ? ?register unsigned long __tbu, __tbl, __tmp; ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?\
> > + ? ?__asm__ volatile ( ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? \
> > + ? ? ? ? ? ? ? ? ? ? "0:\n" ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?\
> > + ? ? ? ? ? ? ? ? ? ? "mftbu %[tbu]\n" ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?\
> > + ? ? ? ? ? ? ? ? ? ? "mftbl %[tbl]\n" ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?\
> > + ? ? ? ? ? ? ? ? ? ? "mftbu %[tmp]\n" ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?\
> > + ? ? ? ? ? ? ? ? ? ? "cmpw %[tbu], %[tmp]\n" ? ? ? ? ? ? ? ? ? ? ? ? ? \
> > + ? ? ? ? ? ? ? ? ? ? "bne- 0b\n" ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? \
> > + ? ? ? ? ? ? ? ? ? ? : [tbu]"=r" (__tbu), [tbl]"=r" (__tbl), ? ? ? ? ? \
> > + ? ? ? ? ? ? ? ? ? ? ? [tmp]"=r" (__tmp) ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? \
> > + ? ? ? ? ? ? ? ? ? ? : ); ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?\
> > + ? ?__tb = ( (__ppc_timebase) __tbu << 32) | __tbl; ? ? ? ? ? ? ? ? ? ?\
> > + ? ?__tb; ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?\
> > + ?})
> 
> I don't think the last __tb; is necessary.  Test it without. This is a
> GCC 'statement expression'.  I believe that it returns the value of
> the last expression in a statement block.

You're right.
In fact __tb is completely unnecessary here.

> 
> > +#endif ?/* __powerpc64__ */
> > +
> > ?#endif ?/* sys/user.h */
> > diff --git a/sysdeps/unix/sysv/linux/powerpc/test-gettimebase.c b/sysdeps/unix/sysv/linux/powerpc/test-gettimebase.c
> > new file mode 100644
> > index 0000000..cb18aab
> > --- /dev/null
> > +++ b/sysdeps/unix/sysv/linux/powerpc/test-gettimebase.c
> > @@ -0,0 +1,35 @@
> > +/* Copyright (C) 2012 Free Software Foundation, Inc.
> > + ? This file is part of the GNU C Library.
> > + ? Contributed by Tulio Magno Quites Machado <tuliom@linux.vnet.ibm.com>, 2012.
> > +
> > + ? The GNU C Library is free software; you can redistribute it and/or
> > + ? modify it under the terms of the GNU Lesser General Public
> > + ? License as published by the Free Software Foundation; either
> > + ? version 2.1 of the License, or (at your option) any later version.
> > +
> > + ? The GNU C Library is distributed in the hope that it will be useful,
> > + ? but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + ? MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. ?See the GNU
> > + ? Lesser General Public License for more details.
> > +
> > + ? You should have received a copy of the GNU Lesser General Public
> > + ? License along with the GNU C Library; if not, write to the Free
> > + ? Software Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA
> > + ? 02111-1307 USA. ?*/
> > +
> > +/* Test if __PPC_GETTIMEBASE() is compatible with the current processor */
> 
> Perhaps add that this is an early notification of a future Power ISA
> and binutils change.
> 
> > +#include <stdio.h>
> > +
> > +#include <sys/user.h>
> > +
> > +static int
> > +do_test(void)
> > +{
> > + ?__ppc_timebase tb = __PPC_GETTIMEBASE();
> > + ?printf("Time Base = %llx\n", tb);
> > + ?return 0;
> > +}
> > +
> > +#define TEST_FUNCTION do_test ()
> > +#include "../test-skeleton.c"
> 
> The testcase looks fine.
> 
> Something to think about for a future addition to user.h and the
> testcase would be to generate valid timestamps from the queried
> timebase values. I believe Power ISA Book II, Chapter 5. Timebase
> (page 706) demonstrates how to do this if we know the update
> frequency.

OK.

> Do we know on which Power Architecture platforms the update frequency
> of the timebase is constant?  How is the timebase update frequency
> queried (the ISA says this is possible).

Linux provides the vsyscall get_tbfreq.
GLIBC has get_clockfreq, but it isn't available for users.

> How often/quickly does the timebase roll-over on POWER systems?  As an
> example the ISA shows that, at 1Ghz CPU frequency, with a timebase
> update frequency of clock-frequency/32 the period of the timebase is
> about 18,700 years.  This would presume that the timebase is
> initialized to zero on system start.

But we can't assume it's always zero.

> If setting the timebase register isn't privileged perhaps you can
> fabricate a test that sets it near the max and generates valid
> timestamps across the rollover.

But unfortunately it is...

I'm preparing another patch that fixes your comments.

Thanks,

-- 
Tulio Magno