Re: [HACKERS] PATCH: two slab-like memory allocators - Mailing list pgsql-hackers

On 2017-02-28 01:44:42 +0100, Tomas Vondra wrote:
> On 02/27/2017 06:42 PM, Andres Freund wrote:
> > 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 :-(

Hm. The only way I can think of to do achieve the right thing here would
be something like:

typedef struct StandardChunkHeader
{   MemoryContext context;      /* owning context */   Size        size;           /* size of data space allocated in
chunk*/
 
#ifdef MEMORY_CONTEXT_CHECKING   /* when debugging memory usage, also store actual requested size */   Size
requested_size;
#endif   union   {       char *data;       /* ensure MAXALIGNed */       int64 alignof_int64;       double
alignof_double;  } d;
 
} StandardChunkHeader;

typedef struct SlabChunk
{   void *block;   StandardChunkHeader header;
} SlabChunk;

That's not overly pretty, but also not absolutely disgusting.  Unifying
the padding calculations between allocators would be a nice side-effect.
Note we at least previously had such union/double tricks in the tree, via
http://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=e1a11d93111ff3fba7a91f3f2ac0b0aca16909a8


It might be a good idea to have configure define maxaligned_type instead
of including both int64/double (although it'll IIRC practically always
be double that's maxaligned).


Independently of this, we really should redefine StandardChunkHeader to
be only the MemoryContext.  There's no need to have size/requested_size
part of StandardChunkHeader, given there's
MemoryContextMethods->get_chunk_space().


> 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.

That'd work as well, although at the very least I'd want to add a
comment explaining the actual memory layout somewhere - this is a bit
too finnicky to expect to get right the next time round.


Any preferences?


Greetings,

Andres Freund