[ofa-general] ***SPAM*** Re: [PATCH] Enhance min hops counters in Ftree

[Sasha Khapyorsky]

Hi Nicolas,

On 16:22 Mon 23 Mar     , Nicolas Morey Chaisemartin wrote:
> This patch enhances the use of the min hop table done in the Fat-Tree algorithm.
> Before this patch, the algorithm was using the osm_sw hops table to store the minhop values toward any lid (Switch or not).
> As this table is allocated as we need it, it required a lot of malloc calls and quite some time to set the hops values on remote ports.
> 
> This patch corrects this behaviour:
> -The osm_sw hops table is only used for switch lid
> -ftree_sw_t struct now has its own hop table (only 1 dimensionnal as we don't need to know which port is used) to store its minhop value
> 
> Signed-off-by: Nicolas Morey-Chaisemartin <nicolas.morey-chaisemartin at ext.bull.net>
> ---
> Here are some performances tests I have realized with and without this patch:
> For ~3500 nodes:
> Without = 0.981s / 169.5MB used memory
> With = 0.549s / 123MB used memory
> 
> For over 5000 nodes:
> Without = 2.25s / 308MB used memory
> With = 1.29s / 186MB used memory
> 
> Computation time is taken in do_ouring function (thus only taking computation not LFT setting time or anything else)

Nice numbers!

However I have a question below.

> 
>  opensm/opensm/osm_ucast_ftree.c |   68 +++++++++++++++++++++++++++++++--------
>  1 files changed, 54 insertions(+), 14 deletions(-)
> 
> diff --git a/opensm/opensm/osm_ucast_ftree.c b/opensm/opensm/osm_ucast_ftree.c
> index 58d1c14..a5da571 100644
> --- a/opensm/opensm/osm_ucast_ftree.c
> +++ b/opensm/opensm/osm_ucast_ftree.c
> @@ -172,6 +172,7 @@ typedef struct ftree_sw_t_ {
>  	uint8_t up_port_groups_num;
>  	boolean_t is_leaf;
>  	unsigned down_port_groups_idx;
> +	uint8_t *hops;
>  } ftree_sw_t;
>  
>  /***************************************************
> @@ -553,6 +554,9 @@ static ftree_sw_t *sw_create(IN ftree_fabric_t * p_ftree,
>  
>  	/* initialize lft buffer */
>  	memset(p_osm_sw->new_lft, OSM_NO_PATH, IB_LID_UCAST_END_HO + 1);
> +	p_sw->hops =
> +	    malloc(cl_ntoh16(p_osm_sw->max_lid_ho) * sizeof(*(p_sw->hops)));
> +	memset(p_sw->hops, OSM_NO_PATH, cl_ntoh16(p_osm_sw->max_lid_ho));

AFAIR p_osm_sw->max_lid_ho is already in host byte order and cl_ntoh16()
should swap bytes, how then did it work?

Also malloc() may fail and return value check is needed.

Sasha