From 764b00023f2bc97f240c3a758ed23ce9c0ad8526 Mon Sep 17 00:00:00 2001 From: Jason Evans Date: Sat, 14 Mar 2015 14:01:35 -0700 Subject: [PATCH] Fix a heap profiling regression. Add the prof_tctx_state_destroying transitionary state to fix a race between a thread destroying a tctx and another thread creating a new equivalent tctx. This regression was introduced by 602c8e0971160e4b85b08b16cf8a2375aa24bc04 (Implement per thread heap profiling.). --- include/jemalloc/internal/prof.h | 1 + src/prof.c | 44 ++++++++++++++++++++++---------- 2 files changed, 32 insertions(+), 13 deletions(-) diff --git a/include/jemalloc/internal/prof.h b/include/jemalloc/internal/prof.h index f5082438..8967333a 100644 --- a/include/jemalloc/internal/prof.h +++ b/include/jemalloc/internal/prof.h @@ -81,6 +81,7 @@ struct prof_cnt_s { typedef enum { prof_tctx_state_initializing, prof_tctx_state_nominal, + prof_tctx_state_destroying, prof_tctx_state_dumping, prof_tctx_state_purgatory /* Dumper must finish destroying. */ } prof_tctx_state_t; diff --git a/src/prof.c b/src/prof.c index 84fa5fda..e86669c2 100644 --- a/src/prof.c +++ b/src/prof.c @@ -642,10 +642,13 @@ prof_tctx_destroy(tsd_t *tsd, prof_tctx_t *tctx) ckh_remove(tsd, &tdata->bt2tctx, &gctx->bt, NULL, NULL); destroy_tdata = prof_tdata_should_destroy(tdata, false); + if (tctx->state == prof_tctx_state_nominal) + tctx->state = prof_tctx_state_destroying; malloc_mutex_unlock(tdata->lock); malloc_mutex_lock(gctx->lock); - if (tctx->state != prof_tctx_state_dumping) { + switch (tctx->state) { + case prof_tctx_state_destroying: tctx_tree_remove(&gctx->tctxs, tctx); destroy_tctx = true; if (prof_gctx_should_destroy(gctx)) { @@ -667,7 +670,8 @@ prof_tctx_destroy(tsd_t *tsd, prof_tctx_t *tctx) destroy_gctx = true; } else destroy_gctx = false; - } else { + break; + case prof_tctx_state_dumping: /* * A dumping thread needs tctx to remain valid until dumping * has finished. Change state such that the dumping thread will @@ -676,6 +680,9 @@ prof_tctx_destroy(tsd_t *tsd, prof_tctx_t *tctx) tctx->state = prof_tctx_state_purgatory; destroy_tctx = false; destroy_gctx = false; + break; + default: + not_reached(); } malloc_mutex_unlock(gctx->lock); if (destroy_gctx) { @@ -1021,21 +1028,30 @@ prof_tctx_merge_tdata(prof_tctx_t *tctx, prof_tdata_t *tdata) { malloc_mutex_lock(tctx->gctx->lock); - if (tctx->state == prof_tctx_state_initializing) { + + switch (tctx->state) { + case prof_tctx_state_initializing: + case prof_tctx_state_destroying: malloc_mutex_unlock(tctx->gctx->lock); return; - } - assert(tctx->state == prof_tctx_state_nominal); - tctx->state = prof_tctx_state_dumping; - malloc_mutex_unlock(tctx->gctx->lock); + case prof_tctx_state_nominal: + tctx->state = prof_tctx_state_dumping; + malloc_mutex_unlock(tctx->gctx->lock); - memcpy(&tctx->dump_cnts, &tctx->cnts, sizeof(prof_cnt_t)); + memcpy(&tctx->dump_cnts, &tctx->cnts, sizeof(prof_cnt_t)); - tdata->cnt_summed.curobjs += tctx->dump_cnts.curobjs; - tdata->cnt_summed.curbytes += tctx->dump_cnts.curbytes; - if (opt_prof_accum) { - tdata->cnt_summed.accumobjs += tctx->dump_cnts.accumobjs; - tdata->cnt_summed.accumbytes += tctx->dump_cnts.accumbytes; + tdata->cnt_summed.curobjs += tctx->dump_cnts.curobjs; + tdata->cnt_summed.curbytes += tctx->dump_cnts.curbytes; + if (opt_prof_accum) { + tdata->cnt_summed.accumobjs += + tctx->dump_cnts.accumobjs; + tdata->cnt_summed.accumbytes += + tctx->dump_cnts.accumbytes; + } + break; + case prof_tctx_state_dumping: + case prof_tctx_state_purgatory: + not_reached(); } } @@ -1059,6 +1075,7 @@ prof_tctx_merge_iter(prof_tctx_tree_t *tctxs, prof_tctx_t *tctx, void *arg) switch (tctx->state) { case prof_tctx_state_nominal: + case prof_tctx_state_destroying: /* New since dumping started; ignore. */ break; case prof_tctx_state_dumping: @@ -1094,6 +1111,7 @@ prof_tctx_finish_iter(prof_tctx_tree_t *tctxs, prof_tctx_t *tctx, void *arg) switch (tctx->state) { case prof_tctx_state_nominal: + case prof_tctx_state_destroying: /* New since dumping started; ignore. */ break; case prof_tctx_state_dumping: