From patchwork Thu Aug 7 12:11:03 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Bernie Ogden X-Patchwork-Id: 35036 Return-Path: X-Original-To: linaro@patches.linaro.org Delivered-To: linaro@patches.linaro.org Received: from mail-pd0-f200.google.com (mail-pd0-f200.google.com [209.85.192.200]) by ip-10-151-82-157.ec2.internal (Postfix) with ESMTPS id 17B8821137 for ; Thu, 7 Aug 2014 12:07:22 +0000 (UTC) Received: by mail-pd0-f200.google.com with SMTP id w10sf23359804pde.11 for ; Thu, 07 Aug 2014 05:07:21 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:delivered-to:mailing-list:precedence:list-id :list-unsubscribe:list-subscribe:list-archive:list-post:list-help :sender:delivered-to:message-id:date:from:user-agent:mime-version:to :cc:subject:references:in-reply-to:x-original-sender :x-original-authentication-results:content-type :content-transfer-encoding; bh=oWj06NMaW36Da3Ypm5thBvqlBEsyp5bho+tsFPb2cGY=; b=VvxJ98nwqB3a15Ijt+R45QBOYBQitTx9hKtLTH6MH3ZC7CFrD6iFqsNp8hglFHiwnq iWlVS3KXZlAlmxocX4Ox86YEbhLmcQ8eCddW/qSfaFeBMuD+rrXyhlLJZiygrE9zibuJ sezMV2IPZooZgjhJ/YsqlyRbRqdV5KSDAMCbsJNoTxo1qbea67asamOYid5MlD8TXnzD 5DWwe19xpERQezxntMubwZFDeqS1ptAn5M5QDtr+7sDs2EXjH8AsTWdjAtnOk0/MkZFH tVlUuqhGGhMUacHTavExZO/pD3FU4Xu/w2OmrSH0402da0gU7jf8sPaK849e4weLnKk+ 10/g== X-Gm-Message-State: ALoCoQkkFdK7nlCmpkT7dBcl6LlPN7xkyglcS9Pp/6oDD9InSe3lj1KKn7PmdUDcyn9JizYFLfqu X-Received: by 10.66.147.227 with SMTP id tn3mr9381074pab.4.1407413241399; Thu, 07 Aug 2014 05:07:21 -0700 (PDT) X-BeenThere: patchwork-forward@linaro.org Received: by 10.140.89.231 with SMTP id v94ls122615qgd.14.gmail; Thu, 07 Aug 2014 05:07:21 -0700 (PDT) X-Received: by 10.220.172.134 with SMTP id l6mr16299vcz.80.1407413241304; Thu, 07 Aug 2014 05:07:21 -0700 (PDT) Received: from mail-vc0-x22c.google.com (mail-vc0-x22c.google.com [2607:f8b0:400c:c03::22c]) by mx.google.com with ESMTPS id d7si1685432vcy.33.2014.08.07.05.07.21 for (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Thu, 07 Aug 2014 05:07:21 -0700 (PDT) Received-SPF: pass (google.com: domain of patch+caf_=patchwork-forward=linaro.org@linaro.org designates 2607:f8b0:400c:c03::22c as permitted sender) client-ip=2607:f8b0:400c:c03::22c; Received: by mail-vc0-f172.google.com with SMTP id im17so6089113vcb.31 for ; Thu, 07 Aug 2014 05:07:21 -0700 (PDT) X-Received: by 10.220.131.207 with SMTP id y15mr24298vcs.71.1407413241222; Thu, 07 Aug 2014 05:07:21 -0700 (PDT) X-Forwarded-To: patchwork-forward@linaro.org X-Forwarded-For: patch@linaro.org patchwork-forward@linaro.org Delivered-To: patch@linaro.org Received: by 10.221.37.5 with SMTP id tc5csp24152vcb; Thu, 7 Aug 2014 05:07:20 -0700 (PDT) X-Received: by 10.70.108.164 with SMTP id hl4mr17435956pdb.70.1407413239983; Thu, 07 Aug 2014 05:07:19 -0700 (PDT) Received: from sourceware.org (server1.sourceware.org. [209.132.180.131]) by mx.google.com with ESMTPS id be10si3337336pad.105.2014.08.07.05.07.19 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 07 Aug 2014 05:07:19 -0700 (PDT) Received-SPF: pass (google.com: domain of libc-alpha-return-52107-patch=linaro.org@sourceware.org designates 209.132.180.131 as permitted sender) client-ip=209.132.180.131; Received: (qmail 22656 invoked by alias); 7 Aug 2014 12:07:10 -0000 Mailing-List: list patchwork-forward@linaro.org; contact patchwork-forward+owners@linaro.org Precedence: list List-Id: List-Unsubscribe: , List-Subscribe: List-Archive: List-Post: , List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 22646 invoked by uid 89); 7 Aug 2014 12:07:09 -0000 X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.6 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_LOW, SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-wg0-f52.google.com X-Received: by 10.180.104.42 with SMTP id gb10mr57340080wib.65.1407413224465; Thu, 07 Aug 2014 05:07:04 -0700 (PDT) Message-ID: <53E36CD7.1000705@linaro.org> Date: Thu, 07 Aug 2014 13:11:03 +0100 From: Bernard Ogden User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.0 MIME-Version: 1.0 To: Roland McGrath CC: "GNU C. Library" Subject: Re: [PATCH][BZ #16892] Check value of futex before updating in __lll_timedlock References: <1D01BCB7-8B02-415F-9244-58C15296B799@linaro.org> <20140806212742.1092C2C3981@topped-with-meat.com> In-Reply-To: <20140806212742.1092C2C3981@topped-with-meat.com> X-Original-Sender: bernie.ogden@linaro.org X-Original-Authentication-Results: mx.google.com; spf=pass (google.com: domain of patch+caf_=patchwork-forward=linaro.org@linaro.org designates 2607:f8b0:400c:c03::22c as permitted sender) smtp.mail=patch+caf_=patchwork-forward=linaro.org@linaro.org; dkim=pass header.i=@sourceware.org X-Google-Group-Id: 836684582541 Changelog fixed (provided my mail client is now behaving). I'm not sure how much detail to put into in the comment - is this better? I may attempt some comments in other part of the file when I've got a little time. 2014-08-07 Bernard Ogden [BZ #16892] * sysdeps/nptl/lowlevellock.h (__lll_timedlock): Use atomic_compare_and_exchange_bool_acq rather than atomic_exchange_acq. --- }) On 06/08/14 22:27, Roland McGrath wrote: >> This bug previously affected arm, aarch64, m68k and sh/sh4. Since mips >> switched to the generic lowlevellock.h, it is also affected. Applying >> this patch will fix arm, aarch64 and mips. m68k and sh would need to >> switch to the generic header to get the fix. > > m68k uses the generic code now. > >> Change is pretty simple, but has been built and tested on arm. > >> 2014-08-05 Bernard Ogden >> >> [BZ #16892] >> * sysdeps/nptl/lowlevellock.h: Use >> atomic_compare_and_exchange_bool_acq rather than atomic_exchange_acq >> in __lll_timedlock. > > This should look like: > > [BZ #16892] > * sysdeps/nptl/lowlevellock.h (__lll_timedlock): Use > atomic_compare_and_exchange_bool_acq rather than atomic_exchange_acq. > > 1. Use 1 tab to indent, not 8 spaces. > 2. Put the name of the affected macro/function/variable in parens after the > file name, not just in regular English. > >> --- a/sysdeps/nptl/lowlevellock.h >> +++ b/sysdeps/nptl/lowlevellock.h >> @@ -93,7 +93,8 @@ extern int __lll_robust_timedlock_wait (int *futex, const struct timespec *, >> int *__futex = (futex); \ >> int __val = 0; \ >> \ >> - if (__glibc_unlikely (atomic_exchange_acq (__futex, 1))) \ >> + if (__glibc_unlikely \ >> + (atomic_compare_and_exchange_bool_acq (__futex, 1, 0))) \ >> __val = __lll_timedlock_wait (__futex, abstime, private); \ >> __val; \ >> }) > > Please add a comment explaining what the code is doing. The rest of the > file needs more comments like this and I'm not asking you to add all those > (unless you want to!). But where you're touching it, and especially > replacing one magic atomic operation with a different magic atomic > operation to fix a bug, a comment is really warranted. If there had been a > good comment on the original code, it probably would have prevented us from > letting the bug go by in the first place. > > > Thanks, > Roland > diff --git a/sysdeps/nptl/lowlevellock.h b/sysdeps/nptl/lowlevellock.h index 548a9c8..28f4ba3 100644 --- a/sysdeps/nptl/lowlevellock.h +++ b/sysdeps/nptl/lowlevellock.h @@ -88,12 +88,15 @@ extern int __lll_timedlock_wait (int *futex, const struct timespec *, extern int __lll_robust_timedlock_wait (int *futex, const struct timespec *, int private) attribute_hidden; +/* Take futex if it is untaken. + Otherwise block until either we get the futex or we time out. */ #define __lll_timedlock(futex, abstime, private) \ ({ \ int *__futex = (futex); \ int __val = 0; \ \ - if (__glibc_unlikely (atomic_exchange_acq (__futex, 1))) \ + if (__glibc_unlikely \ + (atomic_compare_and_exchange_bool_acq (__futex, 1, 0))) \ __val = __lll_timedlock_wait (__futex, abstime, private); \ __val; \