From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 621 invoked by alias); 22 Nov 2016 18:03:47 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Received: (qmail 592 invoked by uid 89); 22 Nov 2016 18:03:46 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.6 required=5.0 tests=BAYES_00,KAM_LAZY_DOMAIN_SECURITY,RCVD_IN_DNSWL_LOW autolearn=no version=3.3.2 spammy=leg, prevents, unneeded, Doing X-HELO: mx0a-001b2d01.pphosted.com Subject: Re: BZ 20822 :powerpc: race condition in __lll_unlock_elision From: Steven Munroe Reply-To: munroesj@linux.vnet.ibm.com To: Torvald Riegel Cc: Rajalakshmi Srinivasaraghavan , libc-alpha@sourceware.org, aaron Sawdey , Ulrich Weigand , Steve Munroe , carlos@redhat.com, adhemerval.zanella@linaro.org, adconrad@ubuntu.com, wschmidt@linux.vnet.ibm.com In-Reply-To: <1479804279.7146.1280.camel@localhost.localdomain> References: <1479394010.7146.1154.camel@localhost.localdomain> <1479771736.9880.42.camel@oc7878010663> <1479804279.7146.1280.camel@localhost.localdomain> Content-Type: text/plain; charset="UTF-8" Date: Tue, 22 Nov 2016 18:03:00 -0000 Mime-Version: 1.0 Content-Transfer-Encoding: 7bit X-TM-AS-GCONF: 00 X-Content-Scanned: Fidelis XPS MAILER x-cbid: 16112218-8235-0000-0000-000009AAA151 X-IBM-SpamModules-Scores: X-IBM-SpamModules-Versions: BY=3.00006124; HX=3.00000240; KW=3.00000007; PH=3.00000004; SC=3.00000189; SDB=6.00783969; UDB=6.00378659; IPR=6.00561587; BA=6.00004902; NDR=6.00000001; ZLA=6.00000005; ZF=6.00000009; ZB=6.00000000; ZP=6.00000000; ZH=6.00000000; ZU=6.00000002; MB=3.00013408; XFM=3.00000011; UTC=2016-11-22 18:03:37 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 16112218-8236-0000-0000-000036AD4305 Message-Id: <1479837812.8455.18.camel@oc7878010663> X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2016-11-22_10:,, signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 spamscore=0 suspectscore=0 malwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1609300000 definitions=main-1611220314 X-SW-Source: 2016-11/txt/msg00803.txt.bz2 On Tue, 2016-11-22 at 09:44 +0100, Torvald Riegel wrote: > On Mon, 2016-11-21 at 17:42 -0600, Steven Munroe wrote: > > On Thu, 2016-11-17 at 15:46 +0100, Torvald Riegel wrote: > > > On Tue, 2016-11-15 at 21:42 +0530, Rajalakshmi Srinivasaraghavan wrote: > > > > The initial problem reported was memory corruption in MongoDB only on > > > > Ubuntu 16.04 ppc64le(not on Ubuntu 15). > > > > The problem was not easily recreatable and debugging with many tools and > > > > creative ideas, the problem is narrowed down to lock elision in glibc. > > > > Ubuntu 16.04 has glibc 2.23 with lock elision enabled by default which > > > > made the testcase fail only on 16.04. > > > > > > > > As stated in BZ 20822, short description is > > > > > > > > "The update of *adapt_count after the release of the lock causes a race > > > > condition. Thread A unlocks, thread B continues and returns, destroying > > > > mutex on stack, > > > > then gets into another function, thread A writes to *adapt_count and > > > > corrupts stack.The window is very narrow and requires that the > > > > machine be in smt mode, so likely the two threads have to be on the same > > > > core in order to hit this" > > > > > > > > Looking back the file changes in > > > > sysdeps/unix/sysv/linux/powerpc/elision-unlock.c, suspecting the commit > > > > https://sourceware.org/git/?p=glibc.git;a=commitdiff;h=fadd2ad9cc36115440d50b0eae9299e65988917d which is introduced to improve > > > > the performance. > > > > > > > > Thinking of the following fixes. > > > > > > > > 1) Update of adapt_count before the unlock. But I have still not > > > > identified if its going to affect the performance. > > > > > > Because of the requirements on mutex destruction (and same for rwlock > > > etc.), a thread must not access any data associated with the mutex after > > > having unlocked it (this includes reads because it's correct to also > > > unmap the memory holding the mutex after destroying the mutex). > > > > > > > 2) In elision-lock.c/elision-trylock.c update adapt_count right after > > > > the lock acquire at the end of the function. > > > > In either case we have to be careful not to introduce another race > > > > condition. > > > > > > Every access to mutex data outside of transactions must use atomic > > > operations if there would be data races otherwise (according to the C11 > > > memory model). I know that this isn't followed in the current mutex > > > code, but it really should be, and your change qualifies as new code so > > > please take care of this too. > > > > > In this case (with the proposed patch) the adapt_count update is within > > the critical region and adding C11 atomics does not add value and would > > generally make the logical harder to read. > > > > __atomic_store_n (adapt_count, (__atomic_load_n(adapt_count, > > __ATOMIC_RELAXED) - 1), __ATOMIC_RELAXED) > > > > Is a lot of syntax without any real value. > > First, it would actually be this in glibc: > atomic_store_relaxed (&adapt_count, > atomic_load_relaxed(&adapt_count) - 1); > This is not a good example for you case. The proposed patch: - lll_unlock ((*lock), pshared); - - /* Update the adapt count AFTER completing the critical section. - Doing this here prevents unneeded stalling when entering - a critical section. Saving about 8% runtime on P8. */ + /* Update the adapt count in the critical section to + prevent race condition as mentioned in BZ 20822. */ if (*adapt_count > 0) (*adapt_count)--; + lll_unlock ((*lock), pshared); makes it clear that this test and update of the adapt_count is contained with the acquire/release semantics of the lll_lock and lll_unlock. Also if we add the explicit syntax you propose, someone, who has not studied the architecture, might leap to the conclusion that add_fetch would be even better. Which would be absolutely wrong. This would generate a larx-add-stcx sequence which has two very bad side-effects. 1) on the fall back leg, it would a force a larx-hit-stcx flush hazard with the following lll_unlock and cause bad performance. 2) on the transaction leg, it would be redundant and just slow things down. There are some other cases, like the if test at the start of __lll_lock_elision where it might be appropriate. > Second, it does add value in that it makes it clear that there are > concurrent accesses elsewhere that are not in transactions. We have > consensus to use the new C11-like atomics in new or changed code, and I > don't see a reason to make an exception here. >