Message ID | 1490103612-9401-1-git-send-email-adhemerval.zanella@linaro.org |
---|---|
Headers | show |
Series | Add support for ISO C11 threads.h | expand |
On Tue, 21 Mar 2017, Adhemerval Zanella wrote: > - Addresses Joseph's comments from [2] and ad indicated in > in C11 thread patch I based this implementation from th > public C11 thread document I have access [3]. I would ask for > suggestion and change if it deviates from the final standard > definition. Did you review those C11 DRs related to threads to make sure the implementation behaves properly regarding issues raised in those DRs (and that there are tests of this if applicable)? Current DR log: http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2109.htm -- Joseph S. Myers joseph@codesourcery.com
On 21/03/2017 13:25, Joseph Myers wrote: > On Tue, 21 Mar 2017, Adhemerval Zanella wrote: > >> - Addresses Joseph's comments from [2] and ad indicated in >> in C11 thread patch I based this implementation from th >> public C11 thread document I have access [3]. I would ask for >> suggestion and change if it deviates from the final standard >> definition. > > Did you review those C11 DRs related to threads to make sure the > implementation behaves properly regarding issues raised in those DRs (and > that there are tests of this if applicable)? > > Current DR log: http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2109.htm > Thanks for the link, I will check this out. Do you know off the top of your head which ones would be applicable?
On Tue, 21 Mar 2017, Adhemerval Zanella wrote: > > Did you review those C11 DRs related to threads to make sure the > > implementation behaves properly regarding issues raised in those DRs (and > > that there are tests of this if applicable)? > > > > Current DR log: http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2109.htm > > > > Thanks for the link, I will check this out. Do you know off the top of your > head which ones would be applicable? It appears to be: 405 414 416 424 449 469 470 479 480 493. -- Joseph S. Myers joseph@codesourcery.com
On 21/03/2017 13:49, Joseph Myers wrote:
> It appears to be: 405 414 416 424 449 469 470 479 480 493.
Thank you.
On 21/03/2017 13:49, Joseph Myers wrote: > On Tue, 21 Mar 2017, Adhemerval Zanella wrote: > >>> Did you review those C11 DRs related to threads to make sure the >>> implementation behaves properly regarding issues raised in those DRs (and >>> that there are tests of this if applicable)? >>> >>> Current DR log: http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2109.htm >>> >> >> Thanks for the link, I will check this out. Do you know off the top of your >> head which ones would be applicable? > > It appears to be: 405 414 416 424 449 469 470 479 480 493. It indeed seems to be one related to threads definitions and in general C11 clarification seems to align with POSIX thread definition, which simplifies its implementation. However I see some problematic ones: 416 (tss_t destruction unspecified) and 493 (Mutex Initialization Underspecified), so I am copying Tovarld's and Martin since they were the one that raised most the DR. It could be good if they is any impending reason to avoid implement C11 thread over POSIX primitives based on current standard. Below some observation of the raised DRs related to current patch proposal: 405: The mutex specification "The C11 specification of mutexes is missing the total order over all the calls on a particular mutex." My understanding is current implementation with POSIX primitives should be enough to guarantee C11 total order over mutex (which is currently based on acquire/release semanthics). 416: tss_t destruction unspecified I could not find any impeding wording from the proposed technical corrigendum that would prevent us to implement tss function on top of pthread key ones. 414: Typos in 6.27 Threads <threads.h> The typos does clarify one definition of mtx_plain and raises a questioning: how should we proceed if mtx_timedlock is used along with a mtx_plain mutex? My understanding is thrd_error should be returned, however current GLIBC implementation makes no distinction between PTHREAD_MUTEX_NORMAL and PTHREAD_MUTEX_TIMED_NP (and current C11 patch follows this semanthic). It is feasible to do so, but it would require maybe an extra field in mtx_t definition or a GNU-only thread type (maybe not intended to be exposed). 424: underspecification of tss_t These issues are covered under DR 416. 449: What is the value of TSS_DTOR_ITERATIONS for implementations with no maximum? The proposed committee response was to intentionally do not define a value of TSS_DTOR_ITERATIONS, so my view is align with PTHREAD_DESTRUCTOR_ITERATIONS should be suffice (even though various architectures redefine _POSIX_THREAD_DESTRUCTOR_ITERATIONS to same value in local_lim.h). 469: lock ownership vs. thread termination Defined as future. However from last committee discussion it seems that they are aiming for similar POSIX description. So I am not sure if there is any impending stardard definition to use underlying POSIX implementation. 470: mtx_trylock should be allowed to fail spuriously It align with POSIX and thus with __pthread_mutex_trylock which uses a atomic_compare_and_exchange_bool_acq (similar to atomic_compare_exchange_weak). 479: unclear specification of mtx_trylock on non-recursive muteness Defined as future. I am not aware of any issue trying to use POSIX in underlying implementation. 480: cnd_wait and cnd_timewait should allow spurious wake-ups Define as open, although my understanding is C11 definitions from last proposed technical corrigendum may now allow spurious wakes ups (from the words "until it is unblocked due to an unspecified reason"). So I think implementing cnd_{timed}wait as pthread_cond_{timed}wait wrapper should be still valid. 493: Mutex Initialization Underspecified Define as open and the proposed committee response follows: - Problems with mtx_init(): I think the only applicable point to curent patch is the last one: 7. thrd_error shall be returned by mtx_init() when passed a NULL pointer. Which currently leads to a segfault (since it is based on pthread_mutex_init). It is simple to align current patch proposal to this specification. - Problems with mtx_destroy(): wording aligns with POSIX definition, so implementing mtx_destroy as a pthread_mutex_destroy wrapper should be ok. - Other Problems: wording aligns with POSIX definition as well, so I think there is no requirement to change for current patch.
On Tue, 2017-03-21 at 16:49 +0000, Joseph Myers wrote: > On Tue, 21 Mar 2017, Adhemerval Zanella wrote: > > > > Did you review those C11 DRs related to threads to make sure the > > > implementation behaves properly regarding issues raised in those DRs (and > > > that there are tests of this if applicable)? > > > > > > Current DR log: http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2109.htm > > > > > > > Thanks for the link, I will check this out. Do you know off the top of your > > head which ones would be applicable? > > It appears to be: 405 414 416 424 449 469 470 479 480 493. > Here's my take on these: 405, 414: Some of these DRs affect implementations, but there's nothing contentious in these I believe. 416, 424, 449: I haven't looked at these before and can't comment on them currently. 469: What I proposed should be clarified by C matches our implementation. The committee seems to agree. POSIX makes stronger requirements on the implementation, which we do not implement (and what I do not intend to implement). I have arguend in a POSIX bug that POSIX should not make these stronger requirements (sorry, don't have the bug number handy). 470: C11's requirements on implementations are weaker than what POSIX requires. Arguably, POSIX should require what C11 requires. We need to check the lowlevellock implementation, in particular on archs that use LL/SC (I'm not aware of an arch with a true CAS instruction that can fail spuriously). Our generic lowlevellock implementation on master still uses the old-style atomics (ie, atomic_compare_and_exchange_bool_acq); if we move to C11 atomics, C11 (with DR 470's proposed corrigendum applied) would require our atomic_compare_exchange_weak_acquire, whereas current POSIX would require the strong variant of the CAS. Thus, what we do is okay in terms of C11. I don't recall right now whether we have an open Austin Group bug about POSIX allowing a spurious failure; I believe we have, or we should, because this is also related to what memory order is required for lock acquisitions (generally, not just trylock). (Stefan, that's why I'm CC'ing you too, this is relevant for pthread_spin_trylock too, but I can't remember right now whether we already discussed this or not.) 479: Our implementation is correct. What C++ requires from a program, and what the committee agrees C11 should require, is stronger than what POSIX requires. This means that the C11 implementation of trylock would not have to conservatively abort transactions in the lock elision implementation for the non-recursive mutexes. 480: A specification bug. Committee agrees with what we implement. 493: The intent of the standard as stated by the committee in this DR seems to be compatible with what glibc implements and could implement in the foreseeable future. Martin, please correct anything I got wrong. Also, I assume you are tracking these DRs, so please give us a heads up if the committee should change its mind in a way that conflicts with what we implement.
On Tue, 2017-03-21 at 17:09 -0300, Adhemerval Zanella wrote: > 405: The mutex specification > > "The C11 specification of mutexes is missing the total order over all the > calls on a particular mutex." > > My understanding is current implementation with POSIX primitives should be > enough to guarantee C11 total order over mutex (which is currently based on > acquire/release semanthics). Yes. > 414: Typos in 6.27 Threads <threads.h> > > The typos does clarify one definition of mtx_plain and raises a questioning: > how should we proceed if mtx_timedlock is used along with a mtx_plain mutex? If C11 requires that mtx_timedlock is supplied a mutex that supports timeout. If the program violates this requirement, it is undefined behavior, and glibc is allowed to just acquire with a timeout. > 469: lock ownership vs. thread termination See my other response to this thread. > > 470: mtx_trylock should be allowed to fail spuriously > > It align with POSIX and thus with __pthread_mutex_trylock which uses a > atomic_compare_and_exchange_bool_acq (similar to atomic_compare_exchange_weak). That's not quite true. See my other response. > 479: unclear specification of mtx_trylock on non-recursive muteness > 480: cnd_wait and cnd_timewait should allow spurious wake-ups See my other response to this thread.
On 03/27/2017 03:25 AM, Torvald Riegel wrote: > On Tue, 2017-03-21 at 16:49 +0000, Joseph Myers wrote: >> On Tue, 21 Mar 2017, Adhemerval Zanella wrote: >> >>>> Did you review those C11 DRs related to threads to make sure the >>>> implementation behaves properly regarding issues raised in those DRs (and >>>> that there are tests of this if applicable)? >>>> >>>> Current DR log: http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2109.htm >>>> >>> >>> Thanks for the link, I will check this out. Do you know off the top of your >>> head which ones would be applicable? >> >> It appears to be: 405 414 416 424 449 469 470 479 480 493. >> > > Here's my take on these: I've reviewed the DRs and your comments below. I agree with your view and just for clarity provide some additional comments of my own. > > 405, 414: > Some of these DRs affect implementations, but there's nothing > contentious in these I believe. Agreed. > > 416, 424, 449: > I haven't looked at these before and can't comment on them currently. The 416 corrignedum was accepted. 424 was rolled into 416, and 449 resulted in no changes. > > 469: > What I proposed should be clarified by C matches our implementation. > The committee seems to agree. > POSIX makes stronger requirements on the implementation, which we do not > implement (and what I do not intend to implement). I have arguend in a > POSIX bug that POSIX should not make these stronger requirements (sorry, > don't have the bug number handy). This issue was closed with the expectation that it will be resolved in C2X. Unless someone else steps up it will likely mean that you (or we) will have to propose the changes you/we would like to see. > 470: > C11's requirements on implementations are weaker than what POSIX > requires. Arguably, POSIX should require what C11 requires. > We need to check the lowlevellock implementation, in particular on archs > that use LL/SC (I'm not aware of an arch with a true CAS instruction > that can fail spuriously). > Our generic lowlevellock implementation on master still uses the > old-style atomics (ie, atomic_compare_and_exchange_bool_acq); if we move > to C11 atomics, C11 (with DR 470's proposed corrigendum applied) would > require our atomic_compare_exchange_weak_acquire, whereas current POSIX > would require the strong variant of the CAS. > Thus, what we do is okay in terms of C11. > I don't recall right now whether we have an open Austin Group bug about > POSIX allowing a spurious failure; I believe we have, or we should, > because this is also related to what memory order is required for lock > acquisitions (generally, not just trylock). > (Stefan, that's why I'm CC'ing you too, this is relevant for > pthread_spin_trylock too, but I can't remember right now whether we > already discussed this or not.) > The resolution for the DR was accepted and the DR is closed. The change will be in the 2017 C11 TC. If other changes are required here a new issue will need to be submitted that will be considered for C2X. > > 479: > Our implementation is correct. > What C++ requires from a program, and what the committee agrees C11 > should require, is stronger than what POSIX requires. This means that > the C11 implementation of trylock would not have to conservatively abort > transactions in the lock elision implementation for the non-recursive > mutexes. Here it sounds like as the authors of the DR we're on the hook for providing wording that the rest of WG14 agrees with. This too will be considered for C2X. > > 480: > A specification bug. Committee agrees with what we implement. > > > 493: > The intent of the standard as stated by the committee in this DR seems > to be compatible with what glibc implements and could implement in the > foreseeable future. This DR is still Open and needs a proposed resolution. > > Martin, > please correct anything I got wrong. Also, I assume you are tracking > these DRs, so please give us a heads up if the committee should change > its mind in a way that conflicts with what we implement. Most of the DRs on the list are in a Closed state with (presumably) clear resolutions. As I understand the C11 schedule, the outstanding ones that are still Open or labeled Future will not be fixed in the final corrigendum that's expected to be finalized this year. After 2017 there will not be any further C11 corrigenda. C2X will the next work item on WG14's list. There's been a lot of talk over the last few WG14 meetings about the whole threads section needing an overhaul. I don't know if anyone is actually working on it but if it were to happen (for C2X) there is some risk that an implementation coded to the C11 spec not conforming to the cleaned up and improved C2X spec. Martin PS Besides the issues list there are at least two other documents that could offer additional insight or guidance here. The C2X charter that outlines the guiding principles for C2X development: http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2021.htm In addition, the WG14 Convener maintains a standing document where he tracks a subset of issues that have come up over the years and that he wants WG14 to review for C2X. This is not a complete list, just some select items. http://www.open-std.org/JTC1/SC22/WG14/www/docs/n2087.htm
On Mon, 2017-03-27 at 10:10 -0600, Martin Sebor wrote: > I've reviewed the DRs and your comments below. I agree with > your view and just for clarity provide some additional comments > of my own. Thanks! > There's been a lot of talk over the last few WG14 meetings about > the whole threads section needing an overhaul. I don't know if > anyone is actually working on it but if it were to happen (for > C2X) there is some risk that an implementation coded to the C11 > spec not conforming to the cleaned up and improved C2X spec. I'd hope that they wouldn't deviate from what C++ specifies. I'm monitoring C++ changes, including whether anything would result in required changes for glibc. IOW, if C doesn't deviate from C++, we shouldn't need additional changes just for C.
On 28/03/2017 05:08, Torvald Riegel wrote: > On Mon, 2017-03-27 at 10:10 -0600, Martin Sebor wrote: >> I've reviewed the DRs and your comments below. I agree with >> your view and just for clarity provide some additional comments >> of my own. > > Thanks! > >> There's been a lot of talk over the last few WG14 meetings about >> the whole threads section needing an overhaul. I don't know if >> anyone is actually working on it but if it were to happen (for >> C2X) there is some risk that an implementation coded to the C11 >> spec not conforming to the cleaned up and improved C2X spec. > > I'd hope that they wouldn't deviate from what C++ specifies. I'm > monitoring C++ changes, including whether anything would result in > required changes for glibc. IOW, if C doesn't deviate from C++, we > shouldn't need additional changes just for C. > Thanks for both extensive inputs and discussion. From the comments I see that a current C11 thread based on POSIX could be still be feasible, however I am not sure if we should prevent its implementation based on the C2X possible different spec. In any way, I see that the still pending DR493 should not pose any implementation issues (we can work out on the wrapper if any other requirement is posed). So I would like the input from the community whether implementing C11 in GLIBC is desirable and if current approach based is most correct one.
On Fri, 2017-03-31 at 10:39 -0300, Adhemerval Zanella wrote: > > On 28/03/2017 05:08, Torvald Riegel wrote: > > On Mon, 2017-03-27 at 10:10 -0600, Martin Sebor wrote: > >> I've reviewed the DRs and your comments below. I agree with > >> your view and just for clarity provide some additional comments > >> of my own. > > > > Thanks! > > > >> There's been a lot of talk over the last few WG14 meetings about > >> the whole threads section needing an overhaul. I don't know if > >> anyone is actually working on it but if it were to happen (for > >> C2X) there is some risk that an implementation coded to the C11 > >> spec not conforming to the cleaned up and improved C2X spec. > > > > I'd hope that they wouldn't deviate from what C++ specifies. I'm > > monitoring C++ changes, including whether anything would result in > > required changes for glibc. IOW, if C doesn't deviate from C++, we > > shouldn't need additional changes just for C. > > > > Thanks for both extensive inputs and discussion. From the comments I > see that a current C11 thread based on POSIX could be still be feasible, > however I am not sure if we should prevent its implementation based on > the C2X possible different spec. > > In any way, I see that the still pending DR493 should not pose any > implementation issues (we can work out on the wrapper if any other > requirement is posed). > > So I would like the input from the community whether implementing C11 > in GLIBC is desirable and if current approach based is most correct > one. I think it is desirable, and it's probably about time that we have something. I'm not aware of anything that would be a huge problem. C11 is in several cases much closer to what we implement than POSIX. I still think it may have been nice to have smaller data structure sizes for things like mutexes; however, we don't have consensus in the community to shrink them, and we don't have the resources I believe to really investigate this. I haven't looked at the TLS issues, or at how you organize headers and such. I also haven't yet reviewed all of your patches. But unless somebody complains, IMHO we should just go forward with what you have. Supporting the C11 threading support functions in the next release would be nice.
On 06/04/2017 08:05, Torvald Riegel wrote: > On Fri, 2017-03-31 at 10:39 -0300, Adhemerval Zanella wrote: >> >> On 28/03/2017 05:08, Torvald Riegel wrote: >>> On Mon, 2017-03-27 at 10:10 -0600, Martin Sebor wrote: >>>> I've reviewed the DRs and your comments below. I agree with >>>> your view and just for clarity provide some additional comments >>>> of my own. >>> >>> Thanks! >>> >>>> There's been a lot of talk over the last few WG14 meetings about >>>> the whole threads section needing an overhaul. I don't know if >>>> anyone is actually working on it but if it were to happen (for >>>> C2X) there is some risk that an implementation coded to the C11 >>>> spec not conforming to the cleaned up and improved C2X spec. >>> >>> I'd hope that they wouldn't deviate from what C++ specifies. I'm >>> monitoring C++ changes, including whether anything would result in >>> required changes for glibc. IOW, if C doesn't deviate from C++, we >>> shouldn't need additional changes just for C. >>> >> >> Thanks for both extensive inputs and discussion. From the comments I >> see that a current C11 thread based on POSIX could be still be feasible, >> however I am not sure if we should prevent its implementation based on >> the C2X possible different spec. >> >> In any way, I see that the still pending DR493 should not pose any >> implementation issues (we can work out on the wrapper if any other >> requirement is posed). >> >> So I would like the input from the community whether implementing C11 >> in GLIBC is desirable and if current approach based is most correct >> one. > > I think it is desirable, and it's probably about time that we have > something. I'm not aware of anything that would be a huge problem. C11 > is in several cases much closer to what we implement than POSIX. > > I still think it may have been nice to have smaller data structure sizes > for things like mutexes; however, we don't have consensus in the > community to shrink them, and we don't have the resources I believe to > really investigate this. Indeed, however this would add some complication to use the POSIX internal implementation directly. We would need to create a common internal function that would be called by POSIX and C11 wrappers, I will check this out in my next rebase. > > I haven't looked at the TLS issues, or at how you organize headers and > such. I also haven't yet reviewed all of your patches. But unless > somebody complains, IMHO we should just go forward with what you have. > Supporting the C11 threading support functions in the next release would > be nice. Now with the removal of the ancient macro CALL_THREAD_FCT [1] and the consolidation of pthreadtypes.h in a different patchset [2] [3], I will rebase the changes and send a new patchset. The consolidation of pthreadtypes.h is mostly a mechanical change without expected code changes, so I think it should be safer to push i [1] https://sourceware.org/ml/libc-alpha/2017-04/msg00057.html [2] https://sourceware.org/ml/libc-alpha/2017-04/msg00018.html [3] https://sourceware.org/ml/libc-alpha/2017-04/msg00019.html