Subject: Re: PATCH: two slab-like memory allocators
To: Andres Freund <andres@anarazel.de>, Tom Lane <tgl@sss.pgh.pa.us>
References: <20170214022221.obhdeyfn34h6hkwj@alap3.anarazel.de>
 <19464ac8-e66b-3598-858f-29c749bec35a@2ndquadrant.com>
 <b3b2245c-b37a-e1e5-ebc4-857c914bc747@2ndquadrant.com>
 <20170224221038.43xf2cyonczzr6hb@alap3.anarazel.de>
 <20170227111732.vrx5v72ighehwpkf@alap3.anarazel.de>
 <20170227120200.5jc3ubg5jrqaznd7@alap3.anarazel.de>
 <26061.1488209545@sss.pgh.pa.us>
 <20170227155532.o7casczq5parlmz4@alap3.anarazel.de>
 <20170227164828.akgzeg6ggl2v4x7s@alap3.anarazel.de>
 <12622.1488216468@sss.pgh.pa.us>
 <20170227174201.yrofai7neyp6shb2@alap3.anarazel.de>
Cc: Petr Jelinek <petr.jelinek@2ndquadrant.com>,
 Petr Jelinek <petr@2ndquadrant.com>, Robert Haas <robertmhaas@gmail.com>,
 John Gorman <johngorman2@gmail.com>,
 pgsql-hackers <pgsql-hackers@postgresql.org>
From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
Message-ID: <c7c1bb85-d1d0-05a5-4d4c-3047d6f053e7@2ndquadrant.com>
Date: Tue, 28 Feb 2017 01:44:42 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101
 Thunderbird/45.4.0
MIME-Version: 1.0
In-Reply-To: <20170227174201.yrofai7neyp6shb2@alap3.anarazel.de>
Content-Type: multipart/mixed;
 boundary="------------B6A19CCF20279557013F9A3E"
Precedence: bulk
Sender: pgsql-hackers-owner@postgresql.org

This is a multi-part message in MIME format.
--------------B6A19CCF20279557013F9A3E
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Transfer-Encoding: 7bit

On 02/27/2017 06:42 PM, Andres Freund wrote:
> On 2017-02-27 12:27:48 -0500, Tom Lane wrote:
>> Andres Freund <andres@anarazel.de> writes:
>>> The best theory I have so far that I have is that slab.c's idea of
>>> StandardChunkHeader's size doesn't match what mcxt.c think it is
>>> (because slab.c simply embeds StandardChunkHeader, but mcxt uses
>>> MAXALIGN(sizeof(StandardChunkHeader))).  That's not good, but I don't
>>> quite see how that'd cause the issue, since StandardChunkHeader's size
>>> should always be properly sized.
>>
>> Uh, wrong.  On a 32-bit machine with debug enabled, StandardChunkHeader
>> will contain 3 4-byte fields.  However, there are some such machines on
>> which MAXALIGN is 8.  For example, looking at termite's configure
>> output:
>>
>> checking size of void *... 4
>> checking size of size_t... 4
>> checking size of long... 4
>> checking alignment of short... 2
>> checking alignment of int... 4
>> checking alignment of long... 4
>> checking alignment of long long int... 8
>> checking alignment of double... 8
>>
>> axolotl's output looks similar.  I expect my old HPPA dinosaur
>> will show the failure as well.
>
> Yea, I hadn't yet realized when writing that that termite actually,
> despite running on ppc64, compiles a 32bit postgres. Will thus
> duplicate StandardChunkHeader's contents in to slab.c :( - I don't
> see an easy way around that...
>

I've tried this - essentially copying the StandardChunkHeader's contents 
into SlabChunk, but that does not seem to do the trick, sadly. Per 
pahole, the structures then (at least on armv7l) look like this:

     struct SlabChunk {
         void *                     block;            /*  0   4 */
         MemoryContext              context;          /*  4   4 */
         Size                       size;             /*  8   4 */
         Size                       requested_size;   /* 12   4 */

         /* size: 16, cachelines: 1, members: 4 */
         /* last cacheline: 16 bytes */
     };

     struct StandardChunkHeader {
         MemoryContext              context;          /*  0   4 */
         Size                       size;             /*  4   4 */
         Size                       requested_size;   /*  8   4 */

         /* size: 12, cachelines: 1, members: 3 */
         /* last cacheline: 12 bytes */
     };

So SlabChunk happens to be perfectly aligned (MAXIMUM_ALIGNOF=8), and so 
pfree() grabs the block pointer but thinks it's the context :-(

Not sure what to do about this - the only thing I can think about is 
splitting SlabChunk into two separate structures, and align them 
independently.

The attached patch does that - it probably needs a bit more work on the 
comments to make it commit-ready, but it fixes the test_deconding tests 
on the rpi3 board I'm using for testing.

regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--------------B6A19CCF20279557013F9A3E
Content-Type: binary/octet-stream;
 name="slab-fix.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename="slab-fix.patch"

diff --git a/src/backend/utils/mmgr/slab.c b/src/backend/utils/mmgr/slab.c
index c673bc3..6fbdb54 100644
--- a/src/backend/utils/mmgr/slab.c
+++ b/src/backend/utils/mmgr/slab.c
@@ -57,11 +57,11 @@
 #include "lib/ilist.h"
 
 
-#define SLAB_CHUNKHDRSZ MAXALIGN(sizeof(SlabChunk))
+#define SLAB_CHUNKHDRSZ (MAXALIGN(sizeof(SlabChunk)) + STANDARDCHUNKHEADERSIZE)
 
 /* Portion of SLAB_CHUNKHDRSZ excluding trailing padding. */
 #define SLAB_CHUNK_USED \
-	(offsetof(SlabChunk, header) + sizeof(StandardChunkHeader))
+	(offsetof(SlabChunk, header) + STANDARDCHUNKHEADERSIZE)
 
 /*
  * SlabContext is a specialized implementation of MemoryContext.
@@ -103,10 +103,6 @@ typedef struct SlabChunk
 {
 	/* block owning this chunk */
 	void	   *block;
-
-	/* include StandardChunkHeader because mcxt.c expects that */
-	StandardChunkHeader header;
-
 } SlabChunk;
 
 
@@ -121,6 +117,8 @@ typedef struct SlabChunk
 	((char *) block + sizeof(SlabBlock))
 #define SlabChunkIndex(slab, block, chunk)	\
 	(((char *) chunk - SlabBlockStart(block)) / slab->fullChunkSize)
+#define SlabChunkStandardHeader(chunk) \
+	( (StandardChunkHeader *) ((char *)chunk + MAXALIGN(sizeof(SlabChunk))))
 
 /*
  * These functions implement the MemoryContext API for Slab contexts.
@@ -164,10 +162,10 @@ static MemoryContextMethods SlabMethods = {
 #ifdef HAVE_ALLOCINFO
 #define SlabFreeInfo(_cxt, _chunk) \
 			fprintf(stderr, "SlabFree: %s: %p, %zu\n", \
-				(_cxt)->header.name, (_chunk), (_chunk)->header.size)
+				(_cxt)->header.name, (_chunk), (_chunk)->size)
 #define SlabAllocInfo(_cxt, _chunk) \
 			fprintf(stderr, "SlabAlloc: %s: %p, %zu\n", \
-				(_cxt)->header.name, (_chunk), (_chunk)->header.size)
+				(_cxt)->header.name, (_chunk), (_chunk)->size)
 #else
 #define SlabFreeInfo(_cxt, _chunk)
 #define SlabAllocInfo(_cxt, _chunk)
@@ -203,7 +201,8 @@ SlabContextCreate(MemoryContext parent,
 					 "MAXALIGN too small to fit int32");
 
 	/* chunk, including SLAB header (both addresses nicely aligned) */
-	fullChunkSize = MAXALIGN(sizeof(SlabChunk) + MAXALIGN(chunkSize));
+	fullChunkSize = MAXALIGN(sizeof(SlabChunk))
+		+ MAXALIGN(sizeof(StandardChunkHeader)) + MAXALIGN(chunkSize);
 
 	/* Make sure the block can store at least one chunk. */
 	if (blockSize - sizeof(SlabBlock) < fullChunkSize)
@@ -324,6 +323,7 @@ SlabAlloc(MemoryContext context, Size size)
 	SlabContext *slab = castNode(SlabContext, context);
 	SlabBlock  *block;
 	SlabChunk  *chunk;
+	StandardChunkHeader *header;
 	int			idx;
 
 	Assert(slab);
@@ -394,6 +394,7 @@ SlabAlloc(MemoryContext context, Size size)
 
 	/* compute the chunk location block start (after the block header) */
 	chunk = SlabBlockGetChunk(slab, block, idx);
+	header = SlabChunkStandardHeader(chunk);
 
 	/*
 	 * Update the block nfree count, and also the minFreeChunks as we've
@@ -449,15 +450,15 @@ SlabAlloc(MemoryContext context, Size size)
 
 	chunk->block = (void *) block;
 
-	chunk->header.context = (MemoryContext) slab;
-	chunk->header.size = MAXALIGN(size);
+	header->context = (MemoryContext) slab;
+	header->size = MAXALIGN(size);
 
 #ifdef MEMORY_CONTEXT_CHECKING
-	chunk->header.requested_size = size;
-	VALGRIND_MAKE_MEM_NOACCESS(&chunk->header.requested_size,
-							   sizeof(chunk->header.requested_size));
+	header->requested_size = size;
+	VALGRIND_MAKE_MEM_NOACCESS(&header->requested_size,
+							   sizeof(header->requested_size));
 	/* slab mark to catch clobber of "unused" space */
-	if (size < chunk->header.size)
+	if (size < header->size)
 		set_sentinel(SlabChunkGetPointer(chunk), size);
 #endif
 #ifdef RANDOMIZE_ALLOCATED_MEMORY
@@ -480,15 +481,16 @@ SlabFree(MemoryContext context, void *pointer)
 	SlabContext *slab = castNode(SlabContext, context);
 	SlabChunk  *chunk = SlabPointerGetChunk(pointer);
 	SlabBlock  *block = chunk->block;
+	StandardChunkHeader *header = SlabChunkStandardHeader(chunk);
 
 	SlabFreeInfo(slab, chunk);
 
 #ifdef MEMORY_CONTEXT_CHECKING
-	VALGRIND_MAKE_MEM_DEFINED(&chunk->header.requested_size,
-							  sizeof(chunk->header.requested_size));
+	VALGRIND_MAKE_MEM_DEFINED(&header->requested_size,
+							  sizeof(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 (header->requested_size < header->size)
+		if (!sentinel_ok(pointer, header->requested_size))
 			elog(WARNING, "detected write past chunk end in %s %p",
 				 slab->header.name, chunk);
 #endif
@@ -507,12 +509,12 @@ 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));
+			 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;
+	header->requested_size = 0;
 #endif
 
 	/* remove the block from a freelist */
@@ -591,8 +593,9 @@ static Size
 SlabGetChunkSpace(MemoryContext context, void *pointer)
 {
 	SlabChunk  *chunk = SlabPointerGetChunk(pointer);
+	StandardChunkHeader *header = SlabChunkStandardHeader(chunk);
 
-	return chunk->header.size + SLAB_CHUNKHDRSZ;
+	return header->size + SLAB_CHUNKHDRSZ;
 }
 
 /*
@@ -741,36 +744,37 @@ SlabCheck(MemoryContext context)
 				if (!freechunks[j])
 				{
 					SlabChunk  *chunk = SlabBlockGetChunk(slab, block, j);
+					StandardChunkHeader *header = SlabChunkStandardHeader(chunk);
 
-					VALGRIND_MAKE_MEM_DEFINED(&chunk->header.requested_size,
-									   sizeof(chunk->header.requested_size));
+					VALGRIND_MAKE_MEM_DEFINED(&header->requested_size,
+									   sizeof(header->requested_size));
 
 					/* we're in a no-freelist branch */
-					VALGRIND_MAKE_MEM_NOACCESS(&chunk->header.requested_size,
-									   sizeof(chunk->header.requested_size));
+					VALGRIND_MAKE_MEM_NOACCESS(&header->requested_size,
+									   sizeof(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 (header->context != (MemoryContext) 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))
+					if (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)
+					if (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))
+					if (header->requested_size < header->size &&
+						!sentinel_ok(chunk, SLAB_CHUNKHDRSZ + header->requested_size))
 						elog(WARNING, "problem in slab %s: detected write past chunk end in block %p, chunk %p",
 							 name, block, chunk);
 				}

--------------B6A19CCF20279557013F9A3E
Content-Type: text/plain
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

--------------B6A19CCF20279557013F9A3E--