From ffd9b813465843c5eda04229a09d2167ed4a4a71 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=81lvaro=20Herrera?= Date: Thu, 9 Jan 2025 07:33:52 +0100 Subject: [PATCH] Fix SLRU bank selection code The originally submitted code (using bit masking) was correct when the number of slots was restricted to be a power of two -- but that limitation was removed during development that led to commit 53c2a97a9266, which made the bank selection code incorrect. This led to always using a smaller number of banks than available. Change said code to use integer modulo instead, which works correctly with an arbitrary number of banks. It's likely that we could improve on this to avoid runtime use of integer division. But with this change we're, at least, not wasting memory on unused banks, and more banks mean less contention, which is likely to have a much higher performance impact than a single instruction's latency. Author: Yura Sokolov Reviewed-by: Andrey Borodin Discussion: https://postgr.es/m/9444dc46-ca47-43ed-9058-89c456316306@postgrespro.ru --- src/backend/access/transam/slru.c | 6 +++--- src/include/access/slru.h | 9 +++------ 2 files changed, 6 insertions(+), 9 deletions(-) diff --git a/src/backend/access/transam/slru.c b/src/backend/access/transam/slru.c index e7f73bf4275..afedb5c039f 100644 --- a/src/backend/access/transam/slru.c +++ b/src/backend/access/transam/slru.c @@ -343,7 +343,7 @@ SimpleLruInit(SlruCtl ctl, const char *name, int nslots, int nlsns, ctl->shared = shared; ctl->sync_handler = sync_handler; ctl->long_segment_names = long_segment_names; - ctl->bank_mask = (nslots / SLRU_BANK_SIZE) - 1; + ctl->nbanks = nbanks; strlcpy(ctl->Dir, subdir, sizeof(ctl->Dir)); } @@ -606,7 +606,7 @@ SimpleLruReadPage_ReadOnly(SlruCtl ctl, int64 pageno, TransactionId xid) { SlruShared shared = ctl->shared; LWLock *banklock = SimpleLruGetBankLock(ctl, pageno); - int bankno = pageno & ctl->bank_mask; + int bankno = pageno % ctl->nbanks; int bankstart = bankno * SLRU_BANK_SIZE; int bankend = bankstart + SLRU_BANK_SIZE; @@ -1177,7 +1177,7 @@ SlruSelectLRUPage(SlruCtl ctl, int64 pageno) int bestinvalidslot = 0; /* keep compiler quiet */ int best_invalid_delta = -1; int64 best_invalid_page_number = 0; /* keep compiler quiet */ - int bankno = pageno & ctl->bank_mask; + int bankno = pageno % ctl->nbanks; int bankstart = bankno * SLRU_BANK_SIZE; int bankend = bankstart + SLRU_BANK_SIZE; diff --git a/src/include/access/slru.h b/src/include/access/slru.h index 97e612cd100..02fcb3bca54 100644 --- a/src/include/access/slru.h +++ b/src/include/access/slru.h @@ -128,10 +128,8 @@ typedef struct SlruCtlData { SlruShared shared; - /* - * Bitmask to determine bank number from page number. - */ - bits16 bank_mask; + /* Number of banks in this SLRU. */ + uint16 nbanks; /* * If true, use long segment file names. Otherwise, use short file names. @@ -163,7 +161,6 @@ typedef struct SlruCtlData * it's always the same, it doesn't need to be in shared memory. */ char Dir[64]; - } SlruCtlData; typedef SlruCtlData *SlruCtl; @@ -179,7 +176,7 @@ SimpleLruGetBankLock(SlruCtl ctl, int64 pageno) { int bankno; - bankno = pageno & ctl->bank_mask; + bankno = pageno % ctl->nbanks; return &(ctl->shared->bank_locks[bankno].lock); }