From 1f50f809c4d885ff2b2c1a626d69ebb4cea0502f Mon Sep 17 00:00:00 2001 From: Martin Quinson Date: Wed, 17 Oct 2012 22:04:27 +0200 Subject: [PATCH 1/1] Ensure that the mallocator are really inactive when running MC - before, they were created during the preinit and then destroyed when MC was activated. - now, they are created inactive, and activated when the configuration stops if MC is still not activated. - this mandates some changes around the initialization mechanism in the whole stack. - one hint that it was a good idea is that before, _surf_init_status were never reaching the value 2 (meaning "config now forbidden", according to the preexisting asserts). - Now this value is set from surf_routing when the first element of platform is created. This is a horrible hack intermixing the layers, but it was already accredited by the parser that configuration could occur until the first , but no later. - we now would need a proper way of increasing the init level of simgrid. I hope that this change did not break anything, but much more work would be mandated to clean the init mess properly. --- include/simgrid_config.h.in | 4 +- include/xbt/mallocator.h | 2 + src/surf/surf_config.c | 26 ++++++------ src/surf/surf_routing.c | 4 ++ src/surf/surfxml_parse.c | 5 +++ src/xbt/dict.c | 6 --- src/xbt/fifo.c | 5 --- src/xbt/mallocator.c | 80 +++++++++++++++++++++++++++++-------- 8 files changed, 88 insertions(+), 44 deletions(-) diff --git a/include/simgrid_config.h.in b/include/simgrid_config.h.in index 06ecec882a..de9e3f1750 100644 --- a/include/simgrid_config.h.in +++ b/include/simgrid_config.h.in @@ -125,8 +125,8 @@ XBT_PUBLIC(char *) bvprintf(const char *fmt, va_list ap); XBT_PUBLIC(char *) bprintf(const char *fmt, ...) _XBT_GNUC_PRINTF(1, 2); /** @} */ -/* Whether mallocators should be enabled or not. */ -#define MALLOCATOR_IS_WANTED @MALLOCATOR_IS_WANTED@ +/* Whether mallocators were enabled in ccmake or not. */ +#define MALLOCATOR_COMPILED_IN @MALLOCATOR_IS_WANTED@ /* Define if xbt contexts are based on our threads implementation or not */ #cmakedefine CONTEXT_THREADS @CONTEXT_THREADS@ diff --git a/include/xbt/mallocator.h b/include/xbt/mallocator.h index e65d0e2db9..9655093a13 100644 --- a/include/xbt/mallocator.h +++ b/include/xbt/mallocator.h @@ -52,6 +52,8 @@ XBT_PUBLIC(void) xbt_mallocator_free(xbt_mallocator_t mallocator); XBT_PUBLIC(void *) xbt_mallocator_get(xbt_mallocator_t mallocator); XBT_PUBLIC(void) xbt_mallocator_release(xbt_mallocator_t mallocator, void *object); + +XBT_PUBLIC(void) xbt_mallocator_initialization_is_done(void); /** @} */ SG_END_DECL() diff --git a/src/surf/surf_config.c b/src/surf/surf_config.c index 6108c02fc9..ebc30f74f9 100644 --- a/src/surf/surf_config.c +++ b/src/surf/surf_config.c @@ -86,16 +86,17 @@ static void surf_config_cmd_line(int *argc, char **argv) } -int _surf_init_status = 0; /* 0: beginning of time; - 1: pre-inited (cfg_set created); - 2: inited (running) */ +int _surf_init_status = 0; /* 0: beginning of time (config cannot be changed yet); + 1: initialized: cfg_set created (config can now be changed); + 2: configured: command line parsed and config part of platform file was integrated also, platform construction ongoing or done. + (Config cannot be changed anymore!) */ /* callback of the workstation/model variable */ static void _surf_cfg_cb__workstation_model(const char *name, int pos) { char *val; - xbt_assert(_surf_init_status < 2, + xbt_assert(_surf_init_status == 1, "Cannot change the model after the initialization"); val = xbt_cfg_get_string(_surf_cfg_set, name); @@ -114,7 +115,7 @@ static void _surf_cfg_cb__cpu_model(const char *name, int pos) { char *val; - xbt_assert(_surf_init_status < 2, + xbt_assert(_surf_init_status == 1, "Cannot change the model after the initialization"); val = xbt_cfg_get_string(_surf_cfg_set, name); @@ -133,7 +134,7 @@ static void _surf_cfg_cb__optimization_mode(const char *name, int pos) { char *val; - xbt_assert(_surf_init_status < 2, + xbt_assert(_surf_init_status == 1, "Cannot change the model after the initialization"); val = xbt_cfg_get_string(_surf_cfg_set, name); @@ -152,7 +153,7 @@ static void _surf_cfg_cb__storage_mode(const char *name, int pos) { char *val; - xbt_assert(_surf_init_status < 2, + xbt_assert(_surf_init_status == 1, "Cannot change the model after the initialization"); val = xbt_cfg_get_string(_surf_cfg_set, name); @@ -171,7 +172,7 @@ static void _surf_cfg_cb__network_model(const char *name, int pos) { char *val; - xbt_assert(_surf_init_status < 2, + xbt_assert(_surf_init_status == 1, "Cannot change the model after the initialization"); val = xbt_cfg_get_string(_surf_cfg_set, name); @@ -232,14 +233,11 @@ static void _surf_cfg_cb_model_check(const char *name, int pos) { _surf_do_model_check = xbt_cfg_get_int(_surf_cfg_set, name); - if (_surf_do_model_check) { #ifndef HAVE_MC + if (_surf_do_model_check) { xbt_die("You tried to activate the model-checking from the command line, but it was not compiled in. Change your settings in cmake, recompile and try again"); -#endif - /* Tell modules using mallocators that they shouldn't. MC don't like them */ - xbt_fifo_preinit(); - xbt_dict_preinit(); } +#endif } extern int _surf_do_verbose_exit; @@ -663,10 +661,10 @@ void surf_config_init(int *argc, char **argv) xbt_cfg_setdefault_string(_surf_cfg_set, "path", initial_path); } + _surf_init_status = 1; surf_config_cmd_line(argc, argv); - _surf_init_status = 1; } else { XBT_WARN("Call to surf_config_init() after initialization ignored"); } diff --git a/src/surf/surf_routing.c b/src/surf/surf_routing.c index 4aadf20d31..b6bb950a70 100644 --- a/src/surf/surf_routing.c +++ b/src/surf/surf_routing.c @@ -321,6 +321,8 @@ static void routing_parse_trace_connect(sg_platf_trace_connect_cbarg_t trace_con } } +extern int _surf_init_status; /* yay, this is an horrible hack */ + /** * \brief Make a new routing component to the platform * @@ -344,6 +346,8 @@ void routing_AS_begin(sg_platf_AS_cbarg_t AS) (as_router_lib, AS->id, ROUTING_ASR_LEVEL), "The AS \"%s\" already exists", AS->id); + _surf_init_status = 2; /* horrible hack: direct access to the global controlling the level of configuration to prevent any further config */ + /* search the routing model */ switch(AS->routing){ case A_surfxml_AS_routing_Cluster: model = &routing_models[SURF_MODEL_CLUSTER];break; diff --git a/src/surf/surfxml_parse.c b/src/surf/surfxml_parse.c index b9fc03b2d1..cbe0e1617a 100644 --- a/src/surf/surfxml_parse.c +++ b/src/surf/surfxml_parse.c @@ -694,10 +694,15 @@ void ETag_surfxml_AS(void){ sg_platf_new_AS_end(); } +extern int _surf_init_status; /* FIXME: find a proper way to export this at some point */ + void STag_surfxml_config(void){ AS_TAG = 0; xbt_assert(current_property_set == NULL, "Someone forgot to reset the property set to NULL in its closing tag (or XML malformed)"); XBT_DEBUG("START configuration name = %s",A_surfxml_config_id); + if (_surf_init_status == 2) { + surf_parse_error("All tags must be given before any platform elements (such as , , , , etc)."); + } } void ETag_surfxml_config(void){ xbt_dict_cursor_t cursor = NULL; diff --git a/src/xbt/dict.c b/src/xbt/dict.c index b844cb464b..f52f98bf08 100644 --- a/src/xbt/dict.c +++ b/src/xbt/dict.c @@ -608,12 +608,6 @@ void xbt_dict_dump_sizes(xbt_dict_t dict) */ void xbt_dict_preinit(void) { - if (dict_elm_mallocator != NULL) { - /* Already created. I guess we want to switch to MC mode, so kill the previously created mallocator */ - xbt_mallocator_free(dict_elm_mallocator); - xbt_mallocator_free(dict_het_elm_mallocator); - } - dict_elm_mallocator = xbt_mallocator_new(256, dict_elm_mallocator_new_f, dict_elm_mallocator_free_f, diff --git a/src/xbt/fifo.c b/src/xbt/fifo.c index 305abed358..7a425f687c 100644 --- a/src/xbt/fifo.c +++ b/src/xbt/fifo.c @@ -525,11 +525,6 @@ xbt_fifo_item_t xbt_fifo_getPrevItem(xbt_fifo_item_t i) */ void xbt_fifo_preinit(void) { - if (item_mallocator != NULL) { - /* Already created. I guess we want to switch to MC mode, so kill the previously created mallocator */ - xbt_mallocator_free(item_mallocator); - } - item_mallocator = xbt_mallocator_new(65536, fifo_item_mallocator_new_f, fifo_item_mallocator_free_f, diff --git a/src/xbt/mallocator.c b/src/xbt/mallocator.c index 5a4f2fcb38..e4abe0de7f 100644 --- a/src/xbt/mallocator.c +++ b/src/xbt/mallocator.c @@ -14,12 +14,52 @@ XBT_LOG_NEW_DEFAULT_SUBCATEGORY(xbt_mallocator, xbt, "Mallocators"); -/* Mallocators and memory mess introduced by model-checking do not mix well +/** Implementation note on the mallocators: + * + * Mallocators and memory mess introduced by model-checking do not mix well * together: the mallocator will give standard memory when we are using raw * memory (so these blocks are killed on restore) and the contrary (so these - * blocks will leak accross restores). + * blocks will leak across restores). + * + * In addition, model-checking is activated when the command-line arguments + * are parsed, at the beginning of main, while most of the mallocators are + * created during the constructor functions launched from xbt_preinit, before + * the beginning of the main function. + * + * We want the code as fast as possible when they are active while we can deal + * with a little slow-down when they are inactive. So we start the mallocators + * as inactive. When they are so, they check at each use whether they should + * switch to the fast active mode or should stay in inactive mode. + * Finally, we give external elements a way to switch them + * all to the active mode (through xbt_mallocator_initialization_is_done). + * + * This design avoids to store all mallocators somewhere for later conversion, + * which would be hard to achieve provided that all our data structures use + * some mallocators internally... */ -#define MALLOCATOR_IS_ENABLED (MALLOCATOR_IS_WANTED && !MC_is_active()) + + +static int initialization_done = 0; +/** + * This function must be called once the framework configuration is done. If not, + * mallocators will never get used. Check the implementation notes in + * src/xbt/mallocator.c for the justification of this. + * + * For example, surf_config uses this function to tell to the mallocators that + * the simgrid + * configuration is now finished and that it can create them if not done yet */ +void xbt_mallocator_initialization_is_done(void) { + initialization_done = 1; +} + +/** used by the module to know if it's time to activate the mallocators yet */ +static XBT_INLINE int xbt_mallocator_is_active(void) { +#ifndef MALLOCATOR_COMPILED_IN + return 0; +#else + return initialization_done && !MC_is_active(); +#endif +} /** * \brief Constructor @@ -50,21 +90,13 @@ xbt_mallocator_t xbt_mallocator_new(int size, m = xbt_new0(s_xbt_mallocator_t, 1); XBT_VERB("Create mallocator %p (%s)", - m, MALLOCATOR_IS_ENABLED ? "enabled" : "disabled"); + m, xbt_mallocator_is_active() ? "enabled" : "disabled"); m->current_size = 0; m->new_f = new_f; m->free_f = free_f; m->reset_f = reset_f; + m->max_size = size; - if (MALLOCATOR_IS_ENABLED) { - m->objects = xbt_new0(void *, size); - m->max_size = size; - m->mutex = xbt_os_mutex_init(); - } else { - m->objects = NULL; - m->max_size = 0; - m->mutex = NULL; - } return m; } @@ -112,7 +144,7 @@ void *xbt_mallocator_get(xbt_mallocator_t m) { void *object; - if (MALLOCATOR_IS_ENABLED) { + if (m->objects != NULL) { // this mallocator is active, stop thinking and go for it! xbt_os_mutex_acquire(m->mutex); if (m->current_size <= 0) { /* No object is ready yet. Create a bunch of them to try to group the @@ -133,7 +165,14 @@ void *xbt_mallocator_get(xbt_mallocator_t m) object = m->objects[--m->current_size]; xbt_os_mutex_release(m->mutex); } else { - object = m->new_f(); + if (xbt_mallocator_is_active()) { + // We have to switch this mallocator from inactive to active (and then get an object) + m->objects = xbt_new0(void *, m->max_size); + m->mutex = xbt_os_mutex_init(); + return xbt_mallocator_get(m); + } else { + object = m->new_f(); + } } if (m->reset_f) @@ -156,7 +195,7 @@ void *xbt_mallocator_get(xbt_mallocator_t m) */ void xbt_mallocator_release(xbt_mallocator_t m, void *object) { - if (MALLOCATOR_IS_ENABLED) { + if (m->objects != NULL) { // Go for it xbt_os_mutex_acquire(m->mutex); if (m->current_size < m->max_size) { /* there is enough place to push the object */ @@ -173,6 +212,13 @@ void xbt_mallocator_release(xbt_mallocator_t m, void *object) m->free_f(object); } } else { - m->free_f(object); + if (xbt_mallocator_is_active()) { + // We have to switch this mallocator from inactive to active (and then store that object) + m->objects = xbt_new0(void *, m->max_size); + m->mutex = xbt_os_mutex_init(); + xbt_mallocator_release(m,object); + } else { + m->free_f(object); + } } } -- 2.20.1