This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: glibc 2.21 - Machine maintainers, please test your machines.


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]