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 <y.sokolov@postgrespro.ru>
Reviewed-by: Andrey Borodin <x4mmm@yandex-team.ru>
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 e7f73bf4275a14cb4afb5a21120364b7acc9f666..afedb5c039f167f448564958283ec5dad1040ceb 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;
 
@@ -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 97e612cd100b983d7ada268a96698dc4840de76a..02fcb3bca5473df7746c00d2b57cf7e3b71cd130 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);
 }