[ofa-general] Re: [PATCH] opensm: support multiple routers in a subnet

[Hal Rosenstock]

On Mon, 2007-11-19 at 20:18 +0000, Sasha Khapyorsky wrote:
> Hi Rolf,
> 
> On 01:08 Fri 16 Nov     , Rolf Manderscheid wrote:
> > Hi Sasha,
> > 
> > If a path record query is made for an off-subnet DGID, the SA needs to
> > return a path record where the DLID points to the router port that
> > handles the DGID prefix.  In the case of a subnet with only one
> > router, the SA could just pick "the router", and that's exactly what
> > the ROUTER_EXP code did.  However, ROUTER_EXP did not look beyond the
> > first available router.
> > 
> > When additional routers are added, the SA needs more information.  The
> > mechanism for gathering this information has not yet been specified,
> > so in the meantime, this patch adds a configuration file that
> > specifies which router ports handle which prefixes.
> > 
> > The patch also removes all occurrences of ROUTER_EXP ifdefs.  The
> > default behaviour remains unchanged with one minor exception:
> > hop limits are set to 0xFF for path records to multicast DGIDs if
> > the scope is non-local and to unicast DGIDs if off-subnet.
> > This used to happen for ROUTER_EXP only.
> > 
> > Now, the same binary can be configured at run-time to enable the
> > ROUTER_EXP behaviour with a generic configuration file, or to handle
> > multiple routers on a subnet with a more explicit configuration file.
> > See the man page for details.
> > 
> > Signed-off-by: Rolf Manderscheid <rvm at obsidianresearch.com>
> 
> Applied. Thanks.
> 
> I like the idea and the patch.
> 
> However have some mostly minor comments. I will be "out of office" and
> likely off-line for next couple of days, so I will do some changes
> in-place. The rest (less critical) improvements could be added as
> subsequent patch.
> 
> > 
> > --
> > 
> > One consequence of this patch is that people accustomed to using
> > ROUTER_EXP will need to specify a configuration file to get the
> > same behaviour.  I toyed with the idea of keeping one ROUTER_EXP ifdef
> > to control the default behaviour, but then we're back to having two
> > versions of opensm with different default behaviours, and the
> > counter-intuitive: empty cfg file != non-existent cfg file.  One of
> > the goals was to get to a single standard binary.  So, to help avoid
> > surprises, I actually added back one ifdef ROUTER_EXP which causes the
> > compilation to fail with a useful message.  This only helps those who
> > both build and configure their special ROUTER_EXP opensms, but I
> > suspect that's most.
> > 
> > Thanks to Hal for reviewing early versions of this patch and providing
> > feedback.
> > 
> >     Rolf
> > 
> > ---
> > diff --git a/opensm/include/opensm/osm_base.h b/opensm/include/opensm/osm_base.h
> > index aa8d378..db58919 100644
> > --- a/opensm/include/opensm/osm_base.h
> > +++ b/opensm/include/opensm/osm_base.h
> > @@ -253,6 +253,22 @@ BEGIN_C_DECLS
> >  #endif /* __WIN__ */
> >  /***********/
> >  
> > +/****d* OpenSM: Base/OSM_DEFAULT_PREFIX_ROUTES_FILE
> > +* NAME
> > +*	OSM_DEFAULT_PREFIX_ROUTES_FILE
> > +*
> > +* DESCRIPTION
> > +*	Specifies the default prefix routes file name
> > +*
> > +* SYNOPSIS
> > +*/
> > +#ifdef __WIN__
> > +#define OSM_DEFAULT_PREFIX_ROUTES_FILE strcat(GetOsmCachePath(), "osm-prefix-routes.conf")
> > +#else
> > +#define OSM_DEFAULT_PREFIX_ROUTES_FILE "/etc/ofa/opensm-prefix-routes.conf"
> > +#endif
> > +/***********/
> > +
> 
> Recently we switched to using configurable config directory for OpenSM.
> I will add this separately.
> 
> >  /****d* OpenSM: Base/OSM_DEFAULT_SWEEP_INTERVAL_SECS
> >  * NAME
> >  *	OSM_DEFAULT_SWEEP_INTERVAL_SECS
> > diff --git a/opensm/include/opensm/osm_prefix_route.h b/opensm/include/opensm/osm_prefix_route.h
> > new file mode 100644
> > index 0000000..cebd532
> > --- /dev/null
> > +++ b/opensm/include/opensm/osm_prefix_route.h
> > @@ -0,0 +1,52 @@
> > +/*
> > + * Copyright (c) 2004, 2005 Voltaire, Inc. All rights reserved.
> > + * Copyright (c) 2002-2005 Mellanox Technologies LTD. All rights reserved.
> > + * Copyright (c) 1996-2003 Intel Corporation. All rights reserved.
> > + *
> > + * This software is available to you under a choice of one of two
> > + * licenses.  You may choose to be licensed under the terms of the GNU
> > + * General Public License (GPL) Version 2, available from the file
> > + * COPYING in the main directory of this source tree, or the
> > + * OpenIB.org BSD license below:
> > + *
> > + *     Redistribution and use in source and binary forms, with or
> > + *     without modification, are permitted provided that the following
> > + *     conditions are met:
> > + *
> > + *      - Redistributions of source code must retain the above
> > + *        copyright notice, this list of conditions and the following
> > + *        disclaimer.
> > + *
> > + *      - Redistributions in binary form must reproduce the above
> > + *        copyright notice, this list of conditions and the following
> > + *        disclaimer in the documentation and/or other materials
> > + *        provided with the distribution.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> > + * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> > + * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
> > + * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
> > + * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
> > + * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
> > + * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
> > + * SOFTWARE.
> > + *
> > + */
> > +
> > +#ifndef _OSM_PREFIX_ROUTES_H_
> > +#define _OSM_PREFIX_ROUTES_H_
> > +
> > +#include <complib/cl_types_osd.h>
> 
> We are not using *_osd.h header files directly. There should be just
> 
> 	#include <complib/cl_types.h>
> 
> Changing this.
> 
> > +#include <complib/cl_qlist.h>
> > +
> > +typedef struct {
> > +	cl_list_item_t list_item;	/* must be first */
> > +	uint64_t prefix;		/* network order, zero means "any" */
> > +	uint64_t guid;			/* network order, zero means "any" */
> > +} osm_prefix_route_t;
> > +
> > +#ifdef ROUTER_EXP
> > +#error ROUTER_EXP is deprecated, specify prefix routes at runtime instead (see opensm man page for details)
> > +#endif
> > +
> > +#endif /* _OSM_PREFIX_ROUTES_H_ */
> 
> What do you think? Will it be helpful to merge osm_prefix_routes.h and
> osm_router.h (or actually to put 'struct osm_prefix_route' definition
> in osm_router.h)?

I think this will ultimately be a separate SM attribute for the router
so separating this seems better to me.

> When adding new header file - you need to add this to
> include/Makefile.am EXTRA_DIST list, it is in order to not break
> 'make dist' functionality. I'm adding this now.

-- Hal

[snip...]