This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: glibc 2.21 - Machine maintainers, please test your machines.
- From: Torvald Riegel <triegel at redhat dot com>
- To: Chris Metcalf <cmetcalf at ezchip dot com>
- Cc: "Carlos O'Donell" <carlos at redhat dot com>, GNU C Library <libc-alpha at sourceware dot org>, "H.J. Lu" <hjl dot tools at gmail dot com>, David Miller <davem at davemloft dot net>, Richard Henderson <rth at redhat dot com>, Mike Frysinger <vapier at gentoo dot org>, Andreas Schwab <schwab at suse dot de>, "Joseph S. Myers" <joseph at codesourcery dot com>, Kaz Kojima <kkojima at rr dot iij4u dot or dot jp>, Thomas Schwinge <thomas at codesourcery dot com>, Marcus Shawcroft <marcus dot shawcroft at linaro dot org>, Chung-Lin Tang <chunglin_tang at mentor dot com>, Adhemerval Zanella <azanella at linux dot vnet dot ibm dot com>, Andreas Krebbel <krebbel at linux dot vnet dot ibm dot com>
- Date: Mon, 26 Jan 2015 13:44:40 +0100
- Subject: Re: glibc 2.21 - Machine maintainers, please test your machines.
- Authentication-results: sourceware.org; auth=none
- References: <54C2BDD7 dot 7000304 at redhat dot com> <54C3B6D5 dot 3090308 at ezchip dot com> <1422119595 dot 29655 dot 42 dot camel at triegel dot csb> <54C5094A dot 8060300 at ezchip dot com> <54C51D94 dot 6030007 at ezchip dot com>
On Sun, 2015-01-25 at 11:45 -0500, Chris Metcalf wrote:
> On 1/25/2015 10:18 AM, Chris Metcalf wrote:
> > I'm now running with the following change, to see if tilegx32 will
> > also pass with it; this implements my suggestion of rounding new_sem to
> > an 8-byte boundary explicitly on ILP32 platforms.
>
> With my proposed change, tilegx32 (and tilegx64) pass all the tests. Repeated
> here with a suitable ChangeLog. Let me know if this is acceptable to commit
> for 2.21.
I think this is the right direction, but I have a few comments below.
>
> 2015-01-25 Chris Metcalf <cmetcalf@ezchip.com>
>
> * sysdeps/nptl/internaltypes.h (to_new_sem): Define. Provides new
> behavior for [__HAVE_64B_ATOMICS && !defined (_LP64)].
> * nptl/sem_getvalue.c (__new_sem_getvalue): Use to_new_sem.
> * nptl/sem_init.c (__new_sem_init): Likewise.
> * nptl/sem_post.c (__new_sem_post): Likewise.
> * nptl/sem_wait.c (__new_sem_wait): Likewise.
> (__new_sem_trywait): Likewise.
> * nptl/sem_timedwait.c (sem_timedwait): Likewise.
> * nptl/sem_open.c (sem_open): Add comment about to_new_sem.
>
> diff --git a/nptl/sem_getvalue.c b/nptl/sem_getvalue.c
> index 1432cc7..08a0789 100644
> --- a/nptl/sem_getvalue.c
> +++ b/nptl/sem_getvalue.c
> @@ -25,7 +25,7 @@
> int
> __new_sem_getvalue (sem_t *sem, int *sval)
> {
> - struct new_sem *isem = (struct new_sem *) sem;
> + struct new_sem *isem = to_new_sem (sem);
>
> /* XXX Check for valid SEM parameter. */
> /* FIXME This uses relaxed MO, even though POSIX specifies that this function
> diff --git a/nptl/sem_init.c b/nptl/sem_init.c
> index 575b661..aaa6af8 100644
> --- a/nptl/sem_init.c
> +++ b/nptl/sem_init.c
> @@ -50,7 +50,7 @@ __new_sem_init (sem_t *sem, int pshared, unsigned int value)
> }
>
> /* Map to the internal type. */
> - struct new_sem *isem = (struct new_sem *) sem;
> + struct new_sem *isem = to_new_sem (sem);
>
> /* Use the values the caller provided. */
> #if __HAVE_64B_ATOMICS
> diff --git a/nptl/sem_open.c b/nptl/sem_open.c
> index bfd2dea..9c1f279 100644
> --- a/nptl/sem_open.c
> +++ b/nptl/sem_open.c
> @@ -186,7 +186,9 @@ sem_open (const char *name, int oflag, ...)
> return SEM_FAILED;
> }
>
> - /* Create the initial file content. */
> + /* Create the initial file content. Note that the new_sem
I think this should be NEWSEM.
> + will have the necessary alignment in this case, so we are
> + not obliged to use the to_new_sem macro in this case. */
I think we should just use to_new_sem here too for consistency, but
change the comment to point out that the address to which we will copy
newsem via the file write and mmap later will have a compatible
alignment with the natural alignment of NEWSEM.
> union
> {
> sem_t initsem;
> diff --git a/nptl/sem_post.c b/nptl/sem_post.c
> index 6e495ed..71818ea 100644
> --- a/nptl/sem_post.c
> +++ b/nptl/sem_post.c
> @@ -56,7 +56,7 @@ futex_wake (unsigned int* futex, int processes_to_wake, int private)
> int
> __new_sem_post (sem_t *sem)
> {
> - struct new_sem *isem = (struct new_sem *) sem;
> + struct new_sem *isem = to_new_sem (sem);
> int private = isem->private;
>
> #if __HAVE_64B_ATOMICS
> diff --git a/nptl/sem_timedwait.c b/nptl/sem_timedwait.c
> index 042b0ac..5e650e0 100644
> --- a/nptl/sem_timedwait.c
> +++ b/nptl/sem_timedwait.c
> @@ -30,8 +30,8 @@ sem_timedwait (sem_t *sem, const struct timespec *abstime)
> return -1;
> }
>
> - if (__new_sem_wait_fast ((struct new_sem *) sem, 0) == 0)
> + if (__new_sem_wait_fast (to_new_sem (sem), 0) == 0)
> return 0;
> else
> - return __new_sem_wait_slow((struct new_sem *) sem, abstime);
> + return __new_sem_wait_slow(to_new_sem (sem), abstime);
> }
> diff --git a/nptl/sem_wait.c b/nptl/sem_wait.c
> index c1fd10c..3dade0c 100644
> --- a/nptl/sem_wait.c
> +++ b/nptl/sem_wait.c
> @@ -22,10 +22,10 @@
> int
> __new_sem_wait (sem_t *sem)
> {
> - if (__new_sem_wait_fast ((struct new_sem *) sem, 0) == 0)
> + if (__new_sem_wait_fast (to_new_sem (sem), 0) == 0)
> return 0;
> else
> - return __new_sem_wait_slow((struct new_sem *) sem, NULL);
> + return __new_sem_wait_slow(to_new_sem (sem), NULL);
> }
> versioned_symbol (libpthread, __new_sem_wait, sem_wait, GLIBC_2_1);
>
> @@ -65,7 +65,7 @@ __new_sem_trywait (sem_t *sem)
> {
> /* We must not fail spuriously, so require a definitive result even if this
> may lead to a long execution time. */
> - if (__new_sem_wait_fast ((struct new_sem *) sem, 1) == 0)
> + if (__new_sem_wait_fast (to_new_sem (sem), 1) == 0)
> return 0;
> __set_errno (EAGAIN);
> return -1;
> diff --git a/sysdeps/nptl/internaltypes.h b/sysdeps/nptl/internaltypes.h
> index 8f5cfa4..45ba99a 100644
> --- a/sysdeps/nptl/internaltypes.h
> +++ b/sysdeps/nptl/internaltypes.h
> @@ -168,6 +168,22 @@ struct new_sem
> #endif
> };
>
> +/* The sem_t alignment is typically just 4 bytes for ILP32, whereas
> + new_sem is 8 bytes. For better atomic performance on platforms
> + that tolerate unaligned atomics (and to make it work at all on
> + platforms that don't), round up the new_sem pointer within the
> + sem_t object to an 8-byte boundary.
> +
> + Note that in this case, the "pad" variable at the end of the
> + new_sem object extends beyond the end of the memory allocated
> + for the sem_t, so it's important that it not be initialized
> + or otherwise used. */
Please just remove "pad", and add a comment like this:
Note that sem_t is always at least 16 bytes in size, so moving new_sem,
which is 12 bytes in size, to offset of 4 bytes within sem_t is okay.
Also, a bitwise memcpy of sem_t is not allowed, and when mapping the
page a sem_t is in to another page, the offset within the page will not
change.
(BTW, I believe that a memcpy is not allowed -- but I couldn't point out
where in POSIX this is required. Does somebody know, just to be sure?)
> +#if __HAVE_64B_ATOMICS && !defined (_LP64)
> +# define to_new_sem(s) ((struct new_sem *)(((uintptr_t)(s) + 4) & -8))
> +#else
> +# define to_new_sem(s) ((struct new_sem *)(s))
> +#endif
> +
Is the compiler aware that if s is actually of a type that is properly
aligned, this can be a noop? If not, have you considered using
__alignof__ ? This would help on new targets that are ILP32 with 64b
atomics and that align sem_t properly.