Hi Matthias,
Thanks for the review!
Matthias van de Meent <boekewurm+postg...@gmail.com> writes:
> On Wed, 10 Jan 2024 at 02:44, Andy Fan <zhihuifan1...@163.com> wrote:
>> Hi,
>>
>> I want to know if Andres or you have plan
>> to do some code review. I don't expect this would happen very soon, just
>> want to make sure this will not happen that both of you think the other
>> one will do, but actually none of them does it in fact. a commit fest
>> [1] has been added for this.
>
>
>> +++ b/src/backend/storage/buffer/bufmgr.c
>> @@ -5419,6 +5419,7 @@ LockBufHdr(BufferDesc *desc)
>> perform_spin_delay(&delayStatus);
>> }
>> finish_spin_delay(&delayStatus);
>> + START_SPIN_LOCK();
>> return old_buf_state | BM_LOCKED;
>> }
>
> I think that we need to 'arm' the checks just before we lock the spin
> lock, and 'disarm' the checks just after we unlock the spin lock,
> rather than after and before, respectively. That way, we won't have a
> chance of false negatives: with your current patch it is possible that
> an interrupt fires between the acquisition of the lock and the code in
> START_SPIN_LOCK() marking the thread as holding a spin lock, which
> would cause any check in that signal handler to incorrectly read that
> we don't hold any spin locks.
That's a good idea. fixed in v2.
>
>> +++ b/src/backend/storage/lmgr/lock.c
>> @@ -776,6 +776,8 @@ LockAcquireExtended(const LOCKTAG *locktag,
>> bool found_conflict;
>> bool log_lock = false;
>>
>> + Assert(SpinLockCount == 0);
>> +
>
> I'm not 100% sure on the policy of this, but theoretically you could
> use LockAquireExtended(dontWait=true) while holding a spin lock, as
> that would not have an unknown duration. Then again, this function
> also does elog/ereport, which would cause issues, still, so this code
> may be the better option.
I thought this statement as "keeping the current patch as it is" since
"not waiting" doesn't means the a few dozen in this case. please
correct me if anything wrong.
>
>> + elog(PANIC, "stuck spinlock detected at %s, %s:%d after waiting for %u
>> ms",
>> + func, file, line, delay_ms);
>
> pg_usleep doesn't actually guarantee that we'll wait for exactly that
> duration; depending on signals received while spinning and/or OS
> scheduling decisions it may be off by orders of magnitude.
True, but I did this for two reasons. a). the other soluation needs call
'time' syscall twice, I didn't want to pay this run-time effort. b). the
possiblity of geting a signals during pg_usleep should be low and
even that happens, because the message is just for human, we don't need
a absolutely accurate number. what do you think?
>
>> +++ b/src/common/scram-common.c
>
> This is unrelated to the main patchset
Fixed in v2.
>
>> +++ b/src/include/storage/spin.h
>
> Minor: I think these changes could better be included in miscadmin, or
> at least the definition for SpinLockCount should be moved there: The
> spin lock system itself shouldn't be needed in places where we need to
> make sure that we don't hold any spinlocks, and miscadmin.h already
> holds things related to "System interrupt and critical section
> handling", which seems quite related.
fixed in v2.
--
Best Regards
Andy Fan
>From 5548477e192abce02a95249f4c1e0c9cf465d668 Mon Sep 17 00:00:00 2001
From: "yizhi.fzh" <yizhi....@alibaba-inc.com>
Date: Thu, 11 Jan 2024 11:12:00 +0800
Subject: [PATCH v2] Detect more misuse of spin lock automatically
spin lock are intended for *very* short-term locks, but it is possible
to be misused in many cases. e.g. Acquiring another LWLocks or regular
locks, memory allocation. In this patch, all of such cases will be
automatically detected in an ASSERT_CHECKING build.
CHECK_FOR_INTERRUPTS should be avoided when holding a spin lock because
it is nearly impossible to release the spin lock correctly if an
error/fatal happens would cause the code jump to some other places.
---
src/backend/access/table/tableam.c | 1 +
src/backend/storage/buffer/bufmgr.c | 1 +
src/backend/storage/ipc/barrier.c | 2 ++
src/backend/storage/lmgr/lock.c | 2 ++
src/backend/storage/lmgr/lwlock.c | 2 ++
src/backend/storage/lmgr/s_lock.c | 15 ++++++++-------
src/backend/utils/hash/dynahash.c | 1 +
src/backend/utils/init/globals.c | 1 +
src/backend/utils/mmgr/mcxt.c | 9 +++++++++
src/include/miscadmin.h | 9 +++++++++
src/include/storage/buf_internals.h | 1 +
src/include/storage/s_lock.h | 2 ++
src/include/storage/shm_toc.h | 1 +
src/include/storage/spin.h | 17 ++++++++++++++---
14 files changed, 54 insertions(+), 10 deletions(-)
diff --git a/src/backend/access/table/tableam.c b/src/backend/access/table/tableam.c
index 6ed8cca05a..6c6a65764c 100644
--- a/src/backend/access/table/tableam.c
+++ b/src/backend/access/table/tableam.c
@@ -24,6 +24,7 @@
#include "access/syncscan.h"
#include "access/tableam.h"
#include "access/xact.h"
+#include "miscadmin.h"
#include "optimizer/plancat.h"
#include "port/pg_bitutils.h"
#include "storage/bufmgr.h"
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index 7d601bef6d..c600a113cf 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -5409,6 +5409,7 @@ LockBufHdr(BufferDesc *desc)
init_local_spin_delay(&delayStatus);
+ START_SPIN_LOCK();
while (true)
{
/* set BM_LOCKED flag */
diff --git a/src/backend/storage/ipc/barrier.c b/src/backend/storage/ipc/barrier.c
index 5b52e060ba..513835ccf2 100644
--- a/src/backend/storage/ipc/barrier.c
+++ b/src/backend/storage/ipc/barrier.c
@@ -84,6 +84,8 @@
*/
#include "postgres.h"
+
+#include "miscadmin.h"
#include "storage/barrier.h"
static inline bool BarrierDetachImpl(Barrier *barrier, bool arrive);
diff --git a/src/backend/storage/lmgr/lock.c b/src/backend/storage/lmgr/lock.c
index c70a1adb9a..f896201244 100644
--- a/src/backend/storage/lmgr/lock.c
+++ b/src/backend/storage/lmgr/lock.c
@@ -776,6 +776,8 @@ LockAcquireExtended(const LOCKTAG *locktag,
bool found_conflict;
bool log_lock = false;
+ Assert(SpinLockCount == 0);
+
if (lockmethodid <= 0 || lockmethodid >= lengthof(LockMethods))
elog(ERROR, "unrecognized lock method: %d", lockmethodid);
lockMethodTable = LockMethods[lockmethodid];
diff --git a/src/backend/storage/lmgr/lwlock.c b/src/backend/storage/lmgr/lwlock.c
index b4b989ac56..974a7c26ea 100644
--- a/src/backend/storage/lmgr/lwlock.c
+++ b/src/backend/storage/lmgr/lwlock.c
@@ -1205,6 +1205,8 @@ LWLockAcquire(LWLock *lock, LWLockMode mode)
Assert(mode == LW_SHARED || mode == LW_EXCLUSIVE);
+ Assert(SpinLockCount == 0);
+
PRINT_LWDEBUG("LWLockAcquire", lock, mode);
#ifdef LWLOCK_STATS
diff --git a/src/backend/storage/lmgr/s_lock.c b/src/backend/storage/lmgr/s_lock.c
index 0e5f7ab0b9..43a05b68be 100644
--- a/src/backend/storage/lmgr/s_lock.c
+++ b/src/backend/storage/lmgr/s_lock.c
@@ -71,18 +71,18 @@ static int spins_per_delay = DEFAULT_SPINS_PER_DELAY;
* s_lock_stuck() - complain about a stuck spinlock
*/
static void
-s_lock_stuck(const char *file, int line, const char *func)
+s_lock_stuck(const char *file, int line, const char *func, uint32_t delay_ms)
{
if (!func)
func = "(unknown)";
#if defined(S_LOCK_TEST)
- fprintf(stderr,
- "\nStuck spinlock detected at %s, %s:%d.\n",
- func, file, line);
+ fprintf(stder,
+ "\nStuck spinlock detected at %s, %s:%d. after waiting for %u ms\n",
+ func, file, line, delay_ms);
exit(1);
#else
- elog(PANIC, "stuck spinlock detected at %s, %s:%d",
- func, file, line);
+ elog(PANIC, "stuck spinlock detected at %s, %s:%d after waiting for %u ms",
+ func, file, line, delay_ms);
#endif
}
@@ -132,7 +132,7 @@ perform_spin_delay(SpinDelayStatus *status)
if (++(status->spins) >= spins_per_delay)
{
if (++(status->delays) > NUM_DELAYS)
- s_lock_stuck(status->file, status->line, status->func);
+ s_lock_stuck(status->file, status->line, status->func, status->delay_ms);
if (status->cur_delay == 0) /* first time to delay? */
status->cur_delay = MIN_DELAY_USEC;
@@ -148,6 +148,7 @@ perform_spin_delay(SpinDelayStatus *status)
pgstat_report_wait_start(WAIT_EVENT_SPIN_DELAY);
pg_usleep(status->cur_delay);
pgstat_report_wait_end();
+ status->delay_ms += (status->cur_delay / 1000);
#if defined(S_LOCK_TEST)
fprintf(stdout, "*");
diff --git a/src/backend/utils/hash/dynahash.c b/src/backend/utils/hash/dynahash.c
index a4152080b5..504dae4d6c 100644
--- a/src/backend/utils/hash/dynahash.c
+++ b/src/backend/utils/hash/dynahash.c
@@ -98,6 +98,7 @@
#include "access/xact.h"
#include "common/hashfn.h"
+#include "miscadmin.h"
#include "port/pg_bitutils.h"
#include "storage/shmem.h"
#include "storage/spin.h"
diff --git a/src/backend/utils/init/globals.c b/src/backend/utils/init/globals.c
index 88b03e8fa3..913f775d2c 100644
--- a/src/backend/utils/init/globals.c
+++ b/src/backend/utils/init/globals.c
@@ -40,6 +40,7 @@ volatile sig_atomic_t IdleStatsUpdateTimeoutPending = false;
volatile uint32 InterruptHoldoffCount = 0;
volatile uint32 QueryCancelHoldoffCount = 0;
volatile uint32 CritSectionCount = 0;
+volatile uint32 SpinLockCount = 0;
int MyProcPid;
pg_time_t MyStartTime;
diff --git a/src/backend/utils/mmgr/mcxt.c b/src/backend/utils/mmgr/mcxt.c
index 1336944084..5acb03ac60 100644
--- a/src/backend/utils/mmgr/mcxt.c
+++ b/src/backend/utils/mmgr/mcxt.c
@@ -26,6 +26,7 @@
#include "storage/proc.h"
#include "storage/procarray.h"
#include "storage/procsignal.h"
+#include "storage/spin.h"
#include "utils/fmgrprotos.h"
#include "utils/memdebug.h"
#include "utils/memutils.h"
@@ -1028,6 +1029,8 @@ MemoryContextAlloc(MemoryContext context, Size size)
if (!AllocSizeIsValid(size))
elog(ERROR, "invalid memory alloc request size %zu", size);
+ Assert(SpinLockCount == 0);
+
context->isReset = false;
ret = context->methods->alloc(context, size);
@@ -1071,6 +1074,8 @@ MemoryContextAllocZero(MemoryContext context, Size size)
if (!AllocSizeIsValid(size))
elog(ERROR, "invalid memory alloc request size %zu", size);
+ Assert(SpinLockCount == 0);
+
context->isReset = false;
ret = context->methods->alloc(context, size);
@@ -1197,6 +1202,8 @@ palloc(Size size)
if (!AllocSizeIsValid(size))
elog(ERROR, "invalid memory alloc request size %zu", size);
+ Assert(SpinLockCount == 0);
+
context->isReset = false;
ret = context->methods->alloc(context, size);
@@ -1228,6 +1235,8 @@ palloc0(Size size)
if (!AllocSizeIsValid(size))
elog(ERROR, "invalid memory alloc request size %zu", size);
+ Assert(SpinLockCount == 0);
+
context->isReset = false;
ret = context->methods->alloc(context, size);
diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h
index 0b01c1f093..91d256a943 100644
--- a/src/include/miscadmin.h
+++ b/src/include/miscadmin.h
@@ -103,6 +103,7 @@ extern PGDLLIMPORT volatile sig_atomic_t ClientConnectionLost;
extern PGDLLIMPORT volatile uint32 InterruptHoldoffCount;
extern PGDLLIMPORT volatile uint32 QueryCancelHoldoffCount;
extern PGDLLIMPORT volatile uint32 CritSectionCount;
+extern PGDLLIMPORT volatile uint32 SpinLockCount;
/* in tcop/postgres.c */
extern void ProcessInterrupts(void);
@@ -120,6 +121,7 @@ extern void ProcessInterrupts(void);
/* Service interrupt, if one is pending and it's safe to service it now */
#define CHECK_FOR_INTERRUPTS() \
do { \
+ Assert(SpinLockCount == 0); \
if (INTERRUPTS_PENDING_CONDITION()) \
ProcessInterrupts(); \
} while(0)
@@ -153,6 +155,13 @@ do { \
CritSectionCount--; \
} while(0)
+#define START_SPIN_LOCK() (SpinLockCount++)
+#define END_SPIN_LOCK() \
+do { \
+ Assert(SpinLockCount > 0); \
+ SpinLockCount--; \
+} while(0)
+
/*****************************************************************************
* globals.h -- *
diff --git a/src/include/storage/buf_internals.h b/src/include/storage/buf_internals.h
index e43e616579..ad7e1d0a21 100644
--- a/src/include/storage/buf_internals.h
+++ b/src/include/storage/buf_internals.h
@@ -363,6 +363,7 @@ UnlockBufHdr(BufferDesc *desc, uint32 buf_state)
{
pg_write_barrier();
pg_atomic_write_u32(&desc->state, buf_state & (~BM_LOCKED));
+ END_SPIN_LOCK();
}
/* in bufmgr.c */
diff --git a/src/include/storage/s_lock.h b/src/include/storage/s_lock.h
index aa06e49da2..ac875ab3dd 100644
--- a/src/include/storage/s_lock.h
+++ b/src/include/storage/s_lock.h
@@ -846,6 +846,7 @@ typedef struct
const char *file;
int line;
const char *func;
+ uint32_t delay_ms;
} SpinDelayStatus;
static inline void
@@ -858,6 +859,7 @@ init_spin_delay(SpinDelayStatus *status,
status->file = file;
status->line = line;
status->func = func;
+ status->delay_ms = 0;
}
#define init_local_spin_delay(status) init_spin_delay(status, __FILE__, __LINE__, __func__)
diff --git a/src/include/storage/shm_toc.h b/src/include/storage/shm_toc.h
index a46470d68b..dd0e591340 100644
--- a/src/include/storage/shm_toc.h
+++ b/src/include/storage/shm_toc.h
@@ -23,6 +23,7 @@
#define SHM_TOC_H
#include "storage/shmem.h" /* for add_size() */
+#include "miscadmin.h"
/* shm_toc is an opaque type known only within shm_toc.c */
typedef struct shm_toc shm_toc;
diff --git a/src/include/storage/spin.h b/src/include/storage/spin.h
index c0679c5999..99bced0f93 100644
--- a/src/include/storage/spin.h
+++ b/src/include/storage/spin.h
@@ -56,12 +56,23 @@
#include "storage/pg_sema.h"
#endif
-
#define SpinLockInit(lock) S_INIT_LOCK(lock)
-#define SpinLockAcquire(lock) S_LOCK(lock)
+#define SpinLockAcquire(lock) \
+ do \
+ { \
+ START_SPIN_LOCK(); \
+ S_LOCK(lock); \
+ } while (false);
+
+
+#define SpinLockRelease(lock) \
+ do \
+ { \
+ S_UNLOCK(lock); \
+ END_SPIN_LOCK(); \
+ } while (false);
-#define SpinLockRelease(lock) S_UNLOCK(lock)
#define SpinLockFree(lock) S_LOCK_FREE(lock)
--
2.34.1
