From 9fa41b92e061eb434a9a9f9ac7932fd0770c6511 Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Fri, 10 Apr 2015 15:08:03 -0700 Subject: Eliminate channel-wide lock for grpc_mdelem_ref. We only need to lock on the initial ref from garbage to atomically change mdtab_free. --- src/core/transport/metadata.c | 31 ++++++++++++++++++------------- 1 file changed, 18 insertions(+), 13 deletions(-) (limited to 'src/core/transport') diff --git a/src/core/transport/metadata.c b/src/core/transport/metadata.c index 066cc263a1..e3de6ce455 100644 --- a/src/core/transport/metadata.c +++ b/src/core/transport/metadata.c @@ -34,10 +34,12 @@ #include "src/core/iomgr/sockaddr.h" #include "src/core/transport/metadata.h" +#include #include #include #include +#include #include #include "src/core/support/murmur_hash.h" #include "src/core/transport/chttp2/bin_encoder.h" @@ -68,11 +70,12 @@ typedef struct internal_metadata { internal_string *key; internal_string *value; + gpr_atm refcnt; + /* private only data */ void *user_data; void (*destroy_user_data)(void *user_data); - gpr_uint32 refs; grpc_mdctx *context; struct internal_metadata *bucket_next; } internal_metadata; @@ -129,8 +132,8 @@ static void unlock(grpc_mdctx *ctx) { gpr_mu_unlock(&ctx->mu); } -static void ref_md(internal_metadata *md) { - if (0 == md->refs++) { +static void ref_md_locked(internal_metadata *md) { + if (0 == gpr_atm_no_barrier_fetch_add(&md->refcnt, 1)) { md->context->mdtab_free--; } } @@ -168,7 +171,7 @@ static void discard_metadata(grpc_mdctx *ctx) { for (i = 0; i < ctx->mdtab_capacity; i++) { cur = ctx->mdtab[i]; while (cur) { - GPR_ASSERT(cur->refs == 0); + GPR_ASSERT(gpr_atm_acq_load(&cur->refcnt) == 0); next = cur->bucket_next; internal_string_unref(cur->key); internal_string_unref(cur->value); @@ -349,7 +352,7 @@ static void gc_mdtab(grpc_mdctx *ctx) { prev_next = &ctx->mdtab[i]; for (md = ctx->mdtab[i]; md; md = next) { next = md->bucket_next; - if (md->refs == 0) { + if (gpr_atm_acq_load(&md->refcnt) == 0) { internal_string_unref(md->key); internal_string_unref(md->value); if (md->user_data) { @@ -415,7 +418,7 @@ grpc_mdelem *grpc_mdelem_from_metadata_strings(grpc_mdctx *ctx, /* search for an existing pair */ for (md = ctx->mdtab[hash % ctx->mdtab_capacity]; md; md = md->bucket_next) { if (md->key == key && md->value == value) { - ref_md(md); + ref_md_locked(md); internal_string_unref(key); internal_string_unref(value); unlock(ctx); @@ -425,7 +428,7 @@ grpc_mdelem *grpc_mdelem_from_metadata_strings(grpc_mdctx *ctx, /* not found: create a new pair */ md = gpr_malloc(sizeof(internal_metadata)); - md->refs = 1; + gpr_atm_rel_store(&md->refcnt, 1); md->context = ctx; md->key = key; md->value = value; @@ -468,10 +471,12 @@ grpc_mdelem *grpc_mdelem_from_string_and_buffer(grpc_mdctx *ctx, grpc_mdelem *grpc_mdelem_ref(grpc_mdelem *gmd) { internal_metadata *md = (internal_metadata *)gmd; - grpc_mdctx *ctx = md->context; - lock(ctx); - ref_md(md); - unlock(ctx); + /* we can assume the ref count is >= 1 as the application is calling + 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_acq_load(&md->refcnt) >= 1); + gpr_atm_no_barrier_fetch_add(&md->refcnt, 1); return gmd; } @@ -479,8 +484,8 @@ void grpc_mdelem_unref(grpc_mdelem *gmd) { internal_metadata *md = (internal_metadata *)gmd; grpc_mdctx *ctx = md->context; lock(ctx); - GPR_ASSERT(md->refs); - if (0 == --md->refs) { + assert(gpr_atm_acq_load(&md->refcnt) >= 1); + if (1 == gpr_atm_full_fetch_add(&md->refcnt, -1)) { ctx->mdtab_free++; } unlock(ctx); -- cgit v1.2.3 From 1ae46a2129411b9cbaa5f57afe321b4946b76e3f Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Mon, 13 Apr 2015 10:48:55 -0700 Subject: Change barriers to protect the innocent --- src/core/transport/metadata.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'src/core/transport') diff --git a/src/core/transport/metadata.c b/src/core/transport/metadata.c index e3de6ce455..c9e0fc8f6f 100644 --- a/src/core/transport/metadata.c +++ b/src/core/transport/metadata.c @@ -475,7 +475,7 @@ grpc_mdelem *grpc_mdelem_ref(grpc_mdelem *gmd) { 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_acq_load(&md->refcnt) >= 1); + assert(gpr_atm_no_barrier_load(&md->refcnt) >= 1); gpr_atm_no_barrier_fetch_add(&md->refcnt, 1); return gmd; } @@ -484,7 +484,7 @@ void grpc_mdelem_unref(grpc_mdelem *gmd) { internal_metadata *md = (internal_metadata *)gmd; grpc_mdctx *ctx = md->context; lock(ctx); - assert(gpr_atm_acq_load(&md->refcnt) >= 1); + assert(gpr_atm_nobarrier_load(&md->refcnt) >= 1); if (1 == gpr_atm_full_fetch_add(&md->refcnt, -1)) { ctx->mdtab_free++; } -- cgit v1.2.3 From ba63e8a2a7b111a7e243184428ff3ab38d33567a Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Mon, 13 Apr 2015 15:02:29 -0700 Subject: Fix Typo ... and this, kids, is why you should always compile in debug before pushing. --- src/core/transport/metadata.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'src/core/transport') diff --git a/src/core/transport/metadata.c b/src/core/transport/metadata.c index c9e0fc8f6f..f4075a31f8 100644 --- a/src/core/transport/metadata.c +++ b/src/core/transport/metadata.c @@ -484,7 +484,7 @@ void grpc_mdelem_unref(grpc_mdelem *gmd) { internal_metadata *md = (internal_metadata *)gmd; grpc_mdctx *ctx = md->context; lock(ctx); - assert(gpr_atm_nobarrier_load(&md->refcnt) >= 1); + assert(gpr_atm_no_barrier_load(&md->refcnt) >= 1); if (1 == gpr_atm_full_fetch_add(&md->refcnt, -1)) { ctx->mdtab_free++; } -- cgit v1.2.3