On Sat, 6 Apr 2024, 14:36 David Rowley, <dgrowle...@gmail.com> wrote:
> On Sat, 6 Apr 2024 at 02:30, Matthias van de Meent > <boekewurm+postg...@gmail.com> wrote: > > > > On Thu, 4 Apr 2024 at 22:43, Tom Lane <t...@sss.pgh.pa.us> wrote: > > > > > > Matthias van de Meent <boekewurm+postg...@gmail.com> writes: > > > > It extends memory context IDs to 5 bits (32 values), of which > > > > - 8 have glibc's malloc pattern of 001/010; > > > > - 1 is unused memory's 00000 > > > > - 1 is wipe_mem's 11111 > > > > - 4 are used by existing contexts > (Aset/Generation/Slab/AlignedRedirect) > > > > - 18 are newly available. > > > > > > This seems like it would solve the problem for a good long time > > > to come; and if we ever need more IDs, we could steal one more bit > > > by requiring the offset to the block header to be a multiple of 8. > > > (Really, we could just about do that today at little or no cost ... > > > machines with MAXALIGN less than 8 are very thin on the ground.) > > > > Hmm, it seems like a decent idea, but I didn't want to deal with the > > repercussions of that this late in the cycle when these 2 bits were > > still relatively easy to get hold of. > > Thanks for writing the patch. > > I think 5 bits is 1 too many. 4 seems fine. I also think you've > reserved too many slots in your patch as I disagree that we need to > reserve the glibc malloc pattern anywhere but in the 1 and 2 slots of > the mcxt_methods[] array. I looked again at the 8 bytes prior to a > glibc malloc'd chunk and I see the lowest 4 bits of the headers > consistently set to 0001 for all powers of 2 starting at 8 up to > 65536. Malloc's docs specify the minimum chunk size at 4*sizeof(void*) and itself uses , so using powers of 2 for chunks would indeed fail to detect 1s in the 4th bit. I suspect you'll get different results when you check the allocation patterns of multiples of 8 bytes, starting from 40, especially on 32-bit arm (where MALLOC_ALIGNMENT is 8 bytes, rather than the 16 bytes on i386 and 64-bit architectures, assuming [0] is accurate) 131072 seems to vary and beyond that, they seem to be set to > 0010. > In your updated 0001, you don't seem to fill the RESERVED_GLIBC memctx array entries with BOGUS_MCTX(). With that, there's no increase in the number of reserved slots from > what we have reserved today. Still 4. So having 4 bits instead of 3 > bits gives us a total of 12 slots rather than 4 slots. Having 3x > slots seems enough. We might need an extra bit for something else > sometime. I think keeping it up our sleeve is a good idea. > > Another reason not to make it 5 bits is that I believe that would make > the mcxt_methods[] array 2304 bytes rather than 576 bytes. 4 bits > makes it 1152 bytes, if I'm counting correctly. > I don't think I understand why this would be relevant when only 5 of the contexts are actually in use (thus in caches). Is that size concern about TLB entries then? > I revised the patch to simplify hdrmask logic. This started with me > having trouble finding the best set of words to document that the > offset is "half the bytes between the chunk and block". So, instead > of doing that, I've just made it so these two fields effectively > overlap. The lowest bit of the block offset is the same bit as the > high bit of what MemoryChunkGetValue returns. Works for me, I suppose. I also updated src/backend/utils/mmgr/README to explain this and > adjust the mentions of 3-bits and 61-bits to 4-bits and 60-bits. I > also explained the overlapping part. > Thanks! [0] https://sourceware.org/glibc/wiki/MallocInternals#Platform-specific_Thresholds_and_Constants >
