UCT/IB/DC/RC: Purge internal pending ops to avoid dispatching them when EP failed by dmitrygx · Pull Request #7711 · openucx/ucx

dmitrygx · 2021-11-25T17:07:31Z

What

Purge internal pending ops to avoid dispatching them when EP failed.

Why ?

Fixes:

[swx-ucx08:862  :0:862]   rc_mlx5.inl:457  Assertion `!(txwq->flags & UCT_IB_MLX5_TXWQ_FLAG_FAILED)' failed

/hpc/mtr_scrap/users/dmitrygla/ucx/src/uct/ib/rc/accel/rc_mlx5.inl: [ uct_rc_mlx5_common_post_send() ]
      ...
      454     if (opcode != MLX5_OPCODE_NOP) {
      455         /* If FAILED, allow only NOP sends to be posted (used by endpoint
      456          * flush operations) */
==>   457         ucs_assert(!(txwq->flags & UCT_IB_MLX5_TXWQ_FLAG_FAILED));
      458     }
      459
      460     ctrl = txwq->curr;

==== backtrace (tid:    862) ====
 0 0x0000000000089e48 uct_rc_mlx5_common_post_send()  /hpc/mtr_scrap/users/dmitrygla/ucx/src/uct/ib/rc/accel/rc_mlx5.inl:457
 1 0x0000000000089e48 uct_rc_mlx5_txqp_inline_post()  /hpc/mtr_scrap/users/dmitrygla/ucx/src/uct/ib/rc/accel/rc_mlx5.inl:617
 2 0x0000000000089e48 uct_rc_mlx5_ep_post_check()  /hpc/mtr_scrap/users/dmitrygla/ucx/src/uct/ib/rc/accel/rc_mlx5_ep.c:544
 3 0x0000000000038013 uct_rc_ep_check_internal()  /hpc/mtr_scrap/users/dmitrygla/ucx/src/uct/ib/rc/base/rc_ep.c:549
 4 0x0000000000038088 uct_rc_ep_check_progress()  /hpc/mtr_scrap/users/dmitrygla/ucx/src/uct/ib/rc/base/rc_ep.c:562
 5 0x0000000000037223 uct_rc_iface_invoke_pending_cb()  /hpc/mtr_scrap/users/dmitrygla/ucx/src/uct/ib/rc/base/rc_iface.h:592
 6 0x0000000000037223 uct_rc_ep_process_pending()  /hpc/mtr_scrap/users/dmitrygla/ucx/src/uct/ib/rc/base/rc_ep.c:359
 7 0x00000000000550b3 ucs_arbiter_dispatch_nonempty()  /hpc/mtr_scrap/users/dmitrygla/ucx/src/ucs/datastruct/arbiter.c:321
 8 0x00000000000a1092 ucs_arbiter_dispatch()  /hpc/mtr_scrap/users/dmitrygla/ucx/src/ucs/datastruct/arbiter.h:386
 9 0x00000000000ad274 uct_rc_iface_add_cq_credits_dispatch()  /hpc/mtr_scrap/users/dmitrygla/ucx/src/uct/ib/rc/base/rc_iface.h:488
10 0x00000000000ad274 uct_rc_mlx5_iface_progress()  /hpc/mtr_scrap/users/dmitrygla/ucx/src/uct/ib/rc/accel/rc_mlx5_iface.c:173
11 0x00000000000ad274 uct_rc_mlx5_iface_progress_cyclic()  /hpc/mtr_scrap/users/dmitrygla/ucx/src/uct/ib/rc/accel/rc_mlx5_iface.c:178
12 0x000000000005f5a1 ucs_callbackq_dispatch()  /hpc/mtr_scrap/users/dmitrygla/ucx/src/ucs/datastruct/callbackq.h:211
13 0x000000000006c49a uct_worker_progress()  /hpc/mtr_scrap/users/dmitrygla/ucx/src/uct/api/uct.h:2589
14 0x0000000000404e04 UcxContext::progress_worker_event()  /hpc/mtr_scrap/users/dmitrygla/ucx/test/apps/iodemo/ucx_wrapper.cc:388
15 0x00000000004044b4 UcxContext::progress()  /hpc/mtr_scrap/users/dmitrygla/ucx/test/apps/iodemo/ucx_wrapper.cc:226
16 0x0000000000416f24 DemoClient::wait_for_responses()  /hpc/mtr_scrap/users/dmitrygla/ucx/test/apps/iodemo/io_demo.cc:1934
17 0x000000000041835a DemoClient::run()  /hpc/mtr_scrap/users/dmitrygla/ucx/test/apps/iodemo/io_demo.cc:2204
18 0x000000000040f4b6 do_client()  /hpc/mtr_scrap/users/dmitrygla/ucx/test/apps/iodemo/io_demo.cc:2801
19 0x000000000040f8dc main()  /hpc/mtr_scrap/users/dmitrygla/ucx/test/apps/iodemo/io_demo.cc:2844
20 0x00000000000223d5 __libc_start_main()  ???:0
21 0x00000000004033b9 _start()  ???:0
=================================

EP check operation was scheduled on UCT EP internally, i.e. UCP EP doesn't put it on a pending queue, but then UCP EP failed.
And EP check operation stucked on UCT EP's pending queue and then it was dispatched and we got an assertion failure.

How ?

Introduce the pending purge callback in RC and DC which purges only internal operations.
Dispatch arbiter with the pending purge callback to cancel internal operations when error detected on UCT endpoint.

dmitrygx · 2021-11-25T17:08:10Z

replaces #7704

yosefe · 2021-11-25T17:09:46Z


    ucs_assert_always(iface->config.fc_enabled);
-    status = uct_rc_fc_ctrl(&ep->super.super, UCT_RC_EP_FC_PURE_GRANT, NULL);
+    if (ep->flags & (UCT_RC_EP_FLAG_FAILED | UCT_RC_EP_FLAG_FLUSH_CANCEL)) {


space line before

brminich · 2021-11-25T19:54:54Z

imo calling purge with certain callback in 2 places is better than adding special paths (new ifs) in more places, like it is done here
in general, previous approach from #7704 looks better to me

yosefe · 2021-11-26T08:38:39Z

+void uct_rc_ep_pending_purge_warn_cb(uct_pending_req_t *self, void *arg)
+{
+    uct_rc_ep_t *ep = arg;
+    ucs_warn("ep=%p: pending request %p(%s) was not purged", ep, self,


"ep %p:"
space after %p

yosefe · 2021-11-26T08:44:48Z

imo calling purge with certain callback in 2 places is better than adding special paths (new ifs) in more places, like it is done here
in general, previous approach from #7704 looks better to me

IMO it's safer to not send grand and ep_check when ep is in a failed state, rather than going over the pending queue several times and removing the operation.
on other hand, cleaning the pending queue in one place vs. several IFs has its advantages.

@dmitrygx in which places would we need to clean internal pending reqs (and keep user's pending reqs)?

dmitrygx · 2021-11-26T09:08:13Z

@dmitrygx in which places would we need to clean internal pending reqs (and keep user's pending reqs)?

in error handling of RC and DC:
#7704

dmitrygx · 2021-11-30T08:06:30Z

@brminich @hoopoepg could you review pls?
failure is unrelated

brminich

why need to introduce internal purging functions? Can't we just use special callbacks for purge?

dmitrygx · 2021-11-30T08:31:27Z

why need to introduce internal purging functions? Can't we just use special callbacks for purge?

I checked it, unfortunately we can't do or I don't see a way how to do it

hoopoepg · 2021-11-30T08:33:05Z

I checked it, unfortunately we can't do or I don't see a way how to do it

what is the problem? set of parameters may be passed as pointer to structure

dmitrygx · 2021-11-30T08:46:02Z

I checked it, unfortunately we can't do or I don't see a way how to do it

what is the problem? set of parameters may be passed as pointer to structure

we can't use uct_rc_ep_pending_purge()'s purge callback to remove only internal elements. it purges all elements.

brminich · 2021-11-30T09:00:07Z

we can't use uct_rc_ep_pending_purge()'s purge callback to remove only internal elements. it purges all elements.

It should be possible, please check uct_dc_mlx5_ep_arbiter_purge_cb - there we skip elements not belonging to our ep

dmitrygx · 2021-11-30T09:03:08Z

we can't use uct_rc_ep_pending_purge()'s purge callback to remove only internal elements. it purges all elements.

It should be possible, please check uct_dc_mlx5_ep_arbiter_purge_cb - there we skip elements not belonging to our ep

but how to do it w/o introducing a new common function which accepts callbacks

brminich · 2021-11-30T09:08:17Z

uct_dc_mlx5_ep_arbiter_purge_cb

uct_dc_mlx5_ep_arbiter_purge_cb is already a callback.
can't you create another callback which would check req->func names?

dmitrygx · 2021-11-30T10:05:24Z

can't you create another callback which would check req->func names?

I created it, the name of this callback is uct_dc_mlx5_ep_arbiter_purge_internal_cb.
But uct_dc_mlx5_ep_arbiter_purge_cb needs the same functionality as uct_dc_mlx5_ep_arbiter_purge_internal_cb, so one calls another one in its turn to not duplicate code and functionality.

brminich · 2021-11-30T11:15:55Z

    }

    uct_rc_txqp_purge_outstanding(iface, &ep->super.txqp, ep_status, pi, 0);
+    uct_rc_ep_pending_purge_internal(&ep->super);


looks like you can call something like below:
ucs_arbiter_group_purge(&iface->tx.arbiter, &ep->arb_group, uct_rc_ep_arbiter_purge_internal_cb, NULL);

brminich · 2021-11-30T11:16:39Z

+void uct_rc_ep_pending_purge_internal(uct_rc_ep_t *ep)
+{
+    uct_rc_iface_t *iface    = ucs_derived_of(ep->super.super.iface,
+                                              uct_rc_iface_t);
+    uct_purge_cb_args_t args = {
+        (uct_pending_purge_callback_t)ucs_empty_function_do_assert_void,
+        NULL
+    };
+
+    ucs_arbiter_group_purge(&iface->tx.arbiter, &ep->arb_group,
+                            uct_rc_ep_arbiter_purge_internal_cb, &args);
+}


no need in this func, uct_rc_ep_arbiter_purge_internal_cb does not do anything with args

brminich · 2021-11-30T11:17:26Z

    count = uct_rc_verbs_get_tx_res_count(ep, wc);
    uct_rc_txqp_purge_outstanding(iface, &ep->super.txqp, ep_status,
                                  ep->txcnt.ci + count, 0);
+    uct_rc_ep_pending_purge_internal(&ep->super);


can call ucs_arbiter_group_purge

brminich · 2021-11-30T11:22:44Z

-    void *priv_args[2]         = {ep, arg};
-    uct_purge_cb_args_t args   = {cb, priv_args};
+    void *priv_args[2]       = {ep, arg};
+    uct_purge_cb_args_t args = {cb, priv_args};


you pass arg as NULL from uct_dc_mlx5_iface_set_ep_failed

but I pass user's arg from uct_dc_mlx5_ep_pending_purge, so we should use args here

brminich · 2021-11-30T11:26:16Z

                                uct_dc_mlx5_ep_rand_arb_group(iface, ep),
-                                uct_dc_mlx5_ep_arbiter_purge_cb, &args);
-        return;
+                                arbiter_cb, &args);


looks like with rand scheme it should segfault

why? it should work the same way as previously

brminich · 2021-11-30T11:47:54Z


    uct_rc_txqp_purge_outstanding(iface, &ep->super.txqp, ep_status, pi, 0);
+    ucs_arbiter_group_purge(&iface->tx.arbiter, &ep->super.arb_group,
+                            uct_rc_ep_arbiter_purge_internal_cb, &args);


you can pass NULL instead of args

brminich · 2021-11-30T11:48:13Z

    uct_rc_txqp_purge_outstanding(iface, &ep->super.txqp, ep_status,
                                  ep->txcnt.ci + count, 0);
+    ucs_arbiter_group_purge(&iface->tx.arbiter, &ep->super.arb_group,
+                            uct_rc_ep_arbiter_purge_internal_cb, &args);


can pass NULL

brminich · 2021-11-30T13:49:14Z


+void uct_rc_ep_pending_purge_warn_cb(uct_pending_req_t *self, void *arg)
+{
+    uct_rc_ep_t *ep = arg;


better cast to uct_ep, because you invoke this cb from dc ep destructor and dc ep is not inherited from rc

brminich · 2021-11-30T13:50:33Z

+        return UCS_ARBITER_CB_RESULT_RESCHED_GROUP;
+    }
+
+    /* Non-internal request was found */


the comment is incorrect

brminich · 2021-11-30T13:51:10Z

-    } else if (ucs_likely(req->func != uct_rc_ep_fc_grant)) {
+    result = uct_rc_ep_arbiter_purge_internal_cb(arbiter, group, elem, arg);
+    if (ucs_likely(result != UCS_ARBITER_CB_RESULT_REMOVE_ELEM)) {
        /* Invoke user's callback only if it is not internal FC message */


comment is not valid anymore

brminich · 2021-11-30T13:52:21Z

-        ucs_mpool_put(req);
-    } else if (ucs_likely(req->func != uct_rc_ep_fc_grant)) {
+    result = uct_rc_ep_arbiter_purge_internal_cb(arbiter, group, elem, arg);
+    if (ucs_likely(result != UCS_ARBITER_CB_RESULT_REMOVE_ELEM)) {


i'd avoid likely - it is purge + KA probability is supposed to be higher than just FC

brminich · 2021-11-30T13:56:59Z

                                     ucs_status_t ep_status)
 {
    uct_ib_iface_t *ib_iface = &iface->super.super.super;
+    void *priv_args[2]        = {ep, NULL};


you can pass just ep as a private arg, you do not need 2 args

brminich

👍 looks good, besides minor comments

brminich · 2021-12-01T09:58:22Z

+    ucs_arbiter_cb_result_t result;
+
+    result = uct_dc_mlx5_ep_arbiter_purge_internal_cb(arbiter, group, elem,
+                                                      arg);


you can pass ep pointer here instead of arg

brminich · 2021-12-01T09:59:07Z

    void **priv_args             = cb_args->arg;
    uct_dc_mlx5_ep_t *ep         = priv_args[0];


you can assume arg is a pointer to ep, you do not need to pass an array of args here

dmitrygx · 2021-12-01T14:10:17Z

@yosefe could you review pls?

…en EP failed

yosefe reviewed Nov 25, 2021

View reviewed changes

yosefe reviewed Nov 26, 2021

View reviewed changes

dmitrygx changed the title ~~UCT/IB/RC/DC: Don't send FC or EP_CHECK if FAILED/CANCELED~~ UCT/IB/DC/RC: Purge internal pending ops to avoid dispatching them when EP failed Nov 29, 2021

dmitrygx force-pushed the topic/uct/ib_internal_pending_failed branch 2 times, most recently from fd0ec27 to cac4723 Compare November 29, 2021 14:37

brminich reviewed Nov 30, 2021

View reviewed changes

hoopoepg approved these changes Nov 30, 2021

View reviewed changes

brminich reviewed Nov 30, 2021

View reviewed changes

brminich reviewed Dec 1, 2021

View reviewed changes

dmitrygx force-pushed the topic/uct/ib_internal_pending_failed branch from 1029b7c to acd7350 Compare December 1, 2021 13:29

brminich approved these changes Dec 1, 2021

View reviewed changes

yosefe reviewed Dec 1, 2021

View reviewed changes

Comment thread src/uct/ib/rc/base/rc_ep.c Resolved

yosefe approved these changes Dec 1, 2021

View reviewed changes

UCT/IB/DC/RC: Purge internal pending ops to avoid dispatching them wh…

2ca1f08

…en EP failed

dmitrygx force-pushed the topic/uct/ib_internal_pending_failed branch from acd7350 to 2ca1f08 Compare December 1, 2021 15:48

dmitrygx merged commit 0369765 into openucx:master Dec 1, 2021

svc-ucx mentioned this pull request Sep 6, 2026

GTEST/UCT: Add outstanding purge test base #11891

Merged

		void **priv_args = cb_args->arg;
		uct_dc_mlx5_ep_t *ep = priv_args[0];

Conversation

dmitrygx commented Nov 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

Why ?

How ?

Uh oh!

dmitrygx commented Nov 25, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

brminich commented Nov 25, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yosefe commented Nov 26, 2021

Uh oh!

dmitrygx commented Nov 26, 2021

Uh oh!

dmitrygx commented Nov 30, 2021

Uh oh!

brminich left a comment

Choose a reason for hiding this comment

Uh oh!

dmitrygx commented Nov 30, 2021

Uh oh!

hoopoepg commented Nov 30, 2021

Uh oh!

dmitrygx commented Nov 30, 2021

Uh oh!

brminich commented Nov 30, 2021

Uh oh!

dmitrygx commented Nov 30, 2021

Uh oh!

brminich commented Nov 30, 2021

Uh oh!

dmitrygx commented Nov 30, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

dmitrygx commented Nov 25, 2021 •

edited

Loading

brminich left a comment •

edited

Loading