This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH 3/4] S390: Use new __libc_tbegin_retry macro in elision-lock.c.
- From: Torvald Riegel <triegel at redhat dot com>
- To: Stefan Liebler <stli at linux dot vnet dot ibm dot com>
- Cc: libc-alpha at sourceware dot org
- Date: Tue, 10 Jan 2017 17:53:26 +0100
- Subject: Re: [PATCH 3/4] S390: Use new __libc_tbegin_retry macro in elision-lock.c.
- Authentication-results: sourceware.org; auth=none
- References: <1481032315-12420-1-git-send-email-stli@linux.vnet.ibm.com> <1481032315-12420-3-git-send-email-stli@linux.vnet.ibm.com>
On Tue, 2016-12-06 at 14:51 +0100, Stefan Liebler wrote:
> This patch implements __libc_tbegin_retry macro which is equivalent to
> gcc builtin __builtin_tbegin_retry, except the changes which were applied
> to __libc_tbegin in the previous patch.
>
> If tbegin aborts with _HTM_TBEGIN_TRANSIENT. Then this macros restores
> the fpc, fprs and automatically retries up to retry_cnt tbegins.
> Further saving of the state is omitted as it is already saved in the
> first round. Before retrying a further transaction, the
> transaction-abort-assist instruction is used to support the cpu.
This looks okay to me on the surface of it, but I don't know anything
about s390 asm.
> Use __libc_tbegin_retry macro.
> ---
> sysdeps/unix/sysv/linux/s390/elision-lock.c | 50 ++++++++++++++---------------
> sysdeps/unix/sysv/linux/s390/htm.h | 36 +++++++++++++++++++--
> 2 files changed, 58 insertions(+), 28 deletions(-)
>
> diff --git a/sysdeps/unix/sysv/linux/s390/elision-lock.c b/sysdeps/unix/sysv/linux/s390/elision-lock.c
> index 48cc3db..3dd7fbc 100644
> --- a/sysdeps/unix/sysv/linux/s390/elision-lock.c
> +++ b/sysdeps/unix/sysv/linux/s390/elision-lock.c
> @@ -60,17 +60,16 @@ __lll_lock_elision (int *futex, short *adapt_count, EXTRAARG int private)
> goto use_lock;
> }
>
> - int try_tbegin;
> - for (try_tbegin = aconf.try_tbegin;
> - try_tbegin > 0;
> - try_tbegin--)
> + if (aconf.try_tbegin > 0)
> {
> - int status;
> - if (__builtin_expect
> - ((status = __libc_tbegin ((void *) 0)) == _HTM_TBEGIN_STARTED, 1))
> + int status = __libc_tbegin_retry ((void *) 0, aconf.try_tbegin - 1);
Maybe add a comment that the macro that reminds the reader that the
macro expects a retry count, not how often a transaction should be
tried.
> + if (__builtin_expect (status == _HTM_TBEGIN_STARTED,
> + _HTM_TBEGIN_STARTED))
> {
> - if (*futex == 0)
> + if (__builtin_expect (*futex == 0, 1))
> + /* Lock was free. Return to user code in a transaction. */
> return 0;
> +
> /* Lock was busy. Fall back to normal locking. */
> if (__builtin_expect (__libc_tx_nesting_depth (), 1))
> {
> @@ -81,7 +80,6 @@ __lll_lock_elision (int *futex, short *adapt_count, EXTRAARG int private)
> See above for why relaxed MO is sufficient. */
> if (aconf.skip_lock_busy > 0)
> atomic_store_relaxed (adapt_count, aconf.skip_lock_busy);
> - goto use_lock;
> }
> else /* nesting depth is > 1 */
> {
> @@ -99,28 +97,28 @@ __lll_lock_elision (int *futex, short *adapt_count, EXTRAARG int private)
> __libc_tabort (_HTM_FIRST_USER_ABORT_CODE | 1);
> }
> }
> + else if (status != _HTM_TBEGIN_TRANSIENT)
> + {
> + /* A persistent abort (cc 1 or 3) indicates that a retry is
> + probably futile. Use the normal locking now and for the
> + next couple of calls.
> + Be careful to avoid writing to the lock. See above for why
> + relaxed MO is sufficient. */
> + if (aconf.skip_lock_internal_abort > 0)
> + atomic_store_relaxed (adapt_count,
> + aconf.skip_lock_internal_abort);
> + }
> else
> {
> - if (status != _HTM_TBEGIN_TRANSIENT)
> - {
> - /* A persistent abort (cc 1 or 3) indicates that a retry is
> - probably futile. Use the normal locking now and for the
> - next couple of calls.
> - Be careful to avoid writing to the lock. See above for why
> - relaxed MO is sufficient. */
> - if (aconf.skip_lock_internal_abort > 0)
> - atomic_store_relaxed (adapt_count,
> - aconf.skip_lock_internal_abort);
> - goto use_lock;
> - }
> + /* Same logic as above, but for for a number of temporary failures in
> + a row. */
for for
> + if (aconf.skip_lock_out_of_tbegin_retries > 0)
> + atomic_store_relaxed (adapt_count,
> + aconf.skip_lock_out_of_tbegin_retries);
> }
> }
>
> - /* Same logic as above, but for for a number of temporary failures in a
> - row. See above for why relaxed MO is sufficient. */
> - if (aconf.skip_lock_out_of_tbegin_retries > 0 && aconf.try_tbegin > 0)
> - atomic_store_relaxed (adapt_count, aconf.skip_lock_out_of_tbegin_retries);
> -
> use_lock:
> + /* Use normal locking as fallback path if transaction does not succeed. */
the transaction
> return LLL_LOCK ((*futex), private);
> }