This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] PowerPC: Add Program Priority Register support
- From: "Ryan S. Arnold" <ryan dot arnold at gmail dot com>
- To: Adhemerval Zanella <azanella at linux dot vnet dot ibm dot com>
- Cc: "GNU C. Library" <libc-alpha at sourceware dot org>
- Date: Tue, 14 Aug 2012 13:03:20 -0500
- Subject: Re: [PATCH] PowerPC: Add Program Priority Register support
- References: <502A4C91.3070301@linux.vnet.ibm.com>
On Tue, Aug 14, 2012 at 8:03 AM, Adhemerval Zanella
<azanella@linux.vnet.ibm.com> wrote:
I saw one more thing.
I think that the functionality of the saved PPR value in the TLS
address space is correct but I think that some of the comments are
incorrect.
> 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)
Notice how the PPR_OFFSET (as you've placed it) is a negative offset
from the tcbhead (which is, itself negative offset from the value in
the thread-pointer register by TLS_TCB_OFFSET). This space prior to
the start of the TCB is defined by the TLS ABI as a place for "other
thread library information".
> 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;
The Power TLS ABI defines the start of the TCB as the DTV pointer.
The stack guard and pointer guard are accessed by negative offsets
from the head of the TCB. This means, that you've put the PPR save
value in the correct place because you preserve the existing offsets
to the pointer_guard and stack_guard off the TCB, which preserves
binary compatibility.
...
> 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, the code is only built if not within the loader. */
With my explanation in mind, I think this comment isn't quite correct.
It should read as something like the following:
/* The value in the Priority Program Register (PPR) is saved and restored
by moving its value to, and fetching its value from, a field preceding the
TCB that is reserved by the TLS ABI for "other thread library information".
Since it is the loader's responsibility to setup the TCB (and the fields in
the reserved space), the PPR save/restore facility is only available outside
of the loader. */
You could add a comment that the loader always, therefore, runs in the
default thread priority.
...
> 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 comment applies as for ppc32.
Ryan S. Arnold