This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCHv4] powerpc: Fix write-after-destroy in lock elision
- From: Torvald Riegel <triegel at redhat dot com>
- To: Tulio Magno Quites Machado Filho <tuliom at linux dot vnet dot ibm dot com>
- Cc: libc-alpha at sourceware dot org, adhemerval dot zanella at linaro dot org, raji at linux dot vnet dot ibm dot com, munroesj at linux dot vnet dot ibm dot com
- Date: Tue, 03 Jan 2017 14:02:36 +0100
- Subject: Re: [PATCHv4] powerpc: Fix write-after-destroy in lock elision
- Authentication-results: sourceware.org; auth=none
- References: <1483385613.13143.127.camel@redhat.com> <1483446882-2593-1-git-send-email-tuliom@linux.vnet.ibm.com>
On Tue, 2017-01-03 at 10:34 -0200, Tulio Magno Quites Machado Filho
wrote:
> Changes since v3:
> - Use atomics to update adapt_count in __lll_unlock_elision.
> - Improved the source code comments in __lll_unlock_elision to mention
> the mutex destroy requirements.
>
> Changes since v2:
> - Fixed coding style.
>
> Changes since v1:
> - Removed references to data race by the actual error: write-after-destroy.
> - Enclosed adapt_count accesses in C11 atomics.
>
> --- 8< ---
This is okay, thanks.
For the next time, or when you change non-arch-specific code, please
remember to document memory orders; a brief comment is often sufficient
but reveals the intent behind a specific bit of implementation. For
example:
> diff --git a/sysdeps/unix/sysv/linux/powerpc/elision-lock.c b/sysdeps/unix/sysv/linux/powerpc/elision-lock.c
> index 4589491..044803a 100644
> --- a/sysdeps/unix/sysv/linux/powerpc/elision-lock.c
> +++ b/sysdeps/unix/sysv/linux/powerpc/elision-lock.c
> @@ -45,7 +45,7 @@
> int
> __lll_lock_elision (int *lock, short *adapt_count, EXTRAARG int pshared)
> {
> - if (*adapt_count > 0)
> + if (atomic_load_relaxed (adapt_count) > 0)
Here, you could have just addded:
/* adapt_count is accessed concurrently but is just a hint. Thus,
use atomic accesses but relaxed MO is sufficient. */
This helps others to more quickly understand what was intended. This
isn't critical in this case because we have well-documented similar code
for other architectures, but generally it is better to document why a
choice was made if it is not immediately obvious, or cannot be checked
for consistency based on just local information (eg, the source code
lines around the statement in question); concurrent code obviously can't
be cross-checked with just local information...