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 <[email protected]>
Reviewed-by: Andrey Borodin <[email protected]>
Discussion: https://postgr.es/m/9444dc46-ca47-43ed-9058-89c456316306@postgrespro.ru

diff --git a/src/backend/access/transam/slru.c b/src/backend/access/transam/slru.c
index 7eeaafe2cb3cc430b01e633a32d20db4de627156..9ce628e62a5c5e12e851e5548d1226b06e945cf1 100644 (file)
--- 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;
 
@@ -1180,7 +1180,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 ae871b640f8c866949fda860b247484edf562ce2..e142800aab216cd67bf0363ece8dadc9c7d5df73 100644 (file)
--- 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);
 }