Skip to content
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

Jump to bottom

UCP/PROTO/MULTI: Allow header be greater than max_frag #10263

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
Open

UCP/PROTO/MULTI: Allow header be greater than max_frag #10263

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
ce3f490
UCP/PROTO/MULTI: Allow header be greater than max_frag, sort lanes by BW
iyastreb Oct 28, 2024
cf0ff67
UCP/PROTO/MULTI: Reuse ucp_score_prio_cmp
iyastreb Oct 29, 2024
10d6d3c
UCP/PROTO/MULTI: Addressed PR comments
iyastreb Oct 29, 2024
8c8bf1e
UCP/PROTO/MULTI: Split into separate PRs
iyastreb Oct 30, 2024
897d55c
UCP/PROTO/MULTI: Addressed PR comment
iyastreb Oct 31, 2024
17f82cd
UCP/PROTO/MULTI: Enable skipped tests
iyastreb Oct 31, 2024
c9546a1
UCP/PROTO/MULTI: Use size_t for max_frag
iyastreb Nov 4, 2024
d7fc813
UCP/PROTO/MULTI: Allow zero length payload
iyastreb Nov 6, 2024
e74c74a
UCP/PROTO/MULTI: Fixed build, don't include proto_common.inl in test
iyastreb Nov 6, 2024
8f7d0cf
UCP/PROTO/MULTI: Removed redundant comment
iyastreb Nov 7, 2024
ed9357a
UCP/PROTO/MULTI: Relaxed assertion in iov_next
iyastreb Nov 7, 2024
925fdc9
UCP/PROTO/MULTI: Updated comment
iyastreb Nov 8, 2024
622a5ef
UCP/PROTO/MULTI: Addressed PR comments
iyastreb Nov 11, 2024
573bb76
UCP/PROTO/MULTI: Define enum in proto_am.inl
iyastreb Nov 14, 2024
269b69f
UCP/PROTO/MULTI: Cosmetic change
iyastreb Nov 14, 2024
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
26 changes: 24 additions & 2 deletions src/ucp/proto/proto_multi.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,23 @@
#include "proto_debug.h"
#include "proto_multi.inl"

#include <ucs/algorithm/qsort_r.h>
#include <ucs/debug/assert.h>
#include <ucs/debug/log.h>


static int
ucp_proto_multi_compare_tl_perf(const void *elem1, const void *elem2, void *arg)
{
const ucp_lane_index_t *lane1 = elem1;
const ucp_lane_index_t *lane2 = elem2;
const ucp_proto_common_tl_perf_t *lanes_perf = arg;

/* If bandwidths are equal, prefer to maintain the original ordering */
return ucp_score_prio_cmp(lanes_perf[*lane2].bandwidth, (intptr_t)elem1,
lanes_perf[*lane1].bandwidth, (intptr_t)elem2);
}

ucs_status_t ucp_proto_multi_init(const ucp_proto_multi_init_params_t *params,
const char *perf_name,
ucp_proto_perf_t **perf_p,
Expand Down Expand Up @@ -59,8 +72,7 @@ ucs_status_t ucp_proto_multi_init(const ucp_proto_multi_init_params_t *params,
&params->super, params->first.lane_type, params->first.tl_cap_flags,
1, 0, lanes);
if (num_lanes == 0) {
ucs_trace("no lanes for %s",
ucp_proto_id_field(params->super.super.proto_id, name));
ucs_trace("no lanes for %s", perf_name);
Copy link
Contributor

Choose a reason for hiding this comment

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

it's not the same I'd keep proto_name here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

return UCS_ERR_NO_ELEM;
}

Expand All @@ -86,6 +98,16 @@ ucs_status_t ucp_proto_multi_init(const ucp_proto_multi_init_params_t *params,
max_bandwidth = ucs_max(max_bandwidth, lane_perf->bandwidth);
}

/* Sort lanes by bandwidth */
if (num_lanes > 1) {
/* If type of the first lane is not the same as in the middle, then
* exclude this lane from sorting */
lane = (params->first.lane_type == params->middle.lane_type) ? 0 : 1;
/* Sort lanes array by lanes_perf bandwidth */
Copy link
Contributor

Choose a reason for hiding this comment

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

there is already similar comment on line 101

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

ucs_qsort_r(lanes + lane, num_lanes - lane, sizeof(ucp_lane_index_t),
ucp_proto_multi_compare_tl_perf, lanes_perf);
}

/* Select the lanes to use, and calculate their aggregate performance */
perf.bandwidth = 0;
perf.send_pre_overhead = 0;
Expand Down
7 changes: 5 additions & 2 deletions src/ucp/proto/proto_multi.inl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,8 +66,11 @@ ucp_proto_multi_max_payload(ucp_request_t *req,
size_t max_frag = lpriv->max_frag - hdr_size;
size_t max_payload;

ucs_assertv(lpriv->max_frag > hdr_size, "max_frag=%zu hdr_size=%zu",
lpriv->max_frag, hdr_size);
/* If header is greater than max_frag - that's ok, we allow it, but we send
* only the header data and keep the payload empty */
if (lpriv->max_frag <= hdr_size) {
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe smth like

ssize_t max_frag;
...
max_frag = lpriv->max_frag - hdr_size;
if (max_frag < 0) {
     return 0;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, good idea

Copy link
Contributor Author

@iyastreb iyastreb Nov 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Apparently I'm reverting this change, because signed type ssize_t has only half width of size_t and therefore cannot be reliably used to hold the difference between two size_t variables. The failure is exposed when running test_ucp_rma.put_blocking tests, where lpriv->max_frag is almost equal to SIZE_MAX and hdr_size=0. In that case the difference = SIZE_MAX, but it becomes negative when casted to ssize_t that leads to test failures.

Copy link
Contributor

Choose a reason for hiding this comment

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

AFAIR lpriv->max_frag can be set to lane_perf->max_frag which is a transport limitation, so may there be a situation when some lanes don't really support such big header size so we cannot send even header?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As I see in ucp_worker_set_max_am_header the worker->max_am_header is set so that header can fit in any lane. If user sends something larger than that, then we reject this input:

ucp_am_send_nbx_check_header_length(ucp_worker_h worker, size_t header_length)
{
    if (ENABLE_PARAMS_CHECK && (header_length > worker->max_am_header)) {

Copy link
Contributor

Choose a reason for hiding this comment

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

in this case, better not calculate max_frag = lpriv->max_frag - hdr_size during variable declaration, but do it after checking (lpriv->max_frag <= hdr_size)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

return 0;
}

/* Do not split very small sends to chunks, it's not worth it, and
generic datatype may not be able to pack to a smaller buffer */
Expand Down
Loading