UCP/GTEST: Fix and test ucp requests leak from the ptr_map by dmitrygx · Pull Request #6006 · openucx/ucx

dmitrygx · 2020-12-09T16:48:05Z

What

Fix and test ucp requests leak from the ptr_map.

Why ?

To prevent having leaked requests in ptr map.

How ?

Save request ID to the request to be able remove them just getting an ID form request.
Delete IDs where it is needed.
Fix AMO/RMA protocols to generate request ID from progress function (release an allocated ID if send was not OK) rather than from pack_cb that could be invoked several times.

dmitrygx · 2020-12-11T17:43:21Z

@alinask @brminich @hoopoepg could you review pls?

dmitrygx · 2020-12-14T16:44:20Z

@brminich @yosefe @alinask thank you for the review
changes are squashed

@alinask do you have additional comments?

alinask · 2020-12-14T19:14:19Z

@brminich @yosefe @alinask thank you for the review
changes are squashed

@alinask do you have additional comments?

@dmitrygx I don't have additional comments.

Only one short request.
Could you please run io_demo with this PR with an Eth IP (not RoCE)? to make sure there isn't a leak.
Running io_demo was what releveled this problem in the first place and it's not part of CI yet (with Eth).
One server and one client on different hosts is enough. the original command line was:
server: UCX_SOCKADDR_CM_ENABLE=y ./io_demo -o write,read -d 128:4194304 -i 100000 -w 10
client: UCX_SOCKADDR_CM_ENABLE=y ./io_demo -o write,read -d 128:4194304 -i 100000 -w 10 <ETH_IP>

Thanks!

dmitrygx · 2020-12-14T20:32:38Z

Could you please run io_demo with this PR with an Eth IP (not RoCE)? to make sure there isn't a leak.

@alinask sure, thank you for giving me a reproducer.
I ran the test as you requested. It works fine when I specified IP over commodity Ethernet, no leaks were revealed during the test.
I tested it with and without UCX_TLS="tcp,sockcm", because the master branch selects IB (if it exists) even if we specified an IP address of an Ethernet device (establishes a connection thru sockcm and TCP, and then switches to more optimal transport).

[1607977721.761084] [UCX-connection #1 10.210.0.125:1337] destroying, ep is 0x7fec8111d000
[1607977721.761133] [UCX-connection #1 10.210.0.125:1337] closing ep 0x7fec8111d000 mode force
[1607977721.761254] [UCX-connection #1 10.210.0.125:1337] released
[1607977721.761260] [DEMO] Client exit with status 'OK'
[1607977721.761263] [UCX] destroy_connections
[1607977721.761505] [UCX] destroy_connections
[1607977721.761519] [UCX] iomsg receive request 0xa2ba80 failed: Request canceled

Running the test on the master branch completes with the warning at the end of the testing when UCX_TLS="tcp,sockcm" is specified (but if an IB transport is used, it completes w/o any warnings):

...
[1607977457.603803] [UCX-connection #1 10.210.0.125:1337] destroying, ep is 0x7f8aa1787000
[1607977457.603821] [UCX-connection #1 10.210.0.125:1337] closing ep 0x7f8aa1787000 mode force
[1607977457.603907] [UCX-connection #1 10.210.0.125:1337] released
[1607977457.603914] [DEMO] Client exit with status 'OK'
[1607977457.603916] [UCX] destroy_connections
[1607977457.604121] [UCX] destroy_connections
[1607977457.604131] [UCX] iomsg receive request 0x2322a78 failed: Request canceled
[1607977457.621452] [jazz30:145487:0]      ptr_map.inl:50   UCX  WARN  ptr map 0x2304160 contains 40045 elements on destroy

So, I guess the PR fixes the original problem.

dmitrygx · 2020-12-15T03:22:40Z

failure is #6027

dmitrygx · 2020-12-15T03:22:46Z

/azp run

azure-pipelines · 2020-12-15T03:23:00Z

Azure Pipelines successfully started running 1 pipeline(s).

alinask · 2020-12-15T07:31:46Z

Could you please run io_demo with this PR with an Eth IP (not RoCE)? to make sure there isn't a leak.

@alinask sure, thank you for giving me a reproducer.
I ran the test as you requested. It works fine when I specified IP over commodity Ethernet, no leaks were revealed during the test.
I tested it with and without UCX_TLS="tcp,sockcm", because the master branch selects IB (if it exists) even if we specified an IP address of an Ethernet device (establishes a connection thru sockcm and TCP, and then switches to more optimal transport).
[1607977721.761084] [UCX-connection #1 10.210.0.125:1337] destroying, ep is 0x7fec8111d000
[1607977721.761133] [UCX-connection #1 10.210.0.125:1337] closing ep 0x7fec8111d000 mode force
[1607977721.761254] [UCX-connection #1 10.210.0.125:1337] released
[1607977721.761260] [DEMO] Client exit with status 'OK'
[1607977721.761263] [UCX] destroy_connections
[1607977721.761505] [UCX] destroy_connections
[1607977721.761519] [UCX] iomsg receive request 0xa2ba80 failed: Request canceled
Running the test on the master branch completes with the warning at the end of the testing when UCX_TLS="tcp,sockcm" is specified (but if an IB transport is used, it completes w/o any warnings):
...
[1607977457.603803] [UCX-connection #1 10.210.0.125:1337] destroying, ep is 0x7f8aa1787000
[1607977457.603821] [UCX-connection #1 10.210.0.125:1337] closing ep 0x7f8aa1787000 mode force
[1607977457.603907] [UCX-connection #1 10.210.0.125:1337] released
[1607977457.603914] [DEMO] Client exit with status 'OK'
[1607977457.603916] [UCX] destroy_connections
[1607977457.604121] [UCX] destroy_connections
[1607977457.604131] [UCX] iomsg receive request 0x2322a78 failed: Request canceled
[1607977457.621452] [jazz30:145487:0]      ptr_map.inl:50   UCX  WARN  ptr map 0x2304160 contains 40045 elements on destroy
So, I guess the PR fixes the original problem.

@dmitrygx thanks for testing this!

dmitrygx · 2020-12-15T08:08:08Z

@yosefe @brminich @alinask ok to merge?

dmitrygx force-pushed the topic/ucp/rndv-ptr-map-fix branch 10 times, most recently from 39f4904 to 4c6ba95 Compare December 11, 2020 11:41

alinask reviewed Dec 13, 2020

View reviewed changes

Comment thread src/ucp/core/ucp_worker.inl Resolved

Comment thread src/ucp/core/ucp_worker.inl Resolved

Comment thread src/ucp/core/ucp_request.h Resolved

yosefe reviewed Dec 13, 2020

View reviewed changes

yosefe mentioned this pull request Dec 13, 2020

UCP/CORE/GTEST: Rearrange fields in a UCP request to reduce its size #6010

Merged

brminich reviewed Dec 14, 2020

View reviewed changes

Comment thread src/ucp/core/ucp_request.h Resolved

Comment thread src/ucp/rma/amo_basic.c Resolved

Comment thread src/ucp/rma/rma_sw.c Resolved

yosefe reviewed Dec 14, 2020

View reviewed changes

Comment thread src/ucp/core/ucp_worker.inl Resolved

Comment thread src/ucs/datastruct/ptr_map.inl Resolved

yosefe approved these changes Dec 14, 2020

View reviewed changes

brminich approved these changes Dec 14, 2020

View reviewed changes

dmitrygx force-pushed the topic/ucp/rndv-ptr-map-fix branch from c9ef4c2 to 44d15e4 Compare December 14, 2020 16:43

UCP/GTEST: Fix and test ucp requests leak from the ptr_map

3f3e099

dmitrygx force-pushed the topic/ucp/rndv-ptr-map-fix branch from 44d15e4 to 3f3e099 Compare December 14, 2020 18:41

alinask mentioned this pull request Dec 14, 2020

UCP/GTEST: fix and test rndv requests leak from the ptr_map. #5769

Closed

alinask approved these changes Dec 15, 2020

View reviewed changes

yosefe merged commit 8427c7f into openucx:master Dec 15, 2020

it-is-a-robot mentioned this pull request Nov 30, 2021

Huawei 1230 kunpengcompute/hucx#49

Closed

Conversation

dmitrygx commented Dec 9, 2020

What

Why ?

How ?

Uh oh!

dmitrygx commented Dec 11, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dmitrygx commented Dec 14, 2020

Uh oh!

alinask commented Dec 14, 2020

Uh oh!

dmitrygx commented Dec 14, 2020

Uh oh!

dmitrygx commented Dec 15, 2020

Uh oh!

dmitrygx commented Dec 15, 2020

Uh oh!

azure-pipelines Bot commented Dec 15, 2020

Uh oh!

alinask commented Dec 15, 2020

Uh oh!

dmitrygx commented Dec 15, 2020

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants