Improve stack memory usage at compile time

[michal-shalev]

Waiting for:

• [x] https://github.com/openucx/ucx/pull/10128
• [x] https://github.com/openucx/ucx/pull/10157
• [x] https://github.com/openucx/ucx/pull/10130
• [ ] https://github.com/openucx/ucx/pull/10131
• [ ] https://github.com/openucx/ucx/pull/10127

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?

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

[iyastreb]

I want to express a few concerns about this change.

1. This change might be error prone, because you need to change the flow of every function from return into goto err. It's easy to overlook a memory leaks, like I found above (I checked only 20% of the change).
2. We have a lot of boilerplate code..
3. Changing function flow due to buffer allocations seems like an overkill to me..
4. Replacing stack allocated arrays with malloc/free might not be the best choice for performance reasons.

Can we just use thread local temporary buffer instead of malloc/free? So that we reuse the same buffer and don't have to allocate it again and again Simplest form could be like:

/* In case we're using a C99 compiler */
#ifndef thread_local
#define thread_local __thread
#endif

char * ucs_get_tmp_path()
{
    static thread_local char path[PATH_MAX];
    return path;
}

This way we have a thread safe buffer that is not allocated on stack. And also we can keep existing functions behaviour unchanged, and without tons of boilerplate code.

• @yosefe What do you think?

[gleon99]

I want to express a few concerns about this change.

1. This change might be error prone, because you need to change the flow of every function from return into goto err. It's easy to overlook a memory leaks, like I found above (I checked only 20% of the change).
2. We have a lot of boilerplate code..
3. Changing function flow due to buffer allocations seems like an overkill to me..
4. Replacing stack allocated arrays with malloc/free might not be the best choice for performance reasons.

Can we just use thread local temporary buffer instead of malloc/free? So that we reuse the same buffer and don't have to allocate it again and again Simplest form could be like:

/* In case we're using a C99 compiler */
#ifndef thread_local
#define thread_local __thread
#endif

char * ucs_get_tmp_path()
{
    static thread_local char path[PATH_MAX];
    return path;
}

This way we have a thread safe buffer that is not allocated on stack. And also we can keep existing functions behaviour unchanged, and without tons of boilerplate code.

• @yosefe What do you think?

@iyastreb thanks for the comments, the concerns seem legitimate but the PR is still a draft. Let's address them when the PR is stable and ready for review.

[michal-shalev]

/azp run UCX PR

[azure-pipelines[bot]]

Azure Pipelines successfully started running 1 pipeline(s).

[michal-shalev]

/azp run UCX PR

[azure-pipelines[bot]]

Pull request contains merge conflicts.

[michal-shalev]

@iyastreb @tvegas1 please re-review