From 18830153c242b6532a98c0940ab21fe1e86e542a Mon Sep 17 00:00:00 2001 From: Yao Yue Date: Sat, 11 Feb 2017 19:29:02 -0800 Subject: [PATCH 1/2] 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 --- .travis.yml | 95 ++++++++++++++++++++++++++++++++++++++ CMakeLists.txt | 7 +-- config.h.in | 2 + docs/c-styleguide.txt | 6 ++- include/cc_array.h | 4 +- include/cc_define.h | 7 ++- include/cc_mm.h | 8 ++++ include/cc_signal.h | 2 +- include/stream/cc_sockio.h | 18 +++++++- src/CMakeLists.txt | 7 +++ src/cc_debug.c | 6 +++ src/cc_mm.c | 46 +++++++++++++++--- src/cc_signal.c | 2 + src/stream/cc_sockio.c | 25 +++++++++- 14 files changed, 215 insertions(+), 20 deletions(-) create mode 100644 .travis.yml diff --git a/.travis.yml b/.travis.yml new file mode 100644 index 000000000..47c18dabf --- /dev/null +++ b/.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/CMakeLists.txt b/CMakeLists.txt index 93067d859..f378dec25 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -1,5 +1,5 @@ cmake_minimum_required(VERSION 2.6) -project(ccommon) +project(ccommon C) enable_testing() @@ -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} ) @@ -51,6 +51,7 @@ option(HAVE_ASSERT_LOG "assert_log enabled by default" ON) option(HAVE_ASSERT_PANIC "assert_panic disabled by default" OFF) option(HAVE_LOGGING "logging enabled by default" ON) option(HAVE_STATS "stats enabled by default" ON) +option(HAVE_DEBUG_MM "debugging oriented memory management disabled by default" OFF) option(COVERAGE "code coverage" OFF) include(CheckIncludeFiles) diff --git a/config.h.in b/config.h.in index e5fa50e30..f1dc18bd9 100644 --- a/config.h.in +++ b/config.h.in @@ -17,3 +17,5 @@ #cmakedefine HAVE_LOGGING #cmakedefine HAVE_STATS + +#cmakedefine HAVE_DEBUG_MM diff --git a/docs/c-styleguide.txt b/docs/c-styleguide.txt index 30aa62f20..69661c0d8 100644 --- a/docs/c-styleguide.txt +++ b/docs/c-styleguide.txt @@ -16,11 +16,13 @@ . However, when interfacing with system calls and libraries you cannot get away from using int and char. - Use bool for boolean variables. You have to include -- Avoid using a bool as type for struct member names. Instead use unsigned - 1-bit bit field. Eg: +- If memory usage or alignment is a concern, avoid using a bool as type for + struct member names. Instead use unsigned 1-bit bit field. e.g. struct foo { unsigned is_bar:1; }; + However, if neither memory usage or alignment will be significantly impacted + by the struct, opt for using bool for the sake of readability. - Always use size_t type when dealing with sizes of objects or memory ranges. - Your code should be 64-bit and 32-bit friendly. Bear in mind problems of printing, comparisons, and structure alignment. You have to include diff --git a/include/cc_array.h b/include/cc_array.h index 4e9dcdbe7..b4aa1feb8 100644 --- a/include/cc_array.h +++ b/include/cc_array.h @@ -46,7 +46,7 @@ struct array { uint32_t nalloc; /* # allocated element */ size_t size; /* element size */ uint32_t nelem; /* # element */ - void *data; /* elements */ + uint8_t *data; /* elements */ }; @@ -93,7 +93,7 @@ array_data_assign(struct array *arr, uint32_t nalloc, size_t size, void *data) * element is out of bounds, return -1. */ static inline int -array_locate(struct array *arr, void *elem) { +array_locate(struct array *arr, uint8_t *elem) { int idx; idx = (elem - arr->data) / arr->size; diff --git a/include/cc_define.h b/include/cc_define.h index b542d0201..b521b133e 100644 --- a/include/cc_define.h +++ b/include/cc_define.h @@ -54,10 +54,9 @@ extern "C" { # define CC_BACKTRACE 1 #endif -/* TODO: add compile time option to turn chaining on/off */ -/*#ifdef HAVE_CHAINED*/ -# define CC_HAVE_CHAINED 1 -/*#endif*/ +#ifdef HAVE_DEBUG_MM +#define CC_DEBUG_MM 1 +#endif #define CC_OK 0 #define CC_ERROR -1 diff --git a/include/cc_mm.h b/include/cc_mm.h index a1cc574b3..9705bd840 100644 --- a/include/cc_mm.h +++ b/include/cc_mm.h @@ -17,6 +17,8 @@ #pragma once +#include + #include /* @@ -41,8 +43,13 @@ #define cc_calloc(_n, _s) \ _cc_calloc((size_t)(_n), (size_t)(_s), __FILE__, __LINE__) +#if defined CC_DEBUG_MM && CC_DEBUG_MM == 1 +#define cc_realloc(_p, _s) \ + _cc_realloc_move(_p, (size_t)(_s), __FILE__, __LINE__) +#else #define cc_realloc(_p, _s) \ _cc_realloc(_p, (size_t)(_s), __FILE__, __LINE__) +#endif #define cc_free(_p) do { \ _cc_free(_p, __FILE__, __LINE__); \ @@ -59,6 +66,7 @@ void * _cc_alloc(size_t size, const char *name, int line); void * _cc_zalloc(size_t size, const char *name, int line); void * _cc_calloc(size_t nmemb, size_t size, const char *name, int line); void * _cc_realloc(void *ptr, size_t size, const char *name, int line); +void * _cc_realloc_move(void *ptr, size_t size, const char *name, int line); void _cc_free(void *ptr, const char *name, int line); void * _cc_mmap(size_t size, const char *name, int line); int _cc_munmap(void *p, size_t size, const char *name, int line); diff --git a/include/cc_signal.h b/include/cc_signal.h index 61e151d9a..1fe67c8ff 100644 --- a/include/cc_signal.h +++ b/include/cc_signal.h @@ -48,7 +48,7 @@ struct signal { * - SIGSEGV(debug): print stacktrace before reraise segfault again * - SIGPIPE(channel): ignored, this prevents service from exiting when pipe closes */ -struct signal signals[SIGNAL_MAX]; /* there are only 31 signals from 1 to 31 */ +extern struct signal signals[SIGNAL_MAX]; /* there are only 31 signals from 1 to 31 */ int signal_override(int signo, char *info, int flags, uint32_t mask, sig_fn handler); diff --git a/include/stream/cc_sockio.h b/include/stream/cc_sockio.h index 89fe60471..9d2f9f570 100644 --- a/include/stream/cc_sockio.h +++ b/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/src/CMakeLists.txt b/src/CMakeLists.txt index b96b0b532..43828ce17 100644 --- a/src/CMakeLists.txt +++ b/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/src/cc_debug.c b/src/cc_debug.c index 7aa97dfe2..7c766260b 100644 --- a/src/cc_debug.c +++ b/src/cc_debug.c @@ -105,6 +105,12 @@ _logrotate(int signo) void debug_log_flush(void *arg) { + /* + * arg is unused but necessary for debug_log_flush to be used in conjunction + * with cc_timer and cc_wheel facilities, since to be inserted into a timing + * wheel the function must have the type signature of timeout_cb_fn. + */ + (void)arg; log_flush(dlog->logger); } diff --git a/src/cc_mm.c b/src/cc_mm.c index 4668d8d8e..6a0554289 100644 --- a/src/cc_mm.c +++ b/src/cc_mm.c @@ -34,7 +34,10 @@ _cc_alloc(size_t size, const char *name, int line) { void *p; - ASSERT(size != 0); + if (size == 0) { + log_debug("malloc(0) @ %s:%d", name, line); + return NULL; + } p = malloc(size); if (p == NULL) { @@ -70,7 +73,11 @@ _cc_realloc(void *ptr, size_t size, const char *name, int line) { void *p; - ASSERT(size != 0); + if (size == 0) { + free(ptr); + log_debug("realloc(0) @ %s:%d", name, line); + return NULL; + } p = realloc(ptr, size); if (p == NULL) { @@ -82,10 +89,37 @@ _cc_realloc(void *ptr, size_t size, const char *name, int line) return p; } +void * +_cc_realloc_move(void *ptr, size_t size, const char *name, int line) +{ + void *p = NULL, *pr; + + if (size == 0) { + free(ptr); + log_debug("realloc(0) @ %s:%d", name, line); + return NULL; + } + + /* + * Calling realloc then malloc allows us to force this function call to + * change the address of the allocated memory block. realloc ensures we can + * copy size bytes, and calling malloc before the realloc'd data is free'd + * gives us a new address for the memory object. + */ + if (((pr = realloc(ptr, size)) == NULL || (p = malloc(size)) == NULL)) { + log_error("realloc(%zu) failed @ %s:%d", size, name, line); + } else { + log_vverb("realloc(%zu) at %p @ %s:%d", size, p, name, line); + memcpy(p, pr, size); + } + + free(pr); + return p; +} + void _cc_free(void *ptr, const char *name, int line) { - ASSERT(ptr != NULL); log_vverb("free(%p) @ %s:%d", ptr, name, line); free(ptr); } @@ -103,10 +137,10 @@ _cc_mmap(size_t size, const char *name, int line) * is set appropriately. */ p = mmap(NULL, size, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, - -1, 0); + -1, 0); if (p == ((void *) -1)) { log_error("mmap %zu bytes @ %s:%d failed: %s", size, name, line, - strerror(errno)); + strerror(errno)); return NULL; } @@ -128,7 +162,7 @@ _cc_munmap(void *p, size_t size, const char *name, int line) status = munmap(p, size); if (status < 0) { log_error("munmap %p @ %s:%d failed: %s", p, name, line, - strerror(errno)); + strerror(errno)); } return status; diff --git a/src/cc_signal.c b/src/cc_signal.c index 222ee7e52..726e2c4c2 100644 --- a/src/cc_signal.c +++ b/src/cc_signal.c @@ -8,6 +8,8 @@ #include #include +struct signal signals[SIGNAL_MAX]; + #ifndef CC_HAVE_SIGNAME const char* sys_signame[SIGNAL_MAX + 1] = { "UNDEFINED", diff --git a/src/stream/cc_sockio.c b/src/stream/cc_sockio.c index 906fc463e..09c7af9a6 100644 --- a/src/stream/cc_sockio.c +++ b/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 From 3b8c40f28a0cf7751953132a3038779fc1100645 Mon Sep 17 00:00:00 2001 From: Yao Yue Date: Sat, 11 Feb 2017 20:11:03 -0800 Subject: [PATCH 2/2] sockio metrics help solve the memory leak --- README.cmake | 2 +- README.md | 2 +- 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 ++ 12 files changed, 15 insertions(+), 5 deletions(-) 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/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