Improve stack memory usage at compile time #9963

michal-shalev · 2024-06-17T16:12:19Z

Waiting for:

What?

Improve stack memory usage by introducing an 8KB threshold for stack frames using the GCC flag -Wframe-larger-than=8192. This task involves identifying and addressing functions that exceed this limit.

Why?

Customers with limited stack sizes have recently experienced stack overflow issues when running UCX.
The goal is to ensure that the entire UCX runs within a defined stack size threshold.
Mitigates the risk of stack overflow, particularly in environments with limited stack sizes.

How?

Include -Wframe-larger-than=8192 in the build configuration to trigger warnings for large stack frames.
Review and refactor functions with large stack frames or move large variables to the heap.
Introduce an option for users to adjust or disable the threshold, and update documentation accordingly.

tvegas1 · 2024-06-18T05:34:16Z

src/ucs/debug/log.c

+ new_log_file_name = (char *)ucs_malloc(PATH_MAX, "new_log_file_name");
+
+ if (old_log_file_name == NULL || new_log_file_name == NULL) {
+ ucs_free(old_log_file_name);


I guess no need to free when we ucs_fatal().

valid for all cases.

Thank you :), please check now @tvegas1

src/ucs/debug/log.c

ivankochin · 2024-06-18T06:23:14Z

src/ucs/debug/log.c

 if (access(old_log_file_name, W_OK) != 0) {
+ ucs_free(old_log_file_name);
+ ucs_free(new_log_file_name);
 ucs_fatal("unable to write to %s", old_log_file_name);
 }


Printing log file names after freeing them seems like not a good idea. Also there is a lot of code duplication, so I would propose something like the following:

Suggested change

if (access(old_log_file_name, W_OK) != 0) {

ucs_free(old_log_file_name);

ucs_free(new_log_file_name);

ucs_fatal("unable to write to %s", old_log_file_name);

}

if (access(old_log_file_name, W_OK) != 0) {

ucs_error("unable to write to %s", old_log_file_name);

goto err_out;

}

...

err_out:

ucs_free(old_log_file_name);

ucs_free(new_log_file_name);

if (err_msg) {

ucs_fatal("ucs_log_file_rotate() fatal error (see logs before)");

}

Thank you :), please check now @ivankochin

src/ucs/debug/log.c

tvegas1 · 2024-06-18T09:37:22Z

src/ucs/debug/log.c


 if (access(old_log_file_name, W_OK) != 0) {
- ucs_fatal("unable to write to %s", old_log_file_name);
+ ucs_error("unable to write to %s", old_log_file_name);


since we fatal anyways, maybe set msg = "..." then print with ucs_fatal(".. %s.. ", msg);

or just replace ucs_error() by ucs_fatal()

replace ucs_error() by ucs_fatal()

replaced ucs_error() by ucs_fatal()

tvegas1 · 2024-06-18T09:37:58Z

src/ucs/debug/log.c

+ ucs_free(new_log_file_name);
+
+err_out:
+ ucs_free(old_log_file_name);


no need to free i guess, since there would be so many leaked resources aynways..

removed it, please check again

tvegas1 · 2024-06-18T09:38:18Z

src/ucs/debug/log.c

+err_out:
+ ucs_free(old_log_file_name);
+ ucs_free(new_log_file_name);
+ ucs_fatal("ucs_log_file_rotate() fatal error (see logs before)");


please see previous suggestion

please check again

tvegas1 · 2024-06-18T13:08:58Z

src/ucs/vfs/base/vfs_obj.c

+ if ((rel_path_buf == NULL) || (abs_path_buf == NULL)) {
+ ucs_error("Failed to allocate memory for path buffers");
+ status = UCS_ERR_NO_MEMORY;
+ goto out;


remote out label and use return UCS_ERR_NO_MEMORY instead, rename out_free_buffers to out

return here.

there could be a case where one of the buffer was allocated successfully and the other one failed, so I think we still need to free, so I changed it to "goto out", let me know what do you think

tvegas1 · 2024-06-18T13:10:15Z

src/ucs/vfs/base/vfs_obj.c

@@ -259,13 +259,21 @@ static ucs_status_t ucs_vfs_node_add(void *parent_obj, ucs_vfs_node_type_t type,
 va_list ap, ucs_vfs_node_t **new_node)
 {
 ucs_vfs_node_t *current_node;


coding guideline mandates current_node be after initialized variables

need to move current_node line 264

src/ucs/sys/topo/base/topo.c

tvegas1 · 2024-07-01T07:37:17Z

config/m4/compiler.m4

+# -Wall: All common warnings
+# -Werror: Treat warnings as errors
+# -Wframe-larger-than=8192: Warn if stack frame size exceeds 8192 bytes (8 KB)
+BASE_CFLAGS="-g -Wall -Werror -Wframe-larger-than=8192"


might as well remove comments since the flags are so common

removed "-Wall", "-Werror" and "-g"

tvegas1 · 2024-07-01T07:38:08Z

src/tools/perf/lib/uct_tests.cc

@@ -773,7 +773,7 @@ class uct_perf_test_runner {
 TEST_CASE_ALL_OSD(_perf, _case, UCT_PERF_DATA_LAYOUT_BCOPY) \
 TEST_CASE_ALL_OSD(_perf, _case, UCT_PERF_DATA_LAYOUT_ZCOPY)

-ucs_status_t uct_perf_test_dispatch(ucx_perf_context_t *perf)
+ucs_status_t __attribute__ ((optimize("O1"))) uct_perf_test_dispatch(ucx_perf_context_t *perf)


why is attribute needed here?

Without optimization the stack frame size > 8192 bytes. I tried using "new" at "src/tools/perf/lib/ucp_tests.cc" to make the runner use the heap instead of the stack and it compiled successfully on the HPC node, but then the CI failed because of the "new" and I couldn't reproduce it.
At the moment I changed it to pragma instead of the attribute.

tvegas1 · 2024-07-01T07:39:39Z

src/ucs/debug/log.c


 if (access(old_log_file_name, W_OK) != 0) {
- ucs_fatal("unable to write to %s", old_log_file_name);
+ ucs_error("unable to write to %s", old_log_file_name);


replace ucs_error() by ucs_fatal()

tvegas1 · 2024-07-01T07:41:23Z

src/ucs/vfs/base/vfs_obj.c

@@ -259,13 +259,21 @@ static ucs_status_t ucs_vfs_node_add(void *parent_obj, ucs_vfs_node_type_t type,
 va_list ap, ucs_vfs_node_t **new_node)
 {
 ucs_vfs_node_t *current_node;


need to move current_node line 264

tvegas1 · 2024-07-01T07:41:41Z

src/ucs/vfs/base/vfs_obj.c

+ if ((rel_path_buf == NULL) || (abs_path_buf == NULL)) {
+ ucs_error("Failed to allocate memory for path buffers");
+ status = UCS_ERR_NO_MEMORY;
+ goto out;


return here.

tvegas1 · 2024-07-01T07:43:23Z

test/gtest/common/googletest/gtest-param-test.h

@@ -457,6 +457,11 @@ internal::CartesianProductHolder<Generator...> Combine(const Generator&... g) {
 #define GTEST_GET_FIRST_(first, ...) first
 #define GTEST_GET_SECOND_(first, second, ...) second

+// Modified to reduce stack frame size per test file.


is there a way to remove changes to googletest (disabling check for it)? or make it outside of googletest by including gtest-param-test.h?

I didn't find a workaround for this, so @yosefe advised me to make this change

tvegas1

maybe remove #pragma in gtest by not specifing Wframe-large-than when building .cc files

tvegas1 · 2024-07-02T05:29:55Z

src/tools/perf/lib/ucp_tests.cc

+{
+ ucs_status_t status;
+
+ status = dispatch_osd(perf);


maybe replace by an array of function pointers

thank you, done

tvegas1 · 2024-07-02T05:32:13Z

test/gtest/ucs/test_bitops.cc

@@ -90,6 +90,8 @@ UCS_TEST_F(test_bitops, ptr_ctz) {
 ASSERT_EQ(88, ucs_count_ptr_trailing_zero_bits(buffer, 160));
 }

+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wframe-larger-than="


to avoid all pragma in test/gtest, is there a way to not set the -Wframe-larger-than when compiling gtests?

There are a few macros that I can change to make those tests use the heap instead of the stack, just like I did in the commit "TEST/GTEST: reduce stack frame size in INSTANTIATE_TEST_SUITE_P macro".
Those macros are "UCS_TEST_P_" and "UCS_TEST_P_", I'm trying to make the syntax work, it's challenging so I added the pragma to see if I can make the CI pass meanwhile.
The reason I want to reduce the stack allocation in the tests is that later on I will check the stack usage on run time alongside running the tests and if the stack usage of the tests are initially above the threshold then it'll be harder to add a rule for the CI to catch cases of large stack usage during run time.

…o heap

…) to heap

…() to heap

…heap

…h() to heap

…to heap

…nfo() to heap

…e() to heap

…) to heap

…heap

michal-shalev requested a review from gleon99 June 17, 2024 16:12

michal-shalev added the WIP-DNM Work in progress / Do not review label Jun 17, 2024

michal-shalev marked this pull request as draft June 17, 2024 16:13

tvegas1 reviewed Jun 18, 2024

View reviewed changes

iyastreb reviewed Jun 18, 2024

View reviewed changes

src/ucs/debug/log.c Outdated Show resolved Hide resolved

ivankochin reviewed Jun 18, 2024

View reviewed changes

michal-shalev force-pushed the convert-stack-allocation-to-heap branch from c44c2d2 to 9dd5ab8 Compare June 18, 2024 09:05

tvegas1 reviewed Jun 18, 2024

View reviewed changes

michal-shalev force-pushed the convert-stack-allocation-to-heap branch 3 times, most recently from 90260f5 to daae19b Compare June 18, 2024 11:20

tvegas1 reviewed Jun 18, 2024

View reviewed changes

michal-shalev removed the WIP-DNM Work in progress / Do not review label Jun 18, 2024

michal-shalev marked this pull request as ready for review June 18, 2024 14:11

michal-shalev force-pushed the convert-stack-allocation-to-heap branch 2 times, most recently from 7eac1d4 to e10a8a8 Compare June 19, 2024 23:08

ivankochin reviewed Jun 24, 2024

View reviewed changes

src/ucs/sys/topo/base/topo.c Outdated Show resolved Hide resolved

michal-shalev force-pushed the convert-stack-allocation-to-heap branch 4 times, most recently from ef9cbb9 to 21e1828 Compare June 30, 2024 20:47

tvegas1 reviewed Jul 1, 2024

View reviewed changes

michal-shalev force-pushed the convert-stack-allocation-to-heap branch 4 times, most recently from 5c24d38 to 6aa969d Compare July 1, 2024 20:21

michal-shalev changed the title ~~convert stack allocation to heap~~ add compile time stack memory usage check Jul 1, 2024

tvegas1 reviewed Jul 2, 2024

View reviewed changes

michal-shalev force-pushed the convert-stack-allocation-to-heap branch 2 times, most recently from 5a31e59 to ec83329 Compare July 2, 2024 09:05

michal-shalev added 17 commits September 16, 2024 17:25

UCS/VFS/SOCK: convert stack allocation at ucs_vfs_sock_mkdir() to heap

859d3f6

UCS/VFS/BASE: convert stack allocation at ucs_vfs_node_add_subdir() t…

714dd9a

…o heap

UCS/VFS/FUSE: convert stack allocation at ucs_vfs_fuse_wait_for_path(…

90155e0

…) to heap

UCS/SYS/SOCK: convert stack allocation at ucs_netif_bond_ad_num_ports…

93087c5

…() to heap

UCS/SYS/MODULE: convert stack allocation at ucs_module_load_one() to …

5a88a20

…heap

UCS/SYS/MODULE: convert stack allocation at ucs_module_init() to heap

68d459d

UCT/SM/MM: convert stack allocation at uct_posix_file_open() to heap

51e6a54

UCT/SM/MM: convert stack allocation at uct_posix_procfs_open() to heap

03ef359

UCT/SM/MM: convert stack allocation at uct_posix_unlink() to heap

a654ca8

UCT/TCP/BASE: convert stack allocation at uct_tcp_iface_get_sysfs_pat…

c8cbbcc

…h() to heap

UCT/TCP: convert stack allocation at uct_tcp_iface_query() to heap

fda33cb

UCT/TCP: convert stack allocation at uct_tcp_is_bridge() to heap

a8a2dd7

UCT/IB/BASE: convert stack allocation at uct_ib_device_query() to heap

53dbd1c

UCP/WORKER/GTEST: convert stack allocation at test_pci_bw get_pci_bw …

55a1c8f

…to heap

UCP/PROTO/DEBUG: convert stack allocation at ucp_proto_select_write_i…

a470cb8

…nfo() to heap

COMMON/HELPERS/GTEST: convert stack allocation at netif_has_sysfs_fil…

e9dc94a

…e() to heap

TOOLS/VFS: convert stack allocation at vfs_mount() to heap

739f0cc

michal-shalev force-pushed the convert-stack-allocation-to-heap branch from 6b3ff9e to 42f80b4 Compare September 16, 2024 14:27

TOOLS/VFS: convert stack allocation at vfs_unmount() to heap

bb819ae

michal-shalev force-pushed the convert-stack-allocation-to-heap branch 3 times, most recently from 54c414c to 746ca85 Compare September 16, 2024 18:48

michal-shalev added 7 commits September 16, 2024 21:55

TOOLS/INFO: reduce stack memory usage in print_type_info()

2388f0d

UCT/IB/MLX5/DV: convert stack allocation at uct_ib_mlx5_devx_md_open(…

4f4b782

…) to heap

UCP/PROTO: convert stack allocation at ucp_proto_multi_init() to heap

d9cbe12

BUILD/CONFIGURE: enforce 8KB stack threshold on compilation

416088e

UCT/UD/TEST: convert stack allocation at connect_iface_2k test_ud to …

35c1916

…heap

UCM/TEST: reduce stack memory usage in test_thread test

b0d3de4

BUILD: debug stuff

8072eee

michal-shalev force-pushed the convert-stack-allocation-to-heap branch from 746ca85 to 8072eee Compare September 16, 2024 19:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve stack memory usage at compile time #9963

Improve stack memory usage at compile time #9963

michal-shalev commented Jun 17, 2024 •

edited

Loading

tvegas1 Jun 18, 2024

tvegas1 Jun 18, 2024

michal-shalev Jun 18, 2024

ivankochin Jun 18, 2024

michal-shalev Jun 18, 2024

tvegas1 Jun 18, 2024

tvegas1 Jun 18, 2024

tvegas1 Jul 1, 2024

michal-shalev Jul 2, 2024

tvegas1 Jun 18, 2024

michal-shalev Jul 2, 2024

tvegas1 Jun 18, 2024

michal-shalev Jul 2, 2024

tvegas1 Jun 18, 2024

tvegas1 Jul 1, 2024

michal-shalev Jul 2, 2024

tvegas1 Jun 18, 2024

tvegas1 Jul 1, 2024

michal-shalev Jul 2, 2024

tvegas1 Jul 1, 2024

michal-shalev Jul 2, 2024

tvegas1 Jul 1, 2024

michal-shalev Jul 2, 2024

tvegas1 Jul 1, 2024

tvegas1 Jul 1, 2024

tvegas1 Jul 1, 2024

tvegas1 Jul 1, 2024

michal-shalev Aug 13, 2024

tvegas1 left a comment

tvegas1 Jul 2, 2024

michal-shalev Aug 13, 2024

tvegas1 Jul 2, 2024

michal-shalev Jul 2, 2024

Improve stack memory usage at compile time #9963

Are you sure you want to change the base?

Improve stack memory usage at compile time #9963

Conversation

michal-shalev commented Jun 17, 2024 • edited Loading

Waiting for:

What?

Why?

How?

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

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

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

tvegas1 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

michal-shalev commented Jun 17, 2024 •

edited

Loading