From 399364be07d5896cf36f1ba34a08dfd2fae6d3b8 Mon Sep 17 00:00:00 2001 From: Yao Yue Date: Mon, 13 Feb 2017 11:57:18 -0800 Subject: [PATCH] buf_sock memory leak fixed, merged ccommon change introducing sockio metrics (#128) * Squashed 'deps/ccommon/' changes from bb298bc..ab0edc8 ab0edc8 add metrics to track buf_sock objects (#138) ae02038 add travis ci (copied from pelikan) (#139) 964645a Merge pull request #135 from paegun/fix_cmake_install 70710c2 fixed and re-added cmake install instructions, w/ following notes: include directory made proper relative; opened pattern match b/c include directory should only contain files meant for inclusion. 5b095bc Merge pull request #126 from kevyang/kevyang/120 426d56a return NULL when cc_alloc/cc_realloc is called with size == 0 ad271d4 Merge pull request #133 from kevyang/132 47dbdba suppress unused parameter warning in debug_log_flush 648d19e Merge pull request #127 from kevyang/56 780941a Merge pull request #130 from kevyang/129 b8af6c0 Merge pull request #131 from kevyang/128 6ecc318 fix duplicate symbols in cc_signal 080c41d cc_array - stop doing arithmetic on void * d526f7a add debug oriented memory management a4fb927 Update bool member rules in style guide 05c6e1e explicitly make ccommon a C project to avoid checking for CXX related variables git-subtree-dir: deps/ccommon git-subtree-split: ab0edc889ca64009651c5e324e500b0e30171fc1 * sockio metrics help solve the memory leak --- README.cmake | 2 +- README.md | 2 +- deps/ccommon/.travis.yml | 95 +++++++++++++++++++++++++ deps/ccommon/CMakeLists.txt | 4 +- deps/ccommon/include/stream/cc_sockio.h | 18 ++++- deps/ccommon/src/CMakeLists.txt | 7 ++ deps/ccommon/src/stream/cc_sockio.c | 25 ++++++- src/core/data/server.c | 1 + src/server/pingserver/main.c | 2 +- src/server/pingserver/stats.c | 1 + src/server/pingserver/stats.h | 2 + src/server/slimcache/main.c | 2 +- src/server/slimcache/stats.c | 1 + src/server/slimcache/stats.h | 2 + src/server/twemcache/main.c | 2 +- src/server/twemcache/stats.c | 1 + src/server/twemcache/stats.h | 2 + 17 files changed, 160 insertions(+), 9 deletions(-) create mode 100644 deps/ccommon/.travis.yml diff --git a/README.cmake b/README.cmake index 8f4065068..2d0ea8105 100644 --- a/README.cmake +++ b/README.cmake @@ -18,4 +18,4 @@ cmake .. make -j make test -# executables can be found at $(topdir)/_bin/* +# executables can be found at $(builddir)/_bin/* diff --git a/README.md b/README.md index 51fa74435..a4d905e49 100644 --- a/README.md +++ b/README.md @@ -55,7 +55,7 @@ mkdir _build && cd _build cmake .. make -j ``` -The executables can be found under ``./_bin/`` +The executables can be found under ``_bin/`` (under build directory) To run all the tests, including those on `ccommon`, run: ```sh diff --git a/deps/ccommon/.travis.yml b/deps/ccommon/.travis.yml new file mode 100644 index 000000000..47c18dabf --- /dev/null +++ b/deps/ccommon/.travis.yml @@ -0,0 +1,95 @@ +sudo: required +language: c + +# using anchor to import sources into linux builds +addons: + apt: &apt + sources: + - ubuntu-toolchain-r-test + - llvm-toolchain-precise-3.6 + - llvm-toolchain-precise-3.7 + - llvm-toolchain-precise + +# travis currently does not support directly setting gcc/clang with versions +# (e.g. gcc-4.8) as value for the compiler key. So we will have to manually +# request these packages and use environment varibles to create the matrix. +# +# In the case of osx, use brew to install the paritcular versions, instead of +# specifying with packages. +matrix: + include: + # gcc 4.8 on linux + - env: C_COMPILER=gcc-4.8 + addons: + apt: + <<: *apt + packages: gcc-4.8 + + # gcc 4.9 on linux + - env: C_COMPILER=gcc-4.9 + addons: + apt: + <<: *apt + packages: gcc-4.9 + + # gcc 5 on linux + - env: C_COMPILER=gcc-5 + addons: + apt: + <<: *apt + packages: gcc-5 + + # clang 3.6 on linux + - env: C_COMPILER=clang-3.6 + addons: + apt: + <<: *apt + packages: clang-3.6 + + # clang 3.7 on linux + - env: C_COMPILER=clang-3.7 + addons: + apt: + <<: *apt + packages: clang-3.7 + + ## gcc 4.8 on osx + #- os: osx + # env: FORMULA=gcc48 COMPILER=gcc C_COMPILER=gcc-4.8 + # + ## gcc 4.9 on osx + #- os: osx + # env: FORMULA=gcc49 COMPILER=gcc C_COMPILER=gcc-4.9 + # + ## gcc 5 on osx + #- os: osx + # env: FORMULA=gcc5 COMPILER=gcc C_COMPILER=gcc-5 + + # clang 3.6 on osx + - os: osx + osx_image: xcode6.4 + env: C_COMPILER=clang + + # clang 3.7 on osx + - os: osx + osx_image: xcode7.1 + env: C_COMPILER=clang + + +before_install: + # for osx: 0. update brew; 1. install cmake if missing; 2. (gcc) unlink pre-installed gcc; 3. (gcc) install desired version of gcc + - 'if [[ "$TRAVIS_OS_NAME" == "osx" ]]; then brew update; fi' + - 'if [[ "$TRAVIS_OS_NAME" == "osx" && -z "$(which cmake)" ]]; then brew install cmake; fi' # xcode 7.1 is missing cmake + - 'if [[ "$TRAVIS_OS_NAME" == "osx" && "$COMPILER" == "gcc" ]]; then brew unlink gcc || true; fi' # ignore unlink errors + - 'if [[ "$TRAVIS_OS_NAME" == "osx" && "$COMPILER" == "gcc" ]]; then brew unlink $FORMULA || true; fi' # ignore unlink errors + - 'if [[ "$TRAVIS_OS_NAME" == "osx" && "$COMPILER" == "gcc" ]]; then brew install $FORMULA; fi' + - export CC=$C_COMPILER + - wget https://github.com/libcheck/check/releases/download/0.11.0/check-0.11.0.tar.gz + - tar xvfz check-0.11.0.tar.gz + - cd check-0.11.0 && ./configure && make && sudo make install && cd .. + +script: + - mkdir _build && cd _build + - cmake .. + - make -j + - make check diff --git a/deps/ccommon/CMakeLists.txt b/deps/ccommon/CMakeLists.txt index 8df6f9527..f378dec25 100644 --- a/deps/ccommon/CMakeLists.txt +++ b/deps/ccommon/CMakeLists.txt @@ -37,8 +37,8 @@ endif() # version info set(${PROJECT_NAME}_VERSION_MAJOR 1) -set(${PROJECT_NAME}_VERSION_MINOR 0) -set(${PROJECT_NAME}_VERSION_PATCH 2) +set(${PROJECT_NAME}_VERSION_MINOR 1) +set(${PROJECT_NAME}_VERSION_PATCH 0) set(${PROJECT_NAME}_VERSION ${${PROJECT_NAME}_VERSION_MAJOR}.${${PROJECT_NAME}_VERSION_MINOR}.${${PROJECT_NAME}_VERSION_PATCH} ) diff --git a/deps/ccommon/include/stream/cc_sockio.h b/deps/ccommon/include/stream/cc_sockio.h index 89fe60471..9d2f9f570 100644 --- a/deps/ccommon/include/stream/cc_sockio.h +++ b/deps/ccommon/include/stream/cc_sockio.h @@ -48,6 +48,7 @@ extern "C" { #include #include +#include #include #include @@ -62,6 +63,21 @@ typedef struct { SOCKIO_OPTION(OPTION_DECLARE) } sockio_options_st; +/* name type description */ +#define SOCKIO_METRIC(ACTION) \ + ACTION( buf_sock_create, METRIC_COUNTER, "# buf sock created" )\ + ACTION( buf_sock_create_ex, METRIC_COUNTER, "# buf sock create exceptions" )\ + ACTION( buf_sock_destroy, METRIC_COUNTER, "# buf sock destroyed" )\ + ACTION( buf_sock_curr, METRIC_GAUGE, "# buf sock allocated" )\ + ACTION( buf_sock_borrow, METRIC_COUNTER, "# buf sock borrowed" )\ + ACTION( buf_sock_borrow_ex, METRIC_COUNTER, "# buf sock borrow exceptions" )\ + ACTION( buf_sock_return, METRIC_COUNTER, "# buf sock returned" )\ + ACTION( buf_sock_active, METRIC_GAUGE, "# buf sock being borrowed" ) + +typedef struct { + SOCKIO_METRIC(METRIC_DECLARE) +} sockio_metrics_st; + struct buf_sock { /* these fields are useful for resource managmenet */ STAILQ_ENTRY(buf_sock) next; @@ -79,7 +95,7 @@ struct buf_sock { STAILQ_HEAD(buf_sock_sqh, buf_sock); /* corresponding header type for the STAILQ */ -void sockio_setup(sockio_options_st *options); +void sockio_setup(sockio_options_st *options, sockio_metrics_st *metrics); void sockio_teardown(void); struct buf_sock *buf_sock_create(void); /* stream_get_fn */ diff --git a/deps/ccommon/src/CMakeLists.txt b/deps/ccommon/src/CMakeLists.txt index b96b0b532..43828ce17 100644 --- a/deps/ccommon/src/CMakeLists.txt +++ b/deps/ccommon/src/CMakeLists.txt @@ -35,3 +35,10 @@ set_target_properties (${PROJECT_NAME}-shared OUTPUT_NAME ${PROJECT_NAME} VERSION ${${PROJECT_NAME}_VERSION} SOVERSION 0) + +# install instructions +install(TARGETS ${PROJECT_NAME}-static DESTINATION lib) +install(TARGETS ${PROJECT_NAME}-shared DESTINATION lib) +install(DIRECTORY ../include/ + DESTINATION include/${PROJECT_NAME}-${${PROJECT_NAME}_RELEASE_VERSION} + FILES_MATCHING PATTERN "*") diff --git a/deps/ccommon/src/stream/cc_sockio.c b/deps/ccommon/src/stream/cc_sockio.c index 906fc463e..09c7af9a6 100644 --- a/deps/ccommon/src/stream/cc_sockio.c +++ b/deps/ccommon/src/stream/cc_sockio.c @@ -42,7 +42,9 @@ FREEPOOL(buf_sock_pool, buf_sockq, buf_sock); struct buf_sock_pool bsp; +static bool sockio_init = false; static bool bsp_init = false; +static sockio_metrics_st *sockio_metrics = NULL; rstatus_i buf_tcp_read(struct buf_sock *s) @@ -212,6 +214,7 @@ buf_sock_create(void) s = (struct buf_sock *)cc_alloc(sizeof(struct buf_sock)); if (s == NULL) { + INCR(sockio_metrics, buf_sock_create_ex); return NULL; } STAILQ_NEXT(s, next) = NULL; @@ -235,12 +238,16 @@ buf_sock_create(void) goto error; } + INCR(sockio_metrics, buf_sock_create); + INCR(sockio_metrics, buf_sock_curr); + log_verb("created buffered socket %p", s); return s; error: log_info("buffered socket creation failed"); + INCR(sockio_metrics, buf_sock_create_ex); buf_sock_destroy(&s); return NULL; @@ -261,6 +268,8 @@ buf_sock_destroy(struct buf_sock **s) cc_free(*s); *s = NULL; + INCR(sockio_metrics, buf_sock_destroy); + DECR(sockio_metrics, buf_sock_curr); } static void @@ -331,10 +340,13 @@ buf_sock_borrow(void) FREEPOOL_BORROW(s, &bsp, next, buf_sock_create); if (s == NULL) { log_debug("borrow buffered socket failed: OOM or over limit"); + INCR(sockio_metrics, buf_sock_borrow_ex); return NULL; } buf_sock_reset(s); + INCR(sockio_metrics, buf_sock_borrow); + INCR(sockio_metrics, buf_sock_active); log_verb("borrowed buffered socket %p", s); @@ -354,18 +366,29 @@ buf_sock_return(struct buf_sock **s) FREEPOOL_RETURN(*s, &bsp, next); *s = NULL; + INCR(sockio_metrics, buf_sock_return); + DECR(sockio_metrics, buf_sock_active); } void -sockio_setup(sockio_options_st *options) +sockio_setup(sockio_options_st *options, sockio_metrics_st *metrics) { uint32_t max = BUFSOCK_POOLSIZE; + log_info("set up the %s module", SOCKIO_MODULE_NAME); + + if (sockio_init) { + log_warn("%s has already been setup, overwrite", SOCKIO_MODULE_NAME); + } + + sockio_metrics = metrics; + if (options != NULL) { max = option_uint(&options->buf_sock_poolsize); } buf_sock_pool_create(max); + sockio_init = true; } void diff --git a/src/core/data/server.c b/src/core/data/server.c index 6a47b44bf..990d278ff 100644 --- a/src/core/data/server.c +++ b/src/core/data/server.c @@ -78,6 +78,7 @@ _tcp_accept(struct buf_sock *ss) } if (!ss->hdl->accept(sc, s->ch)) { + buf_sock_return(&s); return false; } diff --git a/src/server/pingserver/main.c b/src/server/pingserver/main.c index 01dca1fb5..46b8b4208 100644 --- a/src/server/pingserver/main.c +++ b/src/server/pingserver/main.c @@ -103,7 +103,7 @@ setup(void) buf_setup(&setting.buf, &stats.buf); dbuf_setup(&setting.dbuf, &stats.dbuf); event_setup(&stats.event); - sockio_setup(&setting.sockio); + sockio_setup(&setting.sockio, &stats.sockio); tcp_setup(&setting.tcp, &stats.tcp); timing_wheel_setup(&stats.timing_wheel); diff --git a/src/server/pingserver/stats.c b/src/server/pingserver/stats.c index 8954fd856..0262e805e 100644 --- a/src/server/pingserver/stats.c +++ b/src/server/pingserver/stats.c @@ -11,6 +11,7 @@ struct stats stats = { { DBUF_METRIC(METRIC_INIT) }, { EVENT_METRIC(METRIC_INIT) }, { LOG_METRIC(METRIC_INIT) }, + { SOCKIO_METRIC(METRIC_INIT) }, { TCP_METRIC(METRIC_INIT) }, { TIMING_WHEEL_METRIC(METRIC_INIT) }, }; diff --git a/src/server/pingserver/stats.h b/src/server/pingserver/stats.h index 358d60023..5073bf1c7 100644 --- a/src/server/pingserver/stats.h +++ b/src/server/pingserver/stats.h @@ -11,6 +11,7 @@ #include #include #include +#include #include