| Message ID | 54C51D94.6030007@ezchip.com |
|---|---|
| State | New |
| Headers | show |
On Sun, Jan 25, 2015 at 8:45 AM, Chris Metcalf <cmetcalf@ezchip.com> 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. > > 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. > > Can you try something similar to siginfo_t in: sysdeps/unix/sysv/linux/x86/bits/siginfo.h We had a similar problem on x32 before.
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.
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.
On Mon, 2015-01-26 at 15:21 -0500, Chris Metcalf wrote: > On 1/26/2015 7:44 AM, Torvald Riegel wrote: > >> + 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. Sorry, what I wrote was misleading, and incomplete: I still think using to_new_sem would be consistent -- but if we do that, we need to force the same alignment as we will have when effectively copy it via file write and mmap later on. I don't think the current code does that. If we don't use to_new_sem as you had in your patch, then we don't need to force the alignment -- but in this case we have to make sure that the code behaves as if to_new_sem didn't adapt the offset of new_sem within sem_t (which your previous code made sure by using the union). Nonetheless, I think that then, we need to point out that we must not use to_new_sem, so that it doesn't assume a wrong alignment (based on sem). I hope that is clearer :) (Sorry, it's been a long day here...) > >> --- 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. Good point, sizeof() vs. the amount of bytes actually accessed. > I'll tweak my comment to discuss the lengths of the non-padding parts of > the structures more explicitly for v2. Thanks. > > 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. I don't think so, because in a badly 4B-aligned (ie, no 8B-aligned) use, struct new_sem is at offset 4 of the semaphore. If you copy that to a 8B-aligned semaphore just bit-by-bit, then the actual data will still start at offset 4, but to_new_sem will assume offset 0. > >> +#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. Good, thanks!
Torvald Riegel <triegel@redhat.com> writes: > I don't think so, because in a badly 4B-aligned (ie, no 8B-aligned) use, > struct new_sem is at offset 4 of the semaphore. If you copy that to a > 8B-aligned semaphore just bit-by-bit, then the actual data will still > start at offset 4, but to_new_sem will assume offset 0. It is undefined to use a copy of a sem_t in any of the semaphore calls. Andreas.
On 1/26/2015 3:55 PM, Torvald Riegel wrote: > On Mon, 2015-01-26 at 15:21 -0500, Chris Metcalf wrote: >> On 1/26/2015 7:44 AM, Torvald Riegel wrote: >>>> + 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. > Sorry, what I wrote was misleading, and incomplete: I still think using > to_new_sem would be consistent -- but if we do that, we need to force > the same alignment as we will have when effectively copy it via file > write and mmap later on. I don't think the current code does that. Yes, I understand your point now. So I believe to accomplish that we need: sem_t sem __attribute__((aligned(__alignof__(struct new_sem))));; struct new_sem *newsem = to_new_sem (&sem); Is this cleaner then the previous code with the union? I guess arguably so, though as you say we still need a comment explaining that we are aligning the sem_t object to match its eventual page-alignment after it is written out to a file and used by mmap. > If we don't use to_new_sem as you had in your patch, then we don't need > to force the alignment -- but in this case we have to make sure that the > code behaves as if to_new_sem didn't adapt the offset of new_sem within > sem_t (which your previous code made sure by using the union). Yes. this was the previous behavior. It was correct but needed more comments to explain how it was copied to a file with offset zero mod eight. > Nonetheless, I think that then, we need to point out that we must not > use to_new_sem, so that it doesn't assume a wrong alignment (based on > sem). Well, to_new_sem() would be OK in the union case, since the sem_t would have alignment zero mod eight due to the struct new_sem sharing the alignment of the overall union. But it doesn't really make sense to use to_new_sem() and the union; it feels either/or. We should just choose one. More importantly, we still need to really confirm that the to_new_sem() approach makes sense as the best overall strategy. For tilegx32, we need either this or just using 32-bit atomics. For x32, we can keep using what we have now for them, or they can use this new aligning code and presumably get somewhat faster semaphores. I don't know for m68k and mips what they need. For aarch64 in ILP32 mode, or other new ILP32 architectures with 64-bit atomics, it probably makes the most sense to force sem_t to be 8-byte aligned and not have to deal with any of this. (I note that aarch64 also does not support unaligned atomics, so it needs to do something, one way or the other.)
On Mon, Jan 26, 2015 at 3:29 PM, Chris Metcalf <cmetcalf@ezchip.com> wrote: > On 1/26/2015 3:55 PM, Torvald Riegel wrote: >> >> On Mon, 2015-01-26 at 15:21 -0500, Chris Metcalf wrote: >>> >>> On 1/26/2015 7:44 AM, Torvald Riegel wrote: >>>>> >>>>> + 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. >> >> Sorry, what I wrote was misleading, and incomplete: I still think using >> to_new_sem would be consistent -- but if we do that, we need to force >> the same alignment as we will have when effectively copy it via file >> write and mmap later on. I don't think the current code does that. > > > Yes, I understand your point now. So I believe to accomplish that we need: > > sem_t sem __attribute__((aligned(__alignof__(struct new_sem))));; > struct new_sem *newsem = to_new_sem (&sem); > > Is this cleaner then the previous code with the union? I guess arguably > so, though as you say we still need a comment explaining that we are > aligning the sem_t object to match its eventual page-alignment after > it is written out to a file and used by mmap. > > >> If we don't use to_new_sem as you had in your patch, then we don't need >> to force the alignment -- but in this case we have to make sure that the >> code behaves as if to_new_sem didn't adapt the offset of new_sem within >> sem_t (which your previous code made sure by using the union). > > > Yes. this was the previous behavior. It was correct but needed more > comments > to explain how it was copied to a file with offset zero mod eight. > >> Nonetheless, I think that then, we need to point out that we must not >> use to_new_sem, so that it doesn't assume a wrong alignment (based on >> sem). > > > Well, to_new_sem() would be OK in the union case, since the sem_t would > have alignment zero mod eight due to the struct new_sem sharing the > alignment of the overall union. But it doesn't really make sense to use > to_new_sem() and the union; it feels either/or. We should just choose one. > > More importantly, we still need to really confirm that the to_new_sem() > approach makes sense as the best overall strategy. > > For tilegx32, we need either this or just using 32-bit atomics. For x32, > we can keep using what we have now for them, or they can use this new > aligning code and presumably get somewhat faster semaphores. I don't > know for m68k and mips what they need. What is the drawback with 32-bit atomics for x32? If there is no real advantage for 64-bit atomics, x32 will use 32-bit atomics. If x32 needs to use 64-bit atomics, I need to investigate which way to go.
On Mon, 2015-01-26 at 23:56 +0100, Andreas Schwab wrote: > Torvald Riegel <triegel@redhat.com> writes: > > > I don't think so, because in a badly 4B-aligned (ie, no 8B-aligned) use, > > struct new_sem is at offset 4 of the semaphore. If you copy that to a > > 8B-aligned semaphore just bit-by-bit, then the actual data will still > > start at offset 4, but to_new_sem will assume offset 0. > > It is undefined to use a copy of a sem_t in any of the semaphore calls. I think we all agree on that. Nonetheless, as I mentioned previously, I'm not actually aware of where in POSIX (or C?) this would be explicitly forbidden. So if you know that, I'd be interested in getting a reference.
Torvald Riegel <triegel@redhat.com> writes: > On Mon, 2015-01-26 at 23:56 +0100, Andreas Schwab wrote: >> Torvald Riegel <triegel@redhat.com> writes: >> >> > I don't think so, because in a badly 4B-aligned (ie, no 8B-aligned) use, >> > struct new_sem is at offset 4 of the semaphore. If you copy that to a >> > 8B-aligned semaphore just bit-by-bit, then the actual data will still >> > start at offset 4, but to_new_sem will assume offset 0. >> >> It is undefined to use a copy of a sem_t in any of the semaphore calls. > > I think we all agree on that. Nonetheless, as I mentioned previously, > I'm not actually aware of where in POSIX (or C?) this would be > explicitly forbidden. So if you know that, I'd be interested in getting > a reference. See sem_init. Only sem itself may be used for performing synchronization. The result of referring to copies of sem in calls to sem_wait(), sem_timedwait(), sem_trywait(), sem_post(), and sem_destroy() is undefined. Andreas.
On Mon, 2015-01-26 at 16:17 -0800, H.J. Lu wrote: > What is the drawback with 32-bit atomics for x32? If there is no real > advantage for 64-bit atomics, x32 will use 32-bit atomics. If x32 needs > to use 64-bit atomics, I need to investigate which way to go. > 64b atomics do not give any advantage in sem_post nor on the fast path of sem_wait (ie, if sem_wait does not use a futex to block, but just can grab a token, or spin-waits (not implemented yet)). 64b atomics allows for a more efficient sem_wait slow path; that is, when we use a futex to block. In a nutshell, we don't need to do what's in sem_waitcommon.c:__sem_wait_32_finish() and another atomic read-modify-write operation before the futex_wait. The additional overhead around the futex_wait call is not a really big problem I would say, as futexes are relatively slow anyway and we shouldn't use them for short periods of waiting (that's where the missing spin-waiting would come into play). The disadvantage that the slow path can have however is potentially decreased scalability, because the additional atomic ops might slow down other threads. But I wouldn't consider this a really critical issue, especially not once we add some spin-waiting. There could also be pathological situations in which we have a high rate of sem_wait / sem_post calls, and the sem_wait's can't agree on who's last, so we might get lots of unnecessary futex_wake calls in __sem_wait_32_finish().
On Tue, 2015-01-27 at 11:59 +0100, Andreas Schwab wrote: > Torvald Riegel <triegel@redhat.com> writes: > > > On Mon, 2015-01-26 at 23:56 +0100, Andreas Schwab wrote: > >> Torvald Riegel <triegel@redhat.com> writes: > >> > >> > I don't think so, because in a badly 4B-aligned (ie, no 8B-aligned) use, > >> > struct new_sem is at offset 4 of the semaphore. If you copy that to a > >> > 8B-aligned semaphore just bit-by-bit, then the actual data will still > >> > start at offset 4, but to_new_sem will assume offset 0. > >> > >> It is undefined to use a copy of a sem_t in any of the semaphore calls. > > > > I think we all agree on that. Nonetheless, as I mentioned previously, > > I'm not actually aware of where in POSIX (or C?) this would be > > explicitly forbidden. So if you know that, I'd be interested in getting > > a reference. > > See sem_init. > > Only sem itself may be used for performing synchronization. The > result of referring to copies of sem in calls to sem_wait(), > sem_timedwait(), sem_trywait(), sem_post(), and sem_destroy() is > undefined. Hmm, that was obvious. Thanks for pointing it out :)
On Tue, Jan 27, 2015 at 3:10 AM, Torvald Riegel <triegel@redhat.com> wrote: > On Mon, 2015-01-26 at 16:17 -0800, H.J. Lu wrote: >> What is the drawback with 32-bit atomics for x32? If there is no real >> advantage for 64-bit atomics, x32 will use 32-bit atomics. If x32 needs >> to use 64-bit atomics, I need to investigate which way to go. >> > > 64b atomics do not give any advantage in sem_post nor on the fast path > of sem_wait (ie, if sem_wait does not use a futex to block, but just can > grab a token, or spin-waits (not implemented yet)). > > 64b atomics allows for a more efficient sem_wait slow path; that is, > when we use a futex to block. In a nutshell, we don't need to do what's > in sem_waitcommon.c:__sem_wait_32_finish() and another atomic > read-modify-write operation before the futex_wait. In that case, I'd like to use 64-bit atomics in sem_wait slow path for x32 and 32-bit atomics elsewhere. I will pay potential extra memory bus cycle on slow path. > The additional overhead around the futex_wait call is not a really big > problem I would say, as futexes are relatively slow anyway and we > shouldn't use them for short periods of waiting (that's where the > missing spin-waiting would come into play). > > The disadvantage that the slow path can have however is potentially > decreased scalability, because the additional atomic ops might slow down > other threads. But I wouldn't consider this a really critical issue, > especially not once we add some spin-waiting. There could also be > pathological situations in which we have a high rate of sem_wait / > sem_post calls, and the sem_wait's can't agree on who's last, so we > might get lots of unnecessary futex_wake calls in > __sem_wait_32_finish(). >
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 + will have the necessary alignment in this case, so we are + not obliged to use the to_new_sem macro in this case. */ 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. */ +#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 + struct old_sem { unsigned int value;