[ofa-general] RE: [PATCH 2/4 v2] multicast multiplexing - implementation of newinfrastructure

Slava Strebkov slavas at voltaire.com
Mon Jul 20 03:10:58 PDT 2009



-----Original Message-----
From: Sasha Khapyorsky [mailto:sashakvolt at gmail.com] On Behalf Of Sasha
Khapyorsky
Sent: Sunday, July 19, 2009 6:51 PM
To: Slava Strebkov
Cc: general at lists.openfabrics.org
Subject: Re: [PATCH 2/4 v2] multicast multiplexing - implementation of
newinfrastructure

On 13:57 Sun 28 Jun     , Slava Strebkov wrote:
> 
> diff --git a/opensm/include/opensm/osm_mcm_info.h
b/opensm/include/opensm/osm_mcm_info.h
> index dec607f..2310d25 100644
> --- a/opensm/include/opensm/osm_mcm_info.h
> +++ b/opensm/include/opensm/osm_mcm_info.h
> @@ -1,5 +1,5 @@
>  /*
> - * Copyright (c) 2004-2007 Voltaire, Inc. All rights reserved.
> + * Copyright (c) 2004-2009 Voltaire, Inc. All rights reserved.
>   * Copyright (c) 2002-2005 Mellanox Technologies LTD. All rights
reserved.
>   * Copyright (c) 1996-2003 Intel Corporation. All rights reserved.
>   *
> @@ -47,6 +47,7 @@
>  #include <iba/ib_types.h>
>  #include <complib/cl_qlist.h>
>  #include <opensm/osm_base.h>
> +#include <opensm/osm_sa.h>
>  
>  #ifdef __cplusplus
>  #  define BEGIN_C_DECLS extern "C" {
> @@ -71,7 +72,7 @@ BEGIN_C_DECLS
>  *
>  * SYNOPSIS
>  */
> -typedef struct osm_mcm_info {
> +    typedef struct osm_mcm_info {
>  	cl_list_item_t list_item;
>  	ib_net16_t mlid;
>  } osm_mcm_info_t;
> @@ -131,6 +132,34 @@ void osm_mcm_info_delete(IN osm_mcm_info_t *
const p_mcm);
>  *
>  * SEE ALSO
>  *********/
> -
> +/****f* OpenSM: Multicast Group Holder
/osm_mgrp_holder_get_mlid_by_mgid
> +* NAME
> +*       osm_mgrp_holder_get_mlid_by_mgid
> +*
> +* DESCRIPTION
> +*       Searches holder which contains mgroup with given mgid
> +*       Returns mlid of the found holder
> +*
> +* SYNOPSIS
> +*/
> +ib_net16_t osm_mgrp_holder_get_mlid_by_mgid(IN osm_sa_t * sa,
> +					    IN const ib_gid_t * const
p_mgid);

Why *sa and not *subn? And why should it be in osm_mcm_info.h (which
just describes *port*'s mcm related list)?
-This may be moved to another file, or perhaps it's better to create a
new file for this stuff?

> +/*
> +* PARAMETERS
> +*
> +*  p_sa
> +*     [in] Pointer to  sa object
> +*
> +*  p_mgid
> +*     [in] pointer to mgid
> +*
> +* RETURN VALUES
> +*       mlid of found holder, or zero.
> +*
> +* NOTES
> +*
> +* SEE ALSO
> +*
> +*********/
>  END_C_DECLS
>  #endif				/* _OSM_MCM_INFO_H_ */
> diff --git a/opensm/include/opensm/osm_multicast.h
b/opensm/include/opensm/osm_multicast.h
> index 02b63bd..9f0cd96 100644
> --- a/opensm/include/opensm/osm_multicast.h
> +++ b/opensm/include/opensm/osm_multicast.h
> @@ -214,6 +214,8 @@ typedef struct osm_mgrp {
>  	uint32_t last_change_id;
>  	uint32_t last_tree_id;
>  	unsigned full_members;
> +	cl_fmap_item_t mgid_item;
> +	cl_list_item_t mgrp_item;
>  } osm_mgrp_t;
>  /*
>  * FIELDS
> @@ -254,6 +256,12 @@ typedef struct osm_mgrp {
>  *	last_tree_id
>  *		the last change id used for building the current tree.
>  *
> +*	mgid_item
> +*		fleximap item for map linkage, sorted by mgid.
> +*
> +*	mgrp_item
> +*		list item for mgroups pointer list.
> +*
>  * SEE ALSO
>  *********/
>  
> @@ -572,6 +580,295 @@ osm_mgrp_apply_func(const osm_mgrp_t * const
p_mgrp,
>  * SEE ALSO
>  *	Multicast Group
>  *********/
> +/****f* OpenSM: Multicast Group Holder /osm_mgrp_holder_new
> +* NAME
> +*       osm_mgrp_holder_new
> +*
> +* DESCRIPTION
> +*       Allocates and initializes a Multicast Group Holder for use.
> +*
> +* SYNOPSIS
> +*/
> +osm_mgrp_holder_t *osm_mgrp_holder_new(IN osm_subn_t * p_subn,
> +				       IN ib_gid_t * p_mgid,
> +				       IN ib_net16_t mlid);
> +/*
> +* PARAMETERS
> +*       p_subn
> +*               (in) pointer to osm_subnet
> +*       p_mgid
> +*               (in)  pointer to  mgid

What is "mgid" there? "Common" or "real"?

> +*       mlid
> +*               [in] Multicast LID for this multicast group holder.
> +*
> +* RETURN VALUES
> +*       pointer to initialized osm_mgrp_holder_t
> +*       or NULL, if unsuccessful
> +*
> +* SEE ALSO
> +*       Multicast Group Holder, osm_mgrp_holder_delete
> +*********/
> +/****f* OpenSM: Multicast Group Holder /osm_mgrp_holder_delete
> +* NAME
> +*       osm_mgrp_holder_delete
> +*
> +* DESCRIPTION
> +*       Removes  entry from  array of holders
> +*       Removes port from mgroup port list
> +*
> +* SYNOPSIS
> +*/
> +void osm_mgrp_holder_delete(IN osm_subn_t * p_subn, IN ib_net16_t
mlid);
> +/*
> +* PARAMETERS
> +*
> +*  p_subn
> +*     [in] Pointer to  osm_subnet
> +*
> +*  mlid
> +*     [in] holder's mlid
> +*
> +* RETURN VALUES
> +*       None.
> +*
> +* NOTES
> +*
> +* SEE ALSO
> +*
> +*********/
> +/****f* OpenSM: Multicast Group Holder /osm_mgrp_holder_add_mgrp_port
> +* NAME
> +*       osm_mgrp_holder_add_mgrp_port
> +*
> +* DESCRIPTION
> +*       Allocates  osm_mgrp_port_t for new port joined to mgroup with
mlid of this holder,
> +*       (or / and) adds mgroup to mgroup map of  existed
osm_mgrp_port_t object.
> +*
> +* SYNOPSIS
> +*/
> +ib_api_status_t osm_mgrp_holder_add_mgrp_port(IN osm_mgrp_holder_t *
> +					      p_mgrp_holder,
> +					      IN osm_mgrp_t * p_mgrp,
> +					      IN ib_net64_t port_guid);

Why do you need both mgrp_holder and mgrp?

> +/*
> +* PARAMETERS
> +*       p_mgrp_holder
> +*               (in) pointer to osm_mgrp_holder_t
> +*       p_mgrp
> +*               (in)  pointer to  osm_mgrp_t
> +*
> +* RETURN VALUES
> +*       IB_SUCCESS or
> +*       IB_INSUFFICIENT_MEMORY
> +*
> +* SEE ALSO
> +*       Multicast Group Holder, osm_mgrp_holder_delete_mgrp_port
> +*********/
> +
> +/****f* OpenSM: Multicast Group Holder
/osm_mgrp_holder_delete_mgrp_port
> +* NAME
> +*       osm_mgrp_holder_delete_mgrp_port
> +*
> +* DESCRIPTION
> +*       Deletes  osm_mgrp_port_t for specified port
> +*
> +* SYNOPSIS
> +*/
> +void osm_mgrp_holder_delete_mgrp_port(IN osm_mgrp_holder_t *
p_mgrp_holder,
> +				      IN osm_mgrp_t * p_mgrp,
> +				      IN ib_net64_t port_guid);

Ditto.

> +/*
> +* PARAMETERS
> +*       p_mgrp_holder
> +*               [in] Pointer to an osm_mgrp_holder_t object.
> +*
> +*       p_mgrp
> +*               (in) Pointer to osm_mgrp_t object
> +*
> +*       port_guid
> +*               [in] Port guid of the departing port.
> +*
> +* RETURN VALUES
> +*       None.
> +*
> +* NOTES
> +*
> +* SEE ALSO
> +        Multicast Group Holder,osm_holder_add_mgrp_port
> +*********/
> +
> +/****f* OpenSM: Multicast Group Holder /osm_mgrp_holder_remove_port
> +* NAME
> +*       osm_mgrp_holder_remove_port
> +*
> +* DESCRIPTION
> +*       Removes  osm_mgrp_port_t from mgrp_port_map of holder
> +*       Removes port from mgroup port list
> +*
> +* SYNOPSIS
> +*/
> +void osm_mgrp_holder_remove_port(IN osm_subn_t * const p_subn,
> +				 IN osm_log_t * const p_log,
> +				 IN osm_mgrp_holder_t * const
p_mgrp_holder,
> +				 IN const ib_net64_t port_guid);
> +/*
> +* PARAMETERS
> +*
> +*  p_subn
> +*     [in] Pointer to the subnet object
> +*
> +*  p_log
> +*     [in] The log object pointer
> +*
> +*       p_mgrp_holder
> +*               [in] Pointer to an osm_mgrp_holder_t object.
> +*
> +*       port_guid
> +*               [in] Port guid of the departing port.
> +*
> +* RETURN VALUES
> +*       None.
> +*
> +* NOTES
> +*
> +* SEE ALSO
> +*
> +*********/
> +
> +/****f* OpenSM: Multicast Group Holder /osm_mgrp_holder_add_mgrp
> +* NAME
> +*       osm_mgrp_holder_add_mgrp
> +*
> +* DESCRIPTION
> +*       Adds mgroup to holder according to its mgid
> +*
> +*
> +* SYNOPSIS
> +*/
> +void osm_mgrp_holder_add_mgrp(IN osm_mgrp_holder_t * p_mgrp_holder,
> +			      IN osm_mgrp_t * p_mgrp,
> +			      IN osm_log_t * const p_log);
> +/*
> +* PARAMETERS
> +*
> +*       p_mgrp_holder
> +*               [in] Pointer to an osm_mgrp_holder_t object.
> +*
> +*       p_mgrp
> +*               [in] mgroup to add.
> +*
> +* RETURN VALUES
> +*       None.
> +*
> +* NOTES
> +* Updates common_mgid when holder is being reused
> +* SEE ALSO
> +*       Multicast Group Holder,osm_mgrp_holder_delete_mgrp
> +*********/
> +
> +/****f* OpenSM: Multicast Group Holder /osm_mgrp_holder_delete_mgrp
> +* NAME
> +*       osm_mgrp_holder_delete_mgrp
> +*
> +* DESCRIPTION
> +*       Deletes mgroup from holder according to its mgid
> +*
> +*
> +* SYNOPSIS
> +*/
> +void osm_mgrp_holder_delete_mgrp(IN osm_mgrp_holder_t *
p_mgrp_holder,
> +				 IN osm_mgrp_t * p_mgrp);
> +/*
> +* PARAMETERS
> +*
> +*       p_mgrp_holder
> +*               [in] Pointer to an osm_mgrp_holder_t object.
> +*
> +*       p_mgrp
> +*               [in] mgroup to delete.
> +*
> +* RETURN VALUES
> +*       None.
> +*
> +* NOTES
> +*
> +* SEE ALSO
> +*       Multicast Group Holder,osm_mgrp_holder_add_mgrp
> +*********/
> +
> +/*      Multicast Group Holder, osm_mgrp_holder_delete_port
> +*********/

To what is this comment related to?
-to be deleted
> +
> +/****f* OpenSM: Multicast Group Holder
/osm_mgrp_holder_prepare_common_mgid
> +* NAME
> +*       osm_mgrp_holder_prepare_common_mgid
> +*
> +* DESCRIPTION
> +*       Prepares mgid, which is common for all mgroups in this holder
> +*
> +* SYNOPSIS
> +*/
> +void osm_mgrp_holder_prepare_common_mgid(IN const ib_gid_t * const
p_mgid,
> +					 OUT ib_gid_t * p_common_mgid);
> +/*
> +* PARAMETERS
> +*
> +*  	p_mgid
> +*     		[in] Pointer to  mgid
> +*
> +*  	p_common_mgid
> +*     		[out] common mgid
> +*
> +* RETURN VALUES
> +*       None.
> +*
> +* NOTES
> +*
> +* SEE ALSO
> +*
> +*********/
> +/****f* OpenSM: Subnet/osm_get_mgrp_by_mlid
> +* NAME
> +*       osm_get_mgrp_by_mlid
> +*
> +* DESCRIPTION
> +*       The looks for the given multicast group in the subnet table
by mlid.
> +*       NOTE: this code is not thread safe. Need to grab the lock
before
> +*       calling it.
> +*
> +* SYNOPSIS
> +*/
> +static inline
> +    struct osm_mgrp_holder *osm_get_mgrp_holder_by_mlid(osm_subn_t
const
> +							*p_subn,
> +							ib_net16_t mlid)
> +{
> +	return p_subn->mgroup_holders[cl_ntoh16(mlid) -
IB_LID_MCAST_START_HO];
> +}
> +
> +/*
> +* PARAMETERS
> +*       p_subn
> +*               [in] Pointer to an osm_subn_t object
> +*
> +*       mlid
> +*               [in] The multicast group mlid in network order
> +*
> +* RETURN VALUES
> +*       The multicast group structure pointer if found. NULL
otherwise.
> +*********/
> +
> +static inline ib_net16_t osm_mgrp_holder_get_mlid(IN
osm_mgrp_holder_t *
> +						  const p_mgrp_holder)
> +{
> +	return (p_mgrp_holder->mlid);
> +}
> +
> +static inline boolean_t osm_mgrp_holder_is_empty(IN const
osm_mgrp_holder_t *
> +						 const p_mgrp_holder)
> +{
> +	return (cl_qmap_count(&p_mgrp_holder->mgrp_port_map) == 0);
> +}
>  
>  END_C_DECLS
>  #endif				/* _OSM_MULTICAST_H_ */
> diff --git a/opensm/include/opensm/osm_subnet.h
b/opensm/include/opensm/osm_subnet.h
> index 59a32ad..6713d4d 100644
> --- a/opensm/include/opensm/osm_subnet.h
> +++ b/opensm/include/opensm/osm_subnet.h
> @@ -1,5 +1,5 @@
>  /*
> - * Copyright (c) 2004-2008 Voltaire, Inc. All rights reserved.
> + * Copyright (c) 2004-2009 Voltaire, Inc. All rights reserved.
>   * Copyright (c) 2002-2008 Mellanox Technologies LTD. All rights
reserved.
>   * Copyright (c) 1996-2003 Intel Corporation. All rights reserved.
>   * Copyright (c) 2008 Xsigo Systems Inc.  All rights reserved.
> @@ -509,6 +509,7 @@ typedef struct osm_subn {
>  	boolean_t coming_out_of_standby;
>  	unsigned need_update;
>  	void *mgroups[IB_LID_MCAST_END_HO - IB_LID_MCAST_START_HO + 1];
> +	void *mgroup_holders[IB_LID_MCAST_END_HO - IB_LID_MCAST_START_HO
+ 1];
>  } osm_subn_t;
>  /*
>  * FIELDS
> @@ -633,6 +634,10 @@ typedef struct osm_subn {
>  *		Array of pointers to all Multicast Group objects in the
subnet.
>  *		Indexed by MLID offset from base MLID.
>  *
> +*	mgroup_holders
> +*		Array of pointers to all Multicast Group Holder objects
in the subnet.
> +*		Indexed by MLID offset from base MLID.
> +*
>  * SEE ALSO
>  *	Subnet object
>  *********/
> diff --git a/opensm/opensm/osm_multicast.c
b/opensm/opensm/osm_multicast.c
> index ae0a818..d2a19ea 100644
> --- a/opensm/opensm/osm_multicast.c
> +++ b/opensm/opensm/osm_multicast.c
> @@ -44,10 +44,12 @@
>  
>  #include <stdlib.h>
>  #include <string.h>
> +#include <opensm/osm_sa.h>
>  #include <opensm/osm_multicast.h>
>  #include <opensm/osm_mcm_port.h>
>  #include <opensm/osm_mtree.h>
>  #include <opensm/osm_inform.h>
> +#include <opensm/osm_mcm_info.h>
>  #include <arpa/inet.h>
>  
>
/**********************************************************************
> @@ -74,6 +76,31 @@ void osm_mgrp_delete(IN osm_mgrp_t * p_mgrp)
>  	free(p_mgrp);
>  }
>  
> +void osm_mgrp_delete_group(IN osm_subn_t * p_subn, IN osm_mgrp_t *
p_mgrp)
> +{
> +	osm_mcm_port_t *p_mcm_port;
> +	osm_mcm_port_t *p_next_mcm_port;
> +
> +	CL_ASSERT(p_mgrp);
> +
> +	osm_mgrp_holder_t *p_mgrp_holder =
> +	    osm_get_mgrp_holder_by_mlid(p_subn, p_mgrp->mlid);
> +	p_next_mcm_port =
> +	    (osm_mcm_port_t *) cl_qmap_head(&p_mgrp->mcm_port_tbl);
> +	while (p_next_mcm_port !=
> +	       (osm_mcm_port_t *) cl_qmap_end(&p_mgrp->mcm_port_tbl)) {
> +		p_mcm_port = p_next_mcm_port;
> +		p_next_mcm_port =
> +		    (osm_mcm_port_t *)
cl_qmap_next(&p_mcm_port->map_item);
> +		osm_mgrp_holder_delete_mgrp_port(p_mgrp_holder, p_mgrp,
> +
p_mcm_port->port_gid.unicast.
> +						 interface_id);
> +		osm_mcm_port_delete(p_mcm_port);
> +	}
> +	osm_mgrp_holder_delete_mgrp(p_mgrp_holder, p_mgrp);
> +	free(p_mgrp);
> +}
> +
>
/**********************************************************************
>
**********************************************************************/
>  osm_mgrp_t *osm_mgrp_new(IN const ib_net16_t mlid)
> @@ -378,3 +405,203 @@ static osm_mgrp_port_t
*osm_mgrp_port_new(ib_net64_t port_guid)
>  	cl_qlist_init(&p_mgrp_port->mgroups);
>  	return p_mgrp_port;
>  }
> +
> +void osm_mgrp_holder_delete(IN osm_subn_t * p_subn, ib_net16_t mlid)
> +{
> +	osm_mgrp_port_t *p_osm_mgr_port;
> +	cl_map_item_t *p_item;
> +	cl_fmap_item_t *p_fitem;
> +	osm_mgrp_t *p_mgrp;
> +
> +	osm_mgrp_holder_t *p_mgrp_holder =
> +	    p_subn->mgroup_holders[cl_ntoh16(mlid) -
IB_LID_MCAST_START_HO];
> +	p_item = cl_qmap_head(&p_mgrp_holder->mgrp_port_map);
> +	while (p_item != cl_qmap_end(&p_mgrp_holder->mgrp_port_map)) {
> +		p_osm_mgr_port = (osm_mgrp_port_t *) p_item;
> +		cl_qlist_remove_all(&p_osm_mgr_port->mgroups);
> +		cl_qmap_remove_item(&p_mgrp_holder->mgrp_port_map,
p_item);
> +		p_item = cl_qmap_head(&p_mgrp_holder->mgrp_port_map);
> +		free(p_osm_mgr_port);
> +	}

I'm not sure that I understand your data model very well (maybe it would
be a good idea to describe and implement this as separate patch). But do
you have port lists in both mgrp and mgrp_holder? Assuming so, why? This
code looks pretty overloaded.

> +	p_fitem = cl_fmap_head(&p_mgrp_holder->mgrp_map);
> +	while (p_fitem != cl_fmap_end(&p_mgrp_holder->mgrp_map)) {
> +		p_mgrp =
> +		    (osm_mgrp_t *) PARENT_STRUCT(p_fitem, osm_mgrp_t,
> +						 mgid_item);
> +		osm_mgrp_delete_group(p_subn, p_mgrp);
> +		p_fitem = cl_fmap_head(&p_mgrp_holder->mgrp_map);
> +	}
> +	/* destroy the mtree_node structure */
> +	osm_mtree_destroy(p_mgrp_holder->p_root);
> +	p_subn->mgroup_holders[cl_ntoh16(mlid) - IB_LID_MCAST_START_HO]
= NULL;
> +	free(p_mgrp_holder);
> +}
> +
> +osm_mgrp_holder_t *osm_mgrp_holder_new(IN osm_subn_t * p_subn,
> +				       ib_gid_t * p_mgid, ib_net16_t
mlid)
> +{
> +	osm_mgrp_holder_t *p_mgrp_holder;
> +	p_mgrp_holder =
> +	    p_subn->mgroup_holders[cl_ntoh16(mlid) -
IB_LID_MCAST_START_HO] =
> +	    (osm_mgrp_holder_t *) malloc(sizeof(*p_mgrp_holder));
> +	if (!p_mgrp_holder)
> +		return NULL;
> +
> +	memset(p_mgrp_holder, 0, sizeof(*p_mgrp_holder));
> +	p_mgrp_holder->mlid = mlid;
> +	cl_fmap_init(&p_mgrp_holder->mgrp_map, __mgid_cmp);
> +	cl_qmap_init(&p_mgrp_holder->mgrp_port_map);
> +	osm_mgrp_holder_prepare_common_mgid(p_mgid,
> +
&p_mgrp_holder->common_mgid);
> +	return p_mgrp_holder;
> +}
> +
> +ib_net16_t osm_mgrp_holder_get_mlid_by_mgid(IN osm_sa_t * sa,
> +					    IN const ib_gid_t * const
p_mgid)
> +{
> +	int i;
> +	ib_gid_t common_mgid;
> +	osm_mgrp_holder_t *p_mgrp_holder;
> +
> +	OSM_LOG_ENTER(sa->p_log);
> +
> +	osm_mgrp_holder_prepare_common_mgid(p_mgid, &common_mgid);
> +	for (i = 0; i <= sa->p_subn->max_mcast_lid_ho -
IB_LID_MCAST_START_HO;
> +	     i++) {
> +		if (sa->p_subn->mgroup_holders[i]) {
> +			p_mgrp_holder =
> +			    (osm_mgrp_holder_t *)
sa->p_subn->mgroup_holders[i];
> +			if (!memcmp
> +			    (&p_mgrp_holder->common_mgid, &common_mgid,
> +			     sizeof(ib_gid_t))) {
> +				char gid_str[INET6_ADDRSTRLEN];
> +				OSM_LOG(sa->p_log, OSM_LOG_DEBUG,
> +					"Found holder  0x%X for MGID
%s\n",
> +					cl_ntoh16(p_mgrp_holder->mlid),
> +					inet_ntop(AF_INET6, p_mgid->raw,
> +						  gid_str,
sizeof(gid_str)));
> +				OSM_LOG_EXIT(sa->p_log);
> +				return p_mgrp_holder->mlid;
> +			}
> +		}
> +	}

Hmm, why do we need linear search her? What was a purpose of storing
mgrps in fleximap?
-Each fleximap contains mgroups with same MLID.
Linear search compares "common" mgid only.

> +	OSM_LOG_EXIT(sa->p_log);
> +	return 0;
 +}
> +
> +void osm_mgrp_holder_remove_port(osm_subn_t * subn, osm_log_t *
p_log,
> +				 osm_mgrp_holder_t * p_mgrp_holder,
> +				 ib_net64_t port_guid)
> +{
> +	osm_mgrp_t *p_mgrp;
> +	cl_list_item_t *p_item;
> +
> +	OSM_LOG_ENTER(p_log);
> +
> +	osm_mgrp_port_t *p_mgrp_port = (osm_mgrp_port_t *)
> +	    cl_qmap_remove(&p_mgrp_holder->mgrp_port_map, port_guid);
> +	if (p_mgrp_port !=
> +	    (osm_mgrp_port_t *)
cl_qmap_end(&p_mgrp_holder->mgrp_port_map)) {
> +		char gid_str[INET6_ADDRSTRLEN];
> +		OSM_LOG(p_log, OSM_LOG_DEBUG,
> +			"port  0x%" PRIx64 " removed from  mlid 0x%X\n",
> +			port_guid, cl_ntoh16(p_mgrp_holder->mlid));
> +		while ((p_item =
> +			cl_qlist_remove_head(&p_mgrp_port->mgroups)) !=
> +		       cl_qlist_end(&p_mgrp_port->mgroups)) {
> +			p_mgrp =
> +			    (osm_mgrp_t *) PARENT_STRUCT(p_item,
osm_mgrp_t,
> +							 mgrp_item);
> +			OSM_LOG(p_log, OSM_LOG_DEBUG,
> +				"removing mgrp mgid %s from port  0x%"
PRIx64
> +				"\n", inet_ntop(AF_INET6,
> +
p_mgrp->mcmember_rec.mgid.raw,
> +						gid_str,
sizeof(gid_str)),
> +				cl_ntoh64(port_guid));
> +			osm_mgrp_delete_port(subn, p_log, p_mgrp,
port_guid);
> +		}
> +		free(p_mgrp_port);
> +	}
> +	OSM_LOG_EXIT(p_log);
> +}
> +
> +void osm_mgrp_holder_add_mgrp(osm_mgrp_holder_t * p_mgrp_holder,
> +			      osm_mgrp_t * p_mgrp, osm_log_t * p_log)
> +{
> +	cl_fmap_item_t *p_fitem;
> +	char gid_str[INET6_ADDRSTRLEN];
> +
> +	OSM_LOG_ENTER(p_log);
> +
> +	if (p_mgrp_holder->to_be_deleted) {
> +		/* this is re-used mgrp_holder, need to update
common_mgid */
> +
osm_mgrp_holder_prepare_common_mgid(&p_mgrp->mcmember_rec.mgid,
> +						    &p_mgrp_holder->
> +						    common_mgid);
> +	}
> +	p_fitem = cl_fmap_insert(&p_mgrp_holder->mgrp_map,
> +				 &p_mgrp->mcmember_rec.mgid,
> +				 &p_mgrp->mgid_item);

Ok, starting to understand (hopefully :)) - you have fleximap only per
mgrp_holder. Why? Wouldn't it better to have single (per subnet) mgrp
fleximap? I guess this will cover all common search cases and will
simplify a data model dramatically.
- I don't think so. In case of single fleximap search will always take
logN.
Having fleximap divided into groups per MLID will give logN in a worst
case.
Also, keeping mgroups per MLID simplifies queries like saquery -m <MLID>
 
Also (unrelated directly to this patch series). I have some local
cleanup/simplification changes in multicast stuff, in particular it
eliminates the need in all those 'to_be_deleted', 'tree_id', etc. flags.
I think this could help you with simpler implementation.  
Sasha
-Your changes are already in git?
	Slava

> +	CL_ASSERT(p_item == &p_mgrp->mgid_item);
> +
> +	OSM_LOG(p_log, OSM_LOG_DEBUG,
> +		"mgrp with MGID:%s added to holder with mlid = 0x%X\n",
> +		inet_ntop(AF_INET6, p_mgrp->mcmember_rec.mgid.raw,
gid_str,
> +			  sizeof(gid_str)),
cl_ntoh16(p_mgrp_holder->mlid));
> +	p_mgrp_holder->last_change_id++;
> +	OSM_LOG_EXIT(p_log);
> +}
> +
> +ib_api_status_t osm_mgrp_holder_add_mgrp_port(osm_mgrp_holder_t *
p_mgrp_holder,
> +					      osm_mgrp_t * p_mgrp,
> +					      ib_net64_t port_guid)
> +{
> +	osm_mgrp_port_t *p_mgrp_port = (osm_mgrp_port_t *)
> +	    cl_qmap_get(&p_mgrp_holder->mgrp_port_map, port_guid);
> +	if (p_mgrp_port ==
> +	    (osm_mgrp_port_t *)
cl_qmap_end(&p_mgrp_holder->mgrp_port_map)) {
> +		/* new port to mlid */
> +		p_mgrp_port = osm_mgrp_port_new(port_guid);
> +		if (!p_mgrp_port) {
> +			return IB_INSUFFICIENT_MEMORY;
> +		}
> +		cl_qmap_insert(&p_mgrp_holder->mgrp_port_map,
> +			       p_mgrp_port->port_guid,
&p_mgrp_port->guid_item);
> +		cl_qlist_insert_tail(&p_mgrp_port->mgroups,
&p_mgrp->mgrp_item);
> +	} else {
> +		cl_qlist_insert_tail(&p_mgrp_port->mgroups,
&p_mgrp->mgrp_item);
> +	}
> +	return IB_SUCCESS;
> +}
> +
> +void osm_mgrp_holder_delete_mgrp_port(osm_mgrp_holder_t *
p_mgrp_holder,
> +				      osm_mgrp_t * p_mgrp, ib_net64_t
port_guid)
> +{
> +	osm_mgrp_port_t *p_mgrp_port = (osm_mgrp_port_t *)
> +	    cl_qmap_get(&p_mgrp_holder->mgrp_port_map, port_guid);
> +	if (p_mgrp_port !=
> +	    (osm_mgrp_port_t *)
cl_qmap_end(&p_mgrp_holder->mgrp_port_map)) {
> +		cl_qlist_remove_item(&p_mgrp_port->mgroups,
&p_mgrp->mgrp_item);
> +		if (0 == cl_qlist_count(&p_mgrp_port->mgroups)) {
> +			/* No mgroups registered on this port for
current mlid */
> +
cl_qmap_remove_item(&p_mgrp_holder->mgrp_port_map,
> +					    &p_mgrp_port->guid_item);
> +			free(p_mgrp_port);
> +		}
> +		p_mgrp_holder->last_change_id++;
> +	}
> +}
> +
> +void osm_mgrp_holder_delete_mgrp(osm_mgrp_holder_t * p_mgrp_holder,
> +				 osm_mgrp_t * p_mgrp)
> +{
> +	p_mgrp->to_be_deleted = 1;
> +	cl_fmap_remove_item(&p_mgrp_holder->mgrp_map,
&p_mgrp->mgid_item);
> +	if (0 == cl_fmap_count(&p_mgrp_holder->mgrp_map)) {
> +		/* No more mgroups on this mlid */
> +		p_mgrp_holder->to_be_deleted = 1;
> +		p_mgrp_holder->last_tree_id = 0;
> +		p_mgrp_holder->last_change_id = 0;
> +		memset(&p_mgrp_holder->common_mgid, 0,
sizeof(ib_gid_t));
> +	}
> +}
> -- 
> 1.5.5
> 



More information about the general mailing list