heap: add functions to get heap runtime statistics #39903

chen-png · 2021-10-29T07:55:01Z

add functions to get the sys_heap runtime statistics,
include total free bytes, total allocated bytes.

Signed-off-by: Chen Peng1 peng1.chen@intel.com

chen-png · 2021-10-29T08:25:57Z

this is for #23076, get used and free bytes from heap.

LeiW000 · 2021-11-01T02:15:43Z

lib/os/heap-validate.c

+		if (c == h->end_chunk) {
+			break;
+		}
+	}


It is better to replace "for(){}" with "do...while()".

do {
} while (c != h->end_chunk)

I have updated it, thanks,

LeiW000 · 2021-11-01T02:43:55Z

lib/os/heap-validate.c

+		if (chunk_used(h, c)) {
+			if ((c != 0) && (c != h->end_chunk)) {
+				stats->allocated_bytes += chunksz_to_bytes(
+						h, chunk_size(h, c));


Can we put the two lines in one line?

According to the Zephyr Coding Guidelines all if-else if constructs shall be terminated with an else statement.

thanks, I have added some empty else {; } for coding guideline.

yes, we can put them into one line like below,

if (chunk_used(h, c) && (c != 0) && (c != h->end_chunk)) { ... } else if (!chunk_used(h, c) && !solo_free_header(h, c)) ... }

but I think using chunk_used(h, c) to distinguish free and allocated firstly is more obvious.

I just meant lines 412 and 413.

that's because the max length can't be more than 80 chars.

LeiW000 · 2021-11-01T02:44:36Z

lib/os/heap-validate.c

+		} else {
+			if (!solo_free_header(h, c)) {
+				free_bytes = chunksz_to_bytes(h,
+						chunk_size(h, c));


Same as above?

LeiW000 · 2021-11-01T02:46:42Z

lib/os/heap-validate.c

+				free_bytes = chunksz_to_bytes(h,
+						chunk_size(h, c));
+				stats->free_bytes += free_bytes;
+				if (free_bytes > stats->max_avail_bytes) {


Is it really possible that free_byyes is larger than max_avail_byyes?

In a heap, there maybe several free blocks, they are not continuous, the max_avail_bytes is the largest one of them.

Got it. Thanks.

chen-png · 2021-11-02T02:56:49Z

@andyross Could you help take a look of this? thanks.

LeiW000 · 2021-11-02T03:29:24Z

include/sys/sys_heap.h

+ * @return -EINVAL if null pointers, otherwise 0
+ */
+int heap_runtime_stats_get(struct sys_heap *heap,
+		struct sys_heap_runtime_stats *stats);


Is it a public API? If not, I think we should declare it as a static function.

this is a public API, because users need to invoke this function if then wants to know the heap usage.

the API sys_heap_runtime_stats_get could be used to get heap usage of any specified sys_heap,
the API heap_runtime_stats_get is only used for the default _system_heap of k_mallock/k_free,
because for k_malloc, it used the os predefined _system_heap, for malloc, it used the os predifined z_malloc_heap,

@andyross so do I need to define another two API for them (like heap_runtime_stats_get for k_malloc, using _system_heap by default), or only define a common sys_heap_runtime_stats_get, the users should specific the sys_heap by themselves?

I vote for defining one API - heap_runtime_status_get().

I think necessary to try use that API and see if it is convenient and user-friendly.

maksimmasalski · 2021-11-02T03:57:17Z

I think, we need test case for that feature too.

andyross

Need to fix the function name.

Also, this seems to be doing a lot of work and incurring a bunch of runtime expense (which requires a locked heap, so that's a latency problem too) just to be able to get "max_available_bytes". Is that really a good thing? What do other heap implementations do? For reference, a quick glance at glibc's mallinfo() seems to say that nothing they track requires anything but O(1) overhead on heap operations.

andyross · 2021-11-08T14:36:57Z

lib/os/heap-validate.c

+	stats->allocated_bytes = 0;
+	stats->max_avail_bytes = 0;
+
+	/* Iterate through all chunks */


Do we really want a O(N) implementation here? Most of the time people just want to know the sum of all the sizes passed to successful allocations. That can be tracked with a simple increment/decrement hook at allocation/free time.

andyross · 2021-11-08T14:38:42Z

include/sys/sys_heap.h

+ * @param stats_t Pointer to struct to copy statistics into
+ * @return -EINVAL if null pointers, otherwise 0
+ */
+int heap_runtime_stats_get(struct sys_heap *heap,


Naming: should be "sys_heap_*()". sys_ is the prefix (one of them) for a public API.

andyross · 2021-11-08T14:40:00Z

lib/os/heap-validate.c

+	return 0;
+}
+
+#if (CONFIG_HEAP_MEM_POOL_SIZE > 0)


No point in putting an extern function inside an ifdef. It won't be linked if it's not referenced.

andyross

Some nitpicks that would be nice to see fixed, but not worth a -1. Looks good to me.

andyross · 2021-11-09T16:46:39Z

lib/os/Kconfig

@@ -46,6 +46,12 @@ config SYS_HEAP_ALLOC_LOOPS
 	  keeps the maximum runtime at a tight bound so that the heap
 	  is useful in locked or ISR contexts.

+config SYS_HEAP_RUNTIME_STATS
+	bool "System heap runtime statistics"
+	default n


Pedantry: "n" is the default state for a bool, you can remove this line.

I will remove it, thanks.

andyross · 2021-11-09T16:47:46Z

lib/os/heap.h

@@ -139,26 +143,6 @@ static inline chunksz_t chunk_size(struct z_heap *h, chunkid_t c)
 	return chunk_field(h, c, SIZE_AND_USED) >> 1;
 }

-static inline void set_chunk_used(struct z_heap *h, chunkid_t c, bool used)


Why did this function move? Seems like a needless change.

because in this function, my change used other functions like chunksz_to_bytes and solo_free_header, which are declared and defined after this set_chunk_used.

npitre

I don't think overloading set_chunk_used() is the best approach.
The if ((c != 0) && (c != h->end_chunk)) test will be applied every
time although it is relevant only exactly once i.e. at heap creation time.

Also, calling set_chunk_used(..., false) in the realloc case for the
count to balance is a bad sign, and doing so despite
CONFIG_SYS_HEAP_RUNTIME_STATS being disabled is wasteful.

I think it would be better and more obvious if the accounting was done
inline. This way the 2 realloc special cases could be better optimized.

Oh, and btw you don't need the empty else when there is no else if.

enjiamai

For Line 263 - 281, how about the changes like this:

#ifdef CONFIG_SYS_HEAP_RUNTIME_STATS
size_t bytes = chunksz_to_bytes(h, chunk_size(h, c));

if (used && (c != 0) && (c != h->end_chunk)) {
	h->allocated_bytes += bytes;
	h->free_bytes -= bytes;
} else if (!solo_free_header(h, c)) {
	h->allocated_bytes -= bytes;
	h->free_bytes += bytes;
} 
#endif

chen-png · 2021-11-10T07:48:04Z

calling set_chunk_used(..., false) in the realloc case for the count to balance is a bad sign

yes, I agree, I will update it.

the accounting was done inline.

hi @npitre Could you help review it again? I added another inline function to calculate size, this is more reasonable then overring set_chunk_used for the special case for realloc.

npitre

I don't think overloading set_chunk_used() is the best approach.
The if ((c != 0) && (c != h->end_chunk)) test will be applied every
time although it is relevant only exactly once i.e. at heap creation time.

Also, calling set_chunk_used(..., false) in the realloc case for the
count to balance is a bad sign, and doing so despite
CONFIG_SYS_HEAP_RUNTIME_STATS being disabled is wasteful.

I think it would be better and more obvious if the accounting was done
inline. This way the 2 realloc special cases could be better optimized.

Oh, and btw you don't need the empty else when there is no else if.

npitre

Please disregard last comment

npitre

This is getting too messy.

Here's what I suggest for heap.c instead:

diff --git a/lib/os/heap.c b/lib/os/heap.c
index d78cd16420..9b22d7fa5f 100644
--- a/lib/os/heap.c
+++ b/lib/os/heap.c
@@ -38,6 +38,10 @@ static void free_list_remove_bidx(struct z_heap *h, chunkid_t c, int bidx)
 		set_next_free_chunk(h, first, second);
 		set_prev_free_chunk(h, second, first);
 	}
+
+#ifdef CONFIG_SYS_HEAP_RUNTIME_STATS
+	h->free_bytes -= chunksz_to_bytes(h, chunk_size(h, c));
+#endif
 }
 
 static void free_list_remove(struct z_heap *h, chunkid_t c)
@@ -72,6 +76,10 @@ static void free_list_add_bidx(struct z_heap *h, chunkid_t c, int bidx)
 		set_next_free_chunk(h, first, c);
 		set_prev_free_chunk(h, second, c);
 	}
+
+#ifdef CONFIG_SYS_HEAP_RUNTIME_STATS
+	h->free_bytes += chunksz_to_bytes(h, chunk_size(h, c));
+#endif
 }
 
 static void free_list_add(struct z_heap *h, chunkid_t c)
@@ -164,6 +172,9 @@ void sys_heap_free(struct sys_heap *heap, void *mem)
 		 mem);
 
 	set_chunk_used(h, c, false);
+#ifdef CONFIG_SYS_HEAP_RUNTIME_STATS
+	h->allocated_bytes -= chunksz_to_bytes(h, chunk_size(h, c)); 
+#endif
 	free_chunk(h, c);
 }
 
@@ -251,6 +262,9 @@ void *sys_heap_alloc(struct sys_heap *heap, size_t bytes)
 	}
 
 	set_chunk_used(h, c, true);
+#ifdef CONFIG_SYS_HEAP_RUNTIME_STATS
+	h->allocated_bytes += chunksz_to_bytes(h, chunk_size(h, c));
+#endif
 	return chunk_mem(h, c);
 }
 
@@ -318,6 +332,9 @@ void *sys_heap_aligned_alloc(struct sys_heap *heap, size_t align, size_t bytes)
 	}
 
 	set_chunk_used(h, c, true);
+#ifdef CONFIG_SYS_HEAP_RUNTIME_STATS
+	h->allocated_bytes += chunksz_to_bytes(h, chunk_size(h, c));
+#endif
 	return mem;
 }
 
@@ -353,6 +370,9 @@ void *sys_heap_aligned_realloc(struct sys_heap *heap, void *ptr,
 		return ptr;
 	} else if (chunk_size(h, c) > chunks_need) {
 		/* Shrink in place, split off and free unused suffix */
+#ifdef CONFIG_SYS_HEAP_RUNTIME_STATS
+		h->allocated_bytes -= (chunk_size(h, c) - chunks_need) * CHUNK_UNIT;
+#endif
 		split_chunks(h, c, c + chunks_need);
 		set_chunk_used(h, c, true);
 		free_chunk(h, c + chunks_need);
@@ -362,6 +382,10 @@ void *sys_heap_aligned_realloc(struct sys_heap *heap, void *ptr,
 		/* Expand: split the right chunk and append */
 		chunkid_t split_size = chunks_need - chunk_size(h, c);
 
+#ifdef CONFIG_SYS_HEAP_RUNTIME_STATS
+		h->allocated_bytes += split_size * CHUNK_UNIT;
+#endif
+
 		free_list_remove(h, rc);
 
 		if (split_size < chunk_size(h, rc)) {
@@ -414,6 +438,10 @@ void sys_heap_init(struct sys_heap *heap, void *mem, size_t bytes)
 	heap->heap = h;
 	h->end_chunk = heap_sz;
 	h->avail_buckets = 0;
+#ifdef CONFIG_SYS_HEAP_RUNTIME_STATS
+	h->free_bytes = 0;
+	h->allocated_bytes = 0;
+#endif
 
 	int nb_buckets = bucket_idx(h, heap_sz) + 1;
 	chunksz_t chunk0_size = chunksz(sizeof(struct z_heap) +

add functions to get the sys_heap runtime statistics, include total free bytes, total allocated bytes. Signed-off-by: Chen Peng1 <peng1.chen@intel.com>

chen-png · 2021-11-11T02:13:29Z

@npitre this logic is easier to understand, thanks.

nashif · 2021-11-11T03:36:51Z

@chen-png can you provide a test as well?

npitre · 2021-11-11T03:56:10Z

A good strategy for a test would be to extract the allocated_bytes and free_bytes computation from heap_print_info() into a function of its own, and call it from sys_heap_validate() as well to compare with the runtime counters when CONFIG_SYS_HEAP_RUNTIME_STATS is set. Then you just need to add that config option to existing tests.

chen-png · 2021-11-11T07:46:37Z

can you provide a test as well?

I think the proposal from @npitre is good, so according to his comments, I enabled existing heap tests to validate this sys_heap_runtime_stats_get API.

add operations to check sys_heap_runtime_stats_get API in existing sys_heap_validate function. Signed-off-by: Chen Peng1 <peng1.chen@intel.com>

Add CONFIG_SYS_HEAP_RUNTIME_STATS in existing heap tests to enable validation for sys_heap_runtime_stats_get API. Signed-off-by: Chen Peng1 <peng1.chen@intel.com>

cfriedt · 2021-11-11T19:41:26Z

This is great - I've been meaning to add something like this for a while, but I'm happy that you beat me to it @chen-png :-)

chen-png requested review from andyross, carlescufi, dcpleung, galak, MaureenHelm and nashif as code owners October 29, 2021 07:55

github-actions bot added the area: API Changes to public APIs label Oct 29, 2021

chen-png requested review from LeiW000, maksimmasalski and enjiamai October 29, 2021 07:56

chen-png force-pushed the add_heap_statistics branch from 170e6cd to 0836938 Compare October 29, 2021 08:09

zephyrbot added the area: Base OS Base OS Library (lib/os) label Oct 29, 2021

zephyrbot assigned andyross Oct 29, 2021

chen-png force-pushed the add_heap_statistics branch from 0836938 to b3d60cc Compare October 29, 2021 15:59

LeiW000 suggested changes Nov 1, 2021

View reviewed changes

chen-png force-pushed the add_heap_statistics branch from b3d60cc to 4e70e71 Compare November 1, 2021 02:41

LeiW000 suggested changes Nov 1, 2021

View reviewed changes

chen-png force-pushed the add_heap_statistics branch from 4e70e71 to 436afc1 Compare November 2, 2021 02:46

LeiW000 reviewed Nov 2, 2021

View reviewed changes

andyross requested changes Nov 8, 2021

View reviewed changes

chen-png force-pushed the add_heap_statistics branch 2 times, most recently from 30c8463 to 5d545c5 Compare November 9, 2021 16:48

andyross approved these changes Nov 9, 2021

View reviewed changes

chen-png force-pushed the add_heap_statistics branch from 5d545c5 to b4e637f Compare November 10, 2021 02:34

nashif requested a review from npitre November 10, 2021 02:54

npitre reviewed Nov 10, 2021

View reviewed changes

enjiamai reviewed Nov 10, 2021

View reviewed changes

chen-png requested a review from KangJianX November 10, 2021 07:41

KangJianX approved these changes Nov 10, 2021

View reviewed changes

chen-png force-pushed the add_heap_statistics branch from b4e637f to 7c2cff9 Compare November 10, 2021 07:42

chen-png requested a review from npitre November 10, 2021 07:48

enjiamai approved these changes Nov 10, 2021

View reviewed changes

npitre reviewed Nov 10, 2021

View reviewed changes

chen-png force-pushed the add_heap_statistics branch from 7c2cff9 to 043c52b Compare November 10, 2021 17:31

npitre reviewed Nov 10, 2021

View reviewed changes

heap: add functions to get heap runtime statistics

2094d12

add functions to get the sys_heap runtime statistics, include total free bytes, total allocated bytes. Signed-off-by: Chen Peng1 <peng1.chen@intel.com>

chen-png force-pushed the add_heap_statistics branch from 043c52b to 2094d12 Compare November 11, 2021 02:17

npitre approved these changes Nov 11, 2021

View reviewed changes

nashif approved these changes Nov 11, 2021

View reviewed changes

github-actions bot added the area: Tests Issues related to a particular existing or missing test label Nov 11, 2021

Chen Peng1 added 2 commits November 11, 2021 15:50

sys_heap: add check for sys_heap_runtime_stats_get API

a017438

add operations to check sys_heap_runtime_stats_get API in existing sys_heap_validate function. Signed-off-by: Chen Peng1 <peng1.chen@intel.com>

tests: heap: validate sys_heap_runtime_stats_get API

3db723a

Add CONFIG_SYS_HEAP_RUNTIME_STATS in existing heap tests to enable validation for sys_heap_runtime_stats_get API. Signed-off-by: Chen Peng1 <peng1.chen@intel.com>

chen-png force-pushed the add_heap_statistics branch from 84269c7 to 3db723a Compare November 11, 2021 07:51

nashif merged commit f4cf484 into zephyrproject-rtos:main Nov 11, 2021

heap: add functions to get heap runtime statistics #39903

heap: add functions to get heap runtime statistics #39903

Conversation

chen-png commented Oct 29, 2021 • edited Loading

chen-png commented Oct 29, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chen-png commented Nov 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maksimmasalski commented Nov 2, 2021

andyross left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyross left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

npitre left a comment

Choose a reason for hiding this comment

enjiamai left a comment • edited Loading

Choose a reason for hiding this comment

chen-png commented Nov 10, 2021

npitre left a comment

Choose a reason for hiding this comment

npitre left a comment

Choose a reason for hiding this comment

npitre left a comment

Choose a reason for hiding this comment

chen-png commented Nov 11, 2021

nashif commented Nov 11, 2021

npitre commented Nov 11, 2021 via email

chen-png commented Nov 11, 2021

cfriedt commented Nov 11, 2021

chen-png commented Oct 29, 2021 •

edited

Loading

enjiamai left a comment •

edited

Loading