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