Add a 'hook' framework #2773

jjhursey · 2017-01-20T22:43:29Z

Add a hook static framework to the ompi layer.

Among other uses, this framework is useful as one mechanism for licensing. For licensing it is important that components have the option to be built statically, and not allow the user to force it to be unloaded. The two commits here do just that.

This includes a demo component that provides an example of how the framework might be used.

The framework allows for static components, dynamic components, and dynamic registration of 'hooks' by components in other frameworks. The latter enhancement has proven useful in components that need to know if the application is between MPI_Init and MPI_Finalize.

jjhursey · 2017-01-25T17:40:52Z

ompi/mca/hook/hook.h

+/*
+ * Some functions are specially marked. See decoder below.
+ *
+ * *** Static Only (Allways) ***


SP: always

jjhursey · 2017-01-25T18:08:21Z

Comments from Open MPI face-to-face meeting:

Concern that we are putting in a feature upstream for a component that is not upstream (i.e., licensing example)
By adding this option we are misleading the user when they say -mca fwk foo but component bar is marked as always-consider then they are actually getting foo,bar
- Need to allow the user to know that this is happening. Maybe add a help message to be displayed letting them know that we forced bar to be included.
- Concern about allowing this to be applied to all frameworks.
  - Move the flag to be a hook framework selection only flag?
Use cases mentioned in the room for the always-consider flag: licensing, btl/self, coll/self
- Push back on all but the licensing case

Resolution:

Required / Mandatory flag : Put flag in base, but enforcement in ompi/mca/hook/base, Requires staticly built/linked component.
Suggested / Recommended flag : btl/self, coll/self - Separate PR to add this option.
Disabled flag : Separate PR to add this option

jjhursey · 2017-02-14T22:02:46Z

If the user tries to avoid a required component then a mess like the following is printed just before MPI_Init fails out.

--------------------------------------------------------------------------
Error: A request was made to exclude a hook component from consideration that
is required to be included. This component (noted below) can -not- be excluded
from consideration. The program will fail at this time.
 
Framework: hook
Component: demo
--------------------------------------------------------------------------

jjhursey · 2017-02-14T22:03:57Z

@jsquyres I'm going to ask you to review, if you have time. We'd like to get this into the v2.x series if at all possible.

jsquyres · 2017-02-17T23:19:56Z

ompi/mca/hook/configure.m4

+#
+
+# we only want one :)
+#m4_define(MCA_ompi_hook_CONFIGURE_MODE, STOP_AT_FIRST)


I think you need the delete the above 2 lines.

jsquyres · 2017-02-17T23:21:33Z

ompi/mca/hook/demo/hook_demo_fns.c

+
+void ompi_hook_demo_mpi_initialized_top(int *flag)
+{
+    DEBUG_OUTPUT( mpi_initialized_top );


Did you not want to use __func__ for some reason?

No reason. I suppose I just wanted to make work for myself. :) I'll clean that up on the next iteration.

jsquyres · 2017-02-17T23:24:26Z

opal/mca/mca.h

-      breaking older components. */
-  char reserved[32];
+      breaking older components.
+      NTH/JJH: reduced by 4 bytes for mca_component_flags */


This comment should likely either be explained further or moved to the commit message. I.e., if you look at this comment 12 months from now, will it still be relevant? Or stale? Do we need to know in the code that the padding was reduced by 4? Or only when spelunking through old commits? (I'm guessing the latter)

jsquyres · 2017-02-17T23:26:33Z

ompi/mca/hook/base/hook_base.c

+ * If the framework has not been opened, then we can only use the static components.
+ *
+ * Otherwise we would need to initialize opal outside of ompi_mpi_init and possibly
+ * after ompi_mpi_finalize which gets messy (especially when tring to cleanup).


tring -> trying

jsquyres · 2017-02-17T23:27:34Z

ompi/mca/hook/base/hook_base.c

+ * 2) dynamic components
+ * 3) 'registered' components (those registered by ompi_hook_base_register_callbacks)
+ */
+#define HOOK_CALL_COMMON_IN_MPI(fn_name, ...)                           \


This name (and HOOK_CALL_COMMON_NOT_IN_MPI) is misleading. You call this macro (or the other one) depending on whether the hook framework has been initialized -- not whether we are in MPI or not.

This may be nit-picky, but it also could apply to OSHMEM someday.

jsquyres · 2017-02-17T23:36:06Z

ompi/mca/hook/base/base.h

+
+
+/**
+ * Register another component to be called


Can you put a little more explanation of this functionality here?

jsquyres · 2017-02-17T23:37:13Z

ompi/mca/hook/demo/hook_demo_component.c

+    .hookm_mpi_init_bottom = ompi_hook_demo_extra_mpi_init_bottom,
+};
+
+static int ompi_hook_demo_component_open(void)


Since this is a demo component, can you add some verbose comments here explaining the use of this extra component and the extra registration / deregistration?

jsquyres · 2017-02-17T23:38:09Z

ompi/mca/hook/demo/hook_demo_component.c

+
+#include "ompi_config.h"
+
+#include "hook_demo.h"


Should probably also include opal_output.h in here, since you call opal_output().

jsquyres · 2017-02-17T23:38:17Z

ompi/mca/hook/demo/hook_demo_fns.c

+
+#include "ompi_config.h"
+
+#include "hook_demo.h"


Should probably also include opal_output.h in here, since you call opal_output().

jsquyres · 2017-02-17T23:49:07Z

ompi/mca/hook/demo/hook_demo_fns.c

+
+void ompi_hook_demo_extra_mpi_init_bottom(int argc, char **argv, int requested, int *provided)
+{
+    DEBUG_OUTPUT( mpi_init_bottom (extra) );


I could not find where mpi_init_bottom was defined, nor extra..., but yet this file still compiled for me. Whaaa...? I have to run now and can't investigate further...

I think the macro was masking it by interpreting it as a string. It was a leftover from an older version - I'll clean that out.

jsquyres · 2017-02-17T23:59:19Z

Also, given that this framework will almost certainly be used with out of tree components, there should definitely be thought given to versioning of the components and modules, and also how to expand the component and or module without screwing out-of-tree components.

jsquyres · 2017-02-18T13:34:58Z

One last thought: can you put a README[.md] in the demo component? It would be good to explain a few things to someone who is coming in cold to the hook framework.

* Include a 'demo' component that shows some of the features. * Currently has hooks for: - MPI_Initialized - top, bottom - MPI_Init_thread - top, bottom - MPI_Finalized - top, bottom - MPI_Init - top (pre-opal_init), top (post-opal_init), error, bottom - MPI_Finalize - top, bottom * Other places in ompi can 'register' to hook into any one of these places by passing back a component structure filled with function pointers. * Add a `MCA_BASE_COMPONENT_FLAG_REQUIRED` flag to the MCA structure that is checked by the `hook` framework. If a required, static component has been excluded then the `hook` framework will fail to initialize. - See note in `opal/mca/mca.h` as to why this is checked in the `hook` framework and not in `opal/mca/base/mca_base_component_find.c` Signed-off-by: Joshua Hursey <jhursey@us.ibm.com>

jjhursey · 2017-02-27T18:32:35Z

@jsquyres I just pushed an update that I think addresses all of your comments except the demo/Readme.md. I can try to add that today if you think it's still necessary.

This was delayed a bit because I was experimenting with how best to support forwards compatible components. I have enough of a solution (outside of this PR on a side branch) to feel comfortable moving forward.

jjhursey · 2017-02-28T18:29:26Z

@jsquyres Thanks for the review!

jjhursey added the enhancement label Jan 20, 2017

jjhursey assigned gpaulsen Jan 20, 2017

jjhursey mentioned this pull request Jan 25, 2017

hook/prot: Connectivity Map #2825

Closed

jjhursey commented Jan 25, 2017

View reviewed changes

jjhursey added the ⚠️ WIP-DNM! label Jan 25, 2017

jjhursey force-pushed the topic/hook-fwk branch from c2b6cb4 to e2bc020 Compare February 6, 2017 20:36

jjhursey force-pushed the topic/hook-fwk branch from e2bc020 to 48b7b16 Compare February 14, 2017 21:53

jjhursey added Target: v2.x and removed ⚠️ WIP-DNM! labels Feb 14, 2017

jjhursey requested a review from jsquyres February 14, 2017 22:03

jjhursey force-pushed the topic/hook-fwk branch from 48b7b16 to d90b745 Compare February 14, 2017 22:08

jsquyres requested changes Feb 17, 2017

View reviewed changes

jjhursey force-pushed the topic/hook-fwk branch from d90b745 to 2635a25 Compare February 26, 2017 02:50

jjhursey force-pushed the topic/hook-fwk branch from 2635a25 to c10bbfd Compare February 27, 2017 17:21

jsquyres approved these changes Feb 28, 2017

View reviewed changes

jjhursey merged commit 0006f0d into open-mpi:master Feb 28, 2017

jjhursey deleted the topic/hook-fwk branch February 28, 2017 18:29

jjhursey mentioned this pull request Mar 6, 2017

v3.0.0 feature whitelist #3107

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a 'hook' framework #2773

Add a 'hook' framework #2773

jjhursey commented Jan 20, 2017

jjhursey Jan 25, 2017

jjhursey commented Jan 25, 2017 •

edited by gpaulsen

jjhursey commented Feb 14, 2017

jjhursey commented Feb 14, 2017

jsquyres Feb 17, 2017

jsquyres Feb 17, 2017

jjhursey Feb 26, 2017

jsquyres Feb 17, 2017

jsquyres Feb 17, 2017

jsquyres Feb 17, 2017

jsquyres Feb 17, 2017

jsquyres Feb 17, 2017

jsquyres Feb 17, 2017

jsquyres Feb 17, 2017

jsquyres Feb 17, 2017

jjhursey Feb 26, 2017

jsquyres commented Feb 17, 2017

jsquyres commented Feb 18, 2017

jjhursey commented Feb 27, 2017

jjhursey commented Feb 28, 2017


		#include "ompi_config.h"

		#include "hook_demo.h"


		#include "ompi_config.h"

		#include "hook_demo.h"

Add a 'hook' framework #2773

Add a 'hook' framework #2773

Conversation

jjhursey commented Jan 20, 2017

Choose a reason for hiding this comment

jjhursey commented Jan 25, 2017 • edited by gpaulsen

jjhursey commented Feb 14, 2017

jjhursey commented Feb 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsquyres commented Feb 17, 2017

jsquyres commented Feb 18, 2017

jjhursey commented Feb 27, 2017

jjhursey commented Feb 28, 2017

jjhursey commented Jan 25, 2017 •

edited by gpaulsen