Brett Walker
2018-Feb-03 12:28 UTC
[syslinux] Mismatch between code comments and reality in malloc.h
Hi Devs, I've been doing some deep code diving and I seem to have found a discrepancy between code comments and reality. This difference could hint at a bug or performance degradation, but I doubt it. I thought to ask to get clarification. In syslinux/core/mem/malloc.h, the comments and definition for arena_header, ARENA_PADDING and free_arena_header are: /* * This structure should be a power of two. This becomes the * alignment unit. */ struct arena_header { malloc_tag_t tag; size_t attrs; /* Bits 0..1: Type 2..3: Heap, 4..31: MSB of the size */ struct free_arena_header *next, *prev; #ifdef DEBUG_MALLOC unsigned long _pad[3]; unsigned int magic; #endif }; /* Pad to 2*sizeof(struct arena_header) */ #define ARENA_PADDING ((2 * sizeof(struct arena_header)) - \ (sizeof(struct arena_header) + \ sizeof(struct free_arena_header *) + \ sizeof(struct free_arena_header *))) /* * This structure should be no more than twice the size of the * previous structure. */ struct free_arena_header { struct arena_header a; struct free_arena_header *next_free, *prev_free; size_t _pad[ARENA_PADDING]; }; In my environment, the size of int and pointer are both 32-bits (x86 32-bits). When compiled with DEBUG_MALLOC undefined; I get sizeof(arena_header) == 16 sizeof(free_arena_header) == 56 Which disagrees with the comments. The comments do make a lot of sense to ensure good memory block alignment. To make the code agree with the comments (the easier option); the definition of ARENA_PADDING needs to be altered to: #define ARENA_PADDING (((2 * sizeof(struct arena_header)) - \ (sizeof(struct arena_header) + \ sizeof(struct free_arena_header *) + \ sizeof(struct free_arena_header *))) / sizeof(size_t)) This includes a reduction in the size of the padding by the unit size of the padding, i.e sizeof(size_t). Given that this is the memory sub-system, this small change could have a significant impact. What are your thoughts? Feedback most welcome, Brett
H. Peter Anvin
2018-Feb-05 17:58 UTC
[syslinux] Mismatch between code comments and reality in malloc.h
On 02/03/18 04:28, Brett Walker via Syslinux wrote:> Hi Devs, > > I've been doing some deep code diving and I seem to have found a discrepancy between code comments and reality. This difference could hint at a bug or performance degradation, but I doubt it. I thought to ask to get clarification. > > In syslinux/core/mem/malloc.h, the comments and definition for arena_header, ARENA_PADDING and free_arena_header are: > > > /* > * This structure should be a power of two. This becomes the > * alignment unit. > */ > struct arena_header { > malloc_tag_t tag; > size_t attrs; /* Bits 0..1: Type > 2..3: Heap, > 4..31: MSB of the size */ > struct free_arena_header *next, *prev; > > #ifdef DEBUG_MALLOC > unsigned long _pad[3]; > unsigned int magic; > #endif > }; > > /* Pad to 2*sizeof(struct arena_header) */ > #define ARENA_PADDING ((2 * sizeof(struct arena_header)) - \ > (sizeof(struct arena_header) + \ > sizeof(struct free_arena_header *) + \ > sizeof(struct free_arena_header *))) > > /* > * This structure should be no more than twice the size of the > * previous structure. > */ > struct free_arena_header { > struct arena_header a; > struct free_arena_header *next_free, *prev_free; > size_t _pad[ARENA_PADDING]; > }; > > In my environment, the size of int and pointer are both 32-bits (x86 32-bits). When compiled with DEBUG_MALLOC undefined; I get > > sizeof(arena_header) == 16 > sizeof(free_arena_header) == 56 > > Which disagrees with the comments. The comments do make a lot of sense to ensure good memory block alignment. To make the code agree with the comments (the easier option); the definition of ARENA_PADDING needs to be altered to: > > #define ARENA_PADDING (((2 * sizeof(struct arena_header)) - \ > (sizeof(struct arena_header) + \ > sizeof(struct free_arena_header *) + \ > sizeof(struct free_arena_header *))) / sizeof(size_t)) > > This includes a reduction in the size of the padding by the unit size of the padding, i.e sizeof(size_t). Given that this is the memory sub-system, this small change could have a significant impact. >Right, either ARENA_PADDING needs to be changed as you say, or _pad needs to be defined as char (or uint8_t) instead of size_t. The latter might be better. -hpa