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 7033042e2d89cbdd058e27f6cc8cc5f77a5a49b2..0b017a403165cf475f833d297c78b6a9af058ef1 100644 (file)
--- 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 a748ee266c2010b0e37512d2468499fe4256f937..8dd0a3509564de06ebc6d7708512212918e8f37e 100644 (file)
--- 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 35de6b6d82a537f708987691677d65112473f924..ee2175278d2e1ed4a064ccde8f1fdf089ffdedda 100644 (file)
--- 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