UCP/PROTO: Report single lane performance until multi_lane_thresh
#10327
Conversation
| ucp_proto_common_tl_perf_t *lane_perf; | ||
| ucp_proto_multi_lane_priv_t *lpriv; | ||
| ucp_lane_index_t lane; | ||
| size_t min_length = 0; |
There was a problem hiding this comment.
code style: normally variables with initialization should come first
src/ucp/proto/proto_multi.c
Outdated
| @@ -191,12 +150,12 @@ ucs_status_t ucp_proto_multi_init(const ucp_proto_multi_init_params_t *params, | |||
| /* Make sure fragment is not zero */ | |||
| ucs_assert(max_frag > 0); | |||
|
|
|||
| lpriv->max_frag = max_frag; | |||
| perf.max_frag += max_frag; | |||
| lpriv->max_frag = max_frag; | |||
There was a problem hiding this comment.
minor: align by =, like in the rest of the code
| ucp_md_map_t reg_md_map, const char *perf_name, | ||
| ucp_proto_perf_t *perf) | ||
| { | ||
| ucp_proto_common_init_params_t single_lane_params = params->super; |
There was a problem hiding this comment.
Do we make a copy on purpose?
There was a problem hiding this comment.
Yes, we make copy to change it later
src/ucp/proto/proto_multi.c
Outdated
| ucp_proto_perf_t **perf_p, | ||
| ucp_proto_multi_priv_t *mpriv) | ||
| static ucp_lane_map_t | ||
| ucp_proto_multi_filter_slow_lanes(ucp_lane_index_t *lanes, |
There was a problem hiding this comment.
lanes and lanes_perf can be const?
| &lanes_perf_nodes[lane]); | ||
| if (status != UCS_OK) { | ||
| return status; | ||
| if (ucs_popcount(lane_map) == max_lanes) { |
There was a problem hiding this comment.
minor: maybe?
for (i = 0; (i < num_lanes) && (ucs_popcount(lane_map) < max_lanes); ++i) {There was a problem hiding this comment.
I intentionally didn't made this by this way since I think the current one is more readable
| return lane_map; | ||
| } | ||
|
|
||
| static ucp_proto_common_tl_perf_t |
There was a problem hiding this comment.
pass some params by const
Returning by value is intentional?
There was a problem hiding this comment.
Done, thanks
Yes, it is intentional
| @@ -206,6 +207,7 @@ ucp_proto_get_offload_zcopy_probe(const ucp_proto_init_params_t *init_params) | |||
| .super.reg_mem_info = ucp_proto_common_select_param_mem_info( | |||
| init_params->select_param), | |||
| .max_lanes = context->config.ext.max_rma_lanes, | |||
| .multi_lane_thresh = UCP_MIN_BCOPY, | |||
There was a problem hiding this comment.
Because ucp_proto_multi_max_payload always use only 1 lane until this size
| @@ -259,6 +260,7 @@ ucp_proto_put_offload_zcopy_probe(const ucp_proto_init_params_t *init_params) | |||
| .super.reg_mem_info = ucp_proto_common_select_param_mem_info( | |||
| init_params->select_param), | |||
| .max_lanes = context->config.ext.max_rma_lanes, | |||
| .multi_lane_thresh = UCP_MIN_BCOPY, | |||
There was a problem hiding this comment.
Because ucp_proto_multi_max_payload always use only 1 lane until this size
| @@ -32,6 +32,7 @@ static void ucp_rndv_am_probe_common(ucp_proto_multi_init_params_t *params) | |||
| params->middle.lane_type = UCP_LANE_TYPE_AM_BW; | |||
| params->super.hdr_size = sizeof(ucp_request_data_hdr_t); | |||
| params->max_lanes = context->config.ext.max_rndv_lanes; | |||
| params->multi_lane_thresh = UCP_MIN_BCOPY; | |||
There was a problem hiding this comment.
do we have multi-lane support for AM rndv?
There was a problem hiding this comment.
Yes, we have
static void ucp_rndv_am_probe_common(ucp_proto_multi_init_params_t *params)
{
...
params->max_lanes = context->config.ext.max_rndv_lanes;
....
}
What?
Report one lane performance until certain threshold for multi-lane protocols.
Why?
All the multi-lane protocols have their own thresholds and before that thresholds protocols use only one lane. That PR represents this information in the perfomance structure.
Should fix (RM3606445)[https://redmine.mellanox.com/issues/3606445]