Re: Better shared data structure management and resizable shared data structures

On Mon, Aug 10, 2026 at 11:07 PM Palak Chaturvedi
<chaturvedipalak1911(at)gmail(dot)com> wrote:
>
> Hi Ashutosh,
> Attached is a test-portability fix on top of v20260724-0006 to unblock
> CFbot [1], plus the small diagnostic patch I used to root-cause it, in
> case it's useful later.
>
> v20260810-0001 = your v20260724-0006, unchanged, renumbered so
> CFbot picks up the set as one series
> v20260810-0002 = the proposed fix (test module only; no changes
> outside src/test/modules/test_shmem/)
> v20260810-0003 = diagnostic elog around the existing smaps walker
> (please don't commit this one; it's here so the
> same instrumentation can be reused later)
>
> Also attached:
> smaps-trace-64bit.txt full 64-bit trace, 12 pids
> smaps-trace-32bit.txt per-pid header + first 8 include lines +
> elided marker + stop/result; the full trace
> is available by rerunning 0003
>
> Method
> ------
>
> To root-cause this rather than guess from the CFbot failure messages,
> I forked off the CF tip (50911a16240 with v20260724-0006 applied),
> added logging only to test_shmem_usage() (0003), pushed to trigger a
> GitHub Actions run [2], and read the archived server logs. The green
> run of 0001+0002 is at [3] (9 jobs pass: Linux 32, Linux 64, macOS,
> Windows MinGW, both Windows VS slices, plus SanityCheck,
> CompilerWarnings, Linux Autoconf).

Thanks for the investigation and the diagnostic and fix patch.

>
> Diff summary of 0002 vs your patch:
>
> src/test/modules/test_shmem/t/002_resizable_shmem_struct.pl | 13 +++++-
> src/test/modules/test_shmem/test_shmem.c | 50
> ++++++++--------
> 2 files changed, 35 insertions(+), 28 deletions(-)
>
> Findings
> --------
>
> 1. test_shmem_usage() smaps walker crosses the mapping boundary
> ---------------------------------------------------------------
>
> The walker only checks address contiguity (start == prev_end) between
> adjacent VMAs, so the first VMA of any kind that happens to abut the
> target /dev/zero (deleted) segment is silently pulled into the RSS
> sum. The comment on the loop mentions mprotect() splitting the
> mapping, but the check itself is unaware of the split's identity.
>
> On Linux 64-bit CI, for the "shrink to minimum, session 1" scenario
> (one of the smallest overages, +139264 bytes over sum(allocated_size)):
>
> include target VMA: 7f1b84b0c000-7f1b84de6000 rw-s ... 10315
> /dev/zero (deleted) Rss=1116 kB, total=1116 kB
> include contiguous VMA: 7f1b84de6000-7f1b9dde5000 ---s ... 10315
> /dev/zero (deleted) Rss=0 kB, total=1116 kB
> include contiguous VMA: 7f1b9dde5000-7f1b9de00000 rw-s ... 10315
> /dev/zero (deleted) Rss=0 kB, total=1116 kB
> include contiguous VMA: 7f1b9de00000-7f1b9e600000 rw-p 00000000 00:00 0
> Rss=2048 kB, total=3164 kB <-- adjacent private anonymous VMA
> stop before VMA: 7f1b9e6f7000-7f1b9f200000 rw-p 00000000 00:00 0
> result: rss=3164 kB swap=0 kB shared_hugetlb=0 kB bytes=3239936
>
> Reported shmem_usage is 3164 kB; real shared-segment RSS is 1116 kB.
> The 2048 kB delta is one adjacent private anonymous VMA that happens
> to sit right after the shared segment. Across the 12 failing
> accounting scenarios on 64-bit, the same 2048 kB VMA at
> 7f1b9de00000-7f1b9e600000 is wrongly included every time, and the
> walker's final "bytes=" matches the TAP-reported shmem_usage exactly
> (full log in smaps-trace-64bit.txt).
>
> On 32-bit the same bug is much worse. The walker crosses out of the
> target /dev/zero segment, through one anonymous VMA, and then straight
> into the writable segments of loaded libraries: libcap-ng, libresolv,
> libc, libssl, libcrypto, libm, libicu*, libkrb5*, libldap, liburing,
> libxml2, libzstd, libstdc++, libsystemd, libubsan, and several more,
> before it finally stops at another SysV shared segment. Per-pid,
> roughly 170 adjacent VMAs are wrongly included; overage is about 2 MB
> per scenario. Header excerpt for one pid (target=0xd35d9700), full
> per-pid summary in smaps-trace-32bit.txt:
>
> include target VMA: d3325000-d99da000 rw-s 00000000 00:01 8414
> /dev/zero (deleted)
> include contiguous VMA: d99da000-ec5d9000 ---s 066b5000 00:01 8414
> /dev/zero (deleted)
> include contiguous VMA: ec5d9000-ec5f3000 rw-s 192b4000 00:01 8414
> /dev/zero (deleted)
> include contiguous VMA: ec5f3000-ec5f9000 rw-p 00000000 00:00 0
> include contiguous VMA: ec5f9000-ec5fa000 r--p ...
> /usr/lib/i386-linux-gnu/libcap-ng.so.0.0.0
> ...
> ... (168 more contiguous VMAs elided) ...
> stop before VMA: f02dc000-f02dd000 rw-s ... /SYSV0088a83c (deleted)
> result: rss=<garbage, see Finding #2> bytes=109752320
>
> What 0002 proposes (test_shmem.c): also check permissions[3] (the 's'
> vs 'p' character) when continuing across a split. This still handles
> the case the loop comment cares about, one MAP_SHARED region split by
> mprotect() into a chain of 'rw-s' / '---s' / 'rw-s' adjacent VMAs
> backed by the same inode, while excluding adjacent private mappings.
>
> One thing I'd like your opinion on: the 's' check by itself is not
> airtight. If a *different* MAP_SHARED mapping (different inode /
> pathname) happened to sit immediately contiguous to our target
> segment, the walker would still include it. I haven't hit this on the
> CI runners we tested, but on more crowded address spaces (a backend
> that has attached to additional SysV segments, for example) it seems
> plausible. One option would be to also match on the inode+pathname of
> the target VMA (parse them from the same sscanf) and stop as soon as
> they change. That's a bit more parsing, not much. Happy to add it to
> 0002 or leave it as a follow-up, whichever you'd prefer.

Yes, we should check both the shared permissions as well as the pathname.

>
> 2. sscanf("%ld", int64 *) is a width mismatch on MSVC and 32-bit Linux
> ----------------------------------------------------------------------
>
> The CF-base test_shmem_usage() has:
>
> int64 val;
> ...
> if (sscanf(line, "Rss: %ld kB", &val) == 1)
> total_rss_kb += val;
>
> sscanf("%ld", ...) writes a sizeof(long) value, but val is int64. On
> platforms where long is 32 bits (MSVC on any target, 32-bit Linux),
> sscanf writes only the low 32 bits and leaves the upper 32 bits of
> val uninitialized. This is what makes the totals in the 32-bit trace
> look like garbage: 0003 uses INT64_FORMAT and prints val correctly,
> so what you see is the CF-base sscanf's half-populated int64.
>
> The returned bytes= is still correct because total_rss_kb passes
> through mul_size()/add_size() (unsigned Size, which is 32 bits on
> 32-bit Linux) and the upper garbage gets truncated. So the accounting
> failure isn't hidden -- the delta that TAP compares is right, only
> the diagnostic printout looks wrong.
>
> 0002 switches the four accounting variables (total_rss_kb,
> total_swap_kb, total_shared_hugetlb_kb, val) from int64 to plain
> 'long', which matches %ld on every supported platform and is more
> than wide enough for kB-per-VMA counts.

WFM.

>
> 3. read-past-current-size test: compiler elides the read loop
> -------------------------------------------------------------
>
> The read side of resizable_shmem_access_beyond_size() had:
>
> for (int i = resizable_shmem->num_entries; i < test_max_entries; i++)
> sink = resizable_shmem->data[i];
>
> At -O2 the compiler can legally elide most of that loop: 'sink' is a
> plain static int32, resizable_shmem->data is a non-volatile pointer,
> and only the last store to sink is externally observable. The CF-base
> comment ("Return the last read value so that compiler doesn't optimize
> away the assignment to sink") predicts the risk but doesn't prevent
> it -- returning sink from the SQL function only pins the last
> iteration.
>
> When the read is elided, the backend never faults, so tests 45-47
> fail like this (base + 0003 diagnostic run on Linux 64-bit):
>
> not ok 45 - read past current size crashes the backend
> not ok 46 - read crash reports lost connection
> got: '' (empty stderr -- backend didn't crash)
> not ok 47 - read succeeds after crash recovery
> got: 'f' expected: 't' (no crash, so no recovery)
>
> 0002 fixes this by casting through a volatile pointer:
>
> volatile int32 *volatile_data = resizable_shmem->data;
> for (int i = resizable_shmem->num_entries; i < test_max_entries; i++)
> sink = volatile_data[i];
>
> Each volatile_data[i] load is now a required side-effect and can't be
> elided, so the read actually faults.

this looks fine however, I think there is a simpler way. Perform a
bitwise operation on each value something like sink &=
resizable->data[i], which can't be optimized away and return sink.

>
> 4. Windows: skip 002_resizable_shmem_struct.pl
> ----------------------------------------------
>
> Resizable shared memory isn't supported on Windows yet, and the test
> relies on /proc/self/smaps, which is Linux-only. 0002 adds an early
> `plan skip_all => 'resizable shared memory tests are not supported on
> Windows'` so the two Windows CFbot jobs stop failing. If a future
> revision brings Windows support, we can drop the skip.

The tests should not run on windows because they are gated on
$have_resizable_shmem. Is that not enough? Other than Windows, we do
not support resizable shmem on platforms which do not support required
MADV flags. The SKIP check you have added won't work in that case.

--
Best Wishes,
Ashutosh Bapat