From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
To: Andres Freund <andres@anarazel.de>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Petr Jelinek <petr.jelinek@2ndquadrant.com>
Cc: Petr Jelinek <petr@2ndquadrant.com>
Cc: Robert Haas <robertmhaas@gmail.com>
Cc: John Gorman <johngorman2@gmail.com>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: PATCH: two slab-like memory allocators
Date: Tue, 28 Feb 2017 01:44:42 +0100
Message-ID: <c7c1bb85-d1d0-05a5-4d4c-3047d6f053e7@2ndquadrant.com> (raw)
In-Reply-To: <20170227174201.yrofai7neyp6shb2@alap3.anarazel.de>
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>
List-Unsubscribe: <mailto:majordomo@postgresql.org?body=unsub%20pgsql-hackers>
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackersAttachments:
[binary/octet-stream] slab-fix.patch (7.3K, ../c7c1bb85-d1d0-05a5-4d4c-3047d6f053e7@2ndquadrant.com/2-slab-fix.patch)
download | inline diff:diff --git a/src/backend/utils/mmgr/slab.c b/src/backend/utils/mmgr/slab.cindex 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);
}
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: tomas.vondra@2ndquadrant.com, andres@anarazel.de, tgl@sss.pgh.pa.us, petr.jelinek@2ndquadrant.com, petr@2ndquadrant.com, robertmhaas@gmail.com, johngorman2@gmail.com
Subject: Re: PATCH: two slab-like memory allocators
In-Reply-To: <c7c1bb85-d1d0-05a5-4d4c-3047d6f053e7@2ndquadrant.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox