[Bug nptl/15640] New: lll_unlock uses atomic compare and swap to release a lock

[Bug nptl/15640] New: lll_unlock uses atomic compare and swap to release a lock

[adeb at nvidia dot com]

http://sourceware.org/bugzilla/show_bug.cgi?id=15640

            Bug ID: 15640
           Summary: lll_unlock uses atomic compare and swap to release a
                    lock
           Product: glibc
           Version: unspecified
            Status: NEW
          Severity: enhancement
          Priority: P2
         Component: nptl
          Assignee: unassigned at sourceware dot org
          Reporter: adeb at nvidia dot com
                CC: drepper.fsp at gmail dot com

The arm implementation of lll_unlock uses atomic compare and swap to release a
lock. This is because the macro atomic_exchange_rel, in include/atomic.h, gets
defined to atomic_exchange_acq, which uses atomic compare and swap. 

Releasing a lock would simply require a dmb followed by a simple store. The
benefit would be a bit of performance, in the case when a thread A running on
core 0 trying to acquire a lock competes with a thread B running on core 1
trying to release a lock. Since releasing a lock is currently an atomic compare
and swap, it may fail dut to contention with thread A. However, thread A can't
enter critical section unless thread B releases the lock. 

In the current implementation of lock acquisition the atomic compare and swap
sequence is repeated twice before making a futex wait system call. Hence, in
the worst case lock release could fail atmost twice.

If on the other hand, a dmb followed by a normal store sequence had been used
for releasing the lock. Then the store being a normal store would always
succeed. However, even in this case the line could be snooped away by another
core, but that would result in the store being visible a little later.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug nptl/15640] The ARM port of lll_unlock uses atomic compare and swap to release a lock

[adeb at nvidia dot com]

http://sourceware.org/bugzilla/show_bug.cgi?id=15640

Abhishek Deb <adeb at nvidia dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|lll_unlock uses atomic      |The ARM port of lll_unlock
                   |compare and swap to release |uses atomic compare and
                   |a lock                      |swap to release a lock

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug nptl/15640] The ARM port of lll_unlock uses atomic compare and swap to release a lock

[adeb at nvidia dot com]

http://sourceware.org/bugzilla/show_bug.cgi?id=15640

Abhishek Deb <adeb at nvidia dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |adeb at nvidia dot com

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug nptl/15640] The ARM port of lll_unlock uses atomic compare and swap to release a lock

[chris_s_jones at yahoo dot com]

http://sourceware.org/bugzilla/show_bug.cgi?id=15640

Chris <chris_s_jones at yahoo dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |chris_s_jones at yahoo dot com

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug nptl/15640] The ARM port of lll_unlock uses atomic compare and swap to release a lock

[dtemirbulatov at gmail dot com]

http://sourceware.org/bugzilla/show_bug.cgi?id=15640

Dinar Temirbulatov <dtemirbulatov at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
                 CC|                            |dtemirbulatov at gmail dot com
         Resolution|---                         |INVALID

--- Comment #1 from Dinar Temirbulatov <dtemirbulatov at gmail dot com> ---
This report looks invalid to me. The lll_unlock could not be done without
proper atomic operation atomic_exchange_rel. Here is the use-case:

thread A has the lock and wants to release it with lll_unlock
thread B is about to acquire the lock
thread A does READ, sets oldval to "1"
thread B call lll_lock, sets *__futex to 2 and goes to sleep.
thread A does DMB, sets *__futex to "0"
thread A does NOT call lll_futex_wake because oldval is "1"
thread B sleeps forever.
bug

Resolving bug as INVALID.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug nptl/15640] The ARM port of lll_unlock uses atomic compare and swap to release a lock

[adeb at nvidia dot com]

http://sourceware.org/bugzilla/show_bug.cgi?id=15640

--- Comment #2 from Abhishek Deb <adeb at nvidia dot com> ---
On second thoughts, I think what is required is an atomic_exchange_rel and not
atomic_compare_and_exchange. Though lll_unlock uses atomic_exchange_rel, but
because atomic_exchange_rel was never defined for ARM, it gets defined to
atomic_compare_and_exhange_rel. I think atomic_exchange_rel could perhaps be
implemented with a pair of ldex, stex and a branch if the atomic exchange
failed.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug nptl/15640] The ARM port of lll_unlock uses atomic compare and swap to release a lock

[adeb at nvidia dot com]

http://sourceware.org/bugzilla/show_bug.cgi?id=15640

Abhishek Deb <adeb at nvidia dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |REOPENED
         Resolution|INVALID                     |---

--- Comment #3 from Abhishek Deb <adeb at nvidia dot com> ---
Refer to the Comment 2

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug nptl/15640] The ARM port of lll_unlock uses atomic compare and swap to release a lock

[dtemirbulatov at gmail dot com]

http://sourceware.org/bugzilla/show_bug.cgi?id=15640

--- Comment #5 from Dinar Temirbulatov <dtemirbulatov at gmail dot com> ---
s/yes, This already/yes, But this already

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug nptl/15640] The ARM port of lll_unlock uses atomic compare and swap to release a lock

[adeb at nvidia dot com]

http://sourceware.org/bugzilla/show_bug.cgi?id=15640

Abhishek Deb <adeb at nvidia dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |REOPENED
         Resolution|INVALID                     |---

--- Comment #6 from Abhishek Deb <adeb at nvidia dot com> ---
Since the preprocessor macro atomic_exchange_rel was not explicity defined, it
gets defined to atomic_compare_and_exhange_*, which is defined using gcc's
atomic built-in __sync_val_compare_and_swap.

But I think atomic exchange is not the same as atomic compare and exchange. An
atomic exchange should be defined as ldex, stex, bne, whereas atomic compare
and exchange is defined as ldex, cmp, bne, stex, bne. 

Doesn't look like gcc's atomic builtin in provides an explicit function to do
that http://gcc.gnu.org/onlinedocs/gcc-4.6.2/gcc/Atomic-Builtins.html.

However, gcc's atomic built in provides __sync_fetch_and_* . Can it be used to
build an atomic exchange.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug nptl/15640] The ARM port of lll_unlock uses atomic compare and swap to release a lock

[dtemirbulatov at gmail dot com]

http://sourceware.org/bugzilla/show_bug.cgi?id=15640

--- Comment #7 from Dinar Temirbulatov <dtemirbulatov at gmail dot com> ---
(In reply to Abhishek Deb from comment #6)
> Since the preprocessor macro atomic_exchange_rel was not explicity defined,
> it gets defined to atomic_compare_and_exhange_*, which is defined using
> gcc's atomic built-in __sync_val_compare_and_swap.
> 
> But I think atomic exchange is not the same as atomic compare and exchange.
> An atomic exchange should be defined as ldex, stex, bne, whereas atomic
> compare and exchange is defined as ldex, cmp, bne, stex, bne. 
> 
> Doesn't look like gcc's atomic builtin in provides an explicit function to
> do that http://gcc.gnu.org/onlinedocs/gcc-4.6.2/gcc/Atomic-Builtins.html.
> 
> However, gcc's atomic built in provides __sync_fetch_and_* . Can it be used
> to build an atomic exchange.

yes, All correct, except I think that it needs cmp after stex.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug nptl/15640] The ARM port of lll_unlock uses atomic compare and swap to release a lock

[maxim.kuvyrkov at gmail dot com]

http://sourceware.org/bugzilla/show_bug.cgi?id=15640

Maxim Kuvyrkov <maxim.kuvyrkov at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|REOPENED                    |RESOLVED
                 CC|                            |maxim.kuvyrkov at gmail dot com
         Resolution|---                         |FIXED

--- Comment #8 from Maxim Kuvyrkov <maxim.kuvyrkov at gmail dot com> ---
Fixed by Dinar in d70d6205fabf863ce18e53d49f9d83f5f16c5fee .

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug nptl/15640] The ARM port of lll_unlock uses atomic compare and swap to release a lock

[fweimer at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=15640

Florian Weimer <fweimer at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
              Flags|                            |security-

-- 
You are receiving this mail because:
You are on the CC list for the bug.