On Thu, Dec 11, 2014 at 8:30 PM, Ralph Castain <r...@open-mpi.org> wrote:
> Just to help me understand: I don’t think this change actually changed any > behavior. However, it certainly *allows* a different behavior. Isn’t that > true? > It depends how you look at this. To be extremely clear it prevents the modules from using anything else than their arguments to decide the provided threading model. With the current change, it is possible that some of the modules will continue to follow this "old" behavior, while others might switch to check opal_using_threads to see how they might behave. My point here is not that one is better than the other, just that we inadvertently introduced a possibility for non-consistent behavior. Let me take an example. In the old scheme, the PML was allowed to run each BTL in a separate thread, with absolutely no BTL support for thread safety. Thus, the PML could have managed all the interactions between BTL and requests in an atomic way, without the BTL knowing about. Now, if the BTL make his decision based on the value returned by opal_using_threads this approach is not possible anymore. > If so, I guess the real question is for Pascal at Bull: why do you feel > this earlier setting is required? > This might allow to see if using functions that require protection, such as opal_lifo_push, will work by default or one should use directly their atomic version? George. > > > On Dec 11, 2014, at 4:21 PM, George Bosilca <bosi...@icl.utk.edu> wrote: > > The overall design in OMPI was that no OMPI module should be allowed to > decide if threads are on (thus it should not rely on the value returned by > opal_using_threads > during it's initialization stage). Instead, they should respect the level > of thread support requested as an argument during the initialization step. > > And this is true even for the BTLs. The PML component init function is > propagating the enable_progress_threads and enable_mpi_threads, down to > the BML, and then to the BTL. This 2 variables, enable_progress_threads and > enable_mpi_threads, are exactly what the ompi_mpi_init is using to compute > the the value of the opal) using_thread (and that this patch moved). > > The setting of the opal_using_threads was delayed during the > initialization to ensure that it's value was not used to select a specific > thread-level in any module, a behavior that is allowed now with the new > setting. > > A drastic change in behavior... > > George. > > > On Tue, Dec 9, 2014 at 3:33 AM, Ralph Castain <r...@open-mpi.org> wrote: > >> Kewl - I’ll fix. Thanks! >> >> On Dec 9, 2014, at 12:32 AM, Pascal Deveze <pascal.dev...@bull.net> >> wrote: >> >> Hi Ralph, >> >> This in in the trunk. >> >> *De :* devel [mailto:devel-boun...@open-mpi.org >> <devel-boun...@open-mpi.org>] *De la part de* Ralph Castain >> *Envoyé :* mardi 9 décembre 2014 09:32 >> *À :* Open MPI Developers >> *Objet :* Re: [OMPI devel] Patch proposed: opal_set_using_threads(true) >> in ompi/runtime/ompi_mpi_init.c is called to late >> >> Hi Pascal >> >> Is this in the trunk or in the 1.8 series (or both)? >> >> >> >> On Dec 9, 2014, at 12:28 AM, Pascal Deveze <pascal.dev...@bull.net> >> wrote: >> >> >> In case where MPI is compiled with --enable-mpi-thread-multiple, a call >> to opal_using_threads() always returns 0 in the routine >> btl_xxx_component_init() of the BTLs, event if the application calls >> MPI_Init_thread() with MPI_THREAD_MULTIPLE. >> >> This is because opal_set_using_threads(true) in >> ompi/runtime/ompi_mpi_init.c is called to late. >> >> I propose the following patch that solves the problem for me: >> >> diff --git a/ompi/runtime/ompi_mpi_init.c b/ompi/runtime/ompi_mpi_init.c >> index 35509cf..c2370fc 100644 >> --- a/ompi/runtime/ompi_mpi_init.c >> +++ b/ompi/runtime/ompi_mpi_init.c >> @@ -512,6 +512,13 @@ int ompi_mpi_init(int argc, char **argv, int >> requested, int *provided) >> } >> #endif >> >> + /* If thread support was enabled, then setup OPAL to allow for >> + them. */ >> + if ((OPAL_ENABLE_PROGRESS_THREADS == 1) || >> + (*provided != MPI_THREAD_SINGLE)) { >> + opal_set_using_threads(true); >> + } >> + >> /* initialize datatypes. This step should be done early as it will >> * create the local convertor and local arch used in the proc >> * init. >> @@ -724,13 +731,6 @@ int ompi_mpi_init(int argc, char **argv, int >> requested, int *provided) >> goto error; >> } >> >> - /* If thread support was enabled, then setup OPAL to allow for >> - them. */ >> - if ((OPAL_ENABLE_PROGRESS_THREADS == 1) || >> - (*provided != MPI_THREAD_SINGLE)) { >> - opal_set_using_threads(true); >> - } >> - >> /* start PML/BTL's */ >> ret = MCA_PML_CALL(enable(true)); >> if( OMPI_SUCCESS != ret ) { >> _______________________________________________ >> devel mailing list >> de...@open-mpi.org >> Subscription: http://www.open-mpi.org/mailman/listinfo.cgi/devel >> Link to this post: >> http://www.open-mpi.org/community/lists/devel/2014/12/16459.php >> >> >> _______________________________________________ >> devel mailing list >> de...@open-mpi.org >> Subscription: http://www.open-mpi.org/mailman/listinfo.cgi/devel >> Link to this post: >> http://www.open-mpi.org/community/lists/devel/2014/12/16462.php >> >> >> >> _______________________________________________ >> devel mailing list >> de...@open-mpi.org >> Subscription: http://www.open-mpi.org/mailman/listinfo.cgi/devel >> Link to this post: >> http://www.open-mpi.org/community/lists/devel/2014/12/16463.php >> > > _______________________________________________ > devel mailing list > de...@open-mpi.org > Subscription: http://www.open-mpi.org/mailman/listinfo.cgi/devel > Link to this post: > http://www.open-mpi.org/community/lists/devel/2014/12/16516.php > > > > _______________________________________________ > devel mailing list > de...@open-mpi.org > Subscription: http://www.open-mpi.org/mailman/listinfo.cgi/devel > Link to this post: > http://www.open-mpi.org/community/lists/devel/2014/12/16517.php >
