Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pacific: cmake: re-enable TCMalloc and allocator related cleanups #51282

Closed
wants to merge 3 commits into from

Conversation

pponnuvel
Copy link
Contributor

backport tracker: https://tracker.ceph.com/issues/55541


backport of #46103
parent tracker: https://tracker.ceph.com/issues/55519

this backport was staged using ceph-backport.sh version 16.0.0.6848
find the latest version at https://github.com/ceph/ceph/blob/main/src/script/ceph-backport.sh

before this change, the ALLOCATOR cmake option are handled at two
difference places: one to handle ALLOCATOR option, another to add
compilation options based on ALLOCATOR. after this change:

* the ALLOCATOR option is handled in a single place
* dedup the branches handling different ALLOCATORS into a single
  condition: (NOT ALLOCATOR STREQUAL "libc")
* add_compile_options() calls are consolidated into a single one

Signed-off-by: Kefu Chai <tchaikov@gmail.com>
(cherry picked from commit 677e134)

Conflicts:
	CMakeLists.txt
- Specify version 2.6.2 for gperftools package
before this change, we assume that "ALLOCATOR" is the library name
which can be found by linker with "ld -l <namespec>". but ideally,
cmake can find a library in a more sophiscated way used by
its "find_library()" implementation. so, in this change, instead of
relying on the default paths looked up by "ld", use the path
found by cmake.

Signed-off-by: Kefu Chai <tchaikov@gmail.com>

Conflicts:
	cmake/modules/BuildFIO.cmake
to enable them to use the specified allocators

Fixes: https://tracker.ceph.com/issues/55519
Signed-off-by: Kefu Chai <tchaikov@gmail.com>
@pponnuvel pponnuvel requested a review from a team as a code owner April 28, 2023 14:18
@github-actions github-actions bot added this to the pacific milestone Apr 28, 2023
@pponnuvel
Copy link
Contributor Author

jenkins test api

@pponnuvel
Copy link
Contributor Author

@tchaikov Can you please take a look at this? I am hoping to get this into the possibly-last Pacific point release.

@pponnuvel
Copy link
Contributor Author

Ping.

Can this be looked at again?

ljflores
ljflores previously approved these changes Jan 10, 2024
@ljflores ljflores modified the milestones: pacific, v16.2.15 Jan 10, 2024
Copy link
Contributor

@ljflores ljflores left a comment

Choose a reason for hiding this comment

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

@pponnuvel I built this patch and it is causing a build problem:

https://jenkins.ceph.com/job/ceph-dev-new-build/ARCH=x86_64,AVAILABLE_ARCH=x86_64,AVAILABLE_DIST=bionic,DIST=bionic,MACHINE_SIZE=gigantic/75706/consoleFull#3154186727641bc0b-bacc-4481-8290-0cf28df0ce68

-- Found LZ4: /usr/lib/x86_64-linux-gnu/liblz4.so (found suitable version "1.7.1", minimum required is "1.7") 
CMake Error at /usr/share/cmake-3.10/Modules/FindPackageHandleStandardArgs.cmake:137 (message):
  Could NOT find gperftools: Found unsuitable version "2.5", but required is
  at least "2.6.2" (found /usr/include)
Call Stack (most recent call first):
  /usr/share/cmake-3.10/Modules/FindPackageHandleStandardArgs.cmake:376 (_FPHSA_FAILURE_MESSAGE)
  cmake/modules/Findgperftools.cmake:52 (find_package_handle_standard_args)
  CMakeLists.txt:346 (find_package)

PTAL when you can. As Pacific is going EOL soon, we maybe should not include this.

@ljflores ljflores dismissed their stale review January 11, 2024 20:18

Caused a build failure.

@pponnuvel
Copy link
Contributor Author

@ljflores The gperftools version on Ubuntu/Bionic is 2.5, so obviously this fails to meet the minimum required version.
Looking at this requirment, this was introduced to be able to use aligned_alloc (#28093). So removing this requirement (gperftools >= 2.6.2) isn't straightforward.

Not using tcmalloc at all on Bionic defeats the purpose of this backport as the intention was to link with tcmalloc for performance reasons.

I originally proposed this backport in light of https://bugs.launchpad.net/ubuntu/+source/ceph/+bug/2016845.
However, we have since linked with tcmalloc in our (Ubuntu's) Pacific release anyway. Needing gperftools >= 2.6.2 wasn't
an issue for us since we don't ship Pacific on Ubuntu/Bionic.

So I agree this isn't a critical issue for us in Ubuntu and can be closed since Pacific is going EOL anyway.
Besides the reason for this backport is a performance, not any bug critical for functionality. Hence I am OK to close this PR as you suggested.

Perhaps it's time to drop Ubuntu/Bionic (18.04) from https://docs.ceph.com/en/quincy/start/os-recommendations/#platforms so that Ubuntu/Bionic (18.04) need not be tested/built going forward. But that's a different discussion.

@pponnuvel pponnuvel closed this Jan 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants