You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Adar Dembo (Code Review)" <ge...@cloudera.org> on 2019/11/02 07:50:21 UTC
[kudu-CR] KUDU-2990: remove memkind/libnuma and dynamically link memkind at runtime
Hello Alexey Serbin, Andrew Wong, ye yuqiang, Todd Lipcon,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/14620
to review the following change.
Change subject: KUDU-2990: remove memkind/libnuma and dynamically link memkind at runtime
......................................................................
KUDU-2990: remove memkind/libnuma and dynamically link memkind at runtime
This patch removes memkind and libnuma from the thirdparty tree and changes
the NVM cache implementation to dynamically link memkind at runtime using
dlopen() and friends. KUDU-2990 explains why we need to do this in great
detail so I won't repeat that here. The alternatives I considered:
1. Patch memkind to dlopen() libnuma.
This is probably the most user-friendly approach because libnuma is found in
most systems (or at least in most package repositories), but a new enough
memkind is not, and having Kudu distribute memkind eases adoption of the NVM
cache. However, I was nervous about performing deep surgery in memkind as
it's a codebase with which I'm not familiar, and I wanted to minimize risk
because we'll be backporting this patch to several older branches.
2. Patch memkind to define weak libnuma symbols.
If done correctly, behavior is unchanged when libnuma is present on the host
system, but if it's not there, calls from memkind to libnuma will crash.
Again, I didn't feel comfortable hacking into memkind, plus I've found weak
symbols difficult to use in the past.
3. Remove the NVM cache from Kudu altogether.
This is obviously the safest and simplest option, but it punishes Kudu users
who actually use the NVM cache.
4. Gate the build of the NVM cache behind a CMake option.
This ought to satisfy the ASF's definition of an "optional" feature, but
does nothing for binary redistributors who wish to offer the NVM cache and
who build Kudu as a statically linked "fat" binary.
5. Build as we do today, but forbid static linkage of libnuma.
Binary redistributors will need to choose between including libnuma in their
distributions, or forcing Kudu to look for libnuma at runtime. The former
still violates ASF policy, and the latter means Kudu won't start on a system
lacking libnuma, regardless of whether the NVM cache is actually in use.
So what are the ramifications of the chosen approach?
- Kudu no longer distributes memkind or libnuma. To use the NVM cache, the
host system must provide both, and memkind must be version 1.6.0 or newer.
CentOS 6.6, CentOS 7.3, and Ubuntu 18.04 repositories all carried memkind
1.1.0, so unless you're on a cutting edge distro you'll have to build
memkind from source.
- Tests that exercise the NVM cache will be skipped if they can't find a
conformant memkind (and libnuma).
- When starting Kudu, if you don't set --block_cache_type=NVM, you shoudn't
notice any change.
- If you do, Kudu will crash at startup if it can't find a conformant
memkind. This affects upgrades: if you were already an NVM cache user but
you didn't have memkind installed, your Kudu will crash post-upgrade.
Note: this doesn't preclude implementing alternative #1 (the one I think is
ideal) in the future; we'll just have to revert the bulk of this patch when
we do so.
To test, I ran cfile-test and cache-test as follows:
- Without memkind installed: DRAM tests passed, NVM tests were skipped
- With an old memkind installed: DRAM tests passed, NVM tests were skipped
- With LD_LIBRARY_PATH=/path/to/memkind-1.9.0: All tests passed
I also manually ran a Kudu master with --block_cache_type=NVM to verify the
crashing behavior.
Change-Id: I4f474196aa98b5fa6e5966b9a3aea9a7e466805c
---
M CMakeLists.txt
D cmake_modules/FindMemkind.cmake
D cmake_modules/FindNuma.cmake
M src/kudu/cfile/CMakeLists.txt
M src/kudu/cfile/block_cache.cc
M src/kudu/cfile/cfile-test.cc
M src/kudu/util/CMakeLists.txt
M src/kudu/util/cache-test.cc
M src/kudu/util/nvm_cache.cc
M src/kudu/util/nvm_cache.h
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
D thirdparty/patches/memkind-fix-build-with-old-autoconf.patch
D thirdparty/patches/memkind-fix-build-with-old-glibc.patch
D thirdparty/patches/memkind-fix-jemalloc-build-with-old-autoconf.patch
M thirdparty/vars.sh
17 files changed, 210 insertions(+), 391 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/14620/1
--
To view, visit http://gerrit.cloudera.org:8080/14620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I4f474196aa98b5fa6e5966b9a3aea9a7e466805c
Gerrit-Change-Number: 14620
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: ye yuqiang <yu...@intel.com>
[kudu-CR] KUDU-2990: remove memkind/libnuma and dynamically link memkind at runtime
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14620 )
Change subject: KUDU-2990: remove memkind/libnuma and dynamically link memkind at runtime
......................................................................
Patch Set 2:
> (3 comments)
>
> > I think we can remove libnvm_cache, and that solves the problem
> of compilation at macOS. I put together a micro-patch for that
> (apply on top of this patch):
> > https://gist.github.com/alexeyserbin/04f182dbc91b34e950dcaf263f28eb7c
>
> Thanks for doing this, but I'd prefer to link nvm_cache into macOS
> and rely on the runtime check to gate the feature. That way we
> don't need the runtime linking functionality AND a bunch of
> Linux-specific #ifdefs.
I mean we could get rid of libnvm_cache on both Linux and macOS. The patch I posted is supposed to be applied on top of PS1 of this (i.e. your) patch. That way we could get rid of the two things you mentioned and also get rid of an extra library.
What's the reason to have a separate kudu_nvm library? Maybe I'm missing something, but I don't see why not to remove it once the code for both Linux and macOS is unified and uses dlopen/dlsym to find corresponding functions.
--
To view, visit http://gerrit.cloudera.org:8080/14620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4f474196aa98b5fa6e5966b9a3aea9a7e466805c
Gerrit-Change-Number: 14620
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: ye yuqiang <yu...@intel.com>
Gerrit-Comment-Date: Mon, 04 Nov 2019 20:29:27 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2990: remove memkind/libnuma and dynamically link memkind at runtime
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14620 )
Change subject: KUDU-2990: remove memkind/libnuma and dynamically link memkind at runtime
......................................................................
Patch Set 4:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/14620/3/src/kudu/util/nvm_cache.h
File src/kudu/util/nvm_cache.h:
http://gerrit.cloudera.org:8080/#/c/14620/3/src/kudu/util/nvm_cache.h@27
PS3, Line 27: RETURN_IF_NO_NVM_CAC
> nit: maybe call this RETURN_IF_...?
Done
http://gerrit.cloudera.org:8080/#/c/14620/3/src/kudu/util/nvm_cache.cc
File src/kudu/util/nvm_cache.cc:
http://gerrit.cloudera.org:8080/#/c/14620/3/src/kudu/util/nvm_cache.cc@92
PS3, Line 92: // Taken together, these typedefs and this macro make it easy to call a
> nit: maybe move this indirection layer to a separate source file/header?
To what end? We're only using it in nvm_cache.cc.
--
To view, visit http://gerrit.cloudera.org:8080/14620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4f474196aa98b5fa6e5966b9a3aea9a7e466805c
Gerrit-Change-Number: 14620
Gerrit-PatchSet: 4
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: ye yuqiang <yu...@intel.com>
Gerrit-Comment-Date: Wed, 06 Nov 2019 08:40:31 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2990: remove memkind/libnuma and dynamically link memkind at runtime
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Andrew Wong, ye yuqiang, Grant Henke, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14620
to look at the new patch set (#5).
Change subject: KUDU-2990: remove memkind/libnuma and dynamically link memkind at runtime
......................................................................
KUDU-2990: remove memkind/libnuma and dynamically link memkind at runtime
This patch removes memkind and libnuma from the thirdparty tree and changes
the NVM cache implementation to dynamically link memkind at runtime using
dlopen() and friends. KUDU-2990 explains why we need to do this in great
detail so I won't repeat that here. The alternatives I considered:
1. Patch memkind to dlopen() libnuma.
This is probably the most user-friendly approach because libnuma is found in
most systems (or at least in most package repositories), but a new enough
memkind is not, and having Kudu distribute memkind eases adoption of the NVM
cache. However, I was nervous about performing deep surgery in memkind as
it's a codebase with which I'm not familiar, and I wanted to minimize risk
because we'll be backporting this patch to several older branches.
2. Patch memkind to define weak libnuma symbols.
If done correctly, behavior is unchanged when libnuma is present on the host
system, but if it's not there, calls from memkind to libnuma will crash.
Again, I didn't feel comfortable hacking into memkind, plus I've found weak
symbols difficult to use in the past.
3. Remove the NVM cache from Kudu altogether.
This is obviously the safest and simplest option, but it punishes Kudu users
who actually use the NVM cache.
4. Gate the build of the NVM cache behind a CMake option.
This ought to satisfy the ASF's definition of an "optional" feature, but
does nothing for binary redistributors who wish to offer the NVM cache and
who build Kudu as a statically linked "fat" binary.
5. Build as we do today, but forbid static linkage of libnuma.
Binary redistributors will need to choose between including libnuma in their
distributions, or forcing Kudu to look for libnuma at runtime. The former
still violates ASF policy, and the latter means Kudu won't start on a system
lacking libnuma, regardless of whether the NVM cache is actually in use.
So what are the ramifications of the chosen approach?
- Kudu no longer distributes memkind or libnuma. To use the NVM cache, the
host system must provide both, and memkind must be version 1.6.0 or newer.
CentOS 6 and Ubuntu 18.04 repositories all carried memkind 1.1.0. CentOS 7
has memkind 1.7.0. Persistent memory hardware itself also has a pretty
steep kernel version requirement, so it's unlikely to be found outside of
a new distro in the first place.
- Tests that exercise the NVM cache will be skipped if they can't find a
conformant memkind (and libnuma).
- When starting Kudu, if you don't set --block_cache_type=NVM, you shoudn't
notice any change.
- If you do, Kudu will crash at startup if it can't find a conformant
memkind. This affects upgrades: if you were already an NVM cache user but
you didn't have memkind installed, your Kudu will crash post-upgrade.
Note: this doesn't preclude implementing alternative #1 (the one I think is
ideal) in the future; we'll just have to revert the bulk of this patch when
we do so.
To test, I ran cfile-test and cache-test as follows:
- Without memkind installed: DRAM tests passed, NVM tests were skipped
- With an old memkind installed: DRAM tests passed, NVM tests were skipped
- With LD_LIBRARY_PATH=/path/to/memkind-1.9.0: All tests passed
I also manually ran a Kudu master with --block_cache_type=NVM and without
memkind to verify the crashing behavior.
Change-Id: I4f474196aa98b5fa6e5966b9a3aea9a7e466805c
---
M CMakeLists.txt
D cmake_modules/FindMemkind.cmake
D cmake_modules/FindNuma.cmake
M src/kudu/cfile/CMakeLists.txt
M src/kudu/cfile/block_cache.cc
M src/kudu/cfile/cfile-test.cc
M src/kudu/util/CMakeLists.txt
M src/kudu/util/cache-test.cc
M src/kudu/util/nvm_cache.cc
M src/kudu/util/nvm_cache.h
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
D thirdparty/patches/memkind-fix-build-with-old-autoconf.patch
D thirdparty/patches/memkind-fix-build-with-old-glibc.patch
D thirdparty/patches/memkind-fix-jemalloc-build-with-old-autoconf.patch
M thirdparty/vars.sh
17 files changed, 197 insertions(+), 397 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/14620/5
--
To view, visit http://gerrit.cloudera.org:8080/14620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4f474196aa98b5fa6e5966b9a3aea9a7e466805c
Gerrit-Change-Number: 14620
Gerrit-PatchSet: 5
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: ye yuqiang <yu...@intel.com>
[kudu-CR] KUDU-2990: remove memkind/libnuma and dynamically link memkind at runtime
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/14620 )
Change subject: KUDU-2990: remove memkind/libnuma and dynamically link memkind at runtime
......................................................................
Removed reviewer Kudu Jenkins with the following votes:
* Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/14620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteReviewer
Gerrit-Change-Id: I4f474196aa98b5fa6e5966b9a3aea9a7e466805c
Gerrit-Change-Number: 14620
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: ye yuqiang <yu...@intel.com>
[kudu-CR] KUDU-2990: remove memkind/libnuma and dynamically link memkind at runtime
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14620 )
Change subject: KUDU-2990: remove memkind/libnuma and dynamically link memkind at runtime
......................................................................
Patch Set 1:
(1 comment)
> (1 comment)
I think we can remove libnvm_cache, and that solves the problem of compilation at macOS. I put together a micro-patch for that (apply on top of this patch):
https://gist.github.com/alexeyserbin/04f182dbc91b34e950dcaf263f28eb7c
http://gerrit.cloudera.org:8080/#/c/14620/1/src/kudu/util/nvm_cache.cc
File src/kudu/util/nvm_cache.cc:
http://gerrit.cloudera.org:8080/#/c/14620/1/src/kudu/util/nvm_cache.cc@695
PS1, Line 695:
nit: shifted too far on the right?
--
To view, visit http://gerrit.cloudera.org:8080/14620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4f474196aa98b5fa6e5966b9a3aea9a7e466805c
Gerrit-Change-Number: 14620
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: ye yuqiang <yu...@intel.com>
Gerrit-Comment-Date: Sun, 03 Nov 2019 08:00:31 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2990: remove memkind/libnuma and dynamically link memkind at runtime
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14620 )
Change subject: KUDU-2990: remove memkind/libnuma and dynamically link memkind at runtime
......................................................................
Patch Set 2: Verified+1
Overriding Jenkins, unrelated test flake.
--
To view, visit http://gerrit.cloudera.org:8080/14620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4f474196aa98b5fa6e5966b9a3aea9a7e466805c
Gerrit-Change-Number: 14620
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: ye yuqiang <yu...@intel.com>
Gerrit-Comment-Date: Sun, 03 Nov 2019 22:14:43 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2990: remove memkind/libnuma and dynamically link memkind at runtime
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/14620 )
Change subject: KUDU-2990: remove memkind/libnuma and dynamically link memkind at runtime
......................................................................
KUDU-2990: remove memkind/libnuma and dynamically link memkind at runtime
This patch removes memkind and libnuma from the thirdparty tree and changes
the NVM cache implementation to dynamically link memkind at runtime using
dlopen() and friends. KUDU-2990 explains why we need to do this in great
detail so I won't repeat that here. The alternatives I considered:
1. Patch memkind to dlopen() libnuma.
This is probably the most user-friendly approach because libnuma is found in
most systems (or at least in most package repositories), but a new enough
memkind is not, and having Kudu distribute memkind eases adoption of the NVM
cache. However, I was nervous about performing deep surgery in memkind as
it's a codebase with which I'm not familiar, and I wanted to minimize risk
because we'll be backporting this patch to several older branches.
2. Patch memkind to define weak libnuma symbols.
If done correctly, behavior is unchanged when libnuma is present on the host
system, but if it's not there, calls from memkind to libnuma will crash.
Again, I didn't feel comfortable hacking into memkind, plus I've found weak
symbols difficult to use in the past.
3. Remove the NVM cache from Kudu altogether.
This is obviously the safest and simplest option, but it punishes Kudu users
who actually use the NVM cache.
4. Gate the build of the NVM cache behind a CMake option.
This ought to satisfy the ASF's definition of an "optional" feature, but
does nothing for binary redistributors who wish to offer the NVM cache and
who build Kudu as a statically linked "fat" binary.
5. Build as we do today, but forbid static linkage of libnuma.
Binary redistributors will need to choose between including libnuma in their
distributions, or forcing Kudu to look for libnuma at runtime. The former
still violates ASF policy, and the latter means Kudu won't start on a system
lacking libnuma, regardless of whether the NVM cache is actually in use.
So what are the ramifications of the chosen approach?
- Kudu no longer distributes memkind or libnuma. To use the NVM cache, the
host system must provide both, and memkind must be version 1.6.0 or newer.
CentOS 6 and Ubuntu 18.04 repositories all carried memkind 1.1.0. CentOS 7
has memkind 1.7.0. Persistent memory hardware itself also has a pretty
steep kernel version requirement, so it's unlikely to be found outside of
a new distro in the first place.
- Tests that exercise the NVM cache will be skipped if they can't find a
conformant memkind (and libnuma).
- When starting Kudu, if you don't set --block_cache_type=NVM, you shoudn't
notice any change.
- If you do, Kudu will crash at startup if it can't find a conformant
memkind. This affects upgrades: if you were already an NVM cache user but
you didn't have memkind installed, your Kudu will crash post-upgrade.
Note: this doesn't preclude implementing alternative #1 (the one I think is
ideal) in the future; we'll just have to revert the bulk of this patch when
we do so.
To test, I ran cfile-test and cache-test as follows:
- Without memkind installed: DRAM tests passed, NVM tests were skipped
- With an old memkind installed: DRAM tests passed, NVM tests were skipped
- With LD_LIBRARY_PATH=/path/to/memkind-1.9.0: All tests passed
I also manually ran a Kudu master with --block_cache_type=NVM and without
memkind to verify the crashing behavior.
Change-Id: I4f474196aa98b5fa6e5966b9a3aea9a7e466805c
Reviewed-on: http://gerrit.cloudera.org:8080/14620
Tested-by: Kudu Jenkins
Reviewed-by: Alexey Serbin <as...@cloudera.com>
---
M CMakeLists.txt
D cmake_modules/FindMemkind.cmake
D cmake_modules/FindNuma.cmake
M src/kudu/cfile/CMakeLists.txt
M src/kudu/cfile/block_cache.cc
M src/kudu/cfile/cfile-test.cc
M src/kudu/util/CMakeLists.txt
M src/kudu/util/cache-test.cc
M src/kudu/util/nvm_cache.cc
M src/kudu/util/nvm_cache.h
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
D thirdparty/patches/memkind-fix-build-with-old-autoconf.patch
D thirdparty/patches/memkind-fix-build-with-old-glibc.patch
D thirdparty/patches/memkind-fix-jemalloc-build-with-old-autoconf.patch
M thirdparty/vars.sh
17 files changed, 197 insertions(+), 397 deletions(-)
Approvals:
Kudu Jenkins: Verified
Alexey Serbin: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/14620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I4f474196aa98b5fa6e5966b9a3aea9a7e466805c
Gerrit-Change-Number: 14620
Gerrit-PatchSet: 6
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: ye yuqiang <yu...@intel.com>
[kudu-CR] KUDU-2990: remove memkind/libnuma and dynamically link memkind at runtime
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/14620 )
Change subject: KUDU-2990: remove memkind/libnuma and dynamically link memkind at runtime
......................................................................
Patch Set 5:
As a result of this change should we update the install/build guide to install libmemkind or mention it as an option like we do documentation based deps?
--
To view, visit http://gerrit.cloudera.org:8080/14620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4f474196aa98b5fa6e5966b9a3aea9a7e466805c
Gerrit-Change-Number: 14620
Gerrit-PatchSet: 5
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: ye yuqiang <yu...@intel.com>
Gerrit-Comment-Date: Wed, 06 Nov 2019 18:37:12 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2990: remove memkind/libnuma and dynamically link memkind at runtime
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14620 )
Change subject: KUDU-2990: remove memkind/libnuma and dynamically link memkind at runtime
......................................................................
Patch Set 5: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14620/2/src/kudu/cfile/block_cache.cc
File src/kudu/cfile/block_cache.cc:
http://gerrit.cloudera.org:8080/#/c/14620/2/src/kudu/cfile/block_cache.cc@59
PS2, Line 59: otherwise Kudu will crash.
> Yes: if you specify --block_cache_type=NVM and you have an old (or missing)
SGTM. Thank you for the clarification.
--
To view, visit http://gerrit.cloudera.org:8080/14620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4f474196aa98b5fa6e5966b9a3aea9a7e466805c
Gerrit-Change-Number: 14620
Gerrit-PatchSet: 5
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: ye yuqiang <yu...@intel.com>
Gerrit-Comment-Date: Wed, 06 Nov 2019 12:10:28 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2990: remove memkind/libnuma and dynamically link memkind at runtime
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14620 )
Change subject: KUDU-2990: remove memkind/libnuma and dynamically link memkind at runtime
......................................................................
Patch Set 2:
(1 comment)
> > Thanks for doing this, but I'd prefer to link nvm_cache into macOS
> > and rely on the runtime check to gate the feature. That way we
> > don't need the runtime linking functionality AND a bunch of
> > Linux-specific #ifdefs.
>
> I mean we could get rid of libnvm_cache on both Linux and macOS. The patch I posted is supposed to be applied on top of PS1 of this (i.e. your) patch. That way we could get rid of the two things you mentioned and also get rid of an extra library.
>
> What's the reason to have a separate kudu_nvm library? Maybe I'm missing something, but I don't see why not to remove it once the code for both Linux and macOS is unified and uses dlopen/dlsym to find corresponding functions.
Ah, I misunderstood; I thought you meant "removing libnvm_cache _from the macOS build_", not "merge it back into kudu_util".
I can definitely do that, and I agree it's net simpler.
http://gerrit.cloudera.org:8080/#/c/14620/2/src/kudu/cfile/block_cache.cc
File src/kudu/cfile/block_cache.cc:
http://gerrit.cloudera.org:8080/#/c/14620/2/src/kudu/cfile/block_cache.cc@59
PS2, Line 59: otherwise Kudu will crash.
> Is this still true?
Yes: if you specify --block_cache_type=NVM and you have an old (or missing) memkind, Kudu will crash.
The "forgiving" behavior is only there for unit tests, so that we can skip them if there's no memkind.
--
To view, visit http://gerrit.cloudera.org:8080/14620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4f474196aa98b5fa6e5966b9a3aea9a7e466805c
Gerrit-Change-Number: 14620
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: ye yuqiang <yu...@intel.com>
Gerrit-Comment-Date: Tue, 05 Nov 2019 08:33:43 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2990: remove memkind/libnuma and dynamically link memkind at runtime
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14620 )
Change subject: KUDU-2990: remove memkind/libnuma and dynamically link memkind at runtime
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14620/1/src/kudu/util/nvm_cache.cc
File src/kudu/util/nvm_cache.cc:
http://gerrit.cloudera.org:8080/#/c/14620/1/src/kudu/util/nvm_cache.cc@138
PS1, Line 138: RTLD_LAZY
Maybe, use RTLD_NOW instead to detect possible issues with other symbols required (if any)?
--
To view, visit http://gerrit.cloudera.org:8080/14620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4f474196aa98b5fa6e5966b9a3aea9a7e466805c
Gerrit-Change-Number: 14620
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: ye yuqiang <yu...@intel.com>
Gerrit-Comment-Date: Sun, 03 Nov 2019 08:02:28 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2990: remove memkind/libnuma and dynamically link memkind at runtime
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14620 )
Change subject: KUDU-2990: remove memkind/libnuma and dynamically link memkind at runtime
......................................................................
Patch Set 2:
(3 comments)
> I think we can remove libnvm_cache, and that solves the problem of compilation at macOS. I put together a micro-patch for that (apply on top of this patch):
> https://gist.github.com/alexeyserbin/04f182dbc91b34e950dcaf263f28eb7c
Thanks for doing this, but I'd prefer to link nvm_cache into macOS and rely on the runtime check to gate the feature. That way we don't need the runtime linking functionality AND a bunch of Linux-specific #ifdefs.
http://gerrit.cloudera.org:8080/#/c/14620/1/src/kudu/util/CMakeLists.txt
File src/kudu/util/CMakeLists.txt:
PS1:
> I guess this breaks build on macOS, but I haven't figured yet why:
I think it's because nvm_cache needs to depend on kudu_util, which it doesn't currently.
http://gerrit.cloudera.org:8080/#/c/14620/1/src/kudu/util/nvm_cache.cc
File src/kudu/util/nvm_cache.cc:
http://gerrit.cloudera.org:8080/#/c/14620/1/src/kudu/util/nvm_cache.cc@138
PS1, Line 138:
> Maybe, use RTLD_NOW instead to detect possible issues with other symbols re
Sure. FWIW I tested with libnuma missing and the error was surfaced here regardless of whether RTLD_LAZY or RTLD_NOW was used.
But if libnuma is missing symbols, then I imagine RTLD_NOW will tell us.
http://gerrit.cloudera.org:8080/#/c/14620/1/src/kudu/util/nvm_cache.cc@695
PS1, Line 695: le->kv_data =
> nit: shifted too far on the right?
Done
--
To view, visit http://gerrit.cloudera.org:8080/14620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4f474196aa98b5fa6e5966b9a3aea9a7e466805c
Gerrit-Change-Number: 14620
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: ye yuqiang <yu...@intel.com>
Gerrit-Comment-Date: Sun, 03 Nov 2019 21:43:55 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2990: remove memkind/libnuma and dynamically link memkind at runtime
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Andrew Wong, ye yuqiang, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14620
to look at the new patch set (#3).
Change subject: KUDU-2990: remove memkind/libnuma and dynamically link memkind at runtime
......................................................................
KUDU-2990: remove memkind/libnuma and dynamically link memkind at runtime
This patch removes memkind and libnuma from the thirdparty tree and changes
the NVM cache implementation to dynamically link memkind at runtime using
dlopen() and friends. KUDU-2990 explains why we need to do this in great
detail so I won't repeat that here. The alternatives I considered:
1. Patch memkind to dlopen() libnuma.
This is probably the most user-friendly approach because libnuma is found in
most systems (or at least in most package repositories), but a new enough
memkind is not, and having Kudu distribute memkind eases adoption of the NVM
cache. However, I was nervous about performing deep surgery in memkind as
it's a codebase with which I'm not familiar, and I wanted to minimize risk
because we'll be backporting this patch to several older branches.
2. Patch memkind to define weak libnuma symbols.
If done correctly, behavior is unchanged when libnuma is present on the host
system, but if it's not there, calls from memkind to libnuma will crash.
Again, I didn't feel comfortable hacking into memkind, plus I've found weak
symbols difficult to use in the past.
3. Remove the NVM cache from Kudu altogether.
This is obviously the safest and simplest option, but it punishes Kudu users
who actually use the NVM cache.
4. Gate the build of the NVM cache behind a CMake option.
This ought to satisfy the ASF's definition of an "optional" feature, but
does nothing for binary redistributors who wish to offer the NVM cache and
who build Kudu as a statically linked "fat" binary.
5. Build as we do today, but forbid static linkage of libnuma.
Binary redistributors will need to choose between including libnuma in their
distributions, or forcing Kudu to look for libnuma at runtime. The former
still violates ASF policy, and the latter means Kudu won't start on a system
lacking libnuma, regardless of whether the NVM cache is actually in use.
So what are the ramifications of the chosen approach?
- Kudu no longer distributes memkind or libnuma. To use the NVM cache, the
host system must provide both, and memkind must be version 1.6.0 or newer.
CentOS 6.6, CentOS 7.3, and Ubuntu 18.04 repositories all carried memkind
1.1.0, so unless you're on a cutting edge distro you'll have to build
memkind from source.
- Tests that exercise the NVM cache will be skipped if they can't find a
conformant memkind (and libnuma).
- When starting Kudu, if you don't set --block_cache_type=NVM, you shoudn't
notice any change.
- If you do, Kudu will crash at startup if it can't find a conformant
memkind. This affects upgrades: if you were already an NVM cache user but
you didn't have memkind installed, your Kudu will crash post-upgrade.
Note: this doesn't preclude implementing alternative #1 (the one I think is
ideal) in the future; we'll just have to revert the bulk of this patch when
we do so.
To test, I ran cfile-test and cache-test as follows:
- Without memkind installed: DRAM tests passed, NVM tests were skipped
- With an old memkind installed: DRAM tests passed, NVM tests were skipped
- With LD_LIBRARY_PATH=/path/to/memkind-1.9.0: All tests passed
I also manually ran a Kudu master with --block_cache_type=NVM to verify the
crashing behavior.
Change-Id: I4f474196aa98b5fa6e5966b9a3aea9a7e466805c
---
M CMakeLists.txt
D cmake_modules/FindMemkind.cmake
D cmake_modules/FindNuma.cmake
M src/kudu/cfile/CMakeLists.txt
M src/kudu/cfile/block_cache.cc
M src/kudu/cfile/cfile-test.cc
M src/kudu/util/CMakeLists.txt
M src/kudu/util/cache-test.cc
M src/kudu/util/nvm_cache.cc
M src/kudu/util/nvm_cache.h
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
D thirdparty/patches/memkind-fix-build-with-old-autoconf.patch
D thirdparty/patches/memkind-fix-build-with-old-glibc.patch
D thirdparty/patches/memkind-fix-jemalloc-build-with-old-autoconf.patch
M thirdparty/vars.sh
17 files changed, 197 insertions(+), 397 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/14620/3
--
To view, visit http://gerrit.cloudera.org:8080/14620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4f474196aa98b5fa6e5966b9a3aea9a7e466805c
Gerrit-Change-Number: 14620
Gerrit-PatchSet: 3
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: ye yuqiang <yu...@intel.com>
[kudu-CR] KUDU-2990: remove memkind/libnuma and dynamically link memkind at runtime
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/14620 )
Change subject: KUDU-2990: remove memkind/libnuma and dynamically link memkind at runtime
......................................................................
Patch Set 3: Code-Review+2
(2 comments)
just a couple nits. Thanks for the long commit message
http://gerrit.cloudera.org:8080/#/c/14620/3/src/kudu/util/nvm_cache.h
File src/kudu/util/nvm_cache.h:
http://gerrit.cloudera.org:8080/#/c/14620/3/src/kudu/util/nvm_cache.h@27
PS3, Line 27: SKIP_IF_NO_NVM_CACHE
nit: maybe call this RETURN_IF_...?
http://gerrit.cloudera.org:8080/#/c/14620/3/src/kudu/util/nvm_cache.cc
File src/kudu/util/nvm_cache.cc:
http://gerrit.cloudera.org:8080/#/c/14620/3/src/kudu/util/nvm_cache.cc@92
PS3, Line 92: // Taken together, these typedefs and this macro make it easy to call a
nit: maybe move this indirection layer to a separate source file/header?
--
To view, visit http://gerrit.cloudera.org:8080/14620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4f474196aa98b5fa6e5966b9a3aea9a7e466805c
Gerrit-Change-Number: 14620
Gerrit-PatchSet: 3
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: ye yuqiang <yu...@intel.com>
Gerrit-Comment-Date: Tue, 05 Nov 2019 17:34:36 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2990: remove memkind/libnuma and dynamically link memkind at runtime
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14620 )
Change subject: KUDU-2990: remove memkind/libnuma and dynamically link memkind at runtime
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14620/1/src/kudu/util/CMakeLists.txt
File src/kudu/util/CMakeLists.txt:
PS1:
I guess this breaks build on macOS, but I haven't figured yet why:
[ 10%] Linking CXX shared library ../../../lib/libnvm_cache.dylib
Undefined symbols for architecture x86_64:
"fLB::FLAGS_cache_force_single_shard", referenced from:
kudu::(anonymous namespace)::DetermineShardBits() in nvm_cache.cc.o
"kudu::flag_tags_internal::FlagTagger::FlagTagger(char const*, char const*)", referenced from:
___cxx_global_var_init.10 in nvm_cache.cc.o
___cxx_global_var_init.12 in nvm_cache.cc.o
___cxx_global_var_init.17 in nvm_cache.cc.o
"kudu::flag_tags_internal::FlagTagger::~FlagTagger()", referenced from:
___cxx_global_var_init.10 in nvm_cache.cc.o
___cxx_global_var_init.12 in nvm_cache.cc.o
___cxx_global_var_init.17 in nvm_cache.cc.o
"kudu::Cache::~Cache()", referenced from:
kudu::(anonymous namespace)::ShardedLRUCache::ShardedLRUCache(unsigned long, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, memkind*) in nvm_cache.cc.o
kudu::(anonymous namespace)::ShardedLRUCache::~ShardedLRUCache() in nvm_cache.cc.o
"kudu::Status::Status(kudu::Status::Code, kudu::Slice const&, kudu::Slice const&, short)", referenced from:
kudu::Status::NotSupported(kudu::Slice const&, kudu::Slice const&, short) in nvm_cache.cc.o
"kudu::Counter::Increment()", referenced from:
kudu::(anonymous namespace)::NvmLRUCache::FreeEntry(kudu::(anonymous namespace)::LRUHandle*) in nvm_cache.cc.o
kudu::(anonymous namespace)::NvmLRUCache::Lookup(kudu::Slice const&, unsigned int, bool) in nvm_cache.cc.o
kudu::(anonymous namespace)::NvmLRUCache::Insert(kudu::(anonymous namespace)::LRUHandle*, kudu::Cache::EvictionCallback*) in nvm_cache.cc.o
"kudu::Status::ToString() const", referenced from:
kudu::(anonymous namespace)::InitMemkindOps() in nvm_cache.cc.o
"typeinfo for kudu::Cache", referenced from:
typeinfo for kudu::(anonymous namespace)::ShardedLRUCache in nvm_cache.cc.o
"vtable for kudu::Cache", referenced from:
kudu::Cache::Cache() in nvm_cache.cc.o
NOTE: a missing vtable usually means the first non-inline virtual member function has no definition.
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)
make[2]: *** [lib/libnvm_cache.dylib] Error 1
make[1]: *** [src/kudu/util/CMakeFiles/nvm_cache.dir/all] Error 2
make[1]: *** Waiting for unfinished jobs....
--
To view, visit http://gerrit.cloudera.org:8080/14620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4f474196aa98b5fa6e5966b9a3aea9a7e466805c
Gerrit-Change-Number: 14620
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: ye yuqiang <yu...@intel.com>
Gerrit-Comment-Date: Sat, 02 Nov 2019 20:09:22 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2990: remove memkind/libnuma and dynamically link memkind at runtime
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14620 )
Change subject: KUDU-2990: remove memkind/libnuma and dynamically link memkind at runtime
......................................................................
Patch Set 2: Code-Review+2
(2 comments)
http://gerrit.cloudera.org:8080/#/c/14620/2/src/kudu/cfile/block_cache.cc
File src/kudu/cfile/block_cache.cc:
http://gerrit.cloudera.org:8080/#/c/14620/2/src/kudu/cfile/block_cache.cc@59
PS2, Line 59: otherwise Kudu will crash.
Is this still true?
http://gerrit.cloudera.org:8080/#/c/14620/1/src/kudu/util/CMakeLists.txt
File src/kudu/util/CMakeLists.txt:
PS1:
> I think it's because nvm_cache needs to depend on kudu_util, which it doesn
Yep, that's was the reason as I understand -- this code became dependent on the bunch of symbols defined in cache.cc
--
To view, visit http://gerrit.cloudera.org:8080/14620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4f474196aa98b5fa6e5966b9a3aea9a7e466805c
Gerrit-Change-Number: 14620
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: ye yuqiang <yu...@intel.com>
Gerrit-Comment-Date: Mon, 04 Nov 2019 17:40:18 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2990: remove memkind/libnuma and dynamically link memkind at runtime
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14620 )
Change subject: KUDU-2990: remove memkind/libnuma and dynamically link memkind at runtime
......................................................................
Patch Set 6:
> As a result of this change should we update the install/build guide to install libmemkind or mention it as an option like we do documentation based deps?
Yeah I'll prepare a follow-up patch for that.
--
To view, visit http://gerrit.cloudera.org:8080/14620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4f474196aa98b5fa6e5966b9a3aea9a7e466805c
Gerrit-Change-Number: 14620
Gerrit-PatchSet: 6
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: ye yuqiang <yu...@intel.com>
Gerrit-Comment-Date: Thu, 07 Nov 2019 00:41:23 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2990: remove memkind/libnuma and dynamically link memkind at runtime
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Andrew Wong, ye yuqiang, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14620
to look at the new patch set (#2).
Change subject: KUDU-2990: remove memkind/libnuma and dynamically link memkind at runtime
......................................................................
KUDU-2990: remove memkind/libnuma and dynamically link memkind at runtime
This patch removes memkind and libnuma from the thirdparty tree and changes
the NVM cache implementation to dynamically link memkind at runtime using
dlopen() and friends. KUDU-2990 explains why we need to do this in great
detail so I won't repeat that here. The alternatives I considered:
1. Patch memkind to dlopen() libnuma.
This is probably the most user-friendly approach because libnuma is found in
most systems (or at least in most package repositories), but a new enough
memkind is not, and having Kudu distribute memkind eases adoption of the NVM
cache. However, I was nervous about performing deep surgery in memkind as
it's a codebase with which I'm not familiar, and I wanted to minimize risk
because we'll be backporting this patch to several older branches.
2. Patch memkind to define weak libnuma symbols.
If done correctly, behavior is unchanged when libnuma is present on the host
system, but if it's not there, calls from memkind to libnuma will crash.
Again, I didn't feel comfortable hacking into memkind, plus I've found weak
symbols difficult to use in the past.
3. Remove the NVM cache from Kudu altogether.
This is obviously the safest and simplest option, but it punishes Kudu users
who actually use the NVM cache.
4. Gate the build of the NVM cache behind a CMake option.
This ought to satisfy the ASF's definition of an "optional" feature, but
does nothing for binary redistributors who wish to offer the NVM cache and
who build Kudu as a statically linked "fat" binary.
5. Build as we do today, but forbid static linkage of libnuma.
Binary redistributors will need to choose between including libnuma in their
distributions, or forcing Kudu to look for libnuma at runtime. The former
still violates ASF policy, and the latter means Kudu won't start on a system
lacking libnuma, regardless of whether the NVM cache is actually in use.
So what are the ramifications of the chosen approach?
- Kudu no longer distributes memkind or libnuma. To use the NVM cache, the
host system must provide both, and memkind must be version 1.6.0 or newer.
CentOS 6.6, CentOS 7.3, and Ubuntu 18.04 repositories all carried memkind
1.1.0, so unless you're on a cutting edge distro you'll have to build
memkind from source.
- Tests that exercise the NVM cache will be skipped if they can't find a
conformant memkind (and libnuma).
- When starting Kudu, if you don't set --block_cache_type=NVM, you shoudn't
notice any change.
- If you do, Kudu will crash at startup if it can't find a conformant
memkind. This affects upgrades: if you were already an NVM cache user but
you didn't have memkind installed, your Kudu will crash post-upgrade.
Note: this doesn't preclude implementing alternative #1 (the one I think is
ideal) in the future; we'll just have to revert the bulk of this patch when
we do so.
To test, I ran cfile-test and cache-test as follows:
- Without memkind installed: DRAM tests passed, NVM tests were skipped
- With an old memkind installed: DRAM tests passed, NVM tests were skipped
- With LD_LIBRARY_PATH=/path/to/memkind-1.9.0: All tests passed
I also manually ran a Kudu master with --block_cache_type=NVM to verify the
crashing behavior.
Change-Id: I4f474196aa98b5fa6e5966b9a3aea9a7e466805c
---
M CMakeLists.txt
D cmake_modules/FindMemkind.cmake
D cmake_modules/FindNuma.cmake
M src/kudu/cfile/CMakeLists.txt
M src/kudu/cfile/block_cache.cc
M src/kudu/cfile/cfile-test.cc
M src/kudu/util/CMakeLists.txt
M src/kudu/util/cache-test.cc
M src/kudu/util/nvm_cache.cc
M src/kudu/util/nvm_cache.h
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
D thirdparty/patches/memkind-fix-build-with-old-autoconf.patch
D thirdparty/patches/memkind-fix-build-with-old-glibc.patch
D thirdparty/patches/memkind-fix-jemalloc-build-with-old-autoconf.patch
M thirdparty/vars.sh
17 files changed, 215 insertions(+), 391 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/14620/2
--
To view, visit http://gerrit.cloudera.org:8080/14620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4f474196aa98b5fa6e5966b9a3aea9a7e466805c
Gerrit-Change-Number: 14620
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: ye yuqiang <yu...@intel.com>
[kudu-CR] KUDU-2990: remove memkind/libnuma and dynamically link memkind at runtime
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Andrew Wong, ye yuqiang, Grant Henke, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14620
to look at the new patch set (#4).
Change subject: KUDU-2990: remove memkind/libnuma and dynamically link memkind at runtime
......................................................................
KUDU-2990: remove memkind/libnuma and dynamically link memkind at runtime
This patch removes memkind and libnuma from the thirdparty tree and changes
the NVM cache implementation to dynamically link memkind at runtime using
dlopen() and friends. KUDU-2990 explains why we need to do this in great
detail so I won't repeat that here. The alternatives I considered:
1. Patch memkind to dlopen() libnuma.
This is probably the most user-friendly approach because libnuma is found in
most systems (or at least in most package repositories), but a new enough
memkind is not, and having Kudu distribute memkind eases adoption of the NVM
cache. However, I was nervous about performing deep surgery in memkind as
it's a codebase with which I'm not familiar, and I wanted to minimize risk
because we'll be backporting this patch to several older branches.
2. Patch memkind to define weak libnuma symbols.
If done correctly, behavior is unchanged when libnuma is present on the host
system, but if it's not there, calls from memkind to libnuma will crash.
Again, I didn't feel comfortable hacking into memkind, plus I've found weak
symbols difficult to use in the past.
3. Remove the NVM cache from Kudu altogether.
This is obviously the safest and simplest option, but it punishes Kudu users
who actually use the NVM cache.
4. Gate the build of the NVM cache behind a CMake option.
This ought to satisfy the ASF's definition of an "optional" feature, but
does nothing for binary redistributors who wish to offer the NVM cache and
who build Kudu as a statically linked "fat" binary.
5. Build as we do today, but forbid static linkage of libnuma.
Binary redistributors will need to choose between including libnuma in their
distributions, or forcing Kudu to look for libnuma at runtime. The former
still violates ASF policy, and the latter means Kudu won't start on a system
lacking libnuma, regardless of whether the NVM cache is actually in use.
So what are the ramifications of the chosen approach?
- Kudu no longer distributes memkind or libnuma. To use the NVM cache, the
host system must provide both, and memkind must be version 1.6.0 or newer.
CentOS 6 and Ubuntu 18.04 repositories all carried memkind 1.1.0. CentOS 7
has memkind 1.7.0. Persistent memory hardware itself also has a pretty
steep kernel version requirement, so it's unlikely to be found outside of
a new distro in the first place.
- Tests that exercise the NVM cache will be skipped if they can't find a
conformant memkind (and libnuma).
- When starting Kudu, if you don't set --block_cache_type=NVM, you shoudn't
notice any change.
- If you do, Kudu will crash at startup if it can't find a conformant
memkind. This affects upgrades: if you were already an NVM cache user but
you didn't have memkind installed, your Kudu will crash post-upgrade.
Note: this doesn't preclude implementing alternative #1 (the one I think is
ideal) in the future; we'll just have to revert the bulk of this patch when
we do so.
To test, I ran cfile-test and cache-test as follows:
- Without memkind installed: DRAM tests passed, NVM tests were skipped
- With an old memkind installed: DRAM tests passed, NVM tests were skipped
- With LD_LIBRARY_PATH=/path/to/memkind-1.9.0: All tests passed
I also manually ran a Kudu master with --block_cache_type=NVM and without
memkind to verify the crashing behavior.
Change-Id: I4f474196aa98b5fa6e5966b9a3aea9a7e466805c
---
M CMakeLists.txt
D cmake_modules/FindMemkind.cmake
D cmake_modules/FindNuma.cmake
M src/kudu/cfile/CMakeLists.txt
M src/kudu/cfile/block_cache.cc
M src/kudu/cfile/cfile-test.cc
M src/kudu/util/CMakeLists.txt
M src/kudu/util/cache-test.cc
M src/kudu/util/nvm_cache.cc
M src/kudu/util/nvm_cache.h
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
D thirdparty/patches/memkind-fix-build-with-old-autoconf.patch
D thirdparty/patches/memkind-fix-build-with-old-glibc.patch
D thirdparty/patches/memkind-fix-jemalloc-build-with-old-autoconf.patch
M thirdparty/vars.sh
17 files changed, 197 insertions(+), 397 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/14620/4
--
To view, visit http://gerrit.cloudera.org:8080/14620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4f474196aa98b5fa6e5966b9a3aea9a7e466805c
Gerrit-Change-Number: 14620
Gerrit-PatchSet: 4
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: ye yuqiang <yu...@intel.com>