UCP/PROTO: Fix RNDV_SCHEME logic #10230

ivankochin · 2024-10-15T10:07:32Z

What

Change RNDV_SCHEME and RNDV_THRESH logic to work properly.

Why ?

Currently if user set certain RNDV_SCHEME without setting RNDV_THRESH this scheme would be forced on all sizes since cfg_thresh for this protocol would be 0. This change fixes that.

How ?

With this change remote variant can report only 2 values as cfg_thresh: AUTO and INF. AUTO reported for:

all protocols if RNDV_SCHEME=auto
protocol that is set in RNDV_SCHEME

INF is reported for all protocols that are not set in RNDV_SCHEME if RNDV_SCHEME is set to some certain protocol.

Manual testing cases:

RNDV_THRESH	RNDV_SCHEME	RESULT
AUTO	AUTO	All protocols are selected basing on perf comparison
AUTO	GET_ZCOPY	No hard thresh, but only GET_ZCOPY protocol participates in proto selection
AUTO	PUT_ZCOPY	No hard thresh, but only GET_ZCOPY protocol participates in proto selection
1024	AUTO	Starting from 1024 RNDV is forced but which RNDV protocol would be used decided by proto selection
1024	GET_ZCOPY	RNDV_GET used starting from 1024
1024	PUT_ZCOPY	RNDV_PUT used starting from 1024
INF	AUTO	RNDV is not used
INF	GET_ZCOPY	RNDV is not used
INF	PUT_ZCOPY	RNDV is not used

ivankochin · 2024-10-15T10:56:54Z

I think it worth covering this logic by tests. If we merge #9989 it would allow to cover it by the following way:

UCS_TEST_P(test_ucp_proto_mock, rndv_thresh, "RNDV_THRESH=1024", "NET_DEVICES=mlx5_0:1")
{
    setup_mock("ib").set_mock_iface_attr("rc_mlx5/mlx5_0:1",
        [](uct_iface_attr_t &iface_attr) {
            iface_attr.dev_num_paths    = 1;
            iface_attr.cap.am.max_short = 208;
            iface_attr.bandwidth.shared = 10000000000;
            iface_attr.latency.c        = 0.000006;
            iface_attr.latency.m        = 0.000000001;
        });

    connect();

    ucp_proto_select_key_t key = any_key();
    key.param.op_id_flags      = UCP_OP_ID_AM_SEND;
    key.param.op_attr          = 0;

    check_ep_config(sender(), {
        {"0",          "200",    "short",                "rc_mlx5/mlx5_0:1"},
        {"201",        "1023",   "copy-in",              "rc_mlx5/mlx5_0:1"},
        {"1024",       "91894",  "rendezvous fragmented copy-in copy-out",
                                                         "rc_mlx5/mlx5_0:1"},
        {"91895",       "inf",   "rendezvous zero-copy read from remote",
                                                         "rc_mlx5/mlx5_0:1"},
    }, key);
}

UCS_TEST_P(test_ucp_proto_mock, rndv_thresh_and_scheme, "RNDV_THRESH=1024", "RNDV_SCHEME=get_zcopy", "NET_DEVICES=mlx5_0:1")
{
    setup_mock("ib").set_mock_iface_attr("rc_mlx5/mlx5_0:1",
        [](uct_iface_attr_t &iface_attr) {
            iface_attr.dev_num_paths    = 1;
            iface_attr.cap.am.max_short = 208;
            iface_attr.bandwidth.shared = 10000000000;
            iface_attr.latency.c        = 0.000006;
            iface_attr.latency.m        = 0.000000001;
        });

    connect();

    ucp_proto_select_key_t key = any_key();
    key.param.op_id_flags      = UCP_OP_ID_AM_SEND;
    key.param.op_attr          = 0;

    check_ep_config(sender(), {
        {"0",          "200",    "short",                "rc_mlx5/mlx5_0:1"},
        {"201",        "1023",   "copy-in",              "rc_mlx5/mlx5_0:1"},
        {"1024",       "inf",    "rendezvous zero-copy read from remote",
                                                         "rc_mlx5/mlx5_0:1"},
    }, key);
}

brminich · 2024-10-15T10:36:25Z

src/ucp/rndv/proto_rndv.inl

-        return UCS_MEMUNITS_INF; /* used only as last resort */
+    if ((context->config.ext.rndv_mode == UCP_RNDV_MODE_AUTO) ||
+        (rndv_modes & UCS_BIT(context->config.ext.rndv_mode))) {
+        return UCS_MEMUNITS_AUTO; /* used only as last resort */


the comment is wrong

brminich · 2024-10-15T10:38:24Z

src/ucp/rndv/proto_rndv.c

-                params->super.cfg_priority, remote_proto->cfg_priority);
+    if (remote_proto->cfg_thresh != UCS_MEMUNITS_AUTO) {
+        /* If RNDV_SCHEME is set, all protocols except forced one reports INF */
+        ucs_assert(remote_proto->cfg_thresh == UCS_MEMUNITS_INF);


src/ucp/rndv/proto_rndv.inl

ivankochin · 2024-10-16T08:49:15Z

src/ucp/rndv/proto_rndv.c

-                params->super.cfg_priority, remote_proto->cfg_priority);
+    if (remote_proto->cfg_thresh != UCS_MEMUNITS_AUTO) {
+        /* If RNDV_SCHEME is set, all protocols except forced one reports INF */
+        ucs_assert(remote_proto->cfg_thresh == UCS_MEMUNITS_INF);


src/ucp/rndv/proto_rndv.c

yosefe · 2024-10-18T08:56:08Z

/azp run UCX PR

azure-pipelines · 2024-10-18T08:56:20Z

Azure Pipelines successfully started running 1 pipeline(s).

ivankochin · 2024-10-21T07:49:31Z

Drawback of the proposed logic is that if defined RNDV_SCHEME protocol is unavailable, other protocols can be selected only as last resort since they would have cfg_tresh==inf.

So if we set RNDV_SCHME=get_zcopy and there are no rma_bw_lanes protocols selection wouldn't be fallen back to rndv/am/bcopy/zcopy and eager will be used for all messages. That can cause many changes in tests which are relates on that fallback.

… iface flags

test/gtest/ucp/test_ucp_sockaddr.cc

brminich · 2024-11-11T15:45:57Z

test/gtest/ucp/test_ucp_am.cc

+    {
+        if (!sender().is_rndv_supported()) {
+            UCS_TEST_MESSAGE << "RNDV is not supported";
+            m_check_recv_rndv_flags = false;


why? what if this is AM-based rndv?
also if rndv is not supported this test suite should be skipped

Maybe message confused you. That case is created to handler AM-based RNDV case. AFAIR AM RNDV doesn't set recv_attr flags.

I can change message here to "RMA is not supported" would it be better from your point of view?

what flag do you mean? AM-based RNDV is using generic flow RTS->RTR->data chunks

My bad, you are right. The thing here is that due to new logic if RNDV_SCHEME=get_zcopy and rndv/get/zcopy is unavailable eager can be selected instead of RNDV.

So we cannot rely on fact that UCP_AM_RECV_ATTR_FLAG_RNDV and UCP_AM_RECV_ATTR_FLAG_DATA will be set in am_data_handler

We can skip these tests if RNDV not supported but I don't think that it is important

UCP/PROTO: Fix RNDV_SCHEME logic

64e1a00

ivankochin self-assigned this Oct 15, 2024

brminich reviewed Oct 15, 2024

View reviewed changes

rakhmets reviewed Oct 15, 2024

View reviewed changes

src/ucp/rndv/proto_rndv.inl Outdated Show resolved Hide resolved

UCP/PROTO: Apply CR comments

b86a367

brminich previously approved these changes Oct 16, 2024

View reviewed changes

ivankochin commented Oct 17, 2024

View reviewed changes

UCP/PROTO: FIx static check

5914432

ivankochin dismissed brminich’s stale review via 5914432 October 17, 2024 09:32

brminich previously approved these changes Oct 17, 2024

View reviewed changes

UCP/TEST: Handle new behavior when RNDV_SCHEME is not supported

ee6bf97

ivankochin dismissed brminich’s stale review via ee6bf97 October 22, 2024 07:21

UCP/TEST: Increase size to meet rndv_get requirement + check required…

8865649

… iface flags

ivankochin commented Oct 23, 2024

View reviewed changes

test/gtest/ucp/test_ucp_sockaddr.cc Outdated Show resolved Hide resolved

ivankochin commented Oct 23, 2024

View reviewed changes

test/gtest/ucp/test_ucp_sockaddr.cc Outdated Show resolved Hide resolved

UCP/PROTO: Increase size to cover all cases of rndv/get min_length

b6cb6ad

ivankochin requested a review from yosefe October 24, 2024 06:51

ivankochin requested a review from rakhmets November 4, 2024 08:13

rakhmets reviewed Nov 6, 2024

View reviewed changes

test/gtest/ucp/test_ucp_sockaddr.cc Outdated Show resolved Hide resolved

test/gtest/ucp/test_ucp_sockaddr.cc Outdated Show resolved Hide resolved

test/gtest/ucp/test_ucp_sockaddr.cc Outdated Show resolved Hide resolved

test/gtest/ucp/test_ucp_sockaddr.cc Outdated Show resolved Hide resolved

UCP/ATOMIC: Apply review fixes

a9b6ef3

rakhmets approved these changes Nov 7, 2024

View reviewed changes

brminich reviewed Nov 11, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UCP/PROTO: Fix RNDV_SCHEME logic #10230

UCP/PROTO: Fix RNDV_SCHEME logic #10230

ivankochin commented Oct 15, 2024 •

edited

Loading

ivankochin commented Oct 15, 2024

brminich Oct 15, 2024

ivankochin Oct 16, 2024

brminich Oct 15, 2024

ivankochin Oct 16, 2024

ivankochin Oct 16, 2024

yosefe commented Oct 18, 2024

azure-pipelines bot commented Oct 18, 2024

ivankochin commented Oct 21, 2024 •

edited

Loading

brminich Nov 11, 2024

ivankochin Nov 12, 2024

brminich Nov 12, 2024

ivankochin Nov 12, 2024

ivankochin Nov 12, 2024

UCP/PROTO: Fix RNDV_SCHEME logic #10230

Are you sure you want to change the base?

UCP/PROTO: Fix RNDV_SCHEME logic #10230

Conversation

ivankochin commented Oct 15, 2024 • edited Loading

What

Why ?

How ?

Manual testing cases:

ivankochin commented Oct 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yosefe commented Oct 18, 2024

azure-pipelines bot commented Oct 18, 2024

ivankochin commented Oct 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivankochin commented Oct 15, 2024 •

edited

Loading

ivankochin commented Oct 21, 2024 •

edited

Loading