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.

Sign up for GitHub

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

Jump to bottom

cmake: link against DPDK shared libraries to avoid DPDK EAL double initialization #31877

Merged
merged 1 commit into from Oct 28, 2021
Merged

cmake: link against DPDK shared libraries to avoid DPDK EAL double initialization #31877

merged 1 commit into from Oct 28, 2021

Conversation

rosinL
Copy link
Member

@rosinL rosinL commented Nov 26, 2019 •
edited

The dpdk library initializes the EAL using constructors and global
variables, and cannot be re-initialized. Both test application and
libceph-common.so have a DPDK constructor that causes repeated
initialization. Link the ceph-common static library so that there
is only one constructor in test application.

Fixes: https://tracker.ceph.com/issues/42861
Signed-off-by: Chunsong Feng fengchunsong@huawei.com
Signed-off-by: luo rixin luorixin@huawei.com

Checklist

  • References tracker ticket
  • Updates documentation if necessary
  • Includes tests for new functionality or reproducer for bug
Show available Jenkins commands
  • jenkins retest this please
  • jenkins test crimson perf
  • jenkins test signed
  • jenkins test make check
  • jenkins test make check arm64
  • jenkins test submodules
  • jenkins test dashboard
  • jenkins test dashboard backend
  • jenkins test docs
  • jenkins render docs
  • jenkins test ceph-volume all
  • jenkins test ceph-volume tox

@tchaikov tchaikov self-requested a review November 26, 2019 12:28
@fengchunsong
Copy link
Contributor

@tchaikov @adamemerson @cbodley @yuyuyu101 Please help review, thank you！

@fengchunsong
Copy link
Contributor

The new solution is to modify the test application to link the ceph-common static library,this PR will close

@fengchunsong
Copy link
Contributor

@tchaikov please help to review,thanks

@tchaikov
Copy link
Contributor

@fengchunsong will take a look shortly.

@rosinL rosinL changed the title cmake: modify the link attribute private to avoid re-initialization test/msgr/cmake: link global-static to avoid DPDK EAL double initialization Dec 12, 2019
tchaikov
tchaikov reviewed Jan 6, 2020
View reviewed changes
src/test/msgr/CMakeLists.txt Outdated
@@ -16,15 +16,15 @@ add_executable(ceph_test_async_networkstack
test_async_networkstack.cc
$<TARGET_OBJECTS:unit-main>
)
target_link_libraries(ceph_test_async_networkstack global ${CRYPTO_LIBS} ${BLKID_LIBRARIES} ${CMAKE_DL_LIBS} ${UNITTEST_LIBS})
target_link_libraries(ceph_test_async_networkstack global-static ${CRYPTO_LIBS} ${BLKID_LIBRARIES} ${CMAKE_DL_LIBS} ${UNITTEST_LIBS})
Copy link
Contributor

Choose a reason for hiding this comment

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

how is ceph_test_async_networkstack linked against two copies of DPDK?

Copy link
Contributor

Choose a reason for hiding this comment

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

target_link_libraries use the PUBLIC link property by default, Libraries following PUBLIC are linked to, and are made part of the link interface. check build/src/test/msgr/CMakeFiles/ceph_perf_msgr_client.dir/link.txt:
../../../lib/libceph-common.so ../../../lib/libjson_spirit.a ../../../lib/libcommon_ utf8.a ../../../lib/liberasure_code.a ../../../lib/libcrc32.a ../../../lib/libarch.a -lresolv ../../../boost/lib/libboost_thread.a ../../../boost/lib/libboost_chrono.a ../../../boo st/lib/libboost_atomic.a ../../../boost/lib/libboost_system.a ../../../boost/lib/libboost_random.a ../../../boost/lib/libboost_program_options.a ../../../boost/lib/libboost_date_ti me.a ../../../boost/lib/libboost_iostreams.a ../../../boost/lib/libboost_regex.a -lstdc++fs ../../../lib/libfmt.a /usr/lib/aarch64-linux-gnu/libblkid.so /usr/lib/aarch64-linux-gnu/ libcrypto.so /lib/aarch64-linux-gnu/libudev.so /usr/lib/aarch64-linux-gnu/libibverbs.so /usr/lib/aarch64-linux-gnu/librdmacm.so /usr/lib/aarch64-linux-gnu/libz.so ../../../lib/libc ommon_async_dpdk.a -Wl,--whole-archive /home/chunsong/ceph/obj-aarch64-linux-gnu/src/dpdk/lib/librte_bus_pci.a /home/chunsong/ceph/obj-aarch64-linux-gnu/src/dpdk/lib/librte_cmdline.a /home/chunsong/ceph/obj-aarch64-linux-gnu/src/dpdk/lib/librte_eal.a /home/chunsong/ceph/obj-aarch64-linux-gnu/src/dpdk/lib/librte_ethdev.a /home/chunsong/ceph/obj-aarch64-linux- gnu/src/dpdk/lib/librte_kvargs.a /home/chunsong/ceph/obj-aarch64-linux-gnu/src/dpdk/lib/librte_mbuf.a /home/chunsong/ceph/obj-aarch64-linux-gnu/src/dpdk/lib/librte_mempool.a /home/ chunsong/ceph/obj-aarch64-linux-gnu/src/dpdk/lib/librte_mempool_ring.a /home/chunsong/ceph/obj-aarch64-linux-gnu/src/dpdk/lib/librte_pci.a /home/chunsong/ceph/obj-aarch64-linux-gnu /src/dpdk/lib/librte_hash.a /home/chunsong/ceph/obj-aarch64-linux-gnu/src/dpdk/lib/librte_net.a /home/chunsong/ceph/obj-aarch64-linux-gnu/src/dpdk/lib/librte_pmd_i40e.a /home/chuns ong/ceph/obj-aarch64-linux-gnu/src/dpdk/lib/librte_pmd_ixgbe.a /home/chunsong/ceph/obj-aarch64-linux-gnu/src/dpdk/lib/librte_pmd_hns3.a /home/chunsong/ceph/obj-aarch64-linux-gnu/sr c/dpdk/lib/librte_pmd_hinic.a /home/chunsong/ceph/obj-aarch64-linux-gnu/src/dpdk/lib/librte_ring.a -Wl,--no-whole-archive -Wl,-lnuma -Wl,-lpthread,-ldl ../../../lib/libgmock.a ../ ../../lib/libgtest.a -lpthread -ldl
it link libceph-common.so and librte_eal.a ,this cause two copies of eal

src/test/msgr/CMakeLists.txt Outdated

#ceph_perf_msgr_server
add_executable(ceph_perf_msgr_server perf_msgr_server.cc)
target_link_libraries(ceph_perf_msgr_server os global ${UNITTEST_LIBS})
target_link_libraries(ceph_perf_msgr_server os global-static ${UNITTEST_LIBS})
Copy link
Contributor

Choose a reason for hiding this comment

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

can we expose dpdk::dpdk as an INTERFACE_LINK_LIBRARIES exposed by os and ceph-common ?

Copy link
Contributor

Choose a reason for hiding this comment

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

ceph-common uses common-msg-objs. When WITH_DPDK is defined, common-msg-objs links common_async_dpdk. Extracting the dpdk separately will cause the common_async_dpdk link to fail.

@fengchunsong
Copy link
Contributor

@tchaikov @liu-chunmei please help to review, thks

@stale
Copy link

stale bot commented Apr 13, 2020

This pull request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs for another 30 days.
If you are a maintainer or core committer, please follow-up on this pull request to identify what steps should be taken by the author to move this proposed change forward.
If you are the author of this pull request, thank you for your proposed contribution. If you believe this change is still appropriate, please ensure that any feedback has been addressed and ask for a code review.

@stale stale bot added the stale label Apr 13, 2020
@tchaikov tchaikov removed the stale label Apr 13, 2020
@stale
Copy link

stale bot commented Jun 12, 2020

This pull request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs for another 30 days.
If you are a maintainer or core committer, please follow-up on this pull request to identify what steps should be taken by the author to move this proposed change forward.
If you are the author of this pull request, thank you for your proposed contribution. If you believe this change is still appropriate, please ensure that any feedback has been addressed and ask for a code review.

@stale stale bot added the stale label Jun 12, 2020
@tchaikov
Copy link
Contributor

unstable please

@stale stale bot removed the stale label Jun 12, 2020
@fengchunsong
Copy link
Contributor

Do not modify the link attributes, use other methods to solve, are there any good suggestions?

@stale
Copy link

stale bot commented Aug 13, 2020

This pull request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs for another 30 days.
If you are a maintainer or core committer, please follow-up on this pull request to identify what steps should be taken by the author to move this proposed change forward.
If you are the author of this pull request, thank you for your proposed contribution. If you believe this change is still appropriate, please ensure that any feedback has been addressed and ask for a code review.

@stale stale bot added the stale label Aug 13, 2020
@fengchunsong
Copy link
Contributor

unstable please
This problem is caused by link properties,maybe caused by https://github.com/ceph/ceph/pull/23650/files "-Wl,--whole-archive $&lt;JOIN:${DPDK_ARCHIVES}, > -Wl,--no-whole-archive")
The libc ommon_async_dpdk.a depends on the dpdk library librte_eal.a. All libceph-common.so and applications that use the libc ommon_async_dpdk.a are linked to librte_eal.a, if each application is linked to libceph-common.so, double librte_eal.a appears.

@fengchunsong
Copy link
Contributor

I re-modified the thread exit code by referring to the RDMADispatcher.

@stale stale bot removed the stale label Aug 17, 2020
@tchaikov tchaikov self-requested a review August 17, 2020 11:28
@fengchunsong
Copy link
Contributor

@ tchaikov please help to review,thanks

@stale
Copy link

stale bot commented Oct 31, 2020

This pull request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs for another 30 days.
If you are a maintainer or core committer, please follow-up on this pull request to identify what steps should be taken by the author to move this proposed change forward.
If you are the author of this pull request, thank you for your proposed contribution. If you believe this change is still appropriate, please ensure that any feedback has been addressed and ask for a code review.

@stale stale bot added the stale label Oct 31, 2020
@github-actions
Copy link

This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved

@stale
Copy link

stale bot commented Jul 21, 2021

This pull request has been automatically closed because there has been no activity for 90 days. Please feel free to reopen this pull request (or open a new one) if the proposed change is still appropriate. Thank you for your contribution!

@stale stale bot closed this Jul 21, 2021
@tchaikov tchaikov reopened this Jul 21, 2021
@stale stale bot removed the stale label Jul 21, 2021
@github-actions github-actions bot added the core label Jul 21, 2021
@rosinL rosinL closed this Oct 14, 2021
@rosinL rosinL reopened this Oct 14, 2021
@fengchunsong @rosinL
cmake: link DPDK shared libraries to avoid DPDK initialization failure
6fdb468 
The common_async_dpdk library depends on the dpdk librte_eal file.
If the static library is used, the dependency is transferred to
libceph-common. By default, libceph-common uses the PUBLIC keyword,
the keyword PUBLIC determines that both the application and
libceph-common link to dpdk librte_eal.a. As a result, the global
variables in dpdk librte_eal.a has two copies. So link DPDK shared
libs to avoid DPDK initialization failure.

Fixes: https://tracker.ceph.com/issues/42861

Signed-off-by: Chunsong Feng <fengchunsong@huawei.com>
Signed-off-by: luorixin <luorixin@huawei.com>
@github-actions github-actions bot added this to In progress in Dashboard Oct 14, 2021
@fengchunsong
Copy link
Contributor

@tchaikov please review again，thanks

Dashboard automation moved this from In progress to Reviewer approved Oct 14, 2021
@tchaikov tchaikov removed this from Reviewer approved in Dashboard Oct 14, 2021
@tchaikov tchaikov changed the title test/msgr/cmake: link global-static to avoid DPDK EAL double initialization cmake: link against DPDK shared libraries to avoid DPDK EAL double initialization Oct 14, 2021
@fengchunsong
Copy link
Contributor

jenkins retest this please

@fengchunsong
Copy link
Contributor

jenkins test dashboard cephadm

@fengchunsong
Copy link
Contributor

@tchaikov This PR has been approved, but is not automatically merged. please help check

@neha-ojha
Copy link
Member

@fengchunsong what kind of testing is required for this PR? Ideally, needs-qa means we create builds and run PRs through teuthology testing.

@tchaikov
Copy link
Contributor

pushed to https://shaman.ceph.com/builds/ceph/wip-31877/e107fc73164e4464bcf8a43f68edf0e30defef55/ .

once it builds, will merge it.

@tchaikov tchaikov self-assigned this Oct 27, 2021
@tchaikov tchaikov merged commit c2c7297 into ceph:master Oct 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fengchunsong fengchunsong fengchunsong left review comments

@tchaikov tchaikov tchaikov approved these changes

Assignees

@tchaikov tchaikov

Labels
build/ops core needs-qa
Projects
None yet
Milestone
No milestone
4 participants
@rosinL @fengchunsong @tchaikov @neha-ojha