Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
Skip to content

Commit 3de170f

Browse files
tglsfdcCommitfest Bot
authored and
Commitfest Bot
committed
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> Discussion: https://postgr.es/m/285483.1746756246@sss.pgh.pa.us
1 parent fadb777 commit 3de170f

File tree

3 files changed

+56
-25
lines changed

3 files changed

+56
-25
lines changed

src/backend/storage/buffer/localbuf.c

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -925,10 +925,11 @@ GetLocalBufferStorage(void)
925925
num_bufs = Min(num_bufs, MaxAllocSize / BLCKSZ);
926926

927927
/* Buffers should be I/O aligned. */
928-
cur_block = (char *)
929-
TYPEALIGN(PG_IO_ALIGN_SIZE,
930-
MemoryContextAlloc(LocalBufferContext,
931-
num_bufs * BLCKSZ + PG_IO_ALIGN_SIZE));
928+
cur_block = MemoryContextAllocAligned(LocalBufferContext,
929+
num_bufs * BLCKSZ,
930+
PG_IO_ALIGN_SIZE,
931+
0);
932+
932933
next_buf_in_block = 0;
933934
num_bufs_in_block = num_bufs;
934935
}

src/backend/utils/mmgr/alignedalloc.c

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,15 @@ AlignedAllocFree(void *pointer)
4545
GetMemoryChunkContext(unaligned)->name, chunk);
4646
#endif
4747

48+
/*
49+
* Create a dummy vchunk covering the start of the unaligned chunk, but
50+
* not overlapping the aligned chunk. This will be freed while pfree'ing
51+
* the unaligned chunk, keeping Valgrind happy. Then when we return to
52+
* the outer pfree, that will clean up the vchunk for the aligned chunk.
53+
*/
54+
VALGRIND_MEMPOOL_ALLOC(GetMemoryChunkContext(unaligned), unaligned,
55+
(char *) pointer - (char *) unaligned);
56+
4857
/* Recursively pfree the unaligned chunk */
4958
pfree(unaligned);
5059
}
@@ -123,6 +132,15 @@ AlignedAllocRealloc(void *pointer, Size size, int flags)
123132
VALGRIND_MAKE_MEM_DEFINED(pointer, old_size);
124133
memcpy(newptr, pointer, Min(size, old_size));
125134

135+
/*
136+
* Create a dummy vchunk covering the start of the old unaligned chunk,
137+
* but not overlapping the aligned chunk. This will be freed while
138+
* pfree'ing the old unaligned chunk, keeping Valgrind happy. Then when
139+
* we return to repalloc, it will move the vchunk for the aligned chunk.
140+
*/
141+
VALGRIND_MEMPOOL_ALLOC(ctx, unaligned,
142+
(char *) pointer - (char *) unaligned);
143+
126144
pfree(unaligned);
127145

128146
return newptr;

src/backend/utils/mmgr/mcxt.c

Lines changed: 33 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -1432,7 +1432,13 @@ MemoryContextAllocAligned(MemoryContext context,
14321432
void *unaligned;
14331433
void *aligned;
14341434

1435-
/* wouldn't make much sense to waste that much space */
1435+
/*
1436+
* Restrict alignto to ensure that it can fit into the "value" field of
1437+
* the redirection MemoryChunk, and that the distance back to the start of
1438+
* the unaligned chunk will fit into the space available for that. This
1439+
* isn't a limitation in practice, since it wouldn't make much sense to
1440+
* waste that much space.
1441+
*/
14361442
Assert(alignto < (128 * 1024 * 1024));
14371443

14381444
/* ensure alignto is a power of 2 */
@@ -1469,10 +1475,15 @@ MemoryContextAllocAligned(MemoryContext context,
14691475
alloc_size += 1;
14701476
#endif
14711477

1472-
/* perform the actual allocation */
1473-
unaligned = MemoryContextAllocExtended(context, alloc_size, flags);
1478+
/*
1479+
* Perform the actual allocation, but do not pass down MCXT_ALLOC_ZERO.
1480+
* This ensures that wasted bytes beyond the aligned chunk do not become
1481+
* DEFINED.
1482+
*/
1483+
unaligned = MemoryContextAllocExtended(context, alloc_size,
1484+
flags & ~MCXT_ALLOC_ZERO);
14741485

1475-
/* set the aligned pointer */
1486+
/* compute the aligned pointer */
14761487
aligned = (void *) TYPEALIGN(alignto, (char *) unaligned +
14771488
sizeof(MemoryChunk));
14781489

@@ -1500,12 +1511,23 @@ MemoryContextAllocAligned(MemoryContext context,
15001511
set_sentinel(aligned, size);
15011512
#endif
15021513

1503-
/* Mark the bytes before the redirection header as noaccess */
1504-
VALGRIND_MAKE_MEM_NOACCESS(unaligned,
1505-
(char *) alignedchunk - (char *) unaligned);
1514+
/*
1515+
* MemoryContextAllocExtended marked the whole unaligned chunk as a
1516+
* vchunk. Undo that, instead making just the aligned chunk be a vchunk.
1517+
* This prevents Valgrind from complaining that the vchunk is possibly
1518+
* leaked, since only pointers to the aligned chunk will exist.
1519+
*
1520+
* After these calls, the aligned chunk will be marked UNDEFINED, and all
1521+
* the rest of the unaligned chunk (the redirection chunk header, the
1522+
* padding bytes before it, and any wasted trailing bytes) will be marked
1523+
* NOACCESS, which is what we want.
1524+
*/
1525+
VALGRIND_MEMPOOL_FREE(context, unaligned);
1526+
VALGRIND_MEMPOOL_ALLOC(context, aligned, size);
15061527

1507-
/* Disallow access to the redirection chunk header. */
1508-
VALGRIND_MAKE_MEM_NOACCESS(alignedchunk, sizeof(MemoryChunk));
1528+
/* Now zero (and make DEFINED) just the aligned chunk, if requested */
1529+
if ((flags & MCXT_ALLOC_ZERO) != 0)
1530+
MemSetAligned(aligned, 0, size);
15091531

15101532
return aligned;
15111533
}
@@ -1539,16 +1561,12 @@ void
15391561
pfree(void *pointer)
15401562
{
15411563
#ifdef USE_VALGRIND
1542-
MemoryContextMethodID method = GetMemoryChunkMethodID(pointer);
15431564
MemoryContext context = GetMemoryChunkContext(pointer);
15441565
#endif
15451566

15461567
MCXT_METHOD(pointer, free_p) (pointer);
15471568

1548-
#ifdef USE_VALGRIND
1549-
if (method != MCTX_ALIGNED_REDIRECT_ID)
1550-
VALGRIND_MEMPOOL_FREE(context, pointer);
1551-
#endif
1569+
VALGRIND_MEMPOOL_FREE(context, pointer);
15521570
}
15531571

15541572
/*
@@ -1558,9 +1576,6 @@ pfree(void *pointer)
15581576
void *
15591577
repalloc(void *pointer, Size size)
15601578
{
1561-
#ifdef USE_VALGRIND
1562-
MemoryContextMethodID method = GetMemoryChunkMethodID(pointer);
1563-
#endif
15641579
#if defined(USE_ASSERT_CHECKING) || defined(USE_VALGRIND)
15651580
MemoryContext context = GetMemoryChunkContext(pointer);
15661581
#endif
@@ -1583,10 +1598,7 @@ repalloc(void *pointer, Size size)
15831598
*/
15841599
ret = MCXT_METHOD(pointer, realloc) (pointer, size, 0);
15851600

1586-
#ifdef USE_VALGRIND
1587-
if (method != MCTX_ALIGNED_REDIRECT_ID)
1588-
VALGRIND_MEMPOOL_CHANGE(context, pointer, ret, size);
1589-
#endif
1601+
VALGRIND_MEMPOOL_CHANGE(context, pointer, ret, size);
15901602

15911603
return ret;
15921604
}

0 commit comments

Comments
 (0)