Re: [PATCH] PowerPC: Add Program Priority Register support

["Ryan S. Arnold" <ryan dot arnold at gmail dot com>]

On Tue, Aug 14, 2012 at 8:03 AM, Adhemerval Zanella
<azanella@linux.vnet.ibm.com> wrote:
> Second spin of my previous Program Priority Register (PPR) support for
> POWER. I narrow down the issues I was observing and added the set/restore
> on all syscalls in glibc.
>
> This patch adds save/restore for the PPR for ISA 2.03+ for POWER
> machines. The PPR register is a special register that controls the
> program priority. It is documented on latest ISA 2.06 (Book II,
> Chapter 3.1) and it can be used to improve lock performance by setting
> an higher priority on critical sections and an lower priority on lock
> holding.
>
> The kernel will save/restore the PPR on context switch in future versions,
> so I intend to update the patch with guards for when it happens. I follow
> previous sys/platform/ppc.h update by creating inline static functions for
> POWER specific functions. I also used 'mtspr/mfspr' instead of 'mtppr'
> because although the ISA 2.06 documents the mnemonic, binutils does not
> implements it.
>
> Tested on PPC32 and PPC64.
>
> ---
>
> 2012-08-14 Adhemerval Zanella  <azanella@linux.vnet.ibm.com>
>
>         * sysdeps/powerpc/powerpc32/sysdep.h: Added Program Priority Register
>         (PPR) set/restore support for ISA 2.03+.
>         * sysdeps/powerpc/powerpc64/sysdep.h: Likewise.
>         * sysdeps/unix/sysv/linux/powerpc/powerpc32/sysdep.h: Likewise.
>         * sysdeps/unix/sysv/linux/powerpc/powerpc64/sysdep.h: Likewise.
>         * sysdeps/unix/sysv/linux/powerpc/syscall.S: Likewise.
>         * sysdeps/unix/sysv/linux/powerpc/powerpc32/____longjmp_chk.S:
>         Likewise.
>         * sysdeps/unix/sysv/linux/powerpc/powerpc32/setcontext-common.S:
>         Likewise.
>         * sysdeps/unix/sysv/linux/powerpc/powerpc32/swapcontext-common.S:
>         Likewise.
>         * sysdeps/unix/sysv/linux/powerpc/powerpc64/____longjmp_chk.S:
>         Likewise.
>         * sysdeps/unix/sysv/linux/powerpc/powerpc64/setcontext.S:
>         Likewise.
>         * sysdeps/unix/sysv/linux/powerpc/powerpc64/swapcontext.S:
>         Likewise.
>         * sysdeps/powerpc/sys/platform/ppc.h: Add PPR set functions.
>         * manual/platform.texi: Document PPR set functions.
>         * nptl/sysdeps/powerpc/tls.h: Add PPR field to TCB.
>         * sysdeps/powerpc/test-saveppr.c: New file: test PPR set/restore in
>         syscalls.
>         * sysdeps/powerpc/Makefile: Add test-saveppr test.
>         * nptl/sysdeps/powerpc/tcb-offsets.sym: Add PPR field offset value
>         in TCB.
>         * sysdeps/powerpc/powerpc32/stackguard-macros.h: Updated stack_guard
>         offset value.
>         * sysdeps/powerpc/powerpc64/stackguard-macros.h: Likewise.
>
> diff --git a/manual/platform.texi b/manual/platform.texi
> index 02b5c55..95e06ae 100644
> --- a/manual/platform.texi
> +++ b/manual/platform.texi
> @@ -26,3 +26,21 @@ different from the processor frequency.  More information is available in
>  without requiring assistance from the operating system, so it is very
>  efficient.
>  @end deftypefun
> +
> +@deftypefun {void} __ppc_set_ppr_med (void)
> +Set the Program Priority Register to medium value.
> +
> +The @dfn{Program Prioriry Register} (PPR) is a 64-bit register that controls

Typo "Prioriry" -> "Priority"
> +the program's priority. By adjusting the PPR value the programmer may
> +improve the system throughput by causing the system resouces to be used

"improve the system" -> "improve system"

"the resouces" -> "the resources"

> +more efficiently, especially in contention situations. More information
> +available in @cite{Power ISA 2.06b - Book II - Section 3.1}.
> +@end deftypefun

Perhaps a small indication of what states are available to user state
programs vs. privileged states would be useful here, similar to what
you have in ppc.h?

Also, can you describe what the current default priority is and what
it will be per ISA 2.06b when the kernel has complied?

> +
> +@deftypefun {void} __ppc_set_ppr_low (void)
> +Set the Program Priority Register to low value.
> +@end deftypefun
> +
> +@deftypefun {void} __ppc_set_ppr_med_low (void)
> +Set the Program Priority Register to medium low value.
> +@end deftypefun
> diff --git a/nptl/sysdeps/powerpc/tcb-offsets.sym b/nptl/sysdeps/powerpc/tcb-offsets.sym
> index 8ac133d..7f65b9f 100644
> --- a/nptl/sysdeps/powerpc/tcb-offsets.sym
> +++ b/nptl/sysdeps/powerpc/tcb-offsets.sym
> @@ -14,6 +14,7 @@ MULTIPLE_THREADS_OFFSET               thread_offsetof (header.multiple_threads)
>  #endif
>  PID                            thread_offsetof (pid)
>  TID                            thread_offsetof (tid)
> +PPR_OFFSET                     (offsetof (tcbhead_t, ppr) - TLS_TCB_OFFSET - sizeof (tcbhead_t))
>  POINTER_GUARD                  (offsetof (tcbhead_t, pointer_guard) - TLS_TCB_OFFSET - sizeof (tcbhead_t))
>  #ifndef __ASSUME_PRIVATE_FUTEX
>  PRIVATE_FUTEX_OFFSET           thread_offsetof (header.private_futex)
> diff --git a/nptl/sysdeps/powerpc/tls.h b/nptl/sysdeps/powerpc/tls.h
> index 4c09eec..6ed6e65 100644
> --- a/nptl/sysdeps/powerpc/tls.h
> +++ b/nptl/sysdeps/powerpc/tls.h
> @@ -1,5 +1,5 @@
>  /* Definition for thread-local data handling.  NPTL/PowerPC version.
> -   Copyright (C) 2003, 2005, 2006, 2007, 2011 Free Software Foundation, Inc.
> +   Copyright (C) 2003-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
> @@ -61,6 +61,8 @@ typedef union dtv
>     are private.  */
>  typedef struct
>  {
> +  /* Program Priority Register saved value.  */
> +  uint64_t  ppr;
>    uintptr_t pointer_guard;
>    uintptr_t stack_guard;
>    dtv_t *dtv;
> diff --git a/sysdeps/powerpc/Makefile b/sysdeps/powerpc/Makefile
> index 79dd6fa..371e845 100644
> --- a/sysdeps/powerpc/Makefile
> +++ b/sysdeps/powerpc/Makefile
> @@ -29,5 +29,5 @@ endif
>
>  ifeq ($(subdir),misc)
>  sysdep_headers += sys/platform/ppc.h
> -tests += test-gettimebase
> +tests += test-gettimebase test-saveppr
>  endif
> diff --git a/sysdeps/powerpc/powerpc32/stackguard-macros.h b/sysdeps/powerpc/powerpc32/stackguard-macros.h
> index 839f6a4..65171bc 100644
> --- a/sysdeps/powerpc/powerpc32/stackguard-macros.h
> +++ b/sysdeps/powerpc/powerpc32/stackguard-macros.h
> @@ -1,4 +1,8 @@
>  #include <stdint.h>
>
>  #define STACK_CHK_GUARD \
> -  ({ uintptr_t x; asm ("lwz %0,-28680(2)" : "=r" (x)); x; })
> +  ({ uintptr_t x; \
> +     asm ("lwz %0,%1(2)" : "=r" (x) \
> +         : "i" (offsetof (tcbhead_t, stack_guard) \
> +                - TLS_TCB_OFFSET - sizeof (tcbhead_t))); \
> +     x; })
> diff --git a/sysdeps/powerpc/powerpc32/sysdep.h b/sysdeps/powerpc/powerpc32/sysdep.h
> index 02917a8..a1a5709 100644
> --- a/sysdeps/powerpc/powerpc32/sysdep.h
> +++ b/sysdeps/powerpc/powerpc32/sysdep.h
> @@ -89,9 +89,26 @@ GOT_LABEL:                   ;                                             \
>    cfi_endproc;                                                               \
>    ASM_SIZE_DIRECTIVE(name)
>
> +/* The Priority Program Register (PPR) is saved and restored by moving its
> +   value to ppr field in TBC. Since it is the loader responsible to setup the

"TBC" -> "TCB"

"loader responsible" -> "loader's responsibility"

> +   TBC, the code is only built if not within the loader.  */

"TBC" -> "TCB"

"not within the loader." -> "not being used within the loader."

> +#if defined(_ARCH_PWR5X) && !defined(IS_IN_rtld) && defined(IN_LIB)
> +#define SET_PPR \
> +  mfspr r0,896;                                                                      \
> +  std   r0,PPR_OFFSET(r2);
> +#define RESTORE_PPR \
> +  ld    r0,PPR_OFFSET(r2);                                                   \
> +  mtspr 896,r0;
> +#else
> +#define SET_PPR
> +#define RESTORE_PPR
> +#endif
> +
>  #define DO_CALL(syscall)                                                     \
> +    SET_PPR                                                                  \
>      li 0,syscall;                                                            \
> -    sc
> +    sc;                                                                              \
> +    RESTORE_PPR
>
>  #undef JUMPTARGET
>  #ifdef PIC
> diff --git a/sysdeps/powerpc/powerpc64/stackguard-macros.h b/sysdeps/powerpc/powerpc64/stackguard-macros.h
> index 9da879c..667ba2d 100644
> --- a/sysdeps/powerpc/powerpc64/stackguard-macros.h
> +++ b/sysdeps/powerpc/powerpc64/stackguard-macros.h
> @@ -1,4 +1,8 @@
>  #include <stdint.h>
>
>  #define STACK_CHK_GUARD \
> -  ({ uintptr_t x; asm ("ld %0,-28688(13)" : "=r" (x)); x; })
> +  ({ uintptr_t x; \
> +     asm ("ld %0,%1(13)" : "=r" (x) \
> +          : "i" (offsetof (tcbhead_t, stack_guard) \
> +                - TLS_TCB_OFFSET - sizeof (tcbhead_t))); \
> +     x; })
> diff --git a/sysdeps/powerpc/powerpc64/sysdep.h b/sysdeps/powerpc/powerpc64/sysdep.h
> index ed96478..79566be 100644
> --- a/sysdeps/powerpc/powerpc64/sysdep.h
> +++ b/sysdeps/powerpc/powerpc64/sysdep.h
> @@ -205,9 +205,26 @@ LT_LABELSUFFIX(name,_name_end): ; \
>    TRACEBACK_MASK(name,mask)    \
>    END_2(name)
>
> +/* The Priority Program Register (PPR) is saved and restored by moving its
> +   value to ppr field in TBC. Since it is the loader responsible to setup the
> +   TBC, the code is only built if not within the loader.  */


Same comments as in the powerpc32 version above.

> +#if defined _ARCH_PWR5X && !defined(IS_IN_rtld) && defined(IN_LIB)
> +#define SET_PPR        \
> +  mfspr r0,896;                        \
> +  std   r0,PPR_OFFSET(r13);
> +#define RESTORE_PPR \
> +  ld    r0,PPR_OFFSET(r13);    \
> +  mtspr 896,r0;
> +#else
> +#define SET_PPR
> +#define RESTORE_PPR
> +#endif
> +
>  #define DO_CALL(syscall) \
> -    li 0,syscall; \
> -    sc
> +  SET_PPR                      \
> +  li r0,syscall;               \
> +  sc;                          \
> +  RESTORE_PPR
>

When used in the context of register state preservation rules isn't
the terminology usually SAVE/RESTORE or SET/GET?  Such as the case in
the _context functions?

>  /* ppc64 is always PIC */
>  #undef JUMPTARGET
> diff --git a/sysdeps/powerpc/sys/platform/ppc.h b/sysdeps/powerpc/sys/platform/ppc.h
> index 165652c..5752842 100644
> --- a/sysdeps/powerpc/sys/platform/ppc.h
> +++ b/sysdeps/powerpc/sys/platform/ppc.h
> @@ -44,4 +44,35 @@ __ppc_get_timebase (void)
>  #endif  /* not __powerpc64__ */
>  }
>
> +
> +/*
> + * ISA 2.05 and beyond support the Program Priority Register (PPR) to adjust
> + * thread priorities based on lock acquisition, wait and release. The ISA
> + * defines the use of form 'or Rx,Rx,Rx' as the way to modify the PRI field.
> + * The unprivileged priorities are:
> + *   Rx = 1 (low)
> + *   Rx = 2 (medium)
> + *   Rx = 6 (medium-low/normal)
> + * The 'or' instruction form is a nop in previous hardware, so it is safe to
> + * use unguarded.
> + */

Please indicate what the 'default' priority is now (kernel version)
and what it is intended to be in the future.

> +
> +static inline void
> +__ppc_set_ppr_med (void)
> +{
> +  __asm volatile ("or 2,2,2\n");
> +}
> +
> +static inline void
> +__ppc_set_ppr_med_low (void)
> +{
> +  __asm volatile ("or 6,6,6\n");
> +}
> +
> +static inline void
> +__ppc_set_ppr_low (void)
> +{
> +  __asm volatile ("or 1,1,1\n");
> +}
> +

Ryan S. Arnold