Skip to content

Commit

Permalink
KUDU-2990: remove memkind/libnuma and dynamically link memkind at run…
Browse files Browse the repository at this point in the history
…time

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 <[email protected]>
  • Loading branch information
adembo committed Nov 7, 2019
1 parent 34efee1 commit ba908ef
Show file tree
Hide file tree
Showing 17 changed files with 197 additions and 397 deletions.
24 changes: 0 additions & 24 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -1139,30 +1139,6 @@ if (NOT "${KUDU_USE_ASAN}" AND
set(KUDU_TCMALLOC_AVAILABLE 1)
endif()

# Required memkind libraries
if (NOT APPLE)
find_package(Memkind REQUIRED)
include_directories(${MEMKIND_INCLUDE_DIR})
ADD_THIRDPARTY_LIB(memkind
STATIC_LIB "${MEMKIND_STATIC_LIB}"
SHARED_LIB "${MEMKIND_SHARED_LIB}"
DEPS numa)
endif()

if (EXISTS ${MEMKIND_SHARED_LIB})
set(HAVE_LIB_MEMKIND 1)
add_definitions(-DHAVE_LIB_MEMKIND=1)
endif()

## libnuma
if (NOT APPLE)
find_package(Numa REQUIRED)
include_directories(${NUMA_INCLUDE_DIR})
ADD_THIRDPARTY_LIB(numa
STATIC_LIB "${NUMA_STATIC_LIB}"
SHARED_LIB "${NUMA_SHARED_LIB}")
endif()

## curl
find_package(CURL REQUIRED)

Expand Down
38 changes: 0 additions & 38 deletions cmake_modules/FindMemkind.cmake

This file was deleted.

38 changes: 0 additions & 38 deletions cmake_modules/FindNuma.cmake

This file was deleted.

6 changes: 0 additions & 6 deletions src/kudu/cfile/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -55,12 +55,6 @@ set(CFILE_LIBS
cfile_proto
bitshuffle)

if(HAVE_LIB_MEMKIND)
set(CFILE_LIBS
${CFILE_LIBS}
nvm_cache)
endif()

target_link_libraries(cfile ${CFILE_LIBS})

# Tests
Expand Down
8 changes: 3 additions & 5 deletions src/kudu/cfile/block_cache.cc
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,9 @@ DEFINE_string(block_cache_type, "DRAM",
"Which type of block cache to use for caching data. "
"Valid choices are 'DRAM' or 'NVM'. DRAM, the default, "
"caches data in regular memory. 'NVM' caches data "
"in a memory-mapped file using the memkind library.");
"in a memory-mapped file using the memkind library. To use 'NVM', "
"libmemkind 1.6.0 or newer must be available on the system; "
"otherwise Kudu will crash.");

using strings::Substitute;

Expand All @@ -75,12 +77,8 @@ Cache* CreateCache(int64_t capacity) {
return NewCache<Cache::EvictionPolicy::LRU, Cache::MemoryType::DRAM>(
capacity, "block_cache");
case Cache::MemoryType::NVM:
#if defined(HAVE_LIB_MEMKIND)
return NewCache<Cache::EvictionPolicy::LRU, Cache::MemoryType::NVM>(
capacity, "block_cache");
#else
LOG(FATAL) << "cache of NVM memory type is not supported";
#endif // #if defined(HAVE_LIB_MEMKIND) ... #else ...
default:
LOG(FATAL) << "unsupported LRU cache memory type: " << mem_type;
return nullptr;
Expand Down
Loading

0 comments on commit ba908ef

Please sign in to comment.