UCP/WIREUP: Consider BW for AM/keepalive lanes with same latency in selection#11503
UCP/WIREUP: Consider BW for AM/keepalive lanes with same latency in selection#11503shasson5 wants to merge 4 commits into
Conversation
shasson5
force-pushed
the
umbriel
branch
5 times, most recently
from
5100c56 to
a88d0ed
Compare
shasson5
changed the title
|
🤖 Starting review — findings will be posted here when done. |
svc-nvidia-pr-review
left a comment
svc-nvidia-pr-review
left a comment
There was a problem hiding this comment.
Code Review
6 findings —
6 findings posted as inline comments.
There was a problem hiding this comment.
| * Copyright (c) NVIDIA CORPORATION & AFFILIATES, 2001-2026. ALL RIGHTS RESERVED. |
There was a problem hiding this comment.
| * Copyright (c) NVIDIA CORPORATION & AFFILIATES, 2024-2026. ALL RIGHTS RESERVED. |
| #include <ucs/sys/sock.h> | ||
| #include <ucp/core/ucp_ep.inl> | ||
| #include <string.h> | ||
| #include <inttypes.h> |
There was a problem hiding this comment.
| #include <inttypes.h> | |
| #include <math.h> |
Explicit include for fabs
| int cand_prio, | ||
| const ucp_wireup_select_info_t *sel) | ||
| { | ||
| double ref_score = ucs_min(cand_score, sel->score); |
There was a problem hiding this comment.
I think it's problematic that the ref_score keeps changing while iterating over candidates.
This makes the selection depend on ordering.
Maybe we should find the ref_score in a first pass, and then compare to it in a second pass?
There was a problem hiding this comment.
IMO it overcomplicates the code and not worth it for a minor corner case.
| criteria.local_md_flags = 0; | ||
| criteria.is_keepalive = 1; | ||
| criteria.calc_score = ucp_wireup_keepalive_score_func; | ||
| criteria.calc_tiebreak = ucp_wireup_tiebreak_func; |
There was a problem hiding this comment.
The PR says it's only for AM, should the title/description also mention keepalive?
shasson5
changed the title
shasson5
changed the title
| * (then priority) decides. Returns >0 if the candidate is better, <0 if worse, | ||
| * 0 if equal. | ||
| */ | ||
| static int ucp_wireup_candidate_cmp(double cand_score, double cand_tiebreak, |
There was a problem hiding this comment.
candidate, don't use abbreviations
| * (then priority) decides. Returns >0 if the candidate is better, <0 if worse, | ||
| * 0 if equal. | ||
| */ | ||
| static int ucp_wireup_candidate_cmp(double cand_score, double cand_tiebreak, |
There was a problem hiding this comment.
This func is nor order-dependent.
Meaning, with multiple candidates inside the "window", the lane will be chosen arbitrarily based on bitmap/iter order.
Maybe better find best primary score, then use tiebreak.
| /* best for 4k messages */ | ||
| double local_bw; | ||
|
|
||
| if (unpacked_addr->dst_version < 17) { |
There was a problem hiding this comment.
Remote BW from addr v2 is FP8-unpacked, while local BW here remains exact. Nearby code packs/unpacks the local value for symmetry; the AM/keepalive tiebreak should do the same.
| criteria.local_md_flags = 0; | ||
| criteria.is_keepalive = 1; | ||
| criteria.calc_score = ucp_wireup_keepalive_score_func; | ||
| criteria.calc_tiebreak = ucp_wireup_tiebreak_func; |
There was a problem hiding this comment.
New tiebreak for KA, but the PR tests only AM send selection. Plz add coverage for KA candidates, (w max_inflight_eps..)
4 participants
What?
UCP/WIREUP: Consider BW for AM lanes with same latency in selection
Why?
Better lanes selection in wireup process
solve https://redmine.mellanox.com/issues/4958573
How?
Add optional tiebreak score callback in order to calculate a tiebreak value for equal/similar scores