UCS/ASYNC: Skip released handler in poll missed process #9942

jeynmann · 2024-06-07T14:07:53Z

What

Skip events in poll missed process if its handler has already been removed.

Why ?

Async handler might be removed while events missed.
When the handler be called in poll missed process and same memory is reused by the other worker, the handler will try to access wrong resources.

How ?

Set MPMC queue element's value to -1 if handler removed, then skip the events in poll missed process.

brminich · 2024-06-10T12:27:02Z

@jeynmann, pls fix commit titles to trigger CI.
Seems the 1st commit format is incorrect - need to use capitl letters in the very beginning of the title, like described here:

https://github.com/openucx/ucx/wiki/Guidance-for-contributors

brminich · 2024-06-11T12:00:01Z

pls check CI failures. Coverity error looks relevant:

+ '[' 1 -gt 0 ']'
+ cov-format-errors --dir /home/swx-azure-svc_azpcontainer/82170-20240610.12/build/cov_build_devel --emacs-style
/__w/1/s/test/gtest/ucs/test_async.cc:388
  Type: Calling risky function (DC.WEAK_CRYPTO)

/__w/1/s/test/gtest/ucs/test_async.cc:388:
  dont_call: "rand" should not be used for security-related applications, because linear congruential algorithms are too easy to break.
/__w/1/s/test/gtest/ucs/test_async.cc:388:
  remediation: Use a compliant random number generator, such as "/dev/random" or "/dev/urandom" on Unix-like systems, and CNG (Cryptography API: Next Generation) on Windows.

+ cp -ar /home/swx-azure-svc_azpcontainer/82170-20240610.12/build/cov_build_devel /__w/1/s/cov_build_devel
+ echo 'not ok 1 Coverity Detected 1 failures'

jeynmann · 2024-06-17T06:58:00Z

@evgeny-leksikov
Please help to take a review when you're free.

evgeny-leksikov · 2024-06-17T07:26:56Z

src/ucs/async/async.c

+        ucs_mpmc_queue_for_each(elem, &async->missed)
+        {


Suggested change

ucs_mpmc_queue_for_each(elem, &async->missed)

{

ucs_mpmc_queue_for_each(elem, &async->missed) {

evgeny-leksikov · 2024-06-17T07:27:36Z

src/ucs/async/async.c

+        ucs_mpmc_queue_for_each(elem, &async->missed)
+        {
+            if (ucs_async_missed_event_unpack_id(elem->value) == handler->id) {
+                elem->value = -1;


maybe add a constant for special value (-1)

evgeny-leksikov · 2024-06-17T07:32:34Z

test/gtest/ucs/test_async.cc

+    void create_event() {
+        ucs_status_t status = ucs_async_pipe_create(&m_event_pipe);
+        ASSERT_UCS_OK(status);
+        EXPECT_EQ(ucs_atomic_cswap32(&m_event_valid, 0, 1), 0);


here and below: expected value goes first in arg list, it makes matter in output of failed test

Suggested change

EXPECT_EQ(ucs_atomic_cswap32(&m_event_valid, 0, 1), 0);

EXPECT_EQ(0, ucs_atomic_cswap32(&m_event_valid, 0, 1));

evgeny-leksikov · 2024-06-17T07:38:37Z

test/gtest/ucs/test_async.cc

+            le->set_event();
+            suspend(20 - wait);
+            le->check_miss();
+            ++round;


is this value used anywhere?

src/ucs/datastruct/mpmc.h

src/ucs/async/async.c

gleon99 · 2024-06-18T14:42:05Z

src/ucs/async/async.c

+    async = handler->async;
+    if (async != NULL) {
+        ucs_mpmc_queue_block(&async->missed);
+        ucs_mpmc_queue_for_each(elem, &async->missed) {


Need to add ucs_mpmc_queue_for_each in .clang-format (ForEachMacros)

maybe instead of exposing ucs_mpmc_queue_lock, ucs_mpmc_queue_unlock and foreach, can add ucs_mpmc_remove_if(mpmc, predicate_cb, arg) to take a callback that returns 0/1 whether the given element should be removed?

src/ucs/datastruct/mpmc.h

gleon99 · 2024-06-18T14:44:13Z

src/ucs/datastruct/mpmc.h

+/**
+ * Lock MPMC queue.
+ */
+void ucs_mpmc_queue_block(ucs_mpmc_queue_t *mpmc);


Maybe rename no lock/unlock?

test/gtest/ucs/test_async.cc

iyastreb · 2024-06-19T06:54:23Z

test/gtest/ucs/test_async.cc

@@ -811,6 +889,19 @@ UCS_TEST_SKIP_COND_P(test_async_event_mt, multithread,
    EXPECT_GE(min_count, exp_min_count);
 }

+UCS_TEST_SKIP_COND_P(test_async_event_mt, multithread_create_destory,


typo: multithread_create_destroy

iyastreb · 2024-06-19T06:58:15Z

test/gtest/ucs/test_async.cc

@@ -110,6 +126,7 @@ class base_event : public base_async {
    }

    ucs_async_pipe_t m_event_pipe;
+    volatile uint32_t m_event_valid = 0;


minor: maybe init it in ctor

iyastreb · 2024-06-19T06:58:50Z

test/gtest/ucs/test_async.cc

+        EXPECT_EQ(0, ucs_atomic_cswap32(&m_event_valid, 0, 1));
+    }
+
+    void destory_event() {


typo: destroy_event

iyastreb · 2024-06-19T07:01:15Z

test/gtest/ucs/test_async.cc

    void reset() {
        ucs_async_pipe_drain(&m_event_pipe);
    }

+    void create_event() {
+        ucs_status_t status = ucs_async_pipe_create(&m_event_pipe);


Q: What happens when we call this function on already "created" event?
Shouldn't we check if object is already initialized, and if so - release the previous state first?

IMO, double initialization is an error.
The life time of event is managed by base_event, since we call create/destroy method inside the constructor/destructor.

base_event(ucs_async_mode_t mode) : base_async(mode) { create_event(); } ... virtual ~base_event() { destroy_event(); }

To avoid double initialization, I added assertion inside the create_event method, see

EXPECT_EQ(0, ucs_atomic_cswap32(&m_event_valid, 0, 1));

In addition, if we explicitly call create_event, which means we want to manage the event by ourself, then we're responsible for destroy_event.

iyastreb · 2024-06-19T07:05:16Z

test/gtest/ucs/test_async.cc

+        check_is_blocked(le, false);
+
+        int result = le->count();
+        delete le;


pls consider using std::scoped_ptr to get rid of explicit delete
You can also just return le->count(); in this case without saving in tmp var

yosefe · 2024-06-29T13:04:21Z

src/ucs/async/async.c

+    async = handler->async;
+    if (async != NULL) {
+        ucs_mpmc_queue_block(&async->missed);
+        ucs_mpmc_queue_for_each(elem, &async->missed) {


maybe instead of exposing ucs_mpmc_queue_lock, ucs_mpmc_queue_unlock and foreach, can add ucs_mpmc_remove_if(mpmc, predicate_cb, arg) to take a callback that returns 0/1 whether the given element should be removed?

test/gtest/ucs/test_async.cc

yosefe · 2024-07-01T06:50:06Z

@jeynmann pls force push the last PR to fix the commit title

src/ucs/async/async.c

src/ucs/datastruct/mpmc.c

src/ucs/async/async.c

src/ucs/datastruct/mpmc.c

src/ucs/async/async.c

leibin2014 · 2024-07-04T08:19:57Z

/azp run

azure-pipelines · 2024-07-04T08:20:20Z

Azure Pipelines successfully started running 4 pipeline(s).

yosefe · 2024-07-04T16:01:55Z

@jeynmann pls squash

UCS/ASYNC: avoid call handler if already remove GTEST/UCS: rename test func name GTEST/UCS: use ucs rand UCS/ASYNC: fix test code + format GTEST/UCS: fix format UCS/ASYNC: fix code according to comments UCS/ASYNC: fix code according to comments UCS/ASYNC: fix code according to comments UCS/ASYNC: fix code according to comments UCS/ASYNC: fix code according to comments UCS/ASYNC: fix code according to CI UCS/ASYNC: fix code according to CI UCS/ASYNC: fix code according to CI UCS/ASYNC: fix code according to comments UCS/ASYNC: fix code according to comments UCS/ASYNC: fix code according to comments GTEST/UCS: fix code according to comments

jeynmann force-pushed the skip_handler_of_released branch from db814db to a1408aa Compare June 10, 2024 16:37

jeynmann changed the title ~~[WIP] Skip released handler in poll missed process~~ UCS/ASYNC: Skip released handler in poll missed process Jun 16, 2024

evgeny-leksikov reviewed Jun 17, 2024

View reviewed changes

evgeny-leksikov previously approved these changes Jun 18, 2024

View reviewed changes

gleon99 reviewed Jun 18, 2024

View reviewed changes

iyastreb reviewed Jun 19, 2024

View reviewed changes

jeynmann dismissed evgeny-leksikov’s stale review via 92cc45b June 19, 2024 07:59

yosefe reviewed Jun 29, 2024

View reviewed changes

yosefe reviewed Jul 1, 2024

View reviewed changes

src/ucs/async/async.c Outdated Show resolved Hide resolved

src/ucs/async/async.c Outdated Show resolved Hide resolved

src/ucs/async/async.c Outdated Show resolved Hide resolved

src/ucs/datastruct/mpmc.c Outdated Show resolved Hide resolved

jeynmann force-pushed the skip_handler_of_released branch from fa48d26 to d90ff5c Compare July 1, 2024 15:14

yosefe previously approved these changes Jul 1, 2024

View reviewed changes

jeynmann dismissed yosefe’s stale review via ec7637b July 2, 2024 02:42

yosefe reviewed Jul 2, 2024

View reviewed changes

src/ucs/async/async.c Outdated Show resolved Hide resolved

yosefe reviewed Jul 2, 2024

View reviewed changes

src/ucs/datastruct/mpmc.c Outdated Show resolved Hide resolved

src/ucs/async/async.c Outdated Show resolved Hide resolved

yosefe previously approved these changes Jul 2, 2024

View reviewed changes

jeynmann dismissed yosefe’s stale review via a0e60bc July 2, 2024 14:30

yosefe approved these changes Jul 4, 2024

View reviewed changes

jeynmann force-pushed the skip_handler_of_released branch from a0e60bc to b9c293b Compare July 5, 2024 01:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UCS/ASYNC: Skip released handler in poll missed process #9942

UCS/ASYNC: Skip released handler in poll missed process #9942

jeynmann commented Jun 7, 2024

brminich commented Jun 10, 2024

brminich commented Jun 11, 2024

jeynmann commented Jun 17, 2024

evgeny-leksikov Jun 17, 2024

evgeny-leksikov Jun 17, 2024

evgeny-leksikov Jun 17, 2024

evgeny-leksikov Jun 17, 2024

gleon99 Jun 18, 2024

yosefe Jun 29, 2024

gleon99 Jun 18, 2024

iyastreb Jun 19, 2024

iyastreb Jun 19, 2024

iyastreb Jun 19, 2024

iyastreb Jun 19, 2024

jeynmann Jun 24, 2024

iyastreb Jun 19, 2024

yosefe Jun 29, 2024

yosefe commented Jul 1, 2024

leibin2014 commented Jul 4, 2024

azure-pipelines bot commented Jul 4, 2024

yosefe commented Jul 4, 2024

	ucs_mpmc_queue_for_each(elem, &async->missed)
	{
	ucs_mpmc_queue_for_each(elem, &async->missed) {

	EXPECT_EQ(ucs_atomic_cswap32(&m_event_valid, 0, 1), 0);
	EXPECT_EQ(0, ucs_atomic_cswap32(&m_event_valid, 0, 1));

UCS/ASYNC: Skip released handler in poll missed process #9942

Are you sure you want to change the base?

UCS/ASYNC: Skip released handler in poll missed process #9942

Conversation

jeynmann commented Jun 7, 2024

What

Why ?

How ?

brminich commented Jun 10, 2024

brminich commented Jun 11, 2024

jeynmann commented Jun 17, 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

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

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 Jul 1, 2024

leibin2014 commented Jul 4, 2024

azure-pipelines bot commented Jul 4, 2024

yosefe commented Jul 4, 2024