Skip to content

UCP/WIREUP: Add flush+destroy of UCT EPs used by TMP EP lanes #5613

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
dmitrygx wants to merge 9 commits into from
Closed

UCP/WIREUP: Add flush+destroy of UCT EPs used by TMP EP lanes #5613

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
7de5445
UCP/WIREUP: Add flush+destroy of UCT EPs used by TMP EP lanes
dmitrygx Aug 24, 2020
631b32c
UCP/WIREUP: Fix review comments
dmitrygx Aug 27, 2020
72190be
UCP/WIREUP: Fix comments
dmitrygx Aug 28, 2020
cdd175c
UCP/WIREUP/CORE: Fix review comments
dmitrygx Aug 28, 2020
4e1c626
UCP/WIREUP: Use dst_dev_index instead of dst_md_index
dmitrygx Aug 31, 2020
88b41c8
UCP/CORE/WIREUP: Fix review comments
dmitrygx Sep 1, 2020
9df2346
UCP/WIREUP/CORE: Fix review comments
dmitrygx Sep 1, 2020
41b63de
Merge branch 'master' into topic/ucp/wireup_tmp_ep
dmitrygx Sep 2, 2020
b94d406
PREPARE
dmitrygx Sep 2, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 9 additions & 7 deletions src/ucp/core/ucp_ep.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1016,13 +1016,15 @@ void ucp_ep_destroy(ucp_ep_h ep)

int ucp_ep_config_lane_is_equal(const ucp_ep_config_key_t *key1,
const ucp_ep_config_key_t *key2,
ucp_lane_index_t lane, int compare_types)
ucp_lane_index_t lane1,
ucp_lane_index_t lane2,
int compare_types)
{
return (key1->lanes[lane].rsc_index == key2->lanes[lane].rsc_index) &&
(key1->lanes[lane].proxy_lane == key2->lanes[lane].proxy_lane) &&
(key1->lanes[lane].dst_md_index == key2->lanes[lane].dst_md_index) &&
(key1->lanes[lane].path_index == key2->lanes[lane].path_index) &&
((key1->lanes[lane].lane_types == key2->lanes[lane].lane_types) ||
return (key1->lanes[lane1].rsc_index == key2->lanes[lane2].rsc_index) &&
(key1->lanes[lane1].proxy_lane == key2->lanes[lane2].proxy_lane) &&

@brminich brminich Aug 28, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comparing lane numbers from different configs is erroneous, also does not seem to be needed

@dmitrygx dmitrygx Aug 28, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ye, fixed
no need this

(key1->lanes[lane1].dst_md_index == key2->lanes[lane2].dst_md_index) &&
(key1->lanes[lane1].path_index == key2->lanes[lane2].path_index) &&
((key1->lanes[lane1].lane_types == key2->lanes[lane2].lane_types) ||

@evgeny-leksikov evgeny-leksikov Aug 28, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

memcmp? or even (key1->lanes[lane1] == key2->lanes[lane2]

@dmitrygx dmitrygx Aug 28, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, wouldn't work since we don't want compare lane_tpyes if it wasn't requested

@evgeny-leksikov evgeny-leksikov Aug 28, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry, missed extra () around lane_types comparison, better don't align different block, maybe even separate by extra line to improve readability?

@evgeny-leksikov evgeny-leksikov Aug 28, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or

if (compare_types) {
    return key1->lanes[lane1] == key2->lanes[lane2];
} else {
    return (key1->lanes[lane].rsc_index    == key2->lanes[lane2].rsc_index)    &&
           (key1->lanes[lane].proxy_lane   == key2->lanes[lane2].proxy_lane)   &&
           (key1->lanes[lane].dst_md_index == key2->lanes[lane2].dst_md_index) &&
           (key1->lanes[lane].path_index   == key2->lanes[lane2].path_index);
}

@dmitrygx dmitrygx Aug 28, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

!compare_types);
Comment thread
evgeny-leksikov marked this conversation as resolved.
Outdated
}

Expand Down Expand Up @@ -1052,7 +1054,7 @@ int ucp_ep_config_is_equal(const ucp_ep_config_key_t *key1,
}

for (lane = 0; lane < key1->num_lanes; ++lane) {
if (!ucp_ep_config_lane_is_equal(key1, key2, lane, 1))
if (!ucp_ep_config_lane_is_equal(key1, key2, lane, lane, 1))
{
return 0;
}
Expand Down
4 changes: 3 additions & 1 deletion src/ucp/core/ucp_ep.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -538,7 +538,9 @@ void ucp_ep_config_cleanup(ucp_worker_h worker, ucp_ep_config_t *config);

int ucp_ep_config_lane_is_equal(const ucp_ep_config_key_t *key1,
const ucp_ep_config_key_t *key2,
ucp_lane_index_t lane, int compare_types);
ucp_lane_index_t lane1,
ucp_lane_index_t lane2,
int compare_types);

int ucp_ep_config_is_equal(const ucp_ep_config_key_t *key1,
const ucp_ep_config_key_t *key2);
Expand Down
20 changes: 5 additions & 15 deletions src/ucp/wireup/wireup.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -292,35 +292,25 @@ ucp_wireup_find_remote_p2p_addr(ucp_ep_h ep, ucp_lane_index_t remote_lane,
return UCS_ERR_UNREACHABLE;
}

static ucp_lane_index_t
ucp_wireup_ep_lane_used_by_another_ep_config(ucp_ep_config_key_t *ep_config_key,
ucp_ep_config_key_t *another_ep_config_key,
ucp_lane_index_t lane)
ucp_lane_index_t
ucp_wireup_ep_configs_use_same_lane(ucp_ep_config_key_t *ep_config_key,
ucp_ep_config_key_t *another_ep_config_key,

@evgeny-leksikov evgeny-leksikov Sep 1, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • just key1 and key2 like ucp_ep_config_lane_tl_is_equal
  • and ucp_wireup_ep_configs_use_same_tl_lane
    to be consistent

@dmitrygx dmitrygx Sep 1, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

ucp_lane_index_t lane)
{
ucp_lane_index_t another_lane;

@evgeny-leksikov evgeny-leksikov Sep 1, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lane_idx shorter and typically used

@dmitrygx dmitrygx Sep 1, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed


for (another_lane = 0; another_lane < another_ep_config_key->num_lanes;
++another_lane) {
if (ucp_ep_config_lane_is_equal(ep_config_key,

@evgeny-leksikov evgeny-leksikov Aug 28, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • this condition has the same result for any another_lane value.
  • whole ucp_wireup_ep_lane_used_by_another_ep_config func looks confusing to me

so since major change is based on this func it should be revised

@dmitrygx dmitrygx Aug 28, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch, fixed

another_ep_config_key,
lane, 0)) {
lane, another_lane, 0)) {
Comment thread
evgeny-leksikov marked this conversation as resolved.
Outdated

@brminich brminich Aug 28, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw, why don't we need to compare types?

@dmitrygx dmitrygx Aug 28, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the can be different, we need to check whether this is the same TL+device pair or not

return another_lane;
}
}

return UCP_NULL_LANE;
}

ucp_lane_index_t ucp_wireup_ep_lane_used_by_another_ep(ucp_ep_h ep,
ucp_ep_h another_ep,
ucp_lane_index_t lane)
{
return ucp_wireup_ep_lane_used_by_another_ep_config(
&ucp_ep_config(ep)->key,
&ucp_ep_config(another_ep)->key,
lane);
}

ucs_status_t
ucp_wireup_connect_local(ucp_ep_h ep,
const ucp_unpacked_address_t *remote_address,
Expand Down
7 changes: 4 additions & 3 deletions src/ucp/wireup/wireup.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,9 +145,10 @@ void ucp_wireup_remote_connected(ucp_ep_h ep);
unsigned ucp_ep_init_flags(const ucp_worker_h worker,
const ucp_ep_params_t *params);

ucp_lane_index_t ucp_wireup_ep_lane_used_by_another_ep(ucp_ep_h ep,
ucp_ep_h another_ep,
ucp_lane_index_t lane);
ucp_lane_index_t
ucp_wireup_ep_configs_use_same_lane(ucp_ep_config_key_t *ep_config_key,
ucp_ep_config_key_t *another_ep_config_key,
ucp_lane_index_t lane);

ucs_status_t
ucp_wireup_connect_local(ucp_ep_h ep,
Expand Down
5 changes: 4 additions & 1 deletion src/ucp/wireup/wireup_ep.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,10 @@ int ucp_wireup_tmp_ep_destroy(ucp_ep_h ep, ucp_wireup_ep_t *wireup_ep,
* EPs have to be destroyed */
for (lane = 0; lane < ucp_ep_num_lanes(tmp_ep); ++lane) {
if (tmp_ep->uct_eps[lane] != NULL) {
found_lane = ucp_wireup_ep_lane_used_by_another_ep(tmp_ep, ep, lane);
found_lane =
ucp_wireup_ep_configs_use_same_lane(&ucp_ep_config(tmp_ep)->key,
&ucp_ep_config(ep)->key,
lane);
if (found_lane != UCP_NULL_LANE) {
uct_ep = tmp_ep->uct_eps[lane];
ucs_assert(ucp_wireup_ep_test(uct_ep) &&
Comment thread
dmitrygx marked this conversation as resolved.
Expand Down