Merge pull request #10728 from markalle/safety_valve_update

update to safety valve for MCA callbacks

Author: gpaulsen

ompi/mca/coll/hcoll/coll_hcoll_module.c

@@ -1,6 +1,6 @@
 /**
  * Copyright (c) 2011 Mellanox Technologies. All rights reserved.
- * Copyright (c) 2016      IBM Corporation.  All rights reserved.
+ * Copyright (c) 2016-2022 IBM Corporation.  All rights reserved.
  * Copyright (c) 2017      The University of Tennessee and The University
  *                         of Tennessee Research Foundation.  All rights
  *                         reserved.
@@ -18,8 +18,6 @@
 #include "coll_hcoll.h"
 #include "coll_hcoll_dtypes.h"
 
-static int use_safety_valve = 0;
-
 int hcoll_comm_attr_keyval;
 int hcoll_type_attr_keyval;
 mca_coll_hcoll_dtype_t zero_dte_mapping;
@@ -331,7 +329,6 @@ mca_coll_hcoll_comm_query(struct ompi_communicator_t *comm, int *priority)
                     cm->using_mem_hooks = 1;
                     opal_mem_hooks_register_release(mca_coll_hcoll_mem_release_cb, NULL);
                     setenv("MXM_HCOLL_MEM_ON_DEMAND_MAP", "y", 0);
-                    use_safety_valve = 1;
                 }
             }
         } else {
@@ -452,9 +449,7 @@ OBJ_CLASS_INSTANCE(mca_coll_hcoll_module_t,
         mca_coll_hcoll_module_construct,
         mca_coll_hcoll_module_destruct);
 
-static void safety_valve(void) __attribute__((destructor));
+static void safety_valve(void) __opal_attribute_destructor__;
 void safety_valve(void) {
-    if (use_safety_valve) {
-        opal_mem_hooks_unregister_release(mca_coll_hcoll_mem_release_cb);
-    }
+    opal_mem_hooks_unregister_release(mca_coll_hcoll_mem_release_cb);
 }

opal/mca/btl/uct/btl_uct_component.c

@@ -19,6 +19,7 @@
  *                         reserved.
  * Copyright (c) 2019-2021 Google, LLC. All rights reserved.
  * Copyright (c) 2019      Intel, Inc.  All rights reserved.
+ * Copyright (c) 2022      IBM Corporation.  All rights reserved.
  * $COPYRIGHT$
  *
  * Additional copyrights may follow
@@ -43,8 +44,6 @@
 #include "btl_uct_am.h"
 #include "btl_uct_device_context.h"
 
-static int use_safety_valve = 0;
-
 static int mca_btl_uct_component_register(void)
 {
     mca_btl_uct_module_t *module = &mca_btl_uct_module_template;
@@ -147,7 +146,6 @@ static int mca_btl_uct_component_open(void)
                 & opal_mem_hooks_support_level()))) {
         ucm_set_external_event(UCM_EVENT_VM_UNMAPPED);
         opal_mem_hooks_register_release(mca_btl_uct_mem_release_cb, NULL);
-        use_safety_valve = 1;
     }
 
     return OPAL_SUCCESS;
@@ -673,9 +671,7 @@ mca_btl_uct_component_t mca_btl_uct_component = {
         .btl_progress = mca_btl_uct_component_progress,
     }};
 
-static void safety_valve(void) __attribute__((destructor));
+static void safety_valve(void) __opal_attribute_destructor__;
 void safety_valve(void) {
-    if (use_safety_valve) {
-        opal_mem_hooks_unregister_release(mca_btl_uct_mem_release_cb);
-    }
+    opal_mem_hooks_unregister_release(mca_btl_uct_mem_release_cb);
 }

opal/mca/common/ucx/common_ucx.c

@@ -7,6 +7,7 @@
  * Copyright (c) 2021      Triad National Security, LLC. All rights
  *                         reserved.
  * Copyright (c) 2022      Google, LLC. All rights reserved.
+ * Copyright (c) 2022      IBM Corporation.  All rights reserved.
  *
  * $COPYRIGHT$
  *
@@ -29,8 +30,6 @@
 #include <stdio.h>
 #include <ucm/api/ucm.h>
 
-static int use_safety_valve = 0;
-
 /***********************************************************************/
 
 extern mca_base_framework_t opal_memory_base_framework;
@@ -156,7 +155,6 @@ OPAL_DECLSPEC void opal_common_ucx_mca_register(void)
             MCA_COMMON_UCX_VERBOSE(1, "%s", "using OPAL memory hooks as external events");
             ucm_set_external_event(UCM_EVENT_VM_UNMAPPED);
             opal_mem_hooks_register_release(opal_common_ucx_mem_release_cb, NULL);
-            use_safety_valve = 1;
         }
     }
 }
@@ -478,9 +476,7 @@ OPAL_DECLSPEC int opal_common_ucx_del_procs(opal_common_ucx_del_proc_t *procs, s
     return opal_common_ucx_mca_pmix_fence(worker);
 }
 
-static void safety_valve(void) __attribute__((destructor));
+static void safety_valve(void) __opal_attribute_destructor__;
 void safety_valve(void) {
-    if (use_safety_valve) {
-        opal_mem_hooks_unregister_release(opal_common_ucx_mem_release_cb);
-    }
+    opal_mem_hooks_unregister_release(opal_common_ucx_mem_release_cb);
 }

opal/mca/rcache/base/rcache_base_create.c

@@ -13,6 +13,7 @@
  * Copyright (c) 2015-2016 Los Alamos National Security, LLC. All rights
  *                         reserved.
  * Copyright (c) 2020      Intel, Inc.  All rights reserved.
+ * Copyright (c) 2022      IBM Corporation.  All rights reserved.
  * $COPYRIGHT$
  *
  * Additional copyrights may follow
@@ -37,8 +38,6 @@
 #include "opal/memoryhooks/memory.h"
 #include "opal/runtime/opal_params.h"
 
-static int use_safety_valve = 0;
-
 mca_rcache_base_module_t *
 mca_rcache_base_module_create(const char *name, void *user_data,
                               struct mca_rcache_base_resources_t *resources)
@@ -72,7 +71,6 @@ mca_rcache_base_module_create(const char *name, void *user_data,
                     opal_leave_pinned = !opal_leave_pinned_pipeline;
                 }
                 opal_mem_hooks_register_release(mca_rcache_base_mem_cb, NULL);
-                use_safety_valve = 1;
             } else if (1 == opal_leave_pinned || opal_leave_pinned_pipeline) {
                 opal_show_help("help-rcache-base.txt", "leave pinned failed", true, name,
                                OPAL_NAME_PRINT(OPAL_PROC_MY_NAME), opal_process_info.nodename);
@@ -125,9 +123,7 @@ int mca_rcache_base_module_destroy(mca_rcache_base_module_t *module)
     return OPAL_ERR_NOT_FOUND;
 }
 
-static void safety_valve(void) __attribute__((destructor));
+static void safety_valve(void) __opal_attribute_destructor__;
 void safety_valve(void) {
-    if (use_safety_valve) {
-        opal_mem_hooks_unregister_release(mca_rcache_base_mem_cb);
-    }
+    opal_mem_hooks_unregister_release(mca_rcache_base_mem_cb);
 }

opal/memoryhooks/memory.c

@@ -12,6 +12,7 @@
  *                         All rights reserved.
  * Copyright (c) 2017      Los Alamos National Security, LLC.  All rights
  *                         reserved.
+ * Copyright (c) 2022      IBM Corporation.  All rights reserved.
  * $COPYRIGHT$
  *
  * Additional copyrights may follow
@@ -55,6 +56,7 @@ static int hooks_support = 0;
 static opal_list_t release_cb_list;
 static opal_atomic_lock_t release_lock;
 static int release_run_callbacks;
+static int is_initialized = false;
 
 /**
  * Finalize the memory hooks subsystem
@@ -93,6 +95,7 @@ int opal_mem_hooks_init(void)
     OBJ_CONSTRUCT(&release_cb_list, opal_list_t);
 
     opal_atomic_lock_init(&release_lock, OPAL_ATOMIC_LOCK_UNLOCKED);
+    is_initialized = true;
 
     /* delay running callbacks until there is something in the
        registration */
@@ -196,11 +199,40 @@ int opal_mem_hooks_unregister_release(opal_mem_hooks_callback_fn_t *func)
     callback_list_item_t *cbitem, *found_item = NULL;
     int ret = OPAL_ERR_NOT_FOUND;
 
+// I've added "is_initialized" to allow this call to be safe even if
+// a memory hooks .so was merely loaded but never used so this file's
+// init function was never called.  I'll give more context, first
+// describing a bug hit in open-shmem:
+//
+// Ordinarily the expected behavior of memhook users is they'd register a
+// callback and then deregister it in a nice matched pair.  And I think
+// most OMPI code does, but the open-shmem code isn't as clear and it
+// was loading a callback and just leaving it loaded after open-shmem was
+// unloaded.  This was a problem because upon every malloc/free/etc we're
+// going to keep trying to call their callback, and the function pointer
+// itself became illegal as soon as the open-shmem shared lib was unloaded.
+// So I figured the best solution was to add a "safety valve" to the callback
+// users where they would have a library-level destructor that un-conditionally
+// adds an extra unregister call regardless of whether the code is already
+// matched or not.
+//
+// With that happening, it's necessary to make sure
+// opal_mem_hooks_unregister_release() is safe when the system isn't
+// initialized and/or if it was initialized but the specified callback
+// is already removed.
+//
+// Note also, the reason for checking "cbitem" before looking at cbitem->cbfunc
+// is when the list is empty the OPAL_LIST_FOREACH() empirically still iterates
+// once and gives a null cbitem.  I'm not sure I like that, but that's what
+// it did so I needed to make that case safe too.
+    if (!is_initialized) {
+        return 0;
+    }
     opal_atomic_lock(&release_lock);
 
     /* make sure the callback isn't already in the list */
     OPAL_LIST_FOREACH (cbitem, &release_cb_list, callback_list_item_t) {
-        if (cbitem->cbfunc == func) {
+        if (cbitem && cbitem->cbfunc == func) {
             opal_list_remove_item(&release_cb_list, (opal_list_item_t *) cbitem);
             found_item = cbitem;
             ret = OPAL_SUCCESS;