Message-ID: <e8d4ef94-b8aa-4cda-8f31-081f78194275@iki.fi>
Date: Mon, 29 Jul 2024 20:07:56 +0300
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: Detect double-release of spinlock
To: Andres Freund <andres@anarazel.de>, Tom Lane <tgl@sss.pgh.pa.us>,
 pgsql-hackers@postgresql.org
References: <E1sYSF2-001lEB-D1@gemulon.postgresql.org>
 <2600757.1722267116@sss.pgh.pa.us>
 <20240729161846.kgiafbe2dtgabxgb@awork3.anarazel.de>
 <2802449.1722270793@sss.pgh.pa.us>
 <20240729164026.yurg37tej34o76uk@awork3.anarazel.de>
 <20240729165154.56zqyg34x2ywkpsh@awork3.anarazel.de>
Content-Language: en-US
From: Heikki Linnakangas <hlinnaka@iki.fi>
In-Reply-To: <20240729165154.56zqyg34x2ywkpsh@awork3.anarazel.de>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/e8d4ef94-b8aa-4cda-8f31-081f78194275%40iki.fi>
Precedence: bulk

On 29/07/2024 19:51, Andres Freund wrote:
> On 2024-07-29 09:40:26 -0700, Andres Freund wrote:
>> On 2024-07-29 12:33:13 -0400, Tom Lane wrote:
>>> Andres Freund <andres@anarazel.de> writes:
>>>> On 2024-07-29 11:31:56 -0400, Tom Lane wrote:
>>>>> There was some recent discussion about getting rid of
>>>>> --disable-spinlocks on the grounds that nobody would use
>>>>> hardware that lacked native spinlocks.  But now I wonder
>>>>> if there is a testing/debugging reason to keep it.
>>>
>>>> Seems it'd be a lot more straightforward to just add an assertion to the
>>>> x86-64 spinlock implementation verifying that the spinlock isn't already free?
>>
>> FWIW, I quickly hacked that up, and it indeed quickly fails with 0393f542d72^
>> and passes with 0393f542d72.

+1. Thanks!

>>> Other context from this discussion:
>>> I dunno, is that the only extra check that the --disable-spinlocks
>>> implementation is providing?
>>
>> I think it also provides the (valuable!) check that spinlocks were actually
>> initialized. But that again seems like something we'd be better off adding
>> more general infrastructure for - nobody runs --disable-spinlocks locally, we
>> shouldn't need to run this on the buildfarm to find problems like this.

Note that the "check" for double-release with the fallback 
implementation wasn't an explicit check either. It just incremented the 
underlying semaphore, which caused very weird failures later in 
completely unrelated code. An explicit assert would be much nicer.

+1 for removing --disable-spinlocks, but let's add this assertion first.

>>> I'm kind of allergic to putting Asserts into spinlocked code segments,
>>> mostly on the grounds that it violates the straight-line-code precept.
>>> I suppose it's not really that bad for tests that you don't expect
>>> to fail, but still ...
>>
>> I don't think the spinlock implementation itself is really affected by that
>> rule - after all, the --disable-spinlocks implementation actually consists out
>> of several layers of external function calls (including syscalls in some
>> cases!).

Yeah I'm not worried about that at all. Also, the assert is made when 
you have already released the spinlock; you are already out of the 
critical section.

-- 
Heikki Linnakangas
Neon (https://neon.tech)