From: Tom Lane Date: Fri, 24 Nov 2017 20:50:22 +0000 (-0500) Subject: Mostly-cosmetic improvements in memory chunk header alignment coding. X-Git-Tag: REL_11_BETA1~1172 X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=f65d21b258085bdc8ef2cc282ab1ff12da9c595c;p=postgresql Mostly-cosmetic improvements in memory chunk header alignment coding. Add commentary about what we're doing and why. Apply the method used for padding in GenerationChunk to AllocChunkData, replacing the rather ad-hoc solution used in commit 7e3aa03b4. Reorder fields in GenerationChunk so that the padding calculation will work even if sizeof(size_t) is different from sizeof(void *) --- likely that will never happen, but we don't need the assumption if we do it like this. Improve static assertions about alignment. In passing, fix a couple of oversights in the "large chunk" path in GenerationAlloc(). Discussion: https://postgr.es/m/E1eHa4J-0006hI-Q8@gemulon.postgresql.org --- diff --git a/src/backend/utils/mmgr/aset.c b/src/backend/utils/mmgr/aset.c index 7033042e2d..0b017a4031 100644 --- a/src/backend/utils/mmgr/aset.c +++ b/src/backend/utils/mmgr/aset.c @@ -157,6 +157,14 @@ typedef struct AllocBlockData /* * AllocChunk * The prefix of each piece of memory in an AllocBlock + * + * Note: to meet the memory context APIs, the payload area of the chunk must + * be maxaligned, and the "aset" link must be immediately adjacent to the + * payload area (cf. GetMemoryChunkContext). We simplify matters for this + * module by requiring sizeof(AllocChunkData) to be maxaligned, and then + * we can ensure things work by adding any required alignment padding before + * the "aset" field. There is a static assertion below that the alignment + * is done correctly. */ typedef struct AllocChunkData { @@ -166,15 +174,19 @@ typedef struct AllocChunkData /* 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 +#define ALLOCCHUNK_RAWSIZE (SIZEOF_SIZE_T * 2 + SIZEOF_VOID_P) +#else +#define ALLOCCHUNK_RAWSIZE (SIZEOF_SIZE_T + SIZEOF_VOID_P) #endif /* MEMORY_CONTEXT_CHECKING */ + /* ensure proper alignment by adding padding if needed */ +#if (ALLOCCHUNK_RAWSIZE % MAXIMUM_ALIGNOF) != 0 + char padding[MAXIMUM_ALIGNOF - ALLOCCHUNK_RAWSIZE % MAXIMUM_ALIGNOF]; +#endif + /* 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; @@ -327,8 +339,11 @@ AllocSetContextCreate(MemoryContext parent, { AllocSet set; + /* Assert we padded AllocChunkData properly */ + StaticAssertStmt(ALLOC_CHUNKHDRSZ == MAXALIGN(ALLOC_CHUNKHDRSZ), + "sizeof(AllocChunkData) is not maxaligned"); StaticAssertStmt(offsetof(AllocChunkData, aset) + sizeof(MemoryContext) == - MAXALIGN(sizeof(AllocChunkData)), + ALLOC_CHUNKHDRSZ, "padding calculation in AllocChunkData is wrong"); /* diff --git a/src/backend/utils/mmgr/generation.c b/src/backend/utils/mmgr/generation.c index a748ee266c..8dd0a35095 100644 --- a/src/backend/utils/mmgr/generation.c +++ b/src/backend/utils/mmgr/generation.c @@ -46,19 +46,6 @@ #define Generation_BLOCKHDRSZ MAXALIGN(sizeof(GenerationBlock)) #define Generation_CHUNKHDRSZ sizeof(GenerationChunk) -/* Portion of Generation_CHUNKHDRSZ examined outside generation.c. */ -#define Generation_CHUNK_PUBLIC \ - (offsetof(GenerationChunk, size) + sizeof(Size)) - -/* Portion of Generation_CHUNKHDRSZ excluding trailing padding. */ -#ifdef MEMORY_CONTEXT_CHECKING -#define Generation_CHUNK_USED \ - (offsetof(GenerationChunk, requested_size) + sizeof(Size)) -#else -#define Generation_CHUNK_USED \ - (offsetof(GenerationChunk, size) + sizeof(Size)) -#endif - typedef struct GenerationBlock GenerationBlock; /* forward reference */ typedef struct GenerationChunk GenerationChunk; @@ -103,28 +90,35 @@ struct GenerationBlock /* * GenerationChunk * The prefix of each piece of memory in a GenerationBlock + * + * Note: to meet the memory context APIs, the payload area of the chunk must + * be maxaligned, and the "context" link must be immediately adjacent to the + * payload area (cf. GetMemoryChunkContext). We simplify matters for this + * module by requiring sizeof(GenerationChunk) to be maxaligned, and then + * we can ensure things work by adding any required alignment padding before + * the pointer fields. There is a static assertion below that the alignment + * is done correctly. */ struct GenerationChunk { - /* block owning this chunk */ - void *block; - /* 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; -#define GENERATIONCHUNK_RAWSIZE (SIZEOF_VOID_P * 2 + SIZEOF_SIZE_T * 2) + +#define GENERATIONCHUNK_RAWSIZE (SIZEOF_SIZE_T * 2 + SIZEOF_VOID_P * 2) #else -#define GENERATIONCHUNK_RAWSIZE (SIZEOF_VOID_P * 2 + SIZEOF_SIZE_T) +#define GENERATIONCHUNK_RAWSIZE (SIZEOF_SIZE_T + SIZEOF_VOID_P * 2) #endif /* MEMORY_CONTEXT_CHECKING */ /* ensure proper alignment by adding padding if needed */ #if (GENERATIONCHUNK_RAWSIZE % MAXIMUM_ALIGNOF) != 0 - char padding[MAXIMUM_ALIGNOF - (GENERATIONCHUNK_RAWSIZE % MAXIMUM_ALIGNOF)]; + char padding[MAXIMUM_ALIGNOF - GENERATIONCHUNK_RAWSIZE % MAXIMUM_ALIGNOF]; #endif + GenerationBlock *block; /* block owning this chunk */ GenerationContext *context; /* owning context */ /* there must not be any padding to reach a MAXALIGN boundary here! */ }; @@ -210,8 +204,11 @@ GenerationContextCreate(MemoryContext parent, { GenerationContext *set; + /* Assert we padded GenerationChunk properly */ + StaticAssertStmt(Generation_CHUNKHDRSZ == MAXALIGN(Generation_CHUNKHDRSZ), + "sizeof(GenerationChunk) is not maxaligned"); StaticAssertStmt(offsetof(GenerationChunk, context) + sizeof(MemoryContext) == - MAXALIGN(sizeof(GenerationChunk)), + Generation_CHUNKHDRSZ, "padding calculation in GenerationChunk is wrong"); /* @@ -318,7 +315,6 @@ GenerationAlloc(MemoryContext context, Size size) GenerationContext *set = (GenerationContext *) context; GenerationBlock *block; GenerationChunk *chunk; - Size chunk_size = MAXALIGN(size); /* is it an over-sized chunk? if yes, allocate special block */ @@ -338,6 +334,7 @@ GenerationAlloc(MemoryContext context, Size size) block->freeptr = block->endptr = ((char *) block) + blksize; chunk = (GenerationChunk *) (((char *) block) + Generation_BLOCKHDRSZ); + chunk->block = block; chunk->context = set; chunk->size = chunk_size; @@ -356,17 +353,16 @@ GenerationAlloc(MemoryContext context, Size size) /* add the block to the list of allocated blocks */ dlist_push_head(&set->blocks, &block->node); - GenerationAllocInfo(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 header 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 + Generation_CHUNK_PUBLIC, - chunk_size + Generation_CHUNKHDRSZ - Generation_CHUNK_PUBLIC); + VALGRIND_MAKE_MEM_NOACCESS((char *) chunk + Generation_CHUNKHDRSZ + size, + chunk_size - size); + GenerationAllocInfo(set, chunk); return GenerationChunkGetPointer(chunk); } @@ -442,8 +438,8 @@ GenerationAlloc(MemoryContext context, Size size) /* * GenerationFree - * Update number of chunks on the block, and if all chunks on the block - * are freeed then discard the block. + * Update number of chunks in the block, and if all chunks in the block + * are now free then discard the block. */ static void GenerationFree(MemoryContext context, void *pointer) diff --git a/src/backend/utils/mmgr/slab.c b/src/backend/utils/mmgr/slab.c index 35de6b6d82..ee2175278d 100644 --- a/src/backend/utils/mmgr/slab.c +++ b/src/backend/utils/mmgr/slab.c @@ -91,12 +91,18 @@ typedef struct SlabBlock /* * SlabChunk - * The prefix of each piece of memory in an SlabBlock + * The prefix of each piece of memory in a SlabBlock + * + * Note: to meet the memory context APIs, the payload area of the chunk must + * be maxaligned, and the "slab" link must be immediately adjacent to the + * payload area (cf. GetMemoryChunkContext). Since we support no machines on + * which MAXALIGN is more than twice sizeof(void *), this happens without any + * special hacking in this struct declaration. But there is a static + * assertion below that the alignment is done correctly. */ typedef struct SlabChunk { - /* block owning this chunk */ - void *block; + SlabBlock *block; /* block owning this chunk */ SlabContext *slab; /* owning context */ /* there must not be any padding to reach a MAXALIGN boundary here! */ } SlabChunk; @@ -190,8 +196,11 @@ SlabContextCreate(MemoryContext parent, Size freelistSize; SlabContext *slab; + /* Assert we padded SlabChunk properly */ + StaticAssertStmt(sizeof(SlabChunk) == MAXALIGN(sizeof(SlabChunk)), + "sizeof(SlabChunk) is not maxaligned"); StaticAssertStmt(offsetof(SlabChunk, slab) + sizeof(MemoryContext) == - MAXALIGN(sizeof(SlabChunk)), + sizeof(SlabChunk), "padding calculation in SlabChunk is wrong"); /* Make sure the linked list node fits inside a freed chunk */ @@ -199,7 +208,7 @@ SlabContextCreate(MemoryContext parent, chunkSize = sizeof(int); /* chunk, including SLAB header (both addresses nicely aligned) */ - fullChunkSize = MAXALIGN(sizeof(SlabChunk) + MAXALIGN(chunkSize)); + fullChunkSize = sizeof(SlabChunk) + MAXALIGN(chunkSize); /* Make sure the block can store at least one chunk. */ if (blockSize - sizeof(SlabBlock) < fullChunkSize) @@ -443,7 +452,7 @@ SlabAlloc(MemoryContext context, Size size) /* Prepare to initialize the chunk header. */ VALGRIND_MAKE_MEM_UNDEFINED(chunk, sizeof(SlabChunk)); - chunk->block = (void *) block; + chunk->block = block; chunk->slab = slab; #ifdef MEMORY_CONTEXT_CHECKING