| Message ID | 1312369498.3533.235.camel@triegel.csb |
|---|---|
| State | New |
| Headers | show |
On 08/03/2011 04:04 AM, Torvald Riegel wrote: > Move local_tid from gtm_thread to gtm_transaction. > > * config/generic/tls.h (gtm_thread): Move local_tid from here ... > * libitm_i.h (local_tid): ... to here. > * beginend.cc (GTM::gtm_transaction::begin_transaction): Same. > (GTM::gtm_transaction::operator new): Set up gtm_thread if necessary. This seems very wrong. Why? > Fix missing setup of gtm_thread if no transaction has run yet. > > * query.cc (_ITM_getThreadnum): Set up gtm_thread if necessary. > * testsuite/libitm.c/notx.c: New file. Ok. r~
On Thu, 2011-08-04 at 08:43 -0700, Richard Henderson wrote: > On 08/03/2011 04:04 AM, Torvald Riegel wrote: > > Move local_tid from gtm_thread to gtm_transaction. > > > > * config/generic/tls.h (gtm_thread): Move local_tid from here ... > > * libitm_i.h (local_tid): ... to here. > > * beginend.cc (GTM::gtm_transaction::begin_transaction): Same. > > (GTM::gtm_transaction::operator new): Set up gtm_thread if necessary. > > This seems very wrong. Why? What seems wrong? local_tid is per thread and there is one gtm_transaction object per thread, so moving it is correct, or not? The purpose of this is to not having to access gtm_thread anymore in begin. And it's a step towards merging gtm_thread and gtm_transaction completely. Do you agree?
On 08/04/2011 09:22 AM, Torvald Riegel wrote: > On Thu, 2011-08-04 at 08:43 -0700, Richard Henderson wrote: >> On 08/03/2011 04:04 AM, Torvald Riegel wrote: >>> Move local_tid from gtm_thread to gtm_transaction. >>> >>> * config/generic/tls.h (gtm_thread): Move local_tid from here ... >>> * libitm_i.h (local_tid): ... to here. >>> * beginend.cc (GTM::gtm_transaction::begin_transaction): Same. >>> (GTM::gtm_transaction::operator new): Set up gtm_thread if necessary. >> >> This seems very wrong. Why? > > What seems wrong? > > local_tid is per thread and there is one gtm_transaction object per > thread, so moving it is correct, or not? > The purpose of this is to not having to access gtm_thread anymore in > begin. And it's a step towards merging gtm_thread and gtm_transaction > completely. > > Do you agree? If you're going to merge gtm_thread and gtm_transaction, why are you moving things away from gtm_thread? As for "not having to access gtm_thread", the non-glibc case for accessing gtm_txn is to pull the value out of gtm_thread. So I think you're moving data in the wrong direction. r~
On Thu, 2011-08-04 at 11:55 -0700, Richard Henderson wrote: > On 08/04/2011 09:22 AM, Torvald Riegel wrote: > > On Thu, 2011-08-04 at 08:43 -0700, Richard Henderson wrote: > >> On 08/03/2011 04:04 AM, Torvald Riegel wrote: > >>> Move local_tid from gtm_thread to gtm_transaction. > >>> > >>> * config/generic/tls.h (gtm_thread): Move local_tid from here ... > >>> * libitm_i.h (local_tid): ... to here. > >>> * beginend.cc (GTM::gtm_transaction::begin_transaction): Same. > >>> (GTM::gtm_transaction::operator new): Set up gtm_thread if necessary. > >> > >> This seems very wrong. Why? > > > > What seems wrong? > > > > local_tid is per thread and there is one gtm_transaction object per > > thread, so moving it is correct, or not? > > The purpose of this is to not having to access gtm_thread anymore in > > begin. And it's a step towards merging gtm_thread and gtm_transaction > > completely. > > > > Do you agree? > > If you're going to merge gtm_thread and gtm_transaction, why > are you moving things away from gtm_thread? I believe the per-thread object should be gtm_transaction, not gtm_thread. Regarding the name, "transaction" makes more sense to me because it has almost no uses besides transaction-related stuff. Second, I guess we'd have to keep it in libitm_i.h anyway, and not in tls.h. Therefore, it seems to be the right thing to do to put it together with the related stuff, even if we rename gtm_transaction to gtm_thread later, or move it out of libitm_i.h (why would we? where to?). > As for "not having to access gtm_thread", the non-glibc case > for accessing gtm_txn is to pull the value out of gtm_thread. I'm aware of that, but non-glibc is not the common case we're optimizing for. If so, I believe we'd have kept the tx param in all the _ITM_* load and store functions. Either way, when gtm_thread and gtm_transaction have been merged (and I'm thinking about merging into gtm_transaction here...), this issue should also have gone away. I'm planning to come up with a separate patch for the actual merge. I first need to think a bit more about whether the destruction can be done with just a destructor of the TLS object, what happens when other object's destructors use transactions, etc. Even with one txn per thread, we still need to deregister it and release its dynamically allocated memory. Perhaps adding a destruction-phase mode would help, in which transactions are always destructed after they've run. Torvald
On 08/04/2011 12:24 PM, Torvald Riegel wrote: > On Thu, 2011-08-04 at 11:55 -0700, Richard Henderson wrote: >> On 08/04/2011 09:22 AM, Torvald Riegel wrote: >>> On Thu, 2011-08-04 at 08:43 -0700, Richard Henderson wrote: >>>> On 08/03/2011 04:04 AM, Torvald Riegel wrote: >>>>> Move local_tid from gtm_thread to gtm_transaction. >>>>> >>>>> * config/generic/tls.h (gtm_thread): Move local_tid from here ... >>>>> * libitm_i.h (local_tid): ... to here. >>>>> * beginend.cc (GTM::gtm_transaction::begin_transaction): Same. >>>>> (GTM::gtm_transaction::operator new): Set up gtm_thread if necessary. >>>> >>>> This seems very wrong. Why? >>> >>> What seems wrong? >>> >>> local_tid is per thread and there is one gtm_transaction object per >>> thread, so moving it is correct, or not? >>> The purpose of this is to not having to access gtm_thread anymore in >>> begin. And it's a step towards merging gtm_thread and gtm_transaction >>> completely. >>> >>> Do you agree? >> >> If you're going to merge gtm_thread and gtm_transaction, why >> are you moving things away from gtm_thread? > > I believe the per-thread object should be gtm_transaction, not > gtm_thread. I don't. For the simple fact that *all* per-thread information should be in gtm_thread, not just some of it. If we ever have any non-transaction per-thread information, then it should go into gtm_thread. Suppose we did eliminate everything not directly related to the current transaction from gtm_thread, I think it would look like struct gtm_thread { gtm_transaction txn; }; But even that said, the specific case of local_tid seems like exactly the sort of information that should remain in gtm_thread. It's *not* related to the current transaction, it's related to the current thread. >> As for "not having to access gtm_thread", the non-glibc case >> for accessing gtm_txn is to pull the value out of gtm_thread. > > I'm aware of that, but non-glibc is not the common case we're optimizing > for. If so, I believe we'd have kept the tx param in all the _ITM_* load > and store functions. But what you're suggesting is intentionally making non-glibc worse for no good reason. I, and probably others, take issue with that. r~
On Thu, 2011-08-04 at 12:35 -0700, Richard Henderson wrote: > On 08/04/2011 12:24 PM, Torvald Riegel wrote: > > On Thu, 2011-08-04 at 11:55 -0700, Richard Henderson wrote: > >> On 08/04/2011 09:22 AM, Torvald Riegel wrote: > >>> On Thu, 2011-08-04 at 08:43 -0700, Richard Henderson wrote: > >>>> On 08/03/2011 04:04 AM, Torvald Riegel wrote: > >>>>> Move local_tid from gtm_thread to gtm_transaction. > >>>>> > >>>>> * config/generic/tls.h (gtm_thread): Move local_tid from here ... > >>>>> * libitm_i.h (local_tid): ... to here. > >>>>> * beginend.cc (GTM::gtm_transaction::begin_transaction): Same. > >>>>> (GTM::gtm_transaction::operator new): Set up gtm_thread if necessary. > >>>> > >>>> This seems very wrong. Why? > >>> > >>> What seems wrong? > >>> > >>> local_tid is per thread and there is one gtm_transaction object per > >>> thread, so moving it is correct, or not? > >>> The purpose of this is to not having to access gtm_thread anymore in > >>> begin. And it's a step towards merging gtm_thread and gtm_transaction > >>> completely. > >>> > >>> Do you agree? > >> > >> If you're going to merge gtm_thread and gtm_transaction, why > >> are you moving things away from gtm_thread? > > > > I believe the per-thread object should be gtm_transaction, not > > gtm_thread. > > I don't. For the simple fact that *all* per-thread information > should be in gtm_thread, not just some of it. If we ever have > any non-transaction per-thread information, then it should go > into gtm_thread. > > Suppose we did eliminate everything not directly related to the > current transaction from gtm_thread, I think it would look like > > struct gtm_thread > { > gtm_transaction txn; > }; > > But even that said, the specific case of local_tid seems like > exactly the sort of information that should remain in gtm_thread. > It's *not* related to the current transaction, it's related to > the current thread. As discussed off-list, local_tid can be moved into the transaction. The question of whether and how to merge gtm_thread and gtm_transaction is separate from that. I have provided a first draft for that merge in my previous email to the list. > >> As for "not having to access gtm_thread", the non-glibc case > >> for accessing gtm_txn is to pull the value out of gtm_thread. > > > > I'm aware of that, but non-glibc is not the common case we're optimizing > > for. If so, I believe we'd have kept the tx param in all the _ITM_* load > > and store functions. > > But what you're suggesting is intentionally making non-glibc worse > for no good reason. I, and probably others, take issue with that. That was a misunderstanding/non-issue, but it also considered in the recent merge patch. Torvald
diff --git a/libitm/beginend.cc b/libitm/beginend.cc index d336d60..a16c3f9 100644 --- a/libitm/beginend.cc +++ b/libitm/beginend.cc @@ -43,7 +43,7 @@ static _ITM_transactionId_t global_tid; void * GTM::gtm_transaction::operator new (size_t s) { - gtm_thread *thr = gtm_thr (); + gtm_thread *thr = setup_gtm_thr (); void *tx; assert(s == sizeof(gtm_transaction)); @@ -112,8 +112,6 @@ GTM::gtm_transaction::begin_transaction (uint32_t prop, const gtm_jmpbuf *jb) if (unlikely(prop & pr_undoLogCode)) GTM_fatal("pr_undoLogCode not supported"); - gtm_thread *thr = setup_gtm_thr (); - tx = gtm_tx(); if (tx == NULL) { @@ -214,18 +212,18 @@ GTM::gtm_transaction::begin_transaction (uint32_t prop, const gtm_jmpbuf *jb) // As long as we have not exhausted a previously allocated block of TIDs, // we can avoid an atomic operation on a shared cacheline. - if (thr->local_tid & (tid_block_size - 1)) - tx->id = thr->local_tid++; + if (tx->local_tid & (tid_block_size - 1)) + tx->id = tx->local_tid++; else { #ifdef HAVE_64BIT_SYNC_BUILTINS tx->id = __sync_add_and_fetch (&global_tid, tid_block_size); - thr->local_tid = tx->id + 1; + tx->local_tid = tx->id + 1; #else pthread_mutex_lock (&global_tid_lock); global_tid += tid_block_size; tx->id = global_tid; - thr->local_tid = tx->id + 1; + tx->local_tid = tx->id + 1; pthread_mutex_unlock (&global_tid_lock); #endif } diff --git a/libitm/config/generic/tls.h b/libitm/config/generic/tls.h index 82d7c66..8a39b80 100644 --- a/libitm/config/generic/tls.h +++ b/libitm/config/generic/tls.h @@ -50,12 +50,6 @@ struct gtm_thread void *free_tx[MAX_FREE_TX]; unsigned free_tx_idx, free_tx_count; - // In order to reduce cacheline contention on global_tid during - // beginTransaction, we allocate a block of 2**N ids to the thread - // all at once. This number is the next value to be allocated from - // the block, or 0 % 2**N if no such block is allocated. - _ITM_transactionId_t local_tid; - // The value returned by _ITM_getThreadnum to identify this thread. // ??? At present, this is densely allocated beginning with 1 and // we don't bother filling in this value until it is requested. diff --git a/libitm/libitm_i.h b/libitm/libitm_i.h index 97f535a..7d475be 100644 --- a/libitm/libitm_i.h +++ b/libitm/libitm_i.h @@ -168,6 +168,12 @@ struct gtm_transaction // A bitmask of the above. uint32_t state; + // In order to reduce cacheline contention on global_tid during + // beginTransaction, we allocate a block of 2**N ids to the thread + // all at once. This number is the next value to be allocated from + // the block, or 0 % 2**N if no such block is allocated. + _ITM_transactionId_t local_tid; + // Data used by eh_cpp.c for managing exceptions within the transaction. uint32_t cxa_catch_count; void *cxa_unthrown;
patch3: After the previous change to flat nesting by default, checkpoints, and thus having only a single gtm_transaction object per thread, we can move the local_tid field to gtm_transaction too. As a result, we don't have to access gtm_thread in GTM_begin_transaction anymore. patch4: A small fix that adds a missing set-up call for gtm_thread. And a matching test case, which tests that the functions in query.cc work even if no transaction has been executed so far in this thread. OK for branch? commit 33c3c869c7cb986bcca3f3f44b0412278e880a82 Author: Torvald Riegel <triegel@redhat.com> Date: Fri Jul 29 16:09:05 2011 +0200 Move local_tid from gtm_thread to gtm_transaction. * config/generic/tls.h (gtm_thread): Move local_tid from here ... * libitm_i.h (local_tid): ... to here. * beginend.cc (GTM::gtm_transaction::begin_transaction): Same. (GTM::gtm_transaction::operator new): Set up gtm_thread if necessary. commit 22c19978ba11c0ae0321449d8809260ab5f97741 Author: Torvald Riegel <triegel@redhat.com> Date: Fri Jul 29 16:19:41 2011 +0200 Fix missing setup of gtm_thread if no transaction has run yet. * query.cc (_ITM_getThreadnum): Set up gtm_thread if necessary. * testsuite/libitm.c/notx.c: New file. diff --git a/libitm/query.cc b/libitm/query.cc index 9a95211..02f1813 100644 --- a/libitm/query.cc +++ b/libitm/query.cc @@ -67,7 +67,7 @@ int ITM_REGPARM _ITM_getThreadnum (void) { static int global_num; - struct gtm_thread *thr = gtm_thr(); + struct gtm_thread *thr = setup_gtm_thr(); int num = thr->thread_num; if (num == 0) diff --git a/libitm/testsuite/libitm.c/notx.c b/libitm/testsuite/libitm.c/notx.c new file mode 100644 index 0000000..92ae3e1 --- /dev/null +++ b/libitm/testsuite/libitm.c/notx.c @@ -0,0 +1,36 @@ +/* These tests all check whether initialization happens properly even if no + transaction has been used in the current thread yet. */ +#include <stdlib.h> +#include <pthread.h> +#include <libitm.h> + +static void *test1 (void *dummy __attribute__((unused))) +{ + if (_ITM_inTransaction() != outsideTransaction) + abort(); + return NULL; +} + +static void *test2 (void *dummy __attribute__((unused))) +{ + if (_ITM_getTransactionId() != _ITM_noTransactionId) + abort(); + return NULL; +} + + +int main() +{ + pthread_t thread; + + if (_ITM_getThreadnum() != 1) + abort(); + + pthread_create(&thread, NULL, test1, NULL); + pthread_join(thread, NULL); + + pthread_create(&thread, NULL, test2, NULL); + pthread_join(thread, NULL); + + return 0; +}