From 3490ac0cb31f88a9d1b1cb1fba7de31aa99cb980 Mon Sep 17 00:00:00 2001 From: Jakub Zelenka Date: Sat, 7 Sep 2024 23:11:16 +0100 Subject: [PATCH] Fix GH-13437: FPM: ERROR: scoreboard: failed to lock (already locked) This changes locking for scoreboard to reduce contention between readers and adds retries for acquiring scoreboard for read. Closes GH-15805 --- NEWS | 4 ++ sapi/fpm/fpm/fpm_atomic.h | 19 ++++++++ sapi/fpm/fpm/fpm_scoreboard.c | 82 +++++++++++++++++++++++++++++++---- sapi/fpm/fpm/fpm_scoreboard.h | 4 ++ 4 files changed, 101 insertions(+), 8 deletions(-) diff --git a/NEWS b/NEWS index caad15a445eac..757cab920c22d 100644 --- a/NEWS +++ b/NEWS @@ -11,6 +11,10 @@ PHP NEWS - FFI: . Fixed bug #79075 (FFI header parser chokes on comments). (nielsdos) +- FPM: + . Fixed bug GH-13437 (FPM: ERROR: scoreboard: failed to lock (already + locked)). (Jakub Zelenka) + - Iconv: . Fixed bug GH-17047 (UAF on iconv filter failure). (nielsdos) diff --git a/sapi/fpm/fpm/fpm_atomic.h b/sapi/fpm/fpm/fpm_atomic.h index e3926e708c542..02009f6af9118 100644 --- a/sapi/fpm/fpm/fpm_atomic.h +++ b/sapi/fpm/fpm/fpm_atomic.h @@ -156,6 +156,25 @@ static inline int fpm_spinlock(atomic_t *lock, int try_once) /* {{{ */ } /* }}} */ +static inline int fpm_spinlock_with_max_retries(atomic_t *lock, unsigned int max_retries) +{ + unsigned int retries = 0; + + for (;;) { + if (atomic_cmp_set(lock, 0, 1)) { + return 1; + } + + sched_yield(); + + if (++retries > max_retries) { + return 0; + } + } + + return 1; +} + #define fpm_unlock(lock) lock = 0 #endif diff --git a/sapi/fpm/fpm/fpm_scoreboard.c b/sapi/fpm/fpm/fpm_scoreboard.c index 52d10a0416832..a4611d5edb620 100644 --- a/sapi/fpm/fpm/fpm_scoreboard.c +++ b/sapi/fpm/fpm/fpm_scoreboard.c @@ -73,6 +73,22 @@ int fpm_scoreboard_init_main(void) return 0; } +static inline void fpm_scoreboard_readers_decrement(struct fpm_scoreboard_s *scoreboard) +{ + fpm_spinlock(&scoreboard->lock, 1); + if (scoreboard->reader_count > 0) { + scoreboard->reader_count -= 1; + } +#ifdef PHP_FPM_ZLOG_TRACE + unsigned int current_reader_count = scoreboard->reader_count; +#endif + fpm_unlock(scoreboard->lock); +#ifdef PHP_FPM_ZLOG_TRACE + /* this is useful for debugging but currently needs to be hidden as external logger would always log it */ + zlog(ZLOG_DEBUG, "scoreboard: for proc %d reader decremented to value %u", getpid(), current_reader_count); +#endif +} + static struct fpm_scoreboard_s *fpm_scoreboard_get_for_update(struct fpm_scoreboard_s *scoreboard) /* {{{ */ { if (!scoreboard) { @@ -93,7 +109,33 @@ void fpm_scoreboard_update_begin(struct fpm_scoreboard_s *scoreboard) /* {{{ */ return; } - fpm_spinlock(&scoreboard->lock, 0); + int retries = 0; + while (1) { + fpm_spinlock(&scoreboard->lock, 1); + if (scoreboard->reader_count == 0) { + if (!fpm_spinlock_with_max_retries(&scoreboard->writer_active, FPM_SCOREBOARD_SPINLOCK_MAX_RETRIES)) { + /* in this case the writer might have crashed so just warn and continue as the lock was acquired */ + zlog(ZLOG_WARNING, "scoreboard: writer %d waited too long for another writer to release lock.", getpid()); + } +#ifdef PHP_FPM_ZLOG_TRACE + else { + zlog(ZLOG_DEBUG, "scoreboard: writer lock acquired by writer %d", getpid()); + } +#endif + fpm_unlock(scoreboard->lock); + break; + } + fpm_unlock(scoreboard->lock); + + if (++retries > FPM_SCOREBOARD_SPINLOCK_MAX_RETRIES) { + /* decrement reader count by 1 (assuming a killed or crashed reader) */ + fpm_scoreboard_readers_decrement(scoreboard); + zlog(ZLOG_WARNING, "scoreboard: writer detected a potential crashed reader, decrementing reader count."); + retries = 0; + } + + sched_yield(); + } } /* }}} */ @@ -170,7 +212,10 @@ void fpm_scoreboard_update_commit( scoreboard->active_max = scoreboard->active; } - fpm_unlock(scoreboard->lock); + fpm_unlock(scoreboard->writer_active); +#ifdef PHP_FPM_ZLOG_TRACE + zlog(ZLOG_DEBUG, "scoreboard: writer lock released by writer %d", getpid()); +#endif } /* }}} */ @@ -234,16 +279,37 @@ struct fpm_scoreboard_proc_s *fpm_scoreboard_proc_get_from_child(struct fpm_chil struct fpm_scoreboard_s *fpm_scoreboard_acquire(struct fpm_scoreboard_s *scoreboard, int nohang) /* {{{ */ { - struct fpm_scoreboard_s *s; - - s = scoreboard ? scoreboard : fpm_scoreboard; + struct fpm_scoreboard_s *s = scoreboard ? scoreboard : fpm_scoreboard; if (!s) { return NULL; } - if (!fpm_spinlock(&s->lock, nohang)) { - return NULL; + int retries = 0; + while (1) { + /* increment reader if no writer active */ + fpm_spinlock(&s->lock, 1); + if (!s->writer_active) { + s->reader_count += 1; +#ifdef PHP_FPM_ZLOG_TRACE + unsigned int current_reader_count = s->reader_count; +#endif + fpm_unlock(s->lock); +#ifdef PHP_FPM_ZLOG_TRACE + zlog(ZLOG_DEBUG, "scoreboard: for proc %d reader incremented to value %u", getpid(), current_reader_count); +#endif + break; + } + fpm_unlock(s->lock); + + sched_yield(); + + if (++retries > FPM_SCOREBOARD_SPINLOCK_MAX_RETRIES) { + zlog(ZLOG_WARNING, "scoreboard: reader waited too long for writer to release lock."); + fpm_scoreboard_readers_decrement(s); + return NULL; + } } + return s; } /* }}} */ @@ -253,7 +319,7 @@ void fpm_scoreboard_release(struct fpm_scoreboard_s *scoreboard) { return; } - scoreboard->lock = 0; + fpm_scoreboard_readers_decrement(scoreboard); } struct fpm_scoreboard_s *fpm_scoreboard_copy(struct fpm_scoreboard_s *scoreboard, int copy_procs) diff --git a/sapi/fpm/fpm/fpm_scoreboard.h b/sapi/fpm/fpm/fpm_scoreboard.h index c488c64bfefc4..31824940b3f7b 100644 --- a/sapi/fpm/fpm/fpm_scoreboard.h +++ b/sapi/fpm/fpm/fpm_scoreboard.h @@ -18,6 +18,8 @@ #define FPM_SCOREBOARD_LOCK_HANG 0 #define FPM_SCOREBOARD_LOCK_NOHANG 1 +#define FPM_SCOREBOARD_SPINLOCK_MAX_RETRIES 50000 + struct fpm_scoreboard_proc_s { union { atomic_t lock; @@ -52,6 +54,8 @@ struct fpm_scoreboard_s { atomic_t lock; char dummy[16]; }; + atomic_t writer_active; + unsigned int reader_count; char pool[32]; int pm; time_t start_epoch;