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: Chris Metcalf <cmetcalf at ezchip dot com>
- To: Torvald Riegel <triegel at redhat 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 15:21:49 -0500
- Subject: Re: glibc 2.21 - Machine maintainers, please test your machines.
- Authentication-results: sourceware.org; auth=none
- Authentication-results: linux.vnet.ibm.com; dkim=none (message not signed) header.d=none;linux.vnet.ibm.com; dmarc=none action=none header.from=ezchip.com;
- 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> <1422276280 dot 29655 dot 91 dot camel at triegel dot csb>
On 1/26/2015 7:44 AM, Torvald Riegel wrote:
On Sun, 2015-01-25 at 11:45 -0500, Chris Metcalf wrote:
--- 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.
Well, that's the convention for arguments to a function, but I'm not sure it's
the same for fields in a struct. (It's kind of an odd convention to begin with.)
I'm just mentioning the struct type here, which I'm pretty sure isn't uppercased.
In any case, mooted by your next comment.
+ 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.
The diff to make it look like that is more complicated, so I resisted doing
it, particularly late in the freeze.
All that said, I'm happy to do v2 that way, and you can see what you think.
I don't actually think it's necessary to explain that to_new_sem() is a known
no-op in this case; we're using it just to be consistent anyway.
--- 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.
The thing is, new_sem is actually 16 bytes in size with __HAVE_64B_ATOMICS,
because the int64 type causes the size to round up to a multiple of eight. So I'd
rather be explicit about that padding in the structure definition, which is why
I ended up using the language I did.
I'll tweak my comment to discuss the lengths of the non-padding parts of
the structures more explicitly for v2.
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?)
I'm not actually sure what you mean about bitwise memcpy here. I think
it should be fine to do; and if you memcpy it from an alignof-4 address to
an alignof-8 address or vice versa, that should be fine, as long as you always
pass it through to_new_sem() before using the fields.
+#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.
Yes, I agree. Rather than using the preprocessor, v2 has:
#define to_new_sem(s) \
((__alignof__ (sem_t) == 4 && __alignof__ (struct new_sem) == 8) ? \
(struct new_sem *) (((uintptr_t) (s) + 4) & -8) : \
(struct new_sem *) (s))
The compiler evaluates the constant condition and produces no extra
code for 64-bit targets or for 32-bit targets without __HAVE_64B_ATOMICS.
We can't use an inline function since sem_t is not defined here,
and I'm reluctant to try to #include <semaphore.h> at this point.
In any case, new ILP32 targets that have 64-bit atomics should make
sem_t aligned to 8 bytes so they can avoid the re-alignment cost here.
I'll send out the v2 patch after make check completes.
--
Chris Metcalf, EZChip Semiconductor
http://www.ezchip.com