Hi,
*preliminary* patch attached. This needs a good bit of polishing
(primarily comment work, verifying that valgrind works), but I'm too
tired now.
I'm not quite sure how to deal with mmgr/README - it's written as kind
of a historical document, and the "Mechanisms to Allow Multiple Types of
Contexts" is already quite out of date. I think it'd be good to rip out
all the historical references and just describe the current state, but
I'm not really enthusiastic about tackling that :/
On 2017-02-28 00:29:44 -0500, Tom Lane wrote:
> Not sure either, but suggest we add a StaticAssert asserting there's no
> padding; something along the lines of
> offsetof(AllocSetChunkHeader, context) + sizeof(MemoryContext) ==
> MAXALIGN(sizeof(AllocSetChunkHeader))
Included.
With Craig's help I've verified that the patch as attached works on a
platform that's currently failing. Thanks!
- Andres
>From a59c3200dd127feb0cb09a055250ff6401aee1aa Mon Sep 17 00:00:00 2001
From: Andres Freund <and...@anarazel.de>
Date: Mon, 27 Feb 2017 23:32:22 -0800
Subject: [PATCH] Remove StandardChunkHeader for Slab's benefit.
---
src/backend/utils/mmgr/aset.c | 51 ++++++++---------
src/backend/utils/mmgr/mcxt.c | 121 +++------------------------------------
src/backend/utils/mmgr/slab.c | 81 ++++++++------------------
src/include/utils/memutils.h | 56 +++++++++++-------
src/tools/pgindent/typedefs.list | 1 -
5 files changed, 92 insertions(+), 218 deletions(-)
diff --git a/src/backend/utils/mmgr/aset.c b/src/backend/utils/mmgr/aset.c
index 8056c00ae4..31228ade19 100644
--- a/src/backend/utils/mmgr/aset.c
+++ b/src/backend/utils/mmgr/aset.c
@@ -97,20 +97,7 @@
*/
#define ALLOC_BLOCKHDRSZ MAXALIGN(sizeof(AllocBlockData))
-#define ALLOC_CHUNKHDRSZ MAXALIGN(sizeof(AllocChunkData))
-
-/* Portion of ALLOC_CHUNKHDRSZ examined outside aset.c. */
-#define ALLOC_CHUNK_PUBLIC \
- (offsetof(AllocChunkData, size) + sizeof(Size))
-
-/* Portion of ALLOC_CHUNKHDRSZ excluding trailing padding. */
-#ifdef MEMORY_CONTEXT_CHECKING
-#define ALLOC_CHUNK_USED \
- (offsetof(AllocChunkData, requested_size) + sizeof(Size))
-#else
-#define ALLOC_CHUNK_USED \
- (offsetof(AllocChunkData, size) + sizeof(Size))
-#endif
+#define ALLOC_CHUNKHDRSZ sizeof(struct AllocChunkData)
typedef struct AllocBlockData *AllocBlock; /* forward reference */
typedef struct AllocChunkData *AllocChunk;
@@ -169,20 +156,27 @@ typedef struct AllocBlockData
/*
* AllocChunk
* The prefix of each piece of memory in an AllocBlock
- *
- * NB: this MUST match StandardChunkHeader as defined by utils/memutils.h.
*/
typedef struct AllocChunkData
{
- /* aset is the owning aset if allocated, or the freelist link if free */
- void *aset;
/* size is always the size of the usable space in the chunk */
Size size;
+
#ifdef MEMORY_CONTEXT_CHECKING
/* when debugging memory usage, also store actual requested size */
/* this is zero in a free chunk */
Size requested_size;
+
+#if MAXIMUM_ALIGNOF > 4 && SIZEOF_VOID_P == 4
+ Size padding;
#endif
+
+#endif /* MEMORY_CONTEXT_CHECKING */
+
+ /* aset is the owning aset if allocated, or the freelist link if free */
+ void *aset;
+
+ /* there must not be any padding to reach a MAXALIGN boundary here! */
} AllocChunkData;
/*
@@ -334,6 +328,10 @@ AllocSetContextCreate(MemoryContext parent,
{
AllocSet set;
+ StaticAssertStmt(offsetof(AllocChunkData, aset) + sizeof(MemoryContext) ==
+ MAXALIGN(sizeof(AllocChunkData)),
+ "padding calculation in AllocChunkData is wrong");
+
/*
* First, validate allocation parameters. (If we're going to throw an
* error, we should do so before the context is created, not after.) We
@@ -616,13 +614,14 @@ AllocSetAlloc(MemoryContext context, Size size)
AllocAllocInfo(set, chunk);
/*
- * Chunk header public fields remain DEFINED. The requested
- * allocation itself can be NOACCESS or UNDEFINED; our caller will
- * soon make it UNDEFINED. Make extra space at the end of the chunk,
- * if any, NOACCESS.
+ * Chunk's context fields remain DEFINED. The requested allocation
+ * itself can be NOACCESS or UNDEFINED; our caller will soon make it
+ * UNDEFINED. Make extra space at the end of the chunk, if any,
+ * NOACCESS.
*/
- VALGRIND_MAKE_MEM_NOACCESS((char *) chunk + ALLOC_CHUNK_PUBLIC,
- chunk_size + ALLOC_CHUNKHDRSZ - ALLOC_CHUNK_PUBLIC);
+ VALGRIND_MAKE_MEM_NOACCESS((char *) chunk,
+ chunk_size + ALLOC_CHUNKHDRSZ);
+ VALGRIND_MAKE_MEM_DEFINED(chunk->aset, sizeof(MemoryContext));
return AllocChunkGetPointer(chunk);
}
@@ -709,7 +708,7 @@ AllocSetAlloc(MemoryContext context, Size size)
chunk = (AllocChunk) (block->freeptr);
/* Prepare to initialize the chunk header. */
- VALGRIND_MAKE_MEM_UNDEFINED(chunk, ALLOC_CHUNK_USED);
+ VALGRIND_MAKE_MEM_UNDEFINED(chunk, sizeof(AllocChunkData));
block->freeptr += (availchunk + ALLOC_CHUNKHDRSZ);
availspace -= (availchunk + ALLOC_CHUNKHDRSZ);
@@ -799,7 +798,7 @@ AllocSetAlloc(MemoryContext context, Size size)
chunk = (AllocChunk) (block->freeptr);
/* Prepare to initialize the chunk header. */
- VALGRIND_MAKE_MEM_UNDEFINED(chunk, ALLOC_CHUNK_USED);
+ VALGRIND_MAKE_MEM_UNDEFINED(chunk, sizeof(AllocChunkData));
block->freeptr += (chunk_size + ALLOC_CHUNKHDRSZ);
Assert(block->freeptr <= block->endptr);
diff --git a/src/backend/utils/mmgr/mcxt.c b/src/backend/utils/mmgr/mcxt.c
index 6ad0bb47b6..80b41b0e65 100644
--- a/src/backend/utils/mmgr/mcxt.c
+++ b/src/backend/utils/mmgr/mcxt.c
@@ -389,55 +389,10 @@ MemoryContextAllowInCriticalSection(MemoryContext context, bool allow)
Size
GetMemoryChunkSpace(void *pointer)
{
- StandardChunkHeader *header;
+ MemoryContext context = GetMemoryChunkContext(pointer);
- /*
- * Try to detect bogus pointers handed to us, poorly though we can.
- * Presumably, a pointer that isn't MAXALIGNED isn't pointing at an
- * allocated chunk.
- */
- Assert(pointer != NULL);
- Assert(pointer == (void *) MAXALIGN(pointer));
-
- /*
- * OK, it's probably safe to look at the chunk header.
- */
- header = (StandardChunkHeader *)
- ((char *) pointer - STANDARDCHUNKHEADERSIZE);
-
- AssertArg(MemoryContextIsValid(header->context));
-
- return (*header->context->methods->get_chunk_space) (header->context,
- pointer);
-}
-
-/*
- * GetMemoryChunkContext
- * Given a currently-allocated chunk, determine the context
- * it belongs to.
- */
-MemoryContext
-GetMemoryChunkContext(void *pointer)
-{
- StandardChunkHeader *header;
-
- /*
- * Try to detect bogus pointers handed to us, poorly though we can.
- * Presumably, a pointer that isn't MAXALIGNED isn't pointing at an
- * allocated chunk.
- */
- Assert(pointer != NULL);
- Assert(pointer == (void *) MAXALIGN(pointer));
-
- /*
- * OK, it's probably safe to look at the chunk header.
- */
- header = (StandardChunkHeader *)
- ((char *) pointer - STANDARDCHUNKHEADERSIZE);
-
- AssertArg(MemoryContextIsValid(header->context));
-
- return header->context;
+ return (context->methods->get_chunk_space) (context,
+ pointer);
}
/*
@@ -611,23 +566,9 @@ MemoryContextCheck(MemoryContext context)
bool
MemoryContextContains(MemoryContext context, void *pointer)
{
- StandardChunkHeader *header;
+ MemoryContext ptr_context = GetMemoryChunkContext(pointer);
- /*
- * Try to detect bogus pointers handed to us, poorly though we can.
- * Presumably, a pointer that isn't MAXALIGNED isn't pointing at an
- * allocated chunk.
- */
- if (pointer == NULL || pointer != (void *) MAXALIGN(pointer))
- return false;
-
- /*
- * OK, it's probably safe to look at the chunk header.
- */
- header = (StandardChunkHeader *)
- ((char *) pointer - STANDARDCHUNKHEADERSIZE);
-
- return header->context == context;
+ return ptr_context == context;
}
/*--------------------
@@ -991,23 +932,7 @@ palloc_extended(Size size, int flags)
void
pfree(void *pointer)
{
- MemoryContext context;
-
- /*
- * Try to detect bogus pointers handed to us, poorly though we can.
- * Presumably, a pointer that isn't MAXALIGNED isn't pointing at an
- * allocated chunk.
- */
- Assert(pointer != NULL);
- Assert(pointer == (void *) MAXALIGN(pointer));
-
- /*
- * OK, it's probably safe to look at the chunk header.
- */
- context = ((StandardChunkHeader *)
- ((char *) pointer - STANDARDCHUNKHEADERSIZE))->context;
-
- AssertArg(MemoryContextIsValid(context));
+ MemoryContext context = GetMemoryChunkContext(pointer);
(*context->methods->free_p) (context, pointer);
VALGRIND_MEMPOOL_FREE(context, pointer);
@@ -1020,27 +945,12 @@ pfree(void *pointer)
void *
repalloc(void *pointer, Size size)
{
- MemoryContext context;
+ MemoryContext context = GetMemoryChunkContext(pointer);
void *ret;
if (!AllocSizeIsValid(size))
elog(ERROR, "invalid memory alloc request size %zu", size);
- /*
- * Try to detect bogus pointers handed to us, poorly though we can.
- * Presumably, a pointer that isn't MAXALIGNED isn't pointing at an
- * allocated chunk.
- */
- Assert(pointer != NULL);
- Assert(pointer == (void *) MAXALIGN(pointer));
-
- /*
- * OK, it's probably safe to look at the chunk header.
- */
- context = ((StandardChunkHeader *)
- ((char *) pointer - STANDARDCHUNKHEADERSIZE))->context;
-
- AssertArg(MemoryContextIsValid(context));
AssertNotInCriticalSection(context);
/* isReset must be false already */
@@ -1103,27 +1013,12 @@ MemoryContextAllocHuge(MemoryContext context, Size size)
void *
repalloc_huge(void *pointer, Size size)
{
- MemoryContext context;
+ MemoryContext context = GetMemoryChunkContext(pointer);
void *ret;
if (!AllocHugeSizeIsValid(size))
elog(ERROR, "invalid memory alloc request size %zu", size);
- /*
- * Try to detect bogus pointers handed to us, poorly though we can.
- * Presumably, a pointer that isn't MAXALIGNED isn't pointing at an
- * allocated chunk.
- */
- Assert(pointer != NULL);
- Assert(pointer == (void *) MAXALIGN(pointer));
-
- /*
- * OK, it's probably safe to look at the chunk header.
- */
- context = ((StandardChunkHeader *)
- ((char *) pointer - STANDARDCHUNKHEADERSIZE))->context;
-
- AssertArg(MemoryContextIsValid(context));
AssertNotInCriticalSection(context);
/* isReset must be false already */
diff --git a/src/backend/utils/mmgr/slab.c b/src/backend/utils/mmgr/slab.c
index a5e140eef7..99956fc279 100644
--- a/src/backend/utils/mmgr/slab.c
+++ b/src/backend/utils/mmgr/slab.c
@@ -57,12 +57,6 @@
#include "lib/ilist.h"
-#define SLAB_CHUNKHDRSZ MAXALIGN(sizeof(SlabChunk))
-
-/* Portion of SLAB_CHUNKHDRSZ excluding trailing padding. */
-#define SLAB_CHUNK_USED \
- (offsetof(SlabChunk, header) + sizeof(StandardChunkHeader))
-
/*
* SlabContext is a specialized implementation of MemoryContext.
*/
@@ -103,17 +97,15 @@ typedef struct SlabChunk
{
/* block owning this chunk */
void *block;
-
- /* include StandardChunkHeader because mcxt.c expects that */
- StandardChunkHeader header;
-
+ SlabContext *slab; /* owning context */
+ /* there must not be any padding to reach a MAXALIGN boundary here! */
} SlabChunk;
#define SlabPointerGetChunk(ptr) \
- ((SlabChunk *)(((char *)(ptr)) - SLAB_CHUNKHDRSZ))
+ ((SlabChunk *)(((char *)(ptr)) - sizeof(SlabChunk)))
#define SlabChunkGetPointer(chk) \
- ((void *)(((char *)(chk)) + SLAB_CHUNKHDRSZ))
+ ((void *)(((char *)(chk)) + sizeof(SlabChunk)))
#define SlabBlockGetChunk(slab, block, idx) \
((SlabChunk *) ((char *) (block) + sizeof(SlabBlock) \
+ (idx * slab->fullChunkSize)))
@@ -198,6 +190,10 @@ SlabContextCreate(MemoryContext parent,
Size freelistSize;
SlabContext *slab;
+ StaticAssertStmt(offsetof(SlabChunk, slab) + sizeof(MemoryContext) ==
+ MAXALIGN(sizeof(SlabChunk)),
+ "padding calculation in SlabChunk is wrong");
+
/* otherwise the linked list inside freed chunk isn't guaranteed to fit */
StaticAssertStmt(MAXIMUM_ALIGNOF >= sizeof(int),
"MAXALIGN too small to fit int32");
@@ -207,7 +203,7 @@ SlabContextCreate(MemoryContext parent,
/* Make sure the block can store at least one chunk. */
if (blockSize - sizeof(SlabBlock) < fullChunkSize)
- elog(ERROR, "block size %ld for slab is too small for %ld chunks",
+ elog(ERROR, "block size %zu for slab is too small for %zu chunks",
blockSize, chunkSize);
/* Compute maximum number of chunks per block */
@@ -333,7 +329,7 @@ SlabAlloc(MemoryContext context, Size size)
/* make sure we only allow correct request size */
if (size != slab->chunkSize)
- elog(ERROR, "unexpected alloc chunk size %ld (expected %ld)",
+ elog(ERROR, "unexpected alloc chunk size %zu (expected %zu)",
size, slab->chunkSize);
/*
@@ -445,19 +441,14 @@ SlabAlloc(MemoryContext context, Size size)
slab->minFreeChunks = 0;
/* Prepare to initialize the chunk header. */
- VALGRIND_MAKE_MEM_UNDEFINED(chunk, SLAB_CHUNK_USED);
+ VALGRIND_MAKE_MEM_UNDEFINED(chunk, sizeof(SlabChunk));
chunk->block = (void *) block;
-
- chunk->header.context = (MemoryContext) slab;
- chunk->header.size = MAXALIGN(size);
+ chunk->slab = slab;
#ifdef MEMORY_CONTEXT_CHECKING
- chunk->header.requested_size = size;
- VALGRIND_MAKE_MEM_NOACCESS(&chunk->header.requested_size,
- sizeof(chunk->header.requested_size));
/* slab mark to catch clobber of "unused" space */
- if (size < chunk->header.size)
+ if (slab->chunkSize < (slab->fullChunkSize - sizeof(SlabChunk)))
set_sentinel(SlabChunkGetPointer(chunk), size);
#endif
#ifdef RANDOMIZE_ALLOCATED_MEMORY
@@ -484,11 +475,9 @@ SlabFree(MemoryContext context, void *pointer)
SlabFreeInfo(slab, chunk);
#ifdef MEMORY_CONTEXT_CHECKING
- VALGRIND_MAKE_MEM_DEFINED(&chunk->header.requested_size,
- sizeof(chunk->header.requested_size));
/* Test for someone scribbling on unused space in chunk */
- if (chunk->header.requested_size < chunk->header.size)
- if (!sentinel_ok(pointer, chunk->header.requested_size))
+ if (slab->chunkSize < (slab->fullChunkSize - sizeof(SlabChunk)))
+ if (!sentinel_ok(pointer, slab->chunkSize))
elog(WARNING, "detected write past chunk end in %s %p",
slab->header.name, chunk);
#endif
@@ -507,12 +496,7 @@ SlabFree(MemoryContext context, void *pointer)
#ifdef CLOBBER_FREED_MEMORY
/* XXX don't wipe the int32 index, used for block-level freelist */
wipe_mem((char *) pointer + sizeof(int32),
- chunk->header.size - sizeof(int32));
-#endif
-
-#ifdef MEMORY_CONTEXT_CHECKING
- /* Reset requested_size to 0 in chunks that are on freelist */
- chunk->header.requested_size = 0;
+ slab->chunkSize - sizeof(int32));
#endif
/* remove the block from a freelist */
@@ -590,9 +574,11 @@ SlabRealloc(MemoryContext context, void *pointer, Size size)
static Size
SlabGetChunkSpace(MemoryContext context, void *pointer)
{
- SlabChunk *chunk = SlabPointerGetChunk(pointer);
+ SlabContext *slab = castNode(SlabContext, context);
- return chunk->header.size + SLAB_CHUNKHDRSZ;
+ Assert(slab);
+
+ return slab->fullChunkSize;
}
/*
@@ -742,37 +728,20 @@ SlabCheck(MemoryContext context)
{
SlabChunk *chunk = SlabBlockGetChunk(slab, block, j);
- VALGRIND_MAKE_MEM_DEFINED(&chunk->header.requested_size,
- sizeof(chunk->header.requested_size));
-
- /* we're in a no-freelist branch */
- VALGRIND_MAKE_MEM_NOACCESS(&chunk->header.requested_size,
- sizeof(chunk->header.requested_size));
-
/* chunks have both block and slab pointers, so check both */
if (chunk->block != block)
elog(WARNING, "problem in slab %s: bogus block link in block %p, chunk %p",
name, block, chunk);
- if (chunk->header.context != (MemoryContext) slab)
+ if (chunk->slab != slab)
elog(WARNING, "problem in slab %s: bogus slab link in block %p, chunk %p",
name, block, chunk);
- /* now make sure the chunk size is correct */
- if (chunk->header.size != MAXALIGN(slab->chunkSize))
- elog(WARNING, "problem in slab %s: bogus chunk size in block %p, chunk %p",
- name, block, chunk);
-
- /* now make sure the chunk size is correct */
- if (chunk->header.requested_size != slab->chunkSize)
- elog(WARNING, "problem in slab %s: bogus chunk requested size in block %p, chunk %p",
- name, block, chunk);
-
/* there might be sentinel (thanks to alignment) */
- if (chunk->header.requested_size < chunk->header.size &&
- !sentinel_ok(chunk, SLAB_CHUNKHDRSZ + chunk->header.requested_size))
- elog(WARNING, "problem in slab %s: detected write past chunk end in block %p, chunk %p",
- name, block, chunk);
+ if (slab->chunkSize < (slab->fullChunkSize - sizeof(SlabChunk)))
+ if (!sentinel_ok(chunk, slab->chunkSize))
+ elog(WARNING, "problem in slab %s: detected write past chunk end in block %p, chunk %p",
+ name, block, chunk);
}
}
diff --git a/src/include/utils/memutils.h b/src/include/utils/memutils.h
index 5223a4da39..930e469858 100644
--- a/src/include/utils/memutils.h
+++ b/src/include/utils/memutils.h
@@ -45,27 +45,6 @@
#define AllocHugeSizeIsValid(size) ((Size) (size) <= MaxAllocHugeSize)
-/*
- * All chunks allocated by any memory context manager are required to be
- * preceded by a StandardChunkHeader at a spacing of STANDARDCHUNKHEADERSIZE.
- * A currently-allocated chunk must contain a backpointer to its owning
- * context as well as the allocated size of the chunk. The backpointer is
- * used by pfree() and repalloc() to find the context to call. The allocated
- * size is not absolutely essential, but it's expected to be needed by any
- * reasonable implementation.
- */
-typedef struct StandardChunkHeader
-{
- MemoryContext context; /* owning context */
- Size size; /* size of data space allocated in chunk */
-#ifdef MEMORY_CONTEXT_CHECKING
- /* when debugging memory usage, also store actual requested size */
- Size requested_size;
-#endif
-} StandardChunkHeader;
-
-#define STANDARDCHUNKHEADERSIZE MAXALIGN(sizeof(StandardChunkHeader))
-
/*
* Standard top-level memory contexts.
@@ -100,7 +79,6 @@ extern void MemoryContextDeleteChildren(MemoryContext context);
extern void MemoryContextSetParent(MemoryContext context,
MemoryContext new_parent);
extern Size GetMemoryChunkSpace(void *pointer);
-extern MemoryContext GetMemoryChunkContext(void *pointer);
extern MemoryContext MemoryContextGetParent(MemoryContext context);
extern bool MemoryContextIsEmpty(MemoryContext context);
extern void MemoryContextStats(MemoryContext context);
@@ -114,6 +92,40 @@ extern void MemoryContextCheck(MemoryContext context);
extern bool MemoryContextContains(MemoryContext context, void *pointer);
/*
+ * GetMemoryChunkContext
+ * Given a currently-allocated chunk, determine the context
+ * it belongs to.
+ *
+ * All chunks allocated by any memory context manager are required to be
+ * preceded by the corresponding MemoryContext stored, without padding, in the
+ * preceding sizeof(void*) bytes. A currently-allocated chunk must contain a
+ * backpointer to its owning context. The backpointer is used by pfree() and
+ * repalloc() to find the context to call.
+ */
+static inline MemoryContext
+GetMemoryChunkContext(void *pointer)
+{
+ MemoryContext context;
+
+ /*
+ * Try to detect bogus pointers handed to us, poorly though we can.
+ * Presumably, a pointer that isn't MAXALIGNED isn't pointing at an
+ * allocated chunk.
+ */
+ Assert(pointer != NULL);
+ Assert(pointer == (void *) MAXALIGN(pointer));
+
+ /*
+ * OK, it's probably safe to look at the context.
+ */
+ context = *(MemoryContext*) (((char *) pointer) - sizeof(void *));
+
+ AssertArg(MemoryContextIsValid(context));
+
+ return context;
+}
+
+/*
* This routine handles the context-type-independent part of memory
* context creation. It's intended to be called from context-type-
* specific creation routines, and noplace else.
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index 1fd7ec4256..6717eccbb0 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -2001,7 +2001,6 @@ SplitLR
SplitVar
SplitedPageLayout
StackElem
-StandardChunkHeader
StartBlobPtr
StartBlobsPtr
StartDataPtr
--
2.11.0.22.g8d7a455.dirty
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
