Re: [HACKERS] Deadlock in XLogInsert at AIX

On 02/01/2017 04:12 PM, Konstantin Knizhnik wrote:
> 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

Googling around, it seems that they do more or less the same thing. I 
would guess that they actually produce the same assembly code, but I 
have no machine to test on. If I understand correctly, the difference is 
that __check_lock_mp() is an xlc compiler intrinsic, while _check_lock() 
is a libc function. The libc function presumably does __check_lock_mp() 
or __check_lock_up() depending on whether the system is a multi- or 
uni-processor system.

So I think if we're going to change this, the use of __check_lock_mp() 
needs to be in an #ifdef block to check that you're on the XLC compiler, 
as it's a *compiler* intrinsic, while the current code that uses 
_check_lock() are in an "#ifdef _AIX" block, which is correct for 
_check_lock() because it's defined in libc, not by the compiler.

But if there's no pressing reason to change it, let's leave it alone. 
It's not related to the problem at hand, right?

- Heikki