Bug 15119 - [arm] unnecessary busy loop in __lll_timedlock_wait on ARM
Summary: [arm] unnecessary busy loop in __lll_timedlock_wait on ARM
Status: RESOLVED FIXED
Alias: None
Product: glibc
Classification: Unclassified
Component: nptl (show other bugs)
Version: unspecified
: P2 normal
Target Milestone: ---
Assignee: Bernard Ogden
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-02-08 00:00 UTC by UWATOKO Katsuki
Modified: 2014-06-13 18:53 UTC (History)
6 users (show)

See Also:
Host: arm*-*-linux*
Target:
Build:
Last reconfirmed:
fweimer: security-


Attachments
patch for Avoid unneecssary busy loop in __lll_timedlock_wait on ARM. (501 bytes, patch)
2013-02-08 00:00 UTC, UWATOKO Katsuki
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description UWATOKO Katsuki 2013-02-08 00:00:53 UTC
Created attachment 6856 [details]
patch for Avoid unneecssary busy loop in __lll_timedlock_wait on ARM.

The following sequence causes unnecessary busy loop. 

"A thread" gets the lock. (futex = 1)
"B thread" tries to get the lock, and has not called futex syscall yet. (futex = 2)
"A thread" releases the lock (futex = 0)
"C thread" gets the lock, and does not call futex syscall because of futex=0 (futex = 1)
"B thread" calls futex syscall, and returns with an error.
           Because futex syscall in Linux Kernel checks if the val is changed 
           from 2, which is the 3rd arg of the syscall at futex_wait_setup(),
           but in this case futex is 1.
"B thread" tries to get the lock in userspace but cannot get it 
           because futex is not 0.
           After all the thread keeps calling futex syscall 
           until "C thread" will release it (futex = 0) or timeout.

Therefore I think that the value should be set 2 in every loop 
the same as __lll_lock_wait_private, and attached a patch for this issue.
Comment 1 Maxim Kuvyrkov 2014-04-18 05:34:31 UTC
Bernie Ogden (bernie.ogden@linaro.org) is working on unifying ARM's and other architectures' lowlevellock.c to use generic linux version -- thus fixing this bug.

Assign to myself as Bernie doesn't have a Glibc account yet.
Comment 2 Bernard Ogden 2014-04-24 14:31:52 UTC
Analysis at https://sourceware.org/ml/libc-ports/2013-02/msg00021.html makes clear that switching to the generic lowlevellock.c will fix this problem for arm. Neither arm nor hppa appear to contain any significant differences from the generic lowlevellock.c, so we can eliminate both of these platform specific files.

This leaves sparc as the only platform with its own lowlevellock.c. In this case the locking structures are different, so we leave this one alone.
Comment 3 cvs-commit@gcc.gnu.org 2014-05-01 13:28:32 UTC
This is an automated email from the git hooks/post-receive script. It was
generated because a ref change was pushed to the repository containing
the project "GNU C Library master sources".

The branch, master has been updated
       via  6d96f5e4c0500b19d1c2f4edc37536d2bc592260 (commit)
      from  4fa262fa9e8836f2e513e3ea56797dab2d75e6de (commit)

Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.

- Log -----------------------------------------------------------------
https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=6d96f5e4c0500b19d1c2f4edc37536d2bc592260

commit 6d96f5e4c0500b19d1c2f4edc37536d2bc592260
Author: Will Newton <will.newton@linaro.org>
Date:   Thu May 1 14:25:44 2014 +0100

    ARM: Remove lowlevellock.c
    
    lowlevellock.c for arm differs from the generic lowlevellock.c only in
    insignificant ways, so can be removed. Happily, this fixes BZ 15119
    (unnecessary busy loop in __lll_timedlock_wait on arm).
    
    The notable differences between the arm and generic implementations are:
    
    1) arm __lll_timedlock_wait has a fast path out if futex has been set
    to 0 between since the function was called. This seems unlikely to
    happen very often, so it seems at worst harmless to lose this fast
    path.
    
    2) Some function in arm's lowlevellock.c set futex to 2 if it was 1.
    The generic version always sets the futex to 2. As futex can only be
    0, 1 or 2 on entry into these functions, the behaviour is equivalent.
    (If the futex manages to be 0 on entry then we've just lost another
    unlikely fast path out.)
    
    There are no test suite regressions.
    
    Note that hppa and sparc also have their own lowlevellock.c. I believe
    hppa can also be removed, so I'll send a separate patch for that
    shortly. sparc's seems to be genuinely needed as it uses a different
    locking structure.
    
    Also note that the analysis at
    https://sourceware.org/ml/libc-ports/2013-02/msg00021.html indicates a
    further locking performance bug to fix - I've got a partial patch for
    that which I can submit once I've finished testing.
    
    2014-05-01  Bernard Ogden <bernie.ogden@linaro.org>
    
    	[BZ #15119]
    	* sysdeps/unix/sysv/linux/arm/nptl/lowlevellock.c: Remove file.

-----------------------------------------------------------------------

Summary of changes:
 ChangeLog                                       |    6 +
 sysdeps/unix/sysv/linux/arm/nptl/lowlevellock.c |  132 -----------------------
 2 files changed, 6 insertions(+), 132 deletions(-)
 delete mode 100644 sysdeps/unix/sysv/linux/arm/nptl/lowlevellock.c
Comment 4 Will Newton 2014-05-01 15:28:20 UTC
Fixed in commit 6d96f5e4c0500b19d1c2f4edc37536d2bc592260
Comment 5 maxim.kuvyrkov 2014-05-02 01:36:28 UTC
On May 2, 2014, at 1:28 AM, cvs-commit at gcc dot gnu.org <sourceware-bugzilla@sourceware.org> wrote:

> 
> - Log -----------------------------------------------------------------
> https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=6d96f5e4c0500b19d1c2f4edc37536d2bc592260
> 
> commit 6d96f5e4c0500b19d1c2f4edc37536d2bc592260
> Author: Will Newton <will.newton@linaro.org>
> Date:   Thu May 1 14:25:44 2014 +0100
> 
>    ARM: Remove lowlevellock.c

Hi Will,

Please use --author= git commit option for future commits of patches from other developers.  Various commit-tracking services give credit to people based on Author tag.

Thank you,

--
Maxim Kuvyrkov
www.linaro.org
Comment 6 Will Newton 2014-05-02 07:48:07 UTC
On Fri, May 2, 2014 at 2:36 AM, maxim.kuvyrkov at linaro dot org
<sourceware-bugzilla@sourceware.org> wrote:
> https://sourceware.org/bugzilla/show_bug.cgi?id=15119
>
> --- Comment #5 from maxim.kuvyrkov at linaro dot org ---
> On May 2, 2014, at 1:28 AM, cvs-commit at gcc dot gnu.org
> <sourceware-bugzilla@sourceware.org> wrote:
>
>>
>> - Log -----------------------------------------------------------------
>> https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=6d96f5e4c0500b19d1c2f4edc37536d2bc592260
>>
>> commit 6d96f5e4c0500b19d1c2f4edc37536d2bc592260
>> Author: Will Newton <will.newton@linaro.org>
>> Date:   Thu May 1 14:25:44 2014 +0100
>>
>>    ARM: Remove lowlevellock.c
>
> Hi Will,
>
> Please use --author= git commit option for future commits of patches from other
> developers.  Various commit-tracking services give credit to people based on
> Author tag.

Hi Maxim,

Yes, I am aware of the oversight, I realised as soon as I had pushed
it. Unfortunately its an immutable part of the history and cannot be
changed now.
Comment 7 cvs-commit@gcc.gnu.org 2014-06-10 03:25:02 UTC
This is an automated email from the git hooks/post-receive script. It was
generated because a ref change was pushed to the repository containing
the project "GNU C Library master sources".

The branch, master has been updated
       via  2c3520d98d9773d9e092196c3c36150244eed1dd (commit)
      from  acaa4d24f507976d10a82dc31152eb912b59b4bc (commit)

Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.

- Log -----------------------------------------------------------------
https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=2c3520d98d9773d9e092196c3c36150244eed1dd

commit 2c3520d98d9773d9e092196c3c36150244eed1dd
Author: Bernard Ogden <bernie.ogden@linaro.org>
Date:   Mon Jun 9 23:22:20 2014 -0400

    hppa: Remove lowlevellock.c.
    
    The hppa port has no need of a custom lowlevellock.c, it should
    use the generic version which is updated and correct. This
    similarly fixes bug 15119 for hppa.

-----------------------------------------------------------------------

Summary of changes:
 ChangeLog                                        |    5 +
 sysdeps/unix/sysv/linux/hppa/nptl/lowlevellock.c |  126 ----------------------
 2 files changed, 5 insertions(+), 126 deletions(-)
 delete mode 100644 sysdeps/unix/sysv/linux/hppa/nptl/lowlevellock.c