update to safety valve for MCA callbacks

I used to have a "use_safety_valve" that was meant to conditionally
turn on the extra "unregister" call at MCA .so unload time.  Reviewers
became conserned about whether this was module-safe and wanted locks,
but really the entire "use_safety_valve" bookkeeping is unnecessary.

The logic of a safety valve is to assume bad code (like the oshmem
mismatch) could be using the register/unregister system and thus to
always unconditionally call unregister at unload time, regardless of
whether a nice matched pair of register/deregister already happened or
if we're in the oshmem case where they failed to make such a match.

With that being the design it was just necessary to update the
opal_mem_hooks_unregister_release() function to be safe regardless of
what context it's being called from (eg, if the callback is already
unregistered, or if it was never registered in the first place).

Also for compatibility I switched from directly using
  __attribute__((destructor))
to
  __opal_attribute_destructor__

Signed-off-by: Mark Allen <markalle@us.ibm.com>

Author: markalle

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;