From 58c42b4b0fb5c0a8f46380824bf1a9af7fe23845 Mon Sep 17 00:00:00 2001 From: Yao Yue Date: Mon, 6 Feb 2017 23:18:10 -0800 Subject: [PATCH] disable EVENT_READ when waiting for EVENT_WRITE, and vice versa (#117) * disable EVENT_READ when waiting for EVENT_WRITE, and vice versa * Squashed 'deps/ccommon/' changes from bb298bc..5b095bc 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: 5b095bc2769e5a6a86b7b7e40b14cb6ed43847e7 * upgrade to latest version of ccommon --- deps/ccommon/CMakeLists.txt | 1 + deps/ccommon/config.h.in | 2 ++ deps/ccommon/docs/c-styleguide.txt | 6 ++-- deps/ccommon/include/cc_array.h | 4 +-- deps/ccommon/include/cc_define.h | 7 ++--- deps/ccommon/include/cc_mm.h | 8 +++++ deps/ccommon/include/cc_signal.h | 2 +- deps/ccommon/src/cc_debug.c | 6 ++++ deps/ccommon/src/cc_mm.c | 46 ++++++++++++++++++++++++---- deps/ccommon/src/cc_signal.c | 2 ++ src/core/data/worker.c | 26 ++++++++++++++-- src/core/data/worker.h | 3 +- src/protocol/data/memcache/compose.c | 2 +- 13 files changed, 94 insertions(+), 21 deletions(-) diff --git a/deps/ccommon/CMakeLists.txt b/deps/ccommon/CMakeLists.txt index eee63198e..8df6f9527 100644 --- a/deps/ccommon/CMakeLists.txt +++ b/deps/ccommon/CMakeLists.txt @@ -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/deps/ccommon/config.h.in b/deps/ccommon/config.h.in index e5fa50e30..f1dc18bd9 100644 --- a/deps/ccommon/config.h.in +++ b/deps/ccommon/config.h.in @@ -17,3 +17,5 @@ #cmakedefine HAVE_LOGGING #cmakedefine HAVE_STATS + +#cmakedefine HAVE_DEBUG_MM diff --git a/deps/ccommon/docs/c-styleguide.txt b/deps/ccommon/docs/c-styleguide.txt index 30aa62f20..69661c0d8 100644 --- a/deps/ccommon/docs/c-styleguide.txt +++ b/deps/ccommon/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/deps/ccommon/include/cc_array.h b/deps/ccommon/include/cc_array.h index 4e9dcdbe7..b4aa1feb8 100644 --- a/deps/ccommon/include/cc_array.h +++ b/deps/ccommon/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/deps/ccommon/include/cc_define.h b/deps/ccommon/include/cc_define.h index b542d0201..b521b133e 100644 --- a/deps/ccommon/include/cc_define.h +++ b/deps/ccommon/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/deps/ccommon/include/cc_mm.h b/deps/ccommon/include/cc_mm.h index a1cc574b3..9705bd840 100644 --- a/deps/ccommon/include/cc_mm.h +++ b/deps/ccommon/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/deps/ccommon/include/cc_signal.h b/deps/ccommon/include/cc_signal.h index 61e151d9a..1fe67c8ff 100644 --- a/deps/ccommon/include/cc_signal.h +++ b/deps/ccommon/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/deps/ccommon/src/cc_debug.c b/deps/ccommon/src/cc_debug.c index 7aa97dfe2..7c766260b 100644 --- a/deps/ccommon/src/cc_debug.c +++ b/deps/ccommon/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/deps/ccommon/src/cc_mm.c b/deps/ccommon/src/cc_mm.c index 4668d8d8e..6a0554289 100644 --- a/deps/ccommon/src/cc_mm.c +++ b/deps/ccommon/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/deps/ccommon/src/cc_signal.c b/deps/ccommon/src/cc_signal.c index 222ee7e52..726e2c4c2 100644 --- a/deps/ccommon/src/cc_signal.c +++ b/deps/ccommon/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/core/data/worker.c b/src/core/data/worker.c index c0c9efee9..47dabc6c0 100644 --- a/src/core/data/worker.c +++ b/src/core/data/worker.c @@ -46,7 +46,11 @@ _worker_write(struct buf_sock *s) return status; } -static inline void +/* the caller only needs to check the return status of this function if + * it previously received a write event and wants to re-register the + * read event upon full, successful write. + */ +static inline rstatus_i _worker_event_write(struct buf_sock *s) { rstatus_i status; @@ -54,6 +58,15 @@ _worker_event_write(struct buf_sock *s) status = _worker_write(s); if (status == CC_ERETRY || status == CC_EAGAIN) { /* retry write */ + /* by removing current masks and only listen to write event(s), we are + * effectively stopping processing incoming data until we can write + * something to the (kernel) buffer for the channel. This is sensible + * because either the local network or the client is backed up when + * kernel write buffer is full, and this allows us to propagate back + * pressure to the sending side. + */ + + event_del(ctx->evb, hdl->wid(c)); event_add_write(ctx->evb, hdl->wid(c), s); } else if (status == CC_ERROR) { c->state = CHANNEL_TERM; @@ -62,8 +75,10 @@ _worker_event_write(struct buf_sock *s) if (processor->post_write(&s->rbuf, &s->wbuf, &s->data) < 0) { log_debug("handler signals channel termination"); s->ch->state = CHANNEL_TERM; - return; + return CC_ERROR; } + + return status; } static inline void @@ -171,9 +186,14 @@ _worker_event(void *arg, uint32_t events) INCR(worker_metrics, worker_event_read); _worker_event_read(s); } else if (events & EVENT_WRITE) { + /* got here only when a previous write was incompleted/retried */ log_verb("processing worker write event on buf_sock %p", s); INCR(worker_metrics, worker_event_write); - _worker_event_write(s); + if (_worker_event_write(s) == CC_OK) { + /* write backlog cleared up, re-add read event (only) */ + event_del(ctx->evb, hdl->wid(s->ch)); + event_add_read(ctx->evb, hdl->rid(s->ch), s); + } } else if (events & EVENT_ERR) { s->ch->state = CHANNEL_TERM; INCR(worker_metrics, worker_event_error); diff --git a/src/core/data/worker.h b/src/core/data/worker.h index 5a5240345..3ee79b5f5 100644 --- a/src/core/data/worker.h +++ b/src/core/data/worker.h @@ -22,8 +22,7 @@ typedef struct { ACTION( worker_event_loop, METRIC_COUNTER, "# worker event loops returned" )\ ACTION( worker_event_read, METRIC_COUNTER, "# worker core_read events" )\ ACTION( worker_event_write, METRIC_COUNTER, "# worker core_write events" )\ - ACTION( worker_event_error, METRIC_COUNTER, "# worker core_error events" )\ - ACTION( worker_oom_ex, METRIC_COUNTER, "# worker error due to oom" ) + ACTION( worker_event_error, METRIC_COUNTER, "# worker core_error events" ) typedef struct { CORE_WORKER_METRIC(METRIC_DECLARE) diff --git a/src/protocol/data/memcache/compose.c b/src/protocol/data/memcache/compose.c index ea4cc3cca..8a56cdd50 100644 --- a/src/protocol/data/memcache/compose.c +++ b/src/protocol/data/memcache/compose.c @@ -121,7 +121,7 @@ compose_req(struct buf **buf, struct request *req) { request_type_t type = req->type; struct bstring *str = &req_strings[type]; - struct bstring *key = req->keys->data; + struct bstring *key = (struct bstring *)req->keys->data; int noreply_len = req->noreply * NOREPLY_LEN; int cas_len = (req->type == REQ_CAS) ? CC_UINT64_MAXLEN : 0; uint32_t i;