Date: Thu, 16 Jan 2020 17:25:00 +0100
From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Andres Freund <andres@anarazel.de>, Tomas Vondra <tv@fuzzy.cz>,
	pgsql-hackers@postgresql.org
Subject: Re: SlabCheck leaks memory into TopMemoryContext
Message-ID: <20200116162500.pcgxmykirm677mt6@development>
References: <20200116044119.g45f7pmgz4jmodxj@alap3.anarazel.de>
 <23952.1579151393@sss.pgh.pa.us>
 <20200116061732.vvpyexenaortpu5y@alap3.anarazel.de>
 <27213.1579155900@sss.pgh.pa.us>
 <20200116064143.opcu2vhcktddpzbh@alap3.anarazel.de>
 <19743.1579188421@sss.pgh.pa.us>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="uzdmsenfrfcozwl4"
Content-Disposition: inline
In-Reply-To: <19743.1579188421@sss.pgh.pa.us>
Precedence: bulk


--uzdmsenfrfcozwl4
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Disposition: inline

On Thu, Jan 16, 2020 at 10:27:01AM -0500, Tom Lane wrote:
>Andres Freund <andres@anarazel.de> writes:
>> ... I thought you were asking whether
>> any additional memory could just be avoided...
>
>Well, I was kind of wondering that, but if it's not practical then
>preallocating the space instead will do.
>

I don't think it's practical to rework the checks in a way that would
not require allocations. Maybe it's possible, but I think it's not worth
the extra complexity.

The attached fix should do the trick - it pre-allocates the space when
creating the context. There is a bit of complexity because we want to
allocate the space as part of the context header, but nothin too bad. We
might optimize it a bit by using a regular bitmap (instead of just an
array of bools), but I haven't done that.

regards

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

--uzdmsenfrfcozwl4
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment; filename="slab-leak-fix.patch"

diff --git a/src/backend/utils/mmgr/slab.c b/src/backend/utils/mmgr/slab.c
index c5866d9cc3..7f9749a73f 100644
--- a/src/backend/utils/mmgr/slab.c
+++ b/src/backend/utils/mmgr/slab.c
@@ -70,6 +70,9 @@ typedef struct SlabContext
 	int			chunksPerBlock; /* number of chunks per block */
 	int			minFreeChunks;	/* min number of free chunks in any block */
 	int			nblocks;		/* number of blocks allocated */
+#ifdef MEMORY_CONTEXT_CHECKING
+	char	   *freechunks;		/* bitmap of free chunks on a block */
+#endif
 	/* blocks with free space, grouped by number of free chunks: */
 	dlist_head	freelist[FLEXIBLE_ARRAY_MEMBER];
 } SlabContext;
@@ -229,6 +232,15 @@ SlabContextCreate(MemoryContext parent,
 	/* Size of the memory context header */
 	headerSize = offsetof(SlabContext, freelist) + freelistSize;
 
+#ifdef MEMORY_CONTEXT_CHECKING
+	/*
+	 * With memory checking, we need to allocate extra space for the bitmap
+	 * of free chunks. The space is allocated at the end, and we need proper
+	 * alignment (it's an array of bools, so maybe MAXALIGN is not needed).
+	 */
+	headerSize = MAXALIGN(headerSize) + chunksPerBlock * sizeof(bool);
+#endif
+
 	slab = (SlabContext *) malloc(headerSize);
 	if (slab == NULL)
 	{
@@ -258,6 +270,12 @@ SlabContextCreate(MemoryContext parent,
 	for (i = 0; i < (slab->chunksPerBlock + 1); i++)
 		dlist_init(&slab->freelist[i]);
 
+#ifdef MEMORY_CONTEXT_CHECKING
+	/* set the freechunks pointer after the freelists array (aligned) */
+	slab->freechunks = (char *) slab +
+			MAXALIGN(offsetof(SlabContext, freelist) + freelistSize);
+#endif
+
 	/* Finally, do the type-independent part of context creation */
 	MemoryContextCreate((MemoryContext) slab,
 						T_SlabContext,
@@ -701,14 +719,10 @@ SlabCheck(MemoryContext context)
 	int			i;
 	SlabContext *slab = castNode(SlabContext, context);
 	const char *name = slab->header.name;
-	char	   *freechunks;
 
 	Assert(slab);
 	Assert(slab->chunksPerBlock > 0);
 
-	/* bitmap of free chunks on a block */
-	freechunks = palloc(slab->chunksPerBlock * sizeof(bool));
-
 	/* walk all the freelists */
 	for (i = 0; i <= slab->chunksPerBlock; i++)
 	{
@@ -731,7 +745,7 @@ SlabCheck(MemoryContext context)
 					 name, block->nfree, block, i);
 
 			/* reset the bitmap of free chunks for this block */
-			memset(freechunks, 0, (slab->chunksPerBlock * sizeof(bool)));
+			memset(slab->freechunks, 0, (slab->chunksPerBlock * sizeof(bool)));
 			idx = block->firstFreeChunk;
 
 			/*
@@ -748,7 +762,7 @@ SlabCheck(MemoryContext context)
 
 				/* count the chunk as free, add it to the bitmap */
 				nfree++;
-				freechunks[idx] = true;
+				slab->freechunks[idx] = true;
 
 				/* read index of the next free chunk */
 				chunk = SlabBlockGetChunk(slab, block, idx);
@@ -759,7 +773,7 @@ SlabCheck(MemoryContext context)
 			for (j = 0; j < slab->chunksPerBlock; j++)
 			{
 				/* non-zero bit in the bitmap means chunk the chunk is used */
-				if (!freechunks[j])
+				if (!slab->freechunks[j])
 				{
 					SlabChunk  *chunk = SlabBlockGetChunk(slab, block, j);
 

--uzdmsenfrfcozwl4--