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

cmake: WITH_SPDK=ON by default #18944

Merged
merged 9 commits into from Nov 18, 2017

Conversation

Projects
None yet
4 participants
@tchaikov
Copy link
Contributor

commented Nov 15, 2017

No description provided.

@tchaikov tchaikov requested a review from liupan1111 Nov 15, 2017

@tchaikov tchaikov requested a review from smithfarm Nov 15, 2017

@@ -29,6 +29,7 @@ Build-Depends: bc,
libbabeltrace-ctf-dev,
libbabeltrace-dev,
libblkid-dev (>= 2.17),
libcunit1-dev,

This comment has been minimized.

Copy link
@tchaikov

tchaikov Nov 15, 2017

Author Contributor

@liupan1111 this what i mean by debian/control.

@@ -183,6 +183,7 @@ BuildRequires: keyutils-libs-devel
BuildRequires: libibverbs-devel
BuildRequires: openldap-devel
BuildRequires: openssl-devel
BuildRequires: CUnit-devel

This comment has been minimized.

Copy link
@tchaikov

tchaikov Nov 15, 2017

Author Contributor

@smithfarm does this work for SLE? or do you want to WITH_SPDK=ON for the build also?

set(machine "armv8a")
elseif(CMAKE_SYSTEM_PROCESSOR MATCHES "(powerpc|ppc)64")
set(arch "ppc_64")
set(machine "power8")

This comment has been minimized.

Copy link
@tchaikov

tchaikov Nov 15, 2017

Author Contributor

@smithfarm do you think we should enable DPDK on "power8" also? as http://dpdk.org/doc/guides/linux_gsg/build_dpdk.html claims that DPDK supports power8 as well.

@tchaikov tchaikov force-pushed the tchaikov:wip-dpdk-spdk branch 2 times, most recently from b13d8a7 to ab4bf0c Nov 15, 2017

endif()

if(WITH_KERNEL_DIR)
if(NOT IS_DIRECTORY ${WITH_KERNEL_DIR})

This comment has been minimized.

Copy link
@tchaikov

tchaikov Nov 15, 2017

Author Contributor

@liupan1111 this is my biggest concern. the target machine's kernel is not always the same as the build host. even if we managed to fulfill the build-requirement by installing the package of linux-headers-`uname -r` , there is good chance that the built DPDK kernel module does not work for the deployed machine.

or we don't need to build the kernel module at all? if yes, how?

This comment has been minimized.

Copy link
@liewegas

liewegas Nov 15, 2017

Member

I'm not sure I understand the kernel version dependency. Does the build have to be built against a specific kernel? I.e., if we build on the centos 7.3 kernel will the built resuilt break on 7.4? Or a build on the 16.04 kernel break on a later HWE kernel?

As long as those scenerios behave okay, I think it's fine to continue. If there is a hard kernel dependency, then I'm not sure what we should do.. that would suck. :(

This comment has been minimized.

Copy link
@smithfarm

smithfarm Nov 15, 2017

Contributor

For SLE, I'm pretty sure kernel module code needs to be integrated into, built as part of, and shipped in the kernel package itself. That may be an oversimplification, but certainly I wouldn't expect to be building kernel module packages via ceph.spec.

This comment has been minimized.

Copy link
@smithfarm

smithfarm Nov 15, 2017

Contributor

@ddiss Can you vet my comment, above?

This comment has been minimized.

Copy link
@tchaikov

tchaikov Nov 15, 2017

Author Contributor

yeah, to my best knowledge, kernel modules are not portable across different versions. anyway, i am disabling the kernel modules in DPDK.

@liupan1111 could you help test the build w/o the kernel modules? thanks

@smithfarm

This comment has been minimized.

Copy link
Contributor

commented Nov 15, 2017

@tchaikov As usual, I'm running behind. . . I haven't tried to build master on SLE lately. I will try building this branch and let you know the result.

@tchaikov tchaikov force-pushed the tchaikov:wip-dpdk-spdk branch 7 times, most recently from 2aa0b61 to 2415725 Nov 15, 2017

setconf CONFIG_RTE_KNI_KMOD n
setconf CONFIG_RTE_KNI_PREEMPT_DEFAULT n

setconf CONFIG_RTE_APP_EVENTDEV n

This comment has been minimized.

Copy link
@tchaikov

tchaikov Nov 15, 2017

Author Contributor

i know, this option is not in the used DPDK yet.

@tchaikov tchaikov added the needs-qa label Nov 16, 2017

machine=$1
shift

setconf CONFIG_RTE_MACHINE "${machine}"

This comment has been minimized.

Copy link
@tchaikov

tchaikov Nov 16, 2017

Author Contributor

@yuyuyu101 do you think we can have a functional SPDK backend without all these kernel modules?

@tchaikov tchaikov force-pushed the tchaikov:wip-dpdk-spdk branch 7 times, most recently from 2fd5c65 to 88a1081 Nov 16, 2017

@tchaikov tchaikov force-pushed the tchaikov:wip-dpdk-spdk branch 5 times, most recently from 97e824d to bcc3cb0 Nov 16, 2017

@tchaikov

This comment has been minimized.

Copy link
Contributor Author

commented Nov 17, 2017

@tchaikov tchaikov force-pushed the tchaikov:wip-dpdk-spdk branch 2 times, most recently from 4b11a59 to 42f6fc2 Nov 17, 2017

@tchaikov tchaikov requested a review from yuyuyu101 Nov 17, 2017

@tchaikov tchaikov force-pushed the tchaikov:wip-dpdk-spdk branch from 42f6fc2 to c1dfc49 Nov 17, 2017

@@ -348,6 +348,10 @@ endif()

option(WITH_DPDK "Enable DPDK messaging" OFF)
if(WITH_DPDK)
if(NOT USE_CRYPTOPP)
message(FATAL_ERROR "CRYPTOPP must be supported when enable DPDK.")

This comment has been minimized.

Copy link
@tchaikov

tchaikov Nov 17, 2017

Author Contributor

the the incompatible option settings at the toplevel cmake script.

@@ -687,6 +657,32 @@ if(HAVE_ARMV8_CRC)
list(APPEND ceph_common_deps common_crc_aarch64)
endif(HAVE_ARMV8_CRC)

if(WITH_DPDK)

This comment has been minimized.

Copy link
@tchaikov

tchaikov Nov 17, 2017

Author Contributor

so we can have the WITH_DPDK support in a single place.

target_link_libraries(common_async_dpdk
${DPDK_LIBRARIES})
set_target_properties(common_async_dpdk PROPERTIES
COMPILE_FLAGS "-march=native -I${DPDK_INCLUDE_DIR}")

This comment has been minimized.

Copy link
@tchaikov

tchaikov Nov 17, 2017

Author Contributor

so we can avoid adding "-march=native" globally. this will be a show stopper for downstream packagers/maintainers, if we want to enable DPDK by default.

cmake: build spdk/dpdk in cmake modules
Signed-off-by: Kefu Chai <kchai@redhat.com>
Signed-off-by: wanjun.lp <wanjun.lp@alibaba-inc.com>
Signed-off-by: Ziye Yang <optimistyzy@gmail.com>

@tchaikov tchaikov force-pushed the tchaikov:wip-dpdk-spdk branch from c1dfc49 to 34bb7d3 Nov 17, 2017

tchaikov added some commits Nov 15, 2017

rpm,deb: add cunit1 as build dependency
Signed-off-by: Kefu Chai <kchai@redhat.com>
cmake: WITH_SPDK=ON by default
Signed-off-by: wanjun.lp <wanjun.lp@alibaba-inc.com>
Signed-off-by: Kefu Chai <kchai@redhat.com>

@tchaikov tchaikov force-pushed the tchaikov:wip-dpdk-spdk branch from 34bb7d3 to 0e5e3af Nov 17, 2017

@liu-chunmei

This comment has been minimized.

Copy link
Contributor

commented Nov 17, 2017

@tchaikov does your PR support the following 3 cases:

  1. only enable WITH_DPDK = ON
  2. only enable WITH_SPDK = ON
  3. enable both WITH_DPDK =ON and WITH_SPDK =ON
    Is there build error for these 3 scenarios? and for these 3 scenarios, can the ceph system start up?
    if yes, I think we can merge the patch first. Then for the functional issue we can fix later.
@liu-chunmei

This comment has been minimized.

Copy link
Contributor

commented Nov 17, 2017

@tchaikov Maybe you need squash the several commits into one before merge it.

tchaikov and others added some commits Nov 15, 2017

cmake: refactor dpdk related code
* move the check of `USE_CRYPTOPP` to $top_srcdir/CMakeLists.txt
* remove reference of DPDK_LIBRARY, it's defined nowhere
* move the dpdk code to a single place

Signed-off-by: Kefu Chai <kchai@redhat.com>
cmake: add rte_mempool_{ring,stack} to Finddpdk.cmake
* also refactor it a little bit

Signed-off-by: Kefu Chai <kchai@redhat.com>
cmake: remove duplicated spdk linkage
Signed-off-by: Kefu Chai <kchai@redhat.com>
cmake: use BuildDPDK if libdpdk not found
Signed-off-by: Kefu Chai <kchai@redhat.com>
msg/async: rebase msg/async/dpdk to spdk/dpdk
Signed-off-by: Liu-Chunmei <chunmei.liu@intel.com>

@tchaikov tchaikov force-pushed the tchaikov:wip-dpdk-spdk branch from 0e5e3af to 0610332 Nov 18, 2017

@tchaikov

This comment has been minimized.

Copy link
Contributor Author

commented Nov 18, 2017

@liu-chunmei i squashed the two DPDK cleanup changes into a single one. but i'd like to keep other changes separated commits. as in this way, smaller changes will help the posterity to understand changes.

and all combinations of {WITH_DPDK,WITH_SPDK}={ON/OFF} build, and are tested using rados bench. WITH_SPDK=ON WITH_DPDK=OFF has been passed rados suite at http://pulpito.ceph.com/kchai-2017-11-17_09:57:45-rados-wip-kefu-testing-2017-11-17-1613-distro-basic-smithi/, the failures are not related.

@tchaikov tchaikov merged commit b9c822a into ceph:master Nov 18, 2017

5 checks passed

Docs: build check OK - docs built
Details
Signed-off-by all commits in this PR are signed
Details
Unmodified Submodules submodules for project are unmodified
Details
make check make check succeeded
Details
make check (arm64) make check succeeded
Details

@tchaikov tchaikov deleted the tchaikov:wip-dpdk-spdk branch Nov 18, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.