Horton

Re: Deadlock in XLogInsert at AIX

Konstantin Knizhnik <k.knizhnik@postgrespro.ru>

View thread View on postgr.es
From: Konstantin Knizhnik <k.knizhnik@postgrespro.ru>
To: pgsql-hackers@postgresql.org
Date: 2017-02-01T14:12:23Z
Lists: pgsql-hackers 
On 01.02.2017 15:39, Heikki Linnakangas wrote:
> On 02/01/2017 01:07 PM, Konstantin Knizhnik wrote:
>> Attached please find my patch for XLC/AIX.
>> The most critical fix is adding __sync to pg_atomic_fetch_add_u32_impl.
>> The comment in this file says that:
>>
>>        * __fetch_and_add() emits a leading "sync" and trailing "isync",
>> thereby
>>        * providing sequential consistency.  This is undocumented.
>>
>> But it is not true any more (I checked generated assembler code in
>> debugger).
>> This is why I have added __sync() to this function. Now pgbench working
>> normally.
>
> Seems like it was not so much undocumented, but an implementation 
> detail that was not guaranteed after all..
>
> Does __fetch_and_add emit a trailing isync there either? Seems odd if 
> __compare_and_swap requires it, but __fetch_and_add does not. Unless 
> we can find conclusive documentation on that, I think we should assume 
> that an __isync() is required, too.
>
> There was a long thread on these things the last time this was 
> changed: 
> https://www.postgresql.org/message-id/20160425185204.jrvlghn3jxulsb7i%40alap3.anarazel.de. 
> I couldn't find an explanation there of why we thought that 
> fetch_and_add implicitly performs sync and isync.
>
>> Also there is mysterious disappearance of assembler section function
>> with sync instruction from pg_atomic_compare_exchange_u32_impl.
>> I have fixed it by using __sync() built-in function instead.
>
> __sync() seems more appropriate there, anyway. We're using intrinsics 
> for all the other things in generic-xlc.h. But it sure is scary that 
> the "asm" sections just disappeared.
>
> In arch-ppc.h, shouldn't we have #ifdef __IBMC__ guards for the 
> __sync() and __lwsync() intrinsics? Those are an xlc compiler-specific 
> thing, right? Or if they are expected to work on any ppc compiler, 
> then we should probably use them always, instead of the asm sections.
>
> In summary, I came up with the attached. It's essentially your patch, 
> with tweaks for the above-mentioned things. I don't have a powerpc 
> system to test on, so there are probably some silly typos there.

Why do you prefer to use _check_lock instead of __check_lock_mp ?
First one is even not mentioned in XLC compiler manual:
http://www-01.ibm.com/support/docview.wss?uid=swg27046906&aid=7
or
http://scv.bu.edu/computation/bluegene/IBMdocs/compiler/xlc-8.0/html/compiler/ref/bif_sync.htm

>
> - Heikki
>
>
>

-- 
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Commits

  1. For all ppc compilers, implement compare_exchange and fetch_add with asm.

  2. For PowerPC instruction "addi", use constraint "b".

  3. For all ppc compilers, implement pg_atomic_fetch_add_ with inline asm.

  4. Replace xlc __fetch_and_add() with inline asm.

  5. Test pg_atomic_fetch_add_ with variable addend and 16-bit edge cases.