From: Heikki Linnakangas <hlinnaka@iki.fi>
To: Andres Freund <andres@anarazel.de>
To: Tom Lane <tgl@sss.pgh.pa.us>
To: pgsql-hackers@postgresql.org
Subject: Re: Detect double-release of spinlock
Date: Mon, 29 Jul 2024 20:07:56 +0300
Message-ID: <e8d4ef94-b8aa-4cda-8f31-081f78194275@iki.fi> (raw)
In-Reply-To: <20240729165154.56zqyg34x2ywkpsh@awork3.anarazel.de>
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>
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)
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-committers@postgresql.org
Cc: hlinnaka@iki.fi, andres@anarazel.de, tgl@sss.pgh.pa.us, pgsql-hackers@postgresql.org
Subject: Re: Detect double-release of spinlock
In-Reply-To: <e8d4ef94-b8aa-4cda-8f31-081f78194275@iki.fi>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox