References: <87plyhvama.fsf@163.com>
 <CAEze2Wj6TKjmF0Qscoo3j5ANpDSKa86ELU6Bv4T_x16xKrzHxQ@mail.gmail.com>
User-agent: mu4e 1.10.7; emacs 29.1
From: Andy Fan <zhihuifan1213@163.com>
To: Matthias van de Meent <boekewurm+postgres@gmail.com>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>, Thomas Munro
 <tmunro@postgresql.org>, Robert Haas <robertmhaas@gmail.com>
Subject: Re: the s_lock_stuck on perform_spin_delay
Date: Thu, 04 Jan 2024 22:24:50 +0800
In-reply-to: 
 <CAEze2Wj6TKjmF0Qscoo3j5ANpDSKa86ELU6Bv4T_x16xKrzHxQ@mail.gmail.com>
Message-ID: <871qaxp3ly.fsf@163.com>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="=-=-="
Archived-At: <https://www.postgresql.org/message-id/871qaxp3ly.fsf%40163.com>
Precedence: bulk

--=-=-=
Content-Type: text/plain


Hi Matthias and Robert,

Matthias van de Meent <boekewurm+postgres@gmail.com> writes:

> On Thu, 4 Jan 2024 at 08:09, Andy Fan <zhihuifan1213@163.com> wrote:
>>
>> My question is if someone doesn't obey the rule by mistake (everyone
>> can make mistake), shall we PANIC on a production environment? IMO I
>> think it can be a WARNING on a production environment and be a stuck
>> when 'ifdef USE_ASSERT_CHECKING'.
>> [...]
>> I think a experienced engineer like Thomas can make this mistake and the
>> patch was reviewed by 3 peoples, the bug is still there. It is not easy
>> to say just don't do it.
>>
>> the attached code show the prototype in my mind. Any feedback is welcome.
>
> While I understand your point and could maybe agree with the change
> itself (a crash isn't great),

It's great that both of you agree that the crash is not great. 

> I don't think it's an appropriate fix
> for the problem of holding a spinlock while waiting for a LwLock, as
> spin.h specifically mentions the following (and you quoted the same):
>
> """
> Keep in mind the coding rule that spinlocks must not be held for more
> than a few instructions.
> """

Yes, I agree that the known [LW]LockAcquire after holding a Spin lock
should be fixed at the first chance rather than pander to it with my
previous patch. My previous patch just take care of the *unknown*
cases (and I cced thomas in the hope that he knows the bug). I also
agree that the special case about [LW]LockAcquire should be detected
more effective as you suggested below. So v2 comes and commit 2 is for
this suggestion. 

>
> I suspect that we'd be better off with stronger protections against
> waiting for LwLocks while we hold any spin lock. More specifically,
> I'm thinking about something like tracking how many spin locks we
> hold, and Assert()-ing that we don't hold any such locks when we start
> to wait for an LwLock or run CHECK_FOR_INTERRUPTS-related code (with
> potential manual contextual overrides in specific areas of code where
> specific care has been taken to make it safe to hold spin locks while
> doing those operations - although I consider their existence unlikely
> I can't rule them out as I've yet to go through all lock-touching
> code). This would probably work in a similar manner as
> START_CRIT_SECTION/END_CRIT_SECTION.
>
> Kind regards,
>
> Matthias van de Meent
> Neon (https://neon.tech)


-- 
Best Regards
Andy Fan


--=-=-=
Content-Type: text/x-diff
Content-Disposition: attachment;
 filename=v2-0001-Don-t-call-s_lock_stuck-in-production-environment.patch

From 7d7fd0f0e9b13a290bfffaec0ad40773191155f2 Mon Sep 17 00:00:00 2001
From: "yizhi.fzh" <yizhi.fzh@alibaba-inc.com>
Date: Thu, 4 Jan 2024 14:33:37 +0800
Subject: [PATCH v2 1/2] Don't call s_lock_stuck in production environment

In the current implementation, if a spin lock is misused, the
s_lock_stuck in perform_spin_delay can cause the entire system to
PANIC. In order to balance fault tolerance and the ability to detect
incorrect usage, we can use WARNING to replace s_lock_stuck in the
production environment, but still use s_lock_stuck in builds with
USE_ASSERT_CHECKING.
---
 src/backend/storage/lmgr/s_lock.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/src/backend/storage/lmgr/s_lock.c b/src/backend/storage/lmgr/s_lock.c
index 327ac64f7c..9446605122 100644
--- a/src/backend/storage/lmgr/s_lock.c
+++ b/src/backend/storage/lmgr/s_lock.c
@@ -67,6 +67,7 @@ slock_t		dummy_spinlock;
 static int	spins_per_delay = DEFAULT_SPINS_PER_DELAY;
 
 
+#ifdef USE_ASSERT_CHECKING
 /*
  * s_lock_stuck() - complain about a stuck spinlock
  */
@@ -85,6 +86,7 @@ s_lock_stuck(const char *file, int line, const char *func)
 		 func, file, line);
 #endif
 }
+#endif
 
 /*
  * s_lock(lock) - platform-independent portion of waiting for a spinlock.
@@ -132,7 +134,14 @@ perform_spin_delay(SpinDelayStatus *status)
 	if (++(status->spins) >= spins_per_delay)
 	{
 		if (++(status->delays) > NUM_DELAYS)
+		{
+#ifdef USE_ASSERT_CHECKING
 			s_lock_stuck(status->file, status->line, status->func);
+#else
+			if (status->delays % NUM_DELAYS == 0)
+				elog(WARNING, "perform spin lock on %s %d times", status->func, status->delays);
+#endif
+		}
 
 		if (status->cur_delay == 0) /* first time to delay? */
 			status->cur_delay = MIN_DELAY_USEC;
-- 
2.34.1


--=-=-=
Content-Type: text/x-diff
Content-Disposition: attachment;
 filename=v2-0002-Disallow-LW-LockAcquire-when-holding-a-spin-lock-.patch

From fae6241960a2c0df8df5d83001ce298228f4cbf2 Mon Sep 17 00:00:00 2001
From: "yizhi.fzh" <yizhi.fzh@alibaba-inc.com>
Date: Thu, 4 Jan 2024 22:19:50 +0800
Subject: [PATCH v2 2/2] Disallow [LW]LockAcquire when holding a spin
 lock-file-mode

Spinlocks are intended for *very* short-term locks, but sometimes it is
misused, this commit guards no [LW]LockAcquire should be called when
holding a spin lock.
---
 src/backend/storage/buffer/bufmgr.c |  1 +
 src/backend/storage/lmgr/lock.c     |  2 ++
 src/backend/storage/lmgr/lwlock.c   |  2 ++
 src/backend/utils/init/globals.c    |  1 +
 src/include/storage/buf_internals.h |  1 +
 src/include/storage/spin.h          | 24 ++++++++++++++++++++++--
 6 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index 9f9d3f24ac..d2534ee9aa 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ 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;
 }
 
diff --git a/src/backend/storage/lmgr/lock.c b/src/backend/storage/lmgr/lock.c
index b8c57b3e16..0710adbef8 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 315a78cda9..5709cadcd5 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/utils/init/globals.c b/src/backend/utils/init/globals.c
index 60bc1217fb..125dc53375 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/include/storage/buf_internals.h b/src/include/storage/buf_internals.h
index 2c4fd92e39..be2d090308 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/spin.h b/src/include/storage/spin.h
index 5d809cc980..c1b96a047f 100644
--- a/src/include/storage/spin.h
+++ b/src/include/storage/spin.h
@@ -56,12 +56,32 @@
 #include "storage/pg_sema.h"
 #endif
 
+extern PGDLLIMPORT volatile uint32 SpinLockCount;
+
+#define START_SPIN_LOCK() (SpinLockCount++)
+#define END_SPIN_LOCK() \
+do { \
+	Assert(SpinLockCount > 0); \
+	SpinLockCount--; \
+} while(0)
 
 #define SpinLockInit(lock)	S_INIT_LOCK(lock)
 
-#define SpinLockAcquire(lock) S_LOCK(lock)
+#define SpinLockAcquire(lock) \
+	do \
+	{ \
+		S_LOCK(lock); \
+		START_SPIN_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


--=-=-=--