public inbox for libc-alpha@sourceware.org
 help / color / mirror / Atom feed
From: Carlos O'Donell <carlos@redhat.com>
To: GNU C Library <libc-alpha@sourceware.org>,
	Torvald Riegel <triegel@redhat.com>
Subject: [PATCH] Fix x86 atomic_fetch_xor_release.
Date: Tue, 18 Oct 2016 03:19:00 -0000	[thread overview]
Message-ID: <1eeffe37-c4ec-fef7-537b-9ad17129b6c0@redhat.com> (raw)

Torvald,

No code uses atomic_fetch_xor_release except for the upcoming conditional
variable rewrite. Therefore there is no user visible bug here.

The use of atomic_compare_and_exchange_bool_rel is removed (since it doesn't
exist anymore), and is replaced by atomic_compare_exchange_weak_release.

We use weak_release because it provides better performance in the loop
(the weak semantic) and because the xor is release MO (the release semantic).

We don't reload expected in the loop because atomic_compare_and_exchange_weak_release
does this for us as part of the CAS failure.

It is otherwise a fairly plain conversion that fixes building the new condvar
for 32-bit x86.

I have pushed the new condvar into Fedora Rawhide for testing.

OK to checkin?

Cheers,
Carlos.

2016-10-17  Carlos O'Donell  <carlos@redhat.com>

	* include/atomic.h
	[USE_COMPILER_ATOMIC_BUILTINS && !atomic_fetch_xor_release]
	(atomic_fetch_xor_release): Use atomic_compare_exchange_weak_release.

Index: glibc-2.24-256-g5140d03/include/atomic.h
===================================================================
--- glibc-2.24-256-g5140d03.orig/include/atomic.h
+++ glibc-2.24-256-g5140d03/include/atomic.h
@@ -777,18 +777,22 @@ void __atomic_link_error (void);
 # endif

 # ifndef atomic_fetch_xor_release
+/* Failing the atomic_compare_exchange_weak_release reloads the value in
+   __atg104_expected, so we need only do the XOR again and retry.  */
 # define atomic_fetch_xor_release(mem, operand) \
-  ({ __typeof (*(mem)) __atg104_old;                                         \
-     __typeof (mem) __atg104_memp = (mem);                                   \
+  ({ __typeof (mem) __atg104_memp = (mem);                                   \
+     __typeof (*(mem)) __atg104_expected = (*__atg104_memp);                 \
+     __typeof (*(mem)) __atg104_desired;                                     \
      __typeof (*(mem)) __atg104_op = (operand);                                      \
                                                                              \
      do                                                                              \
-       __atg104_old = (*__atg104_memp);                                              \
-     while (__builtin_expect                                                 \
-           (atomic_compare_and_exchange_bool_rel (                           \
-               __atg104_memp, __atg104_old ^ __atg104_op, __atg104_old), 0));\
+       __atg104_desired = __atg104_expected ^ __atg104_op;                   \
+     while (__glibc_unlikely                                                 \
+           (atomic_compare_exchange_weak_release (                           \
+               __atg104_memp, &__atg104_expected, __atg104_desired)          \
+            == 0));                                                          \
                                                                              \
-     __atg104_old; })
+     __atg104_expected; })
 #endif

 #endif /* !USE_ATOMIC_COMPILER_BUILTINS  */
---

-- 
Cheers,
Carlos.

             reply	other threads:[~2016-10-18  3:19 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-10-18  3:19 Carlos O'Donell [this message]
2016-10-18 11:02 ` Torvald Riegel
2016-10-26  4:04   ` Carlos O'Donell

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1eeffe37-c4ec-fef7-537b-9ad17129b6c0@redhat.com \
    --to=carlos@redhat.com \
    --cc=libc-alpha@sourceware.org \
    --cc=triegel@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).