Skip to content

Conversation

@CheSema
Copy link
Member

@CheSema CheSema commented Apr 14, 2025

Changelog category (leave one):

  • CI Fix or Improvement (changelog entry is not required)

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

Remove guard pages for threads and async_socket_for_remote/use_hedge_requests. Change the allocation method in FiberStack from mmap to aligned_alloc. Since this splits VMAs and under heavy load vm.max_map_count can be reached.

Documentation entry for user-facing changes

  • Documentation is written (mandatory for new features)

@clickhouse-gh
Copy link

clickhouse-gh bot commented Apr 14, 2025

Workflow [PR], commit [93312bd]

@clickhouse-gh clickhouse-gh bot added the pr-ci label Apr 14, 2025
@azat azat self-assigned this Apr 14, 2025
@CheSema CheSema marked this pull request as ready for review April 15, 2025 09:52
@canhld94
Copy link
Contributor

Same effort in #73510. In upstream PR we use mmap for fallback if cache is full, but in internal branch we've already switched to allocator.
I think it's still important to have some stacks with guarded page to catch stack overflow (though it will rarely happens).

@azat
Copy link
Member

azat commented Apr 16, 2025

FiberStack allocates its stack with allocator. Ir previously used mmap call for it.
Also the stack is protected with guarded page only in debug build.

You did it not only for fibers, so let's adjust the PR description and title

@azat
Copy link
Member

azat commented Apr 16, 2025

I think it's still important to have some stacks with guarded page to catch stack overflow (though it will rarely happens).

This is a good idea with respect to how many times we increased the stack size for fibers:

@CheSema WDYT? Maybe we just need to proceed with #73510 them?

@canhld94 we can avoid using mmap at all and simply install guard pages inside the aligned allocation, should work

@CheSema
Copy link
Member Author

CheSema commented Apr 16, 2025

I think it's still important to have some stacks with guarded page to catch stack overflow (though it will rarely happens).

This is a good idea with respect to how many times we increased the stack size for fibers:

Sema Checherinda WDYT? Maybe we just need to proceed with #73510 them?

Duc Canh Le we can avoid using mmap at all and simply install guard pages inside the aligned allocation, should work

This PR is created as a fix for an issue.
There won't be any new functionality or unnecessary changes.
The change about should or should not CurrentMemoryTracker take into account the guarded page is already unnecessary change here, but I share the same opinion here as you, we should account that page.

Make guarded pages with some probability in release is great idea. We could do it after.

Cache for allocations. Also good idea. I have some question there. It is already in progress, it would be discussed and merged in its PR.

@CheSema CheSema force-pushed the chesema-reduce-stack-protection branch from 0bf732d to a117179 Compare April 16, 2025 12:34
@CheSema CheSema force-pushed the chesema-reduce-stack-protection branch from a117179 to 3fbd6e0 Compare April 16, 2025 13:13
@azat
Copy link
Member

azat commented Apr 16, 2025

@CheSema the description is too low level, how about something like this?

Remove guard pages for threads and async_socket_for_remote/use_hedge_requests. Since this splits VMAs and under heavy load vm.max_map_count can be reached.

@canhld94
Copy link
Contributor

Make guarded pages with some probability in release is great idea. We could do it after.

I think we just need a pool of stacks with guard page, when the pool is full, we use normal allocator.

Copy link
Member

@azat azat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (don't forget to update the PR title, right now it says only about fibers)

@CheSema CheSema changed the title Fiber stack allocation stack allocation in Fiber and alternative stack for signals Apr 17, 2025
@CheSema CheSema added this pull request to the merge queue Apr 17, 2025
@azat azat changed the title stack allocation in Fiber and alternative stack for signals Remove guard pages from fibers and alternative stack for signals in threads Apr 17, 2025
@CheSema CheSema added the pr-must-backport Pull request should be backported intentionally. Use this label with great care! label Apr 17, 2025
Merged via the queue into master with commit c946382 Apr 17, 2025
119 checks passed
@CheSema CheSema deleted the chesema-reduce-stack-protection branch April 17, 2025 18:56
@robot-clickhouse-ci-2 robot-clickhouse-ci-2 added the pr-synced-to-cloud The PR is synced to the cloud repo label Apr 17, 2025
robot-clickhouse added a commit that referenced this pull request Apr 17, 2025
Cherry pick #79147 to 25.4: Remove guard pages from fibers and alternative stack for signals in threads
robot-clickhouse added a commit that referenced this pull request Apr 17, 2025
alexey-milovidov added a commit that referenced this pull request Apr 20, 2025
Backport #79147 to 25.4: Remove guard pages from fibers and alternative stack for signals in threads
@robot-ch-test-poll3 robot-ch-test-poll3 added the pr-backports-created Backport PRs are successfully created, it won't be processed by CI script anymore label Apr 22, 2025
@robot-ch-test-poll2 robot-ch-test-poll2 added the pr-backports-created-cloud deprecated label, NOOP label Apr 22, 2025
@CheSema CheSema mentioned this pull request Apr 24, 2025
1 task
@robot-clickhouse robot-clickhouse added the pr-must-backport-synced The `*-must-backport` labels are synced into the cloud Sync PR label Jul 2, 2025
zvonand pushed a commit to Altinity/ClickHouse that referenced this pull request Jul 10, 2025
…ack-protection

stack allocation in Fiber and alternative stack for signals
Enmk added a commit to Altinity/ClickHouse that referenced this pull request Jul 11, 2025
25.3.5 Backport of ClickHouse#79147: Remove guard pages from fibers and alternative stack for signals in threads
zvonand pushed a commit to Altinity/ClickHouse that referenced this pull request Jul 16, 2025
…ack-protection

stack allocation in Fiber and alternative stack for signals
zvonand added a commit to Altinity/ClickHouse that referenced this pull request Jul 18, 2025
25.3.6 Backport of ClickHouse#79147: Remove guard pages from fibers and alternative stack for signals in threads
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-backports-created Backport PRs are successfully created, it won't be processed by CI script anymore pr-backports-created-cloud deprecated label, NOOP pr-ci pr-must-backport Pull request should be backported intentionally. Use this label with great care! pr-must-backport-synced The `*-must-backport` labels are synced into the cloud Sync PR pr-synced-to-cloud The PR is synced to the cloud repo

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants