UCP/API: Flag to silence request leak check #6232

yosefe · 2021-01-28T20:55:55Z

Why

Allow MPI runtime skip request leak check, which will silence errors like reported in open-mpi/ompi#8426

shamisp · 2021-01-28T21:49:14Z

@yosefe - Why MPI_Finalize does not cleanup the resources ? AFAIK, It supposed to track communicators and clean the resources. Adding @hppritcha

brminich · 2021-01-29T06:07:42Z

src/ucp/api/ucp.h

+ * @ingroup UCP_WORKER
+ * @brief UCP worker flags
+ *
+ * This enumeration allows pecifying flags for @ref ucp_worker_params_t.flags,


specifying

brminich · 2021-01-29T06:09:15Z

src/ucp/core/ucp_worker.c


    uct_thread_mode = UCS_THREAD_MODE_SINGLE;
-    worker->flags   = 0;
+    worker->flags   = (params->field_mask & UCP_WORKER_PARAM_FIELD_FLAGS) ?


minor: you can use UCP_PARAM_VALUE

brminich · 2021-01-29T06:11:37Z

src/ucp/core/ucp_worker.h

-    UCP_WORKER_FLAG_EXTERNAL_EVENT_FD = UCS_BIT(0), /**< worker event fd is external */
-    UCP_WORKER_FLAG_EDGE_TRIGGERED    = UCS_BIT(1), /**< events are edge-triggered */
-    UCP_WORKER_FLAG_MT                = UCS_BIT(2)  /**< MT locking is required */
+    /** MT locking is required */


are values moved to allow combining with new params->flags?
Can you please add a comment and some check they are not mixed?

yosefe · 2021-01-29T12:43:28Z

@yosefe - Why MPI_Finalize does not cleanup the resources ? AFAIK, It supposed to track communicators and clean the resources. Adding @hppritcha

Some applications do not release all requests [even though required by MPI standard], and other MPI implementations do not warn about this. There are user applications that start to spew these warnings when ran with UCX, and users see this as UCX issue, and in cases just silence them by setting UCX_LOG_LEVEL=error

See similar reports:

Since that kind of leak does would not usually have side effects (MPI_Finalize is executed right before process exit), it would be better to not print it in case of MPI.

shamisp · 2021-01-29T19:27:07Z

I think part of the annoyance here (from user perspective) is that it generates tons of errors... Would it be better to generate one-liner error: your application leaked NN requests. @hppritcha @pavanbalaji is the preference from MPI community to silence this ? thanks !

pavanbalaji · 2021-01-30T03:37:38Z

I think part of the annoyance here (from user perspective) is that it generates tons of errors... Would it be better to generate one-liner error: your application leaked NN requests. @hppritcha @pavanbalaji is the preference from MPI community to silence this ? thanks !

MPICH only generates this when the user provides a special configure flag, not by default. From MPI's perspective, it doesn't matter if you throw an error in UCX or not. We keep track of MPI requests internally and would throw a warning (if the user specified the configure flag) anyway. The UCX warning would be redundant in that case.

We could also release the UCX requests internally in MPI_Finalize as you suggested. So it shouldn't matter one way or another for MPI libraries. But I'm not an MPI library developer anymore, so I can't speak for them. :-)

shamisp · 2021-01-30T15:32:51Z

I think part of the annoyance here (from user perspective) is that it generates tons of errors... Would it be better to generate one-liner error: your application leaked NN requests. @hppritcha @pavanbalaji is the preference from MPI community to silence this ? thanks !

MPICH only generates this when the user provides a special configure flag, not by default. From MPI's perspective, it doesn't matter if you throw an error in UCX or not. We keep track of MPI requests internally and would throw a warning (if the user specified the configure flag) anyway. The UCX warning would be redundant in that case.

We could also release the UCX requests internally in MPI_Finalize as you suggested. So it shouldn't matter one way or another for MPI libraries. But I'm not an MPI library developer anymore, so I can't speak for them. :-)

As I read - MPICH detects the leak but does not really clean it up.

hppritcha · 2021-02-03T20:09:43Z

I think part of the annoyance here (from user perspective) is that it generates tons of errors... Would it be better to generate one-liner error: your application leaked NN requests. @hppritcha @pavanbalaji is the preference from MPI community to silence this ? thanks !

preference for silence from the OMPI side.

yosefe · 2021-02-04T15:15:04Z

@shamisp are we good to go?

shamisp · 2021-02-04T16:18:42Z

👍 from my end. @tonycurtis can you please review documentation.

tonycurtis

ucp.h:1137: could simplify to "if UCP_WORKER_......." not set, defaults to 0 ?

yosefe · 2021-02-04T21:10:14Z

test output:

[==========] Running 2 tests from 1 test case.
[----------] Global test environment set-up.
[----------] 2 tests from all/test_ucp_worker_request_leak
[ RUN      ] all/test_ucp_worker_request_leak.tag_send_recv/0 <all/leak_check>
[     INFO ] send req: 0x1e0e040, recv req: 0x1e18080
[     INFO ] < object 0x1e0df40 was not returned to mpool ucp_requests >
[     INFO ] < object 0x1e17f80 was not returned to mpool ucp_requests >
[       OK ] all/test_ucp_worker_request_leak.tag_send_recv/0 (371 ms)
[ RUN      ] all/test_ucp_worker_request_leak.tag_send_recv/1 <all/leak_ignore>
[     INFO ] send req: 0x288b040, recv req: 0x2895080
[       OK ] all/test_ucp_worker_request_leak.tag_send_recv/1 (373 ms)
[----------] 2 tests from all/test_ucp_worker_request_leak (744 ms total)

[----------] Global test environment tear-down
[==========] 2 tests from 1 test case ran. (744 ms total)
[  PASSED  ] 2 tests.

yosefe · 2021-02-05T12:14:12Z

@tonycurtis fixed the comment
@brminich can you pls review the gtest?

brminich · 2021-02-05T12:18:26Z

test/gtest/ucp/test_ucp_worker.cc

+        return get_variant_value(0) == LEAK_IGNORE;
+    }
+
+    /// @override


minor extra / here and below

it's supposed to be doxgen comment
https://www.doxygen.nl/manual/docblocks.html

A third alternative is to use a block of at least two C++ comment lines, where each line starts with an additional slash or an exclamation mark. Here are examples of the two cases:

///
/// ... text ...
///

Some runtimes, such as MPI, allow exiting the application without releasing all allocated requests.

yosefe added the API label Jan 28, 2021

brminich reviewed Jan 29, 2021

View reviewed changes

brminich approved these changes Feb 1, 2021

View reviewed changes

hoopoepg approved these changes Feb 2, 2021

View reviewed changes

tonycurtis suggested changes Feb 4, 2021

View reviewed changes

brminich approved these changes Feb 5, 2021

View reviewed changes

tonycurtis approved these changes Feb 5, 2021

View reviewed changes

yosefe added 3 commits February 5, 2021 20:35

UCP/API: Flag to silence request leak check

1cf178d

Some runtimes, such as MPI, allow exiting the application without releasing all allocated requests.

TEST/COMMON: Unite common code of test loggers

62cc974

TEST/UCP: Add test of leak-check disabling API

ebff59d

yosefe force-pushed the topic/ucp-api-flag-to-silence-request-leak branch from 7a382c3 to ebff59d Compare February 5, 2021 18:38

yosefe merged commit ced9ac6 into openucx:master Feb 6, 2021

yosefe deleted the topic/ucp-api-flag-to-silence-request-leak branch February 6, 2021 15:42

yosefe mentioned this pull request Mar 19, 2021

pml/ucx: ignore request leak by default, override by mca param open-mpi/ompi#8661

Merged

gpaulsen mentioned this pull request Mar 26, 2021

MPI_spawn and MPI_Intercomm_merge causes UCX errors open-mpi/ompi#8426

Closed

UCP/API: Flag to silence request leak check #6232

UCP/API: Flag to silence request leak check #6232

Uh oh!

Conversation

yosefe commented Jan 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why

Uh oh!

shamisp commented Jan 28, 2021

Uh oh!

brminich Jan 29, 2021

Choose a reason for hiding this comment

Uh oh!

brminich Jan 29, 2021

Choose a reason for hiding this comment

Uh oh!

brminich Jan 29, 2021

Choose a reason for hiding this comment

Uh oh!

yosefe commented Jan 29, 2021

Uh oh!

shamisp commented Jan 29, 2021

Uh oh!

pavanbalaji commented Jan 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shamisp commented Jan 30, 2021

Uh oh!

hppritcha commented Feb 3, 2021

Uh oh!

yosefe commented Feb 4, 2021

Uh oh!

shamisp commented Feb 4, 2021

Uh oh!

tonycurtis left a comment

Choose a reason for hiding this comment

Uh oh!

yosefe commented Feb 4, 2021

Uh oh!

yosefe commented Feb 5, 2021

Uh oh!

brminich Feb 5, 2021

Choose a reason for hiding this comment

Uh oh!

yosefe Feb 5, 2021

Choose a reason for hiding this comment

Uh oh!

brminich Feb 5, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

yosefe commented Jan 28, 2021 •

edited

Loading

pavanbalaji commented Jan 30, 2021 •

edited

Loading