From 63bda56884f1dece057f84dc6f94f9c2a5ce1605 Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Fri, 9 Oct 2015 17:40:19 -0700 Subject: [PATCH 1/3] Make metadata unref atomic We used to need to lock the metadata context to unref an mdelem. This change makes it possible to lock only when the mdelem refcount would reach zero. --- src/core/surface/call.c | 7 ++-- src/core/transport/chttp2/stream_encoder.c | 7 ++-- src/core/transport/metadata.c | 39 ++++++---------------- src/core/transport/metadata.h | 22 ------------ 4 files changed, 14 insertions(+), 61 deletions(-) diff --git a/src/core/surface/call.c b/src/core/surface/call.c index d15a3bcbade..9bf04514426 100644 --- a/src/core/surface/call.c +++ b/src/core/surface/call.c @@ -1484,7 +1484,6 @@ static void recv_metadata(grpc_exec_ctx *exec_ctx, grpc_call *call, grpc_metadata_array *dest; grpc_metadata *mdusr; int is_trailing; - grpc_mdctx *mdctx = call->metadata_context; is_trailing = call->read_state >= READ_STATE_GOT_INITIAL_METADATA; for (l = md->list.head; l != NULL; l = l->next) { @@ -1532,14 +1531,12 @@ static void recv_metadata(grpc_exec_ctx *exec_ctx, grpc_call *call, call->read_state = READ_STATE_GOT_INITIAL_METADATA; } - grpc_mdctx_lock(mdctx); for (l = md->list.head; l; l = l->next) { - if (l->md) GRPC_MDCTX_LOCKED_MDELEM_UNREF(mdctx, l->md); + if (l->md) GRPC_MDELEM_UNREF(l->md); } for (l = md->garbage.head; l; l = l->next) { - GRPC_MDCTX_LOCKED_MDELEM_UNREF(mdctx, l->md); + GRPC_MDELEM_UNREF(l->md); } - grpc_mdctx_unlock(mdctx); } grpc_call_stack *grpc_call_get_call_stack(grpc_call *call) { diff --git a/src/core/transport/chttp2/stream_encoder.c b/src/core/transport/chttp2/stream_encoder.c index 83227e677db..2cbf90fb845 100644 --- a/src/core/transport/chttp2/stream_encoder.c +++ b/src/core/transport/chttp2/stream_encoder.c @@ -584,7 +584,6 @@ void grpc_chttp2_encode(grpc_stream_op *ops, size_t ops_count, int eof, size_t max_take_size; gpr_uint32 curop = 0; gpr_uint32 unref_op; - grpc_mdctx *mdctx = compressor->mdctx; grpc_linked_mdelem *l; int need_unref = 0; gpr_timespec deadline; @@ -650,17 +649,15 @@ void grpc_chttp2_encode(grpc_stream_op *ops, size_t ops_count, int eof, finish_frame(&st, 1, eof); if (need_unref) { - grpc_mdctx_lock(mdctx); for (unref_op = 0; unref_op < curop; unref_op++) { op = &ops[unref_op]; if (op->type != GRPC_OP_METADATA) continue; for (l = op->data.metadata.list.head; l; l = l->next) { - if (l->md) GRPC_MDCTX_LOCKED_MDELEM_UNREF(mdctx, l->md); + if (l->md) GRPC_MDELEM_UNREF(l->md); } for (l = op->data.metadata.garbage.head; l; l = l->next) { - GRPC_MDCTX_LOCKED_MDELEM_UNREF(mdctx, l->md); + GRPC_MDELEM_UNREF(l->md); } } - grpc_mdctx_unlock(mdctx); } } diff --git a/src/core/transport/metadata.c b/src/core/transport/metadata.c index 3dbb9f0b539..02f9cda754d 100644 --- a/src/core/transport/metadata.c +++ b/src/core/transport/metadata.c @@ -159,6 +159,10 @@ static void ref_md_locked(internal_metadata *md DEBUG_ARGS) { grpc_mdstr_as_c_string((grpc_mdstr *)md->value)); #endif if (0 == gpr_atm_no_barrier_fetch_add(&md->refcnt, 1)) { + /* This ref is dropped if grpc_mdelem_unref reaches 1, + but allows us to safely unref without taking the mdctx lock + until such time */ + gpr_atm_no_barrier_fetch_add(&md->refcnt, 1); md->context->mdtab_free--; } } @@ -465,7 +469,7 @@ grpc_mdelem *grpc_mdelem_from_metadata_strings(grpc_mdctx *ctx, /* not found: create a new pair */ md = gpr_malloc(sizeof(internal_metadata)); - gpr_atm_rel_store(&md->refcnt, 1); + gpr_atm_rel_store(&md->refcnt, 2); md->context = ctx; md->key = key; md->value = value; @@ -534,8 +538,6 @@ grpc_mdelem *grpc_mdelem_ref(grpc_mdelem *gmd DEBUG_ARGS) { void grpc_mdelem_unref(grpc_mdelem *gmd DEBUG_ARGS) { internal_metadata *md = (internal_metadata *)gmd; - grpc_mdctx *ctx = md->context; - lock(ctx); #ifdef GRPC_METADATA_REFCOUNT_DEBUG gpr_log(file, line, GPR_LOG_SEVERITY_DEBUG, "ELM UNREF:%p:%d->%d: '%s' = '%s'", md, @@ -544,11 +546,13 @@ void grpc_mdelem_unref(grpc_mdelem *gmd DEBUG_ARGS) { grpc_mdstr_as_c_string((grpc_mdstr *)md->key), grpc_mdstr_as_c_string((grpc_mdstr *)md->value)); #endif - assert(gpr_atm_no_barrier_load(&md->refcnt) >= 1); - if (1 == gpr_atm_full_fetch_add(&md->refcnt, -1)) { + if (2 == gpr_atm_full_fetch_add(&md->refcnt, -1)) { + grpc_mdctx *ctx = md->context; + lock(ctx); + GPR_ASSERT(1 == gpr_atm_full_fetch_add(&md->refcnt, -1)); ctx->mdtab_free++; + unlock(ctx); } - unlock(ctx); } const char *grpc_mdstr_as_c_string(grpc_mdstr *s) { @@ -627,29 +631,6 @@ gpr_slice grpc_mdstr_as_base64_encoded_and_huffman_compressed(grpc_mdstr *gs) { return slice; } -void grpc_mdctx_lock(grpc_mdctx *ctx) { lock(ctx); } - -void grpc_mdctx_locked_mdelem_unref(grpc_mdctx *ctx, - grpc_mdelem *gmd DEBUG_ARGS) { - internal_metadata *md = (internal_metadata *)gmd; - grpc_mdctx *elem_ctx = md->context; - GPR_ASSERT(ctx == elem_ctx); -#ifdef GRPC_METADATA_REFCOUNT_DEBUG - gpr_log(file, line, GPR_LOG_SEVERITY_DEBUG, - "ELM UNREF:%p:%d->%d: '%s' = '%s'", md, - gpr_atm_no_barrier_load(&md->refcnt), - gpr_atm_no_barrier_load(&md->refcnt) - 1, - grpc_mdstr_as_c_string((grpc_mdstr *)md->key), - grpc_mdstr_as_c_string((grpc_mdstr *)md->value)); -#endif - assert(gpr_atm_no_barrier_load(&md->refcnt) >= 1); - if (1 == gpr_atm_full_fetch_add(&md->refcnt, -1)) { - ctx->mdtab_free++; - } -} - -void grpc_mdctx_unlock(grpc_mdctx *ctx) { unlock(ctx); } - static int conforms_to(grpc_mdstr *s, const gpr_uint8 *legal_bits) { const gpr_uint8 *p = GPR_SLICE_START_PTR(s->slice); const gpr_uint8 *e = GPR_SLICE_END_PTR(s->slice); diff --git a/src/core/transport/metadata.h b/src/core/transport/metadata.h index 136a65f288e..9a8164037c6 100644 --- a/src/core/transport/metadata.h +++ b/src/core/transport/metadata.h @@ -155,28 +155,6 @@ int grpc_mdstr_is_legal_header(grpc_mdstr *s); int grpc_mdstr_is_legal_nonbin_header(grpc_mdstr *s); int grpc_mdstr_is_bin_suffixed(grpc_mdstr *s); -/* Batch mode metadata functions. - These API's have equivalents above, but allow taking the mdctx just once, - performing a bunch of work, and then leaving the mdctx. */ - -/* Lock the metadata context: it's only safe to call _locked_ functions against - this context from the calling thread until grpc_mdctx_unlock is called */ -void grpc_mdctx_lock(grpc_mdctx *ctx); -#ifdef GRPC_METADATA_REFCOUNT_DEBUG -#define GRPC_MDCTX_LOCKED_MDELEM_UNREF(ctx, elem) \ - grpc_mdctx_locked_mdelem_unref((ctx), (elem), __FILE__, __LINE__) -/* Unref a metadata element */ -void grpc_mdctx_locked_mdelem_unref(grpc_mdctx *ctx, grpc_mdelem *elem, - const char *file, int line); -#else -#define GRPC_MDCTX_LOCKED_MDELEM_UNREF(ctx, elem) \ - grpc_mdctx_locked_mdelem_unref((ctx), (elem)) -/* Unref a metadata element */ -void grpc_mdctx_locked_mdelem_unref(grpc_mdctx *ctx, grpc_mdelem *elem); -#endif -/* Unlock the metadata context */ -void grpc_mdctx_unlock(grpc_mdctx *ctx); - #define GRPC_MDSTR_KV_HASH(k_hash, v_hash) (GPR_ROTL((k_hash), 2) ^ (v_hash)) #endif /* GRPC_INTERNAL_CORE_TRANSPORT_METADATA_H */ From 8344daa1dfe627591b5566aaca705c1f8c057f4d Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Fri, 9 Oct 2015 18:10:57 -0700 Subject: [PATCH 2/3] Make getting metadata user data a lock free operation --- src/core/transport/metadata.c | 37 +++++++++++++++++++++-------------- 1 file changed, 22 insertions(+), 15 deletions(-) diff --git a/src/core/transport/metadata.c b/src/core/transport/metadata.c index 02f9cda754d..1b48b48e097 100644 --- a/src/core/transport/metadata.c +++ b/src/core/transport/metadata.c @@ -62,6 +62,8 @@ #define REF_MD_LOCKED(s) ref_md_locked((s)) #endif +typedef void (*destroy_user_data_func)(void *user_data); + typedef struct internal_string { /* must be byte compatible with grpc_mdstr */ gpr_slice slice; @@ -88,8 +90,8 @@ typedef struct internal_metadata { /* private only data */ gpr_mu mu_user_data; - void *user_data; - void (*destroy_user_data)(void *user_data); + gpr_atm destroy_user_data; + gpr_atm user_data; grpc_mdctx *context; struct internal_metadata *bucket_next; @@ -201,12 +203,14 @@ static void discard_metadata(grpc_mdctx *ctx) { for (i = 0; i < ctx->mdtab_capacity; i++) { cur = ctx->mdtab[i]; while (cur) { + void *user_data = (void *)gpr_atm_no_barrier_load(&cur->user_data); GPR_ASSERT(gpr_atm_acq_load(&cur->refcnt) == 0); next = cur->bucket_next; INTERNAL_STRING_UNREF(cur->key); INTERNAL_STRING_UNREF(cur->value); - if (cur->user_data) { - cur->destroy_user_data(cur->user_data); + if (user_data != NULL) { + ((destroy_user_data_func)gpr_atm_no_barrier_load( + &cur->destroy_user_data))(user_data); } gpr_mu_destroy(&cur->mu_user_data); gpr_free(cur); @@ -392,12 +396,14 @@ static void gc_mdtab(grpc_mdctx *ctx) { for (i = 0; i < ctx->mdtab_capacity; i++) { prev_next = &ctx->mdtab[i]; for (md = ctx->mdtab[i]; md; md = next) { + void *user_data = (void *)gpr_atm_no_barrier_load(&md->user_data); next = md->bucket_next; if (gpr_atm_acq_load(&md->refcnt) == 0) { INTERNAL_STRING_UNREF(md->key); INTERNAL_STRING_UNREF(md->value); if (md->user_data) { - md->destroy_user_data(md->user_data); + ((destroy_user_data_func)gpr_atm_no_barrier_load( + &md->destroy_user_data))(user_data); } gpr_free(md); *prev_next = next; @@ -473,8 +479,8 @@ grpc_mdelem *grpc_mdelem_from_metadata_strings(grpc_mdctx *ctx, md->context = ctx; md->key = key; md->value = value; - md->user_data = NULL; - md->destroy_user_data = NULL; + md->user_data = 0; + md->destroy_user_data = 0; md->bucket_next = ctx->mdtab[hash % ctx->mdtab_capacity]; gpr_mu_init(&md->mu_user_data); #ifdef GRPC_METADATA_REFCOUNT_DEBUG @@ -588,13 +594,14 @@ size_t grpc_mdctx_get_mdtab_free_test_only(grpc_mdctx *ctx) { return ctx->mdtab_free; } -void *grpc_mdelem_get_user_data(grpc_mdelem *md, - void (*if_destroy_func)(void *)) { +void *grpc_mdelem_get_user_data(grpc_mdelem *md, void (*destroy_func)(void *)) { internal_metadata *im = (internal_metadata *)md; void *result; - gpr_mu_lock(&im->mu_user_data); - result = im->destroy_user_data == if_destroy_func ? im->user_data : NULL; - gpr_mu_unlock(&im->mu_user_data); + if (gpr_atm_acq_load(&im->destroy_user_data) == (gpr_atm)destroy_func) { + return (void *)gpr_atm_no_barrier_load(&im->user_data); + } else { + return NULL; + } return result; } @@ -603,7 +610,7 @@ void grpc_mdelem_set_user_data(grpc_mdelem *md, void (*destroy_func)(void *), internal_metadata *im = (internal_metadata *)md; GPR_ASSERT((user_data == NULL) == (destroy_func == NULL)); gpr_mu_lock(&im->mu_user_data); - if (im->destroy_user_data) { + if (gpr_atm_no_barrier_load(&im->destroy_user_data)) { /* user data can only be set once */ gpr_mu_unlock(&im->mu_user_data); if (destroy_func != NULL) { @@ -611,8 +618,8 @@ void grpc_mdelem_set_user_data(grpc_mdelem *md, void (*destroy_func)(void *), } return; } - im->destroy_user_data = destroy_func; - im->user_data = user_data; + gpr_atm_no_barrier_store(&im->user_data, (gpr_atm)user_data); + gpr_atm_rel_store(&im->destroy_user_data, (gpr_atm)destroy_func); gpr_mu_unlock(&im->mu_user_data); } From b8e82670ce244434e7570fc4b0459628bd6beee1 Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Sat, 10 Oct 2015 13:52:47 -0700 Subject: [PATCH 3/3] Fix race conditions --- src/core/transport/metadata.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/core/transport/metadata.c b/src/core/transport/metadata.c index 1b48b48e097..68f23177eba 100644 --- a/src/core/transport/metadata.c +++ b/src/core/transport/metadata.c @@ -160,12 +160,10 @@ static void ref_md_locked(internal_metadata *md DEBUG_ARGS) { grpc_mdstr_as_c_string((grpc_mdstr *)md->key), grpc_mdstr_as_c_string((grpc_mdstr *)md->value)); #endif - if (0 == gpr_atm_no_barrier_fetch_add(&md->refcnt, 1)) { - /* This ref is dropped if grpc_mdelem_unref reaches 1, - but allows us to safely unref without taking the mdctx lock - until such time */ - gpr_atm_no_barrier_fetch_add(&md->refcnt, 1); + if (0 == gpr_atm_no_barrier_fetch_add(&md->refcnt, 2)) { md->context->mdtab_free--; + } else { + GPR_ASSERT(1 != gpr_atm_no_barrier_fetch_add(&md->refcnt, -1)); } } @@ -537,7 +535,7 @@ grpc_mdelem *grpc_mdelem_ref(grpc_mdelem *gmd DEBUG_ARGS) { this function - meaning that no adjustment to mdtab_free is necessary, simplifying the logic here to be just an atomic increment */ /* use C assert to have this removed in opt builds */ - assert(gpr_atm_no_barrier_load(&md->refcnt) >= 1); + assert(gpr_atm_no_barrier_load(&md->refcnt) >= 2); gpr_atm_no_barrier_fetch_add(&md->refcnt, 1); return gmd; } @@ -555,8 +553,10 @@ void grpc_mdelem_unref(grpc_mdelem *gmd DEBUG_ARGS) { if (2 == gpr_atm_full_fetch_add(&md->refcnt, -1)) { grpc_mdctx *ctx = md->context; lock(ctx); - GPR_ASSERT(1 == gpr_atm_full_fetch_add(&md->refcnt, -1)); - ctx->mdtab_free++; + if (1 == gpr_atm_no_barrier_load(&md->refcnt)) { + ctx->mdtab_free++; + gpr_atm_no_barrier_store(&md->refcnt, 0); + } unlock(ctx); } }