Open MPI logo

Open MPI Development Mailing List Archives

  |   Home   |   Support   |   FAQ   |  

This web mail archive is frozen.

This page is part of a frozen web archive of this mailing list.

You can still navigate around this archive, but know that no new mails have been added to it since July of 2016.

Click here to be taken to the new web archives of this list; it includes all the mails that are in this frozen archive plus all new mails that have been sent to the list since it was migrated to the new archives.

Subject: Re: [OMPI devel] return value of opal_compress_base_register() in opal/mca/compress/base/compress_base_open.c
From: Adrian Reber (adrian_at_[hidden])
Date: 2014-01-07 14:52:46


I have commited fixes for

opal/mca/compress/base/compress_base_open.c and
opal/mca/crs/base/crs_base_open.c

which return OPAL_SUCCESS but do not open the components if CR is
disabled.

                Adrian

On Tue, Jan 07, 2014 at 01:43:00PM -0600, Josh Hursey wrote:
> Either would be fine with me. If you left in the verbose message then it
> might be a bit confusing to any user that might see it.
>
>
> On Fri, Jan 3, 2014 at 9:13 AM, Ralph Castain <rhc_at_[hidden]> wrote:
>
> > That would work. Alternatively, you could try just returning OPAL_SUCCESS
> > in place of OPAL_ERR_NOT_AVAILABLE. This would still avoid opening the
> > components for no reason (thus saving some memory) while not causing
> > opal_init to abort.
> >
> >
> > On Jan 3, 2014, at 3:19 AM, Adrian Reber <adrian_at_[hidden]> wrote:
> >
> > > So removing all output like in this patch would be ok?
> > >
> > > diff --git a/opal/mca/compress/base/compress_base_open.c
> > b/opal/mca/compress/base/compress_base_open.c
> > > index a09fe59..f487752 100644
> > > --- a/opal/mca/compress/base/compress_base_open.c
> > > +++ b/opal/mca/compress/base/compress_base_open.c
> > > @@ -14,13 +14,9 @@
> > >
> > > #include "opal_config.h"
> > >
> > > -#include <string.h>
> > > -#include "opal/mca/mca.h"
> > > #include "opal/mca/base/base.h"
> > > #include "opal/include/opal/constants.h"
> > > -#include "opal/mca/compress/compress.h"
> > > #include "opal/mca/compress/base/base.h"
> > > -#include "opal/util/output.h"
> > >
> > > #include "opal/mca/compress/base/static-components.h"
> > >
> > > @@ -45,13 +41,6 @@ MCA_BASE_FRAMEWORK_DECLARE(opal, compress, NULL,
> > opal_compress_base_register, op
> > >
> > > static int opal_compress_base_register (mca_base_register_flag_t flags)
> > > {
> > > - /* Compression currently only used with C/R */
> > > - if( !opal_cr_is_enabled ) {
> > > - opal_output_verbose(10,
> > opal_compress_base_framework.framework_output,
> > > - "compress:open: FT is not enabled,
> > skipping!");
> > > - return OPAL_ERR_NOT_AVAILABLE;
> > > - }
> > > -
> > > return OPAL_SUCCESS;
> > > }
> > >
> > > @@ -61,13 +50,6 @@ static int opal_compress_base_register
> > (mca_base_register_flag_t flags)
> > > */
> > > int opal_compress_base_open(mca_base_open_flag_t flags)
> > > {
> > > - /* Compression currently only used with C/R */
> > > - if( !opal_cr_is_enabled ) {
> > > - opal_output_verbose(10,
> > opal_compress_base_framework.framework_output,
> > > - "compress:open: FT is not enabled,
> > skipping!");
> > > - return OPAL_SUCCESS;
> > > - }
> > > -
> > > /* Open up all available components */
> > > return mca_base_framework_components_open
> > (&opal_compress_base_framework, flags);
> > > }
> > >
> > >
> > >
> > > On Thu, Jan 02, 2014 at 12:32:32PM -0500, Josh Hursey wrote:
> > >> I think the only reason I protected that framework is to reduce the
> > >> overhead of an application using a build of Open MPI with CR support,
> > but
> > >> no enabling it at runtime. Nothing in the compress framework depends on
> > the
> > >> CR infrastructure (although the CR infrastructure can use the compress
> > >> framework if the user chooses to). So I bet we can remove the protection
> > >> altogether and be fine.
> > >>
> > >> So I think this patch is fine. I might also go as far as removing the
> > 'if'
> > >> block altogether as the protection should not been needed any longer.
> > >>
> > >> Thanks,
> > >> Josh
> > >>
> > >>
> > >>
> > >> On Fri, Dec 27, 2013 at 3:46 PM, Adrian Reber <adrian_at_[hidden]> wrote:
> > >>
> > >>> Right now the C/R code fails because of a change introduced in
> > >>> opal/mca/compress/base/compress_base_open.c in 2013 with commit
> > >>>
> > >>> git 734c724ff76d9bf814f3ab0396bcd9ee6fddcd1b
> > >>> svn r28239
> > >>>
> > >>> Update OPAL frameworks to use the MCA framework system.
> > >>>
> > >>> This commit changed a lot but also the return value of the function
> > from
> > >>> OPAL_SUCCESS to OPAL_ERR_NOT_AVAILABLE. With following patch I can make
> > >>> it work again:
> > >>>
> > >>> diff --git a/opal/mca/compress/base/compress_base_open.c
> > >>> b/opal/mca/compress/base/compress_base_open.c
> > >>> index a09fe59..69eabfa 100644
> > >>> --- a/opal/mca/compress/base/compress_base_open.c
> > >>> +++ b/opal/mca/compress/base/compress_base_open.c
> > >>> @@ -45,11 +45,11 @@ MCA_BASE_FRAMEWORK_DECLARE(opal, compress, NULL,
> > >>> opal_compress_base_register, op
> > >>>
> > >>> static int opal_compress_base_register (mca_base_register_flag_t flags)
> > >>> {
> > >>> /* Compression currently only used with C/R */
> > >>> if( !opal_cr_is_enabled ) {
> > >>> opal_output_verbose(10,
> > >>> opal_compress_base_framework.framework_output,
> > >>> "compress:open: FT is not enabled,
> > >>> skipping!");
> > >>> - return OPAL_ERR_NOT_AVAILABLE;
> > >>> + return OPAL_SUCCESS;
> > >>> }
> > >>>
> > >>> return OPAL_SUCCESS;
> > >>>
> > >>>
> > >>> My question is if OPAL_ERR_NOT_AVAILABLE is indeed the correct return
> > value
> > >>> and the function calling opal_compress_base_register() should actually
> > >>> handle OPAL_ERR_NOT_AVAILABLE or if returning OPAL_SUCCESS is still the
> > >>> right
> > >>> return value?
> > >>>
> > >>> Adrian
> > >>> _______________________________________________
> > >>> devel mailing list
> > >>> devel_at_[hidden]
> > >>> http://www.open-mpi.org/mailman/listinfo.cgi/devel
> > >>>
> > >>
> > >>
> > >>
> > >> --
> > >> Joshua Hursey
> > >> Assistant Professor of Computer Science
> > >> University of Wisconsin-La Crosse
> > >> http://cs.uwlax.edu/~jjhursey
> > >
> > >> _______________________________________________
> > >> devel mailing list
> > >> devel_at_[hidden]
> > >> http://www.open-mpi.org/mailman/listinfo.cgi/devel
> > > _______________________________________________
> > > devel mailing list
> > > devel_at_[hidden]
> > > http://www.open-mpi.org/mailman/listinfo.cgi/devel
> >
> > _______________________________________________
> > devel mailing list
> > devel_at_[hidden]
> > http://www.open-mpi.org/mailman/listinfo.cgi/devel
> >
>
>
>
> --
> Joshua Hursey
> Assistant Professor of Computer Science
> University of Wisconsin-La Crosse
> http://cs.uwlax.edu/~jjhursey

> _______________________________________________
> devel mailing list
> devel_at_[hidden]
> http://www.open-mpi.org/mailman/listinfo.cgi/devel

                Adrian

-- 
Adrian Reber <adrian_at_[hidden]>            http://lisas.de/~adrian/
Peers's Law:
	The solution to a problem changes the nature of the problem.