Mostly-cosmetic improvements in memory chunk header alignment coding.

tglsfdc · tglsfdc · commit f65d21b25808 · 2017-11-24T15:50:22.000-05:00
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 7e3aa03. 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
@@ -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
@@ -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
@@ -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,16 +196,19 @@ 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 */
 	if (chunkSize < sizeof(int))
 		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
