[ofa-general] Re: [PATCH] Return single PR record for SubnAdmGet

[Sasha Khapyorsky]

Hi Eli,

On 14:54 Wed 03 Jun     , Eli Dorfman (Voltaire) wrote:
> Return single PR record for SubnAdmGet
> 
> Fix to return single PathRecord for SubnAdmGet when SGID and/or DGID
> are 0 in SA component mask (wildcard)
> Also avoid iterating beyond requested number of paths.

I agree with the idea. Couple of comments are below.

> 
> Signed-off-by: Eli Dorfman <elid at voltaire.com>
> ---
>  opensm/opensm/osm_sa_path_record.c |   29 ++++++++++++++++++++---------
>  1 files changed, 20 insertions(+), 9 deletions(-)
> 
> diff --git a/opensm/opensm/osm_sa_path_record.c b/opensm/opensm/osm_sa_path_record.c
> index 6e7d5f6..531115c 100644
> --- a/opensm/opensm/osm_sa_path_record.c
> +++ b/opensm/opensm/osm_sa_path_record.c
> @@ -83,6 +83,8 @@ typedef struct osm_path_parms {
>  	boolean_t reversible;
>  } osm_path_parms_t;
>  
> +#define OSM_SA_PR_MAX_NUM_PATH 127

As far as I understand this does nothing with SubnAdmGet(), but instead
will limit number of returned paths for SubnAdmGetTable() when NumbPath
is not selected. According to the spec SA requester shell select NumbPath
when SubnAdmGetTable() is used, but now OpenSM also supports queries
where NumbPath is not selected. Would we really like to change its
default behavior in this case by limiting number of paths returned?

> +
>  /**********************************************************************
>   **********************************************************************/
>  static inline boolean_t sa_path_rec_is_tavor_port(IN const osm_port_t * p_port)
> @@ -891,7 +893,7 @@ Exit:
>  
>  /**********************************************************************
>   **********************************************************************/
> -static void pr_rcv_get_port_pair_paths(IN osm_sa_t * sa,
> +static int pr_rcv_get_port_pair_paths(IN osm_sa_t * sa,
>  				       IN const osm_madw_t * p_madw,
>  				       IN const osm_port_t * p_req_port,
>  				       IN const osm_port_t * p_src_port,
> @@ -1020,14 +1022,14 @@ static void pr_rcv_get_port_pair_paths(IN osm_sa_t * sa,
>  	   Preferred paths come first in OpenSM
>  	 */
>  	preference = 0;
> -	path_num = 0;
> +	path_num = cl_qlist_count(p_list);
>  
>  	/* If SubnAdmGet, assume NumbPaths 1 (1.2 erratum) */
>  	if (p_sa_mad->method != IB_MAD_METHOD_GET)
>  		if (comp_mask & IB_PR_COMPMASK_NUMBPATH)
>  			iterations = ib_path_rec_num_path(p_pr);
>  		else
> -			iterations = (uintn_t) (-1);
> +			iterations = OSM_SA_PR_MAX_NUM_PATH;
>  	else
>  		iterations = 1;
>  
> @@ -1042,6 +1044,8 @@ static void pr_rcv_get_port_pair_paths(IN osm_sa_t * sa,
>  						     comp_mask, preference);
>  
>  		if (p_pr_item) {
> +			if (p_sa_mad->method == IB_MAD_METHOD_GET && cl_qlist_count(p_list) == 1)
> +				return -1;

Are those lines really needed? 'path_num' is incremented anyway. And the
case 'path_num == iterations' is handled after this loop.

Maybe it is just simple to make this function
(pr_rcv_get_port_pair_paths()) to return path_num? And then to check for
non-zero status below?

Sasha

>  			cl_qlist_insert_tail(p_list, &p_pr_item->list_item);
>  			++path_num;
>  		}
> @@ -1105,6 +1109,8 @@ static void pr_rcv_get_port_pair_paths(IN osm_sa_t * sa,
>  						     comp_mask, preference);
>  
>  		if (p_pr_item) {
> +			if (p_sa_mad->method == IB_MAD_METHOD_GET && cl_qlist_count(p_list) == 1)
> +				return -1;
>  			cl_qlist_insert_tail(p_list, &p_pr_item->list_item);
>  			++path_num;
>  		}
> @@ -1112,6 +1118,7 @@ static void pr_rcv_get_port_pair_paths(IN osm_sa_t * sa,
>  
>  Exit:
>  	OSM_LOG_EXIT(sa->p_log);
> +	return 0;
>  }
>  
>  /**********************************************************************
> @@ -1350,9 +1357,10 @@ static void pr_rcv_process_world(IN osm_sa_t * sa, IN const osm_madw_t * p_madw,
>  	while (p_dest_port != (osm_port_t *) cl_qmap_end(p_tbl)) {
>  		p_src_port = (osm_port_t *) cl_qmap_head(p_tbl);
>  		while (p_src_port != (osm_port_t *) cl_qmap_end(p_tbl)) {
> -			pr_rcv_get_port_pair_paths(sa, p_madw, requester_port,
> +			if (pr_rcv_get_port_pair_paths(sa, p_madw, requester_port,
>  						   p_src_port, p_dest_port,
> -						   p_dgid, comp_mask, p_list);
> +						   p_dgid, comp_mask, p_list) < 0)
> +				return;
>  
>  			p_src_port =
>  			    (osm_port_t *) cl_qmap_next(&p_src_port->map_item);
> @@ -1393,9 +1401,10 @@ static void pr_rcv_process_half(IN osm_sa_t * sa, IN const osm_madw_t * p_madw,
>  		 */
>  		p_port = (osm_port_t *) cl_qmap_head(p_tbl);
>  		while (p_port != (osm_port_t *) cl_qmap_end(p_tbl)) {
> -			pr_rcv_get_port_pair_paths(sa, p_madw, requester_port,
> +			if (pr_rcv_get_port_pair_paths(sa, p_madw, requester_port,
>  						   p_src_port, p_port, p_dgid,
> -						   comp_mask, p_list);
> +						   comp_mask, p_list) < 0)
> +				goto Exit;
>  			p_port = (osm_port_t *) cl_qmap_next(&p_port->map_item);
>  		}
>  	} else {
> @@ -1404,13 +1413,15 @@ static void pr_rcv_process_half(IN osm_sa_t * sa, IN const osm_madw_t * p_madw,
>  		 */
>  		p_port = (osm_port_t *) cl_qmap_head(p_tbl);
>  		while (p_port != (osm_port_t *) cl_qmap_end(p_tbl)) {
> -			pr_rcv_get_port_pair_paths(sa, p_madw, requester_port,
> +			if (pr_rcv_get_port_pair_paths(sa, p_madw, requester_port,
>  						   p_port, p_dest_port, p_dgid,
> -						   comp_mask, p_list);
> +						   comp_mask, p_list) < 0)
> +				goto Exit;
>  			p_port = (osm_port_t *) cl_qmap_next(&p_port->map_item);
>  		}
>  	}
>  
> +Exit:
>  	OSM_LOG_EXIT(sa->p_log);
>  }
>  
> -- 
> 1.5.5
>