Skip to content
Closed
Show file tree
Hide file tree
Changes from 13 commits
Commits
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
11 changes: 7 additions & 4 deletions src/ucp/wireup/wireup.c
Original file line number Diff line number Diff line change
Expand Up @@ -701,12 +701,12 @@ void ucp_wireup_assign_lane(ucp_ep_h ep, ucp_lane_index_t lane, uct_ep_h uct_ep,
ucs_assert(ucp_wireup_ep_test(ep->uct_eps[lane]));
ucs_trace("ep %p: wireup uct_ep[%d]=%p next set to %p%s", ep, lane,
ep->uct_eps[lane], uct_ep, info);
ucp_wireup_ep_set_next_ep(ep->uct_eps[lane], uct_ep);
ucp_wireup_ep_set_next_ep(ep->uct_eps[lane], uct_ep, 1, 1);
ucp_wireup_ep_remote_connected(ep->uct_eps[lane]);
}
}

static uct_ep_h ucp_wireup_extract_lane(ucp_ep_h ep, ucp_lane_index_t lane)
uct_ep_h ucp_wireup_extract_lane(ucp_ep_h ep, ucp_lane_index_t lane)
{
uct_ep_h uct_ep = ep->uct_eps[lane];

Expand Down Expand Up @@ -868,6 +868,10 @@ ucs_status_t ucp_wireup_resolve_proxy_lanes(ucp_ep_h ep)
iface_attr->cap.am.max_bcopy);
}

ucs_assert((ucp_wireup_ep(ep->uct_eps[proxy_lane]) == NULL) ||
(ucp_wireup_ep(ep->uct_eps[proxy_lane])->flags &
UCP_WIREUP_EP_FLAG_LOCAL_CONNECTED));

/* Create a signaling ep to the proxy lane */
if (proxy_lane == lane) {
/* If proxy is to the same lane, temporarily remove the existing
Expand All @@ -876,7 +880,6 @@ ucs_status_t ucp_wireup_resolve_proxy_lanes(ucp_ep_h ep)
* proxy, so ucp_wireup_extract_lane() handles both cases.
*/
uct_ep = ucp_wireup_extract_lane(ep, proxy_lane);
ucs_assert_always(uct_ep != NULL);
status = ucp_signaling_ep_create(ep, uct_ep, 1, &signaling_ep);
if (status != UCS_OK) {
/* coverity[leaked_storage] */
Expand Down Expand Up @@ -1223,7 +1226,7 @@ ucs_status_t ucp_wireup_connect_remote(ucp_ep_h ep, ucp_lane_index_t lane)
uct_ep_pending_purge(uct_ep, ucp_wireup_connect_remote_purge_cb, &tmp_q);

/* the wireup ep should use the existing [am_lane] as next_ep */
ucp_wireup_ep_set_next_ep(ep->uct_eps[lane], uct_ep);
ucp_wireup_ep_set_next_ep(ep->uct_eps[lane], uct_ep, 1, 1);

if (!(ep->flags & UCP_EP_FLAG_CONNECT_REQ_QUEUED)) {
status = ucp_wireup_send_request(ep);
Expand Down
2 changes: 2 additions & 0 deletions src/ucp/wireup/wireup.h
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,8 @@ ucs_status_t ucp_signaling_ep_create(ucp_ep_h ucp_ep, uct_ep_h uct_ep,
void ucp_wireup_assign_lane(ucp_ep_h ep, ucp_lane_index_t lane, uct_ep_h uct_ep,
const char *info);

uct_ep_h ucp_wireup_extract_lane(ucp_ep_h ep, ucp_lane_index_t lane);

ucs_status_t
ucp_wireup_connect_lane(ucp_ep_h ep, unsigned ep_init_flags,
ucp_lane_index_t lane, unsigned path_index,
Expand Down
97 changes: 74 additions & 23 deletions src/ucp/wireup/wireup_cm.c
Original file line number Diff line number Diff line change
Expand Up @@ -245,24 +245,75 @@ ucp_cm_client_connect_prog_arg_free(ucp_cm_client_connect_progress_arg_t *arg)
ucs_free(arg);
}

static void ucp_cm_client_restore_ep(ucp_wireup_ep_t *wireup_cm_ep,
ucp_ep_h ucp_ep)
/**
* Copies lanes from the one UCP EP to the another UCP EP. The function
* creates new WIREUP EPs for all lanes in @to_ep and sets UCT EP of
* the TLs from @from_ep. Both EPs have to be created and initalized.
* @to_ep should not have initialized the lanes by UCT EPs that will be
* overwritten by UCT EPs from the @from_ep's lanes.
* After copying UCT EPs, @to_ep should try to reconfigure lanes and
* some of the copied UCT EPs could be re-used in the new configuration.
*
* @param [in] to_ep UCP EP handle to copy the lanes to.
* @param [in] from_ep UCP EP handle to copy the lanes from.
* @param [in] change_ownership Make WIREUP EPs in @to_ep are owner for
* copied UCT EPs from @from_ep.
*/
static void ucp_cm_copy_ep_lanes(ucp_ep_h to_ep, ucp_ep_h from_ep,

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.

pls document all the parameters. How the EPs has to be created and initialized before the call? if to_ep is not initialized, then maybe make it [out] parameter of ucp_cm_ep_dup?

@dmitrygx dmitrygx Aug 26, 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.

done, added the description
to_ep has to be initialized, so, no need to duplicate it here

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.

so in this case here should be 2 iterators and to_ep should be reconfigured since to_ep can have some lanes initialized.

@dmitrygx dmitrygx Aug 26, 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.

we have assert that to_ep doesn't have UCT_EPs for the lanes

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.

ok

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.

which is removed?

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.

which is removed?

assert() doesn't work, since configurations are not equal:

  • TMP EP has two lanes: CM lane and transport lane
  • UCP EP has only one lane: CM lane

After we copy UCT EPs from TMP EP to UCP EP, it will do init_lanes() where new config will be selected
The new config could be equal to what we have in TMP EP, then UCT EPs copied to UCP EP will be re-used in the new config. In the current code, it has to be the same.

int change_ownership)
{
ucp_ep_h tmp_ep = wireup_cm_ep->tmp_ep;
ucp_wireup_ep_t *w_ep;
int to_is_owner, from_is_owner;
ucp_lane_index_t lane_idx;
ucs_status_t status;
uct_ep_h uct_ep;
int is_local_connected;
ucp_wireup_ep_t *from_wireup_ep;

for (lane_idx = 0; lane_idx < ucp_ep_num_lanes(tmp_ep); ++lane_idx) {
if (tmp_ep->uct_eps[lane_idx] != NULL) {
ucs_assert(ucp_ep->uct_eps[lane_idx] == NULL);
ucp_ep->uct_eps[lane_idx] = tmp_ep->uct_eps[lane_idx];
w_ep = ucs_derived_of(ucp_ep->uct_eps[lane_idx], ucp_wireup_ep_t);
w_ep->super.ucp_ep = ucp_ep;
to_is_owner = change_ownership;
from_is_owner = !change_ownership;

for (lane_idx = 0; lane_idx < ucp_ep_num_lanes(from_ep); ++lane_idx) {
if ((lane_idx == ucp_ep_get_cm_lane(from_ep)) ||
(from_ep->uct_eps[lane_idx] == NULL)) {
continue;
}

ucs_assert_always(to_ep->uct_eps[lane_idx] == NULL);

uct_ep = ucp_wireup_extract_lane(from_ep, lane_idx);
Comment thread
brminich marked this conversation as resolved.
if (uct_ep == NULL) {
/* UCT EP could be NULL only for non-P2P TLs */
ucs_assert(ucp_worker_is_tl_2iface(from_ep->worker,
ucp_ep_config(from_ep)->key.
lanes[lane_idx].rsc_index));
continue;
}
}

ucp_ep_destroy_base(tmp_ep); /* not needed anymore */
wireup_cm_ep->tmp_ep = NULL;
from_wireup_ep = ucp_wireup_ep(from_ep->uct_eps[lane_idx]);

is_local_connected = (from_wireup_ep == NULL) ||
(from_wireup_ep->flags &
UCP_WIREUP_EP_FLAG_LOCAL_CONNECTED);

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.

should not we inherit other wireup flags? if yes - maybe set them outside of ucp_wireup_ep_set_next_ep or pass instead of is_local_connected?

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.

done


status = ucp_wireup_ep_create(to_ep, &to_ep->uct_eps[lane_idx]);

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.

can we just take existing wireup ep, like it was done before?

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, since we will need this to set who is the owner of the UCT EP- TMP EP or user's UCP EP

if (status != UCS_OK) {
ucs_fatal("%p: failed to create WIREUP EP to wrap %p UCT EP: %s",
to_ep, uct_ep, ucs_status_string(status));
}

ucp_wireup_ep_set_next_ep(to_ep->uct_eps[lane_idx], uct_ep,
to_is_owner, is_local_connected);

if (from_wireup_ep == NULL) {
/* from_ep must be the owner of the UCT EP in this case */

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.

can you please elaborate? why?

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.

done

ucs_assert(from_is_owner);
from_ep->uct_eps[lane_idx] = uct_ep;
} else {
ucs_assert(from_wireup_ep->super.is_owner);
ucp_wireup_ep_set_next_ep(from_ep->uct_eps[lane_idx], uct_ep,
from_is_owner, is_local_connected);
}
}
}

/*
Expand All @@ -275,7 +326,7 @@ static unsigned ucp_cm_client_connect_progress(void *arg)
ucp_worker_h worker = ucp_ep->worker;
ucp_context_h context = worker->context;
uct_ep_h uct_cm_ep = ucp_ep_get_cm_uct_ep(ucp_ep);
ucp_wireup_ep_t *wireup_ep;
ucp_wireup_ep_t *cm_wireup_ep;
ucp_unpacked_address_t addr;
uint64_t tl_bitmap;
ucp_rsc_index_t dev_index;
Expand All @@ -286,9 +337,9 @@ static unsigned ucp_cm_client_connect_progress(void *arg)

UCS_ASYNC_BLOCK(&worker->async);

wireup_ep = ucp_ep_get_cm_wireup_ep(ucp_ep);
ucs_assert(wireup_ep != NULL);
ucs_assert(wireup_ep->ep_init_flags & UCP_EP_INIT_CM_WIREUP_CLIENT);
cm_wireup_ep = ucp_ep_get_cm_wireup_ep(ucp_ep);
ucs_assert(cm_wireup_ep != NULL);
ucs_assert(cm_wireup_ep->ep_init_flags & UCP_EP_INIT_CM_WIREUP_CLIENT);

status = ucp_address_unpack(worker, progress_arg->sa_data + 1,
UCP_ADDRESS_PACK_FLAG_IFACE_ADDR |
Expand All @@ -311,14 +362,14 @@ static unsigned ucp_cm_client_connect_progress(void *arg)
ucp_ep_update_remote_id(ucp_ep, progress_arg->sa_data->ep_id);

/* Get tl bitmap from tmp_ep, because it contains initial configuration. */
tl_bitmap = ucp_ep_get_tl_bitmap(wireup_ep->tmp_ep);
tl_bitmap = ucp_ep_get_tl_bitmap(cm_wireup_ep->tmp_ep);
ucs_assert(tl_bitmap != 0);
rsc_index = ucs_ffs64(tl_bitmap);
dev_index = context->tl_rscs[rsc_index].dev_index;

/* Restore initial configuration from tmp_ep created for packing local
* addresses. */
ucp_cm_client_restore_ep(wireup_ep, ucp_ep);
ucp_cm_copy_ep_lanes(ucp_ep, cm_wireup_ep->tmp_ep, 1);

#ifdef ENABLE_ASSERT
ucs_for_each_bit(rsc_index, tl_bitmap) {
Expand All @@ -327,7 +378,7 @@ static unsigned ucp_cm_client_connect_progress(void *arg)
#endif

tl_bitmap = ucp_context_dev_idx_tl_bitmap(context, dev_index);
status = ucp_wireup_init_lanes(ucp_ep, wireup_ep->ep_init_flags,
status = ucp_wireup_init_lanes(ucp_ep, cm_wireup_ep->ep_init_flags,
tl_bitmap, &addr, addr_indices);
if (status != UCS_OK) {
goto out_free_addr;
Expand All @@ -351,7 +402,7 @@ static unsigned ucp_cm_client_connect_progress(void *arg)
ucs_free(addr.address_list);
out:
if (status != UCS_OK) {
ucp_worker_set_ep_failed(worker, ucp_ep, &wireup_ep->super.super,
ucp_worker_set_ep_failed(worker, ucp_ep, &cm_wireup_ep->super.super,
ucp_ep_get_cm_lane(ucp_ep), status);
}

Expand Down Expand Up @@ -634,7 +685,7 @@ ucs_status_t ucp_ep_client_cm_connect_start(ucp_ep_h ucp_ep,
return status;
}

ucp_wireup_ep_set_next_ep(&wireup_ep->super.super, cm_ep);
ucp_wireup_ep_set_next_ep(&wireup_ep->super.super, cm_ep, 1, 1);
ucp_ep_flush_state_reset(ucp_ep);

return UCS_OK;
Expand Down Expand Up @@ -960,7 +1011,7 @@ ucs_status_t ucp_ep_cm_connect_server_lane(ucp_ep_h ep,
return status;
}

ucp_wireup_ep_set_next_ep(ep->uct_eps[lane], uct_ep);
ucp_wireup_ep_set_next_ep(ep->uct_eps[lane], uct_ep, 1, 1);
return UCS_OK;
}

Expand Down
11 changes: 7 additions & 4 deletions src/ucp/wireup/wireup_ep.c
Original file line number Diff line number Diff line change
Expand Up @@ -439,7 +439,7 @@ ucs_status_t ucp_wireup_ep_connect(uct_ep_h uct_ep, unsigned ep_init_flags,
goto err;
}

ucp_proxy_ep_set_uct_ep(&wireup_ep->super, next_ep, 1);
ucp_wireup_ep_set_next_ep(uct_ep, next_ep, 1, 0);

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.

why needed?

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.

ucp_wireup_ep_set_next_ep() does additional checks


ucs_debug("ep %p: created next_ep %p to %s using " UCT_TL_RESOURCE_DESC_FMT,
ucp_ep, wireup_ep->super.uct_ep, ucp_ep_peer_name(ucp_ep),
Expand Down Expand Up @@ -647,14 +647,17 @@ ucs_status_t ucp_wireup_ep_connect_to_sockaddr(uct_ep_h uct_ep,
return status;
}

void ucp_wireup_ep_set_next_ep(uct_ep_h uct_ep, uct_ep_h next_ep)
void ucp_wireup_ep_set_next_ep(uct_ep_h uct_ep, uct_ep_h next_ep,
int is_owner, int is_local_connected)
{
ucp_wireup_ep_t *wireup_ep = ucp_wireup_ep(uct_ep);

ucs_assert(wireup_ep != NULL);
ucs_assert(wireup_ep->super.uct_ep == NULL);
wireup_ep->flags |= UCP_WIREUP_EP_FLAG_LOCAL_CONNECTED;
ucp_proxy_ep_set_uct_ep(&wireup_ep->super, next_ep, 1);
if (is_local_connected) {
wireup_ep->flags |= UCP_WIREUP_EP_FLAG_LOCAL_CONNECTED;
}

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.

why need this? can just call ucp_proxy_ep_set_uct_ep instead like it is done now

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.

I wanted to use ucp_wireup_ep_set_next_ep() akways instead of:

if (local_conencted) {
    ucp_wireup_ep_set_next_ep();
} else {
    ucp_proxy_ep_set_uct_ep();
}

ucp_proxy_ep_set_uct_ep(&wireup_ep->super, next_ep, is_owner);
}

uct_ep_h ucp_wireup_ep_extract_next_ep(uct_ep_h uct_ep)
Expand Down
3 changes: 2 additions & 1 deletion src/ucp/wireup/wireup_ep.h
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,8 @@ ucs_status_t
ucp_wireup_ep_connect_aux(ucp_wireup_ep_t *wireup_ep, unsigned ep_init_flags,
const ucp_unpacked_address_t *remote_address);

void ucp_wireup_ep_set_next_ep(uct_ep_h uct_ep, uct_ep_h next_ep);
void ucp_wireup_ep_set_next_ep(uct_ep_h uct_ep, uct_ep_h next_ep,
int is_owner, int is_local_connected);

uct_ep_h ucp_wireup_ep_extract_next_ep(uct_ep_h uct_ep);

Expand Down