Message ID | 53E36CD7.1000705@linaro.org |
---|---|
State | Committed |
Headers |
Received: (qmail 22656 invoked by alias); 7 Aug 2014 12:07:10 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <libc-alpha.sourceware.org> List-Unsubscribe: <mailto:libc-alpha-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:libc-alpha-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/libc-alpha/> List-Post: <mailto:libc-alpha@sourceware.org> List-Help: <mailto:libc-alpha-help@sourceware.org>, <http://sourceware.org/ml/#faqs> 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 Authentication-Results: sourceware.org; auth=none 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-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:message-id:date:from:user-agent:mime-version:to :cc:subject:references:in-reply-to:content-type :content-transfer-encoding; bh=oWj06NMaW36Da3Ypm5thBvqlBEsyp5bho+tsFPb2cGY=; b=kJPL29OgCnSz9lDN9W1s0e9HGC/41fx3rLd6P4GQKfdeDslHMjrbCsf38PWbplQjov o6pXu5LLgM0g9bD29pbRP4i3tc6CUQ6EjRmlXQE1HZ/4NWSc+1gJMKiDpaffmHgE5JIR Z+sFIvM+6638MZd4A9PA2IHm0KMZGLT1B+SRJRo0kTJ/xbRuOck9b1EUkJGTi1wh4tEo TT4OqELM8WpycYze3ICMTqW6jNn+vFOsWJvNkK2QM9gcbbTsXq/s4RKhX2HuZ5U6Flx4 Jn/F+aW4xKDMxEu8KjEi6NF9NoVwhuGhDdbj9BVtKLUOLJOTWKvMOadxG71ArwI8IoNt WZXg== X-Gm-Message-State: ALoCoQldbiSUATHZ8tag/QAxRhcM5EMO12TRJayH1deIsOEncq6ueuCLsqWcC7QAXDEFUfb7IXJT 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 <bernie.ogden@linaro.org> User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.0 MIME-Version: 1.0 To: Roland McGrath <roland@hack.frob.com> CC: "GNU C. Library" <libc-alpha@sourceware.org> 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> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit |
Commit Message
Bernard Ogden
Aug. 7, 2014, 12:11 p.m. UTC
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 <bernie.ogden@linaro.org> [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 <bernie.ogden@linaro.org> >> >> [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 >
Comments
On 7 August 2014 13:11, Bernard Ogden <bernie.ogden@linaro.org> wrote: > Changelog fixed (provided my mail client is now behaving). It looks like your mail client may still be mangling the patch with line-wrapping. It is possible to get Thunderbird to stop doing that by setting the line length to something huge although I think its a hidden setting somewhere. Alternatively git send-email does the right thing too. The patch itself looks good to me though and it would be nice to get this into 2.20 particularly as it has regressed on MIPS. > 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 <bernie.ogden@linaro.org> > [BZ #16892] > * sysdeps/nptl/lowlevellock.h (__lll_timedlock): Use > atomic_compare_and_exchange_bool_acq rather than atomic_exchange_acq. > > --- > 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; \ > }) > > > 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 <bernie.ogden@linaro.org> >>> >>> [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 >>
The comment could certainly be more verbose about what the exact values and meanings are. But that can come with more commentary throughout the file.
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; \