Fix MemoryContextAllocAligned's interaction with Valgrind.

Arrange that only the "aligned chunk" part of the allocated space is
included in a Valgrind vchunk.  This suppresses complaints about that
vchunk being possibly lost because PG is retaining only pointers to
the aligned chunk.  Also make sure that trailing wasted space is
marked NOACCESS.

As a tiny performance improvement, arrange that MCXT_ALLOC_ZERO zeroes
only the returned "aligned chunk", not the wasted padding space.

In passing, fix GetLocalBufferStorage to use MemoryContextAllocAligned
instead of rolling its own implementation, which was equally broken
according to Valgrind.

Author: Tom Lane <tgl@sss.pgh.pa.us>
Reviewed-by: Andres Freund <andres@anarazel.de>
Discussion: https://postgr.es/m/285483.1746756246@sss.pgh.pa.us

diff --git a/src/backend/storage/buffer/localbuf.c b/src/backend/storage/buffer/localbuf.c
index 3da9c41ee1d7ad73dc260efd6a01696538e2c801..3c0d20f4659d2da7e2d2ffa6b86fea98d7da9f9a 100644 (file)
--- a/src/backend/storage/buffer/localbuf.c
+++ b/src/backend/storage/buffer/localbuf.c
@@ -932,10 +932,11 @@ GetLocalBufferStorage(void)
                num_bufs = Min(num_bufs, MaxAllocSize / BLCKSZ);
 
                /* Buffers should be I/O aligned. */
-               cur_block = (char *)
-                       TYPEALIGN(PG_IO_ALIGN_SIZE,
-                                         MemoryContextAlloc(LocalBufferContext,
-                                                                                num_bufs * BLCKSZ + PG_IO_ALIGN_SIZE));
+               cur_block = MemoryContextAllocAligned(LocalBufferContext,
+                                                                                         num_bufs * BLCKSZ,
+                                                                                         PG_IO_ALIGN_SIZE,
+                                                                                         0);
+
                next_buf_in_block = 0;
                num_bufs_in_block = num_bufs;
        }

diff --git a/src/backend/utils/mmgr/alignedalloc.c b/src/backend/utils/mmgr/alignedalloc.c
index 7eea695de62c5c03a8fc3e58409e0238f8a46dc2..b1be7426914974c1d6fde912c1e9115e72e1e646 100644 (file)
--- a/src/backend/utils/mmgr/alignedalloc.c
+++ b/src/backend/utils/mmgr/alignedalloc.c
@@ -45,6 +45,15 @@ AlignedAllocFree(void *pointer)
                         GetMemoryChunkContext(unaligned)->name, chunk);
 #endif
 
+       /*
+        * Create a dummy vchunk covering the start of the unaligned chunk, but
+        * not overlapping the aligned chunk.  This will be freed while pfree'ing
+        * the unaligned chunk, keeping Valgrind happy.  Then when we return to
+        * the outer pfree, that will clean up the vchunk for the aligned chunk.
+        */
+       VALGRIND_MEMPOOL_ALLOC(GetMemoryChunkContext(unaligned), unaligned,
+                                                  (char *) pointer - (char *) unaligned);
+
        /* Recursively pfree the unaligned chunk */
        pfree(unaligned);
 }
@@ -123,6 +132,15 @@ AlignedAllocRealloc(void *pointer, Size size, int flags)
        VALGRIND_MAKE_MEM_DEFINED(pointer, old_size);
        memcpy(newptr, pointer, Min(size, old_size));
 
+       /*
+        * Create a dummy vchunk covering the start of the old unaligned chunk,
+        * but not overlapping the aligned chunk.  This will be freed while
+        * pfree'ing the old unaligned chunk, keeping Valgrind happy.  Then when
+        * we return to repalloc, it will move the vchunk for the aligned chunk.
+        */
+       VALGRIND_MEMPOOL_ALLOC(ctx, unaligned,
+                                                  (char *) pointer - (char *) unaligned);
+
        pfree(unaligned);
 
        return newptr;

diff --git a/src/backend/utils/mmgr/mcxt.c b/src/backend/utils/mmgr/mcxt.c
index 073aa6c4fc5b32137efc2a4b7665aad0f2abcfbf..47fd774c7d28042bd5fdf9ccae2efb1aec3e2781 100644 (file)
--- a/src/backend/utils/mmgr/mcxt.c
+++ b/src/backend/utils/mmgr/mcxt.c
@@ -1465,7 +1465,13 @@ MemoryContextAllocAligned(MemoryContext context,
        void       *unaligned;
        void       *aligned;
 
-       /* wouldn't make much sense to waste that much space */
+       /*
+        * Restrict alignto to ensure that it can fit into the "value" field of
+        * the redirection MemoryChunk, and that the distance back to the start of
+        * the unaligned chunk will fit into the space available for that.  This
+        * isn't a limitation in practice, since it wouldn't make much sense to
+        * waste that much space.
+        */
        Assert(alignto < (128 * 1024 * 1024));
 
        /* ensure alignto is a power of 2 */
@@ -1502,10 +1508,15 @@ MemoryContextAllocAligned(MemoryContext context,
        alloc_size += 1;
 #endif
 
-       /* perform the actual allocation */
-       unaligned = MemoryContextAllocExtended(context, alloc_size, flags);
+       /*
+        * Perform the actual allocation, but do not pass down MCXT_ALLOC_ZERO.
+        * This ensures that wasted bytes beyond the aligned chunk do not become
+        * DEFINED.
+        */
+       unaligned = MemoryContextAllocExtended(context, alloc_size,
+                                                                                  flags & ~MCXT_ALLOC_ZERO);
 
-       /* set the aligned pointer */
+       /* compute the aligned pointer */
        aligned = (void *) TYPEALIGN(alignto, (char *) unaligned +
                                                                 sizeof(MemoryChunk));
 
@@ -1533,12 +1544,23 @@ MemoryContextAllocAligned(MemoryContext context,
        set_sentinel(aligned, size);
 #endif
 
-       /* Mark the bytes before the redirection header as noaccess */
-       VALGRIND_MAKE_MEM_NOACCESS(unaligned,
-                                                          (char *) alignedchunk - (char *) unaligned);
+       /*
+        * MemoryContextAllocExtended marked the whole unaligned chunk as a
+        * vchunk.  Undo that, instead making just the aligned chunk be a vchunk.
+        * This prevents Valgrind from complaining that the vchunk is possibly
+        * leaked, since only pointers to the aligned chunk will exist.
+        *
+        * After these calls, the aligned chunk will be marked UNDEFINED, and all
+        * the rest of the unaligned chunk (the redirection chunk header, the
+        * padding bytes before it, and any wasted trailing bytes) will be marked
+        * NOACCESS, which is what we want.
+        */
+       VALGRIND_MEMPOOL_FREE(context, unaligned);
+       VALGRIND_MEMPOOL_ALLOC(context, aligned, size);
 
-       /* Disallow access to the redirection chunk header. */
-       VALGRIND_MAKE_MEM_NOACCESS(alignedchunk, sizeof(MemoryChunk));
+       /* Now zero (and make DEFINED) just the aligned chunk, if requested */
+       if ((flags & MCXT_ALLOC_ZERO) != 0)
+               MemSetAligned(aligned, 0, size);
 
        return aligned;
 }
@@ -1572,16 +1594,12 @@ void
 pfree(void *pointer)
 {
 #ifdef USE_VALGRIND
-       MemoryContextMethodID method = GetMemoryChunkMethodID(pointer);
        MemoryContext context = GetMemoryChunkContext(pointer);
 #endif
 
        MCXT_METHOD(pointer, free_p) (pointer);
 
-#ifdef USE_VALGRIND
-       if (method != MCTX_ALIGNED_REDIRECT_ID)
-               VALGRIND_MEMPOOL_FREE(context, pointer);
-#endif
+       VALGRIND_MEMPOOL_FREE(context, pointer);
 }
 
 /*
@@ -1591,9 +1609,6 @@ pfree(void *pointer)
 void *
 repalloc(void *pointer, Size size)
 {
-#ifdef USE_VALGRIND
-       MemoryContextMethodID method = GetMemoryChunkMethodID(pointer);
-#endif
 #if defined(USE_ASSERT_CHECKING) || defined(USE_VALGRIND)
        MemoryContext context = GetMemoryChunkContext(pointer);
 #endif
@@ -1616,10 +1631,7 @@ repalloc(void *pointer, Size size)
         */
        ret = MCXT_METHOD(pointer, realloc) (pointer, size, 0);
 
-#ifdef USE_VALGRIND
-       if (method != MCTX_ALIGNED_REDIRECT_ID)
-               VALGRIND_MEMPOOL_CHANGE(context, pointer, ret, size);
-#endif
+       VALGRIND_MEMPOOL_CHANGE(context, pointer, ret, size);
 
        return ret;
 }