Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GTEST/UCT: Unit tests for UMR XGVMI #9838

Merged
merged 1 commit into from
May 27, 2024

Conversation

iyastreb
Copy link
Contributor

@iyastreb iyastreb commented Apr 22, 2024

What

Few unit tests for XGVMI UMR approach to check that:

  • Lazy initialization works as expected
  • All UMR invariants are respected
  • UMR mkey pool is properly used on exporter when XGVMI is supported and when it's not
  • UMR mkey hash map is properly used on importer side with and without XGVMI support
  • All allocated objects are freed

We introduce new config option UCX_XGVMI_UMR_ENABLE option as a part of this commit, which enables XGVMI UMR workflow.

test/gtest/uct/ib/test_devx.cc Show resolved Hide resolved
test/gtest/uct/ib/test_devx.cc Outdated Show resolved Hide resolved
test/gtest/uct/ib/test_devx.cc Outdated Show resolved Hide resolved
test/gtest/uct/ib/test_devx.cc Outdated Show resolved Hide resolved
test/gtest/uct/ib/test_devx.cc Outdated Show resolved Hide resolved
test/gtest/uct/ib/test_devx.cc Outdated Show resolved Hide resolved
destroy_memh(imported_memh2);
destroy_memh(exported_memh);
}

UCT_INSTANTIATE_IB_TEST_CASE(test_devx);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pls move it before the new class, typically we initialize test classes right after their (and their tests) definitions

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure

brminich
brminich previously approved these changes Apr 23, 2024
tvegas1
tvegas1 previously approved these changes Apr 23, 2024
Copy link
Contributor

@yosefe yosefe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as general comment, make the tests more high level to capture more flows in less tests

test/gtest/uct/ib/test_devx.cc Outdated Show resolved Hide resolved
test/gtest/uct/ib/test_devx.cc Outdated Show resolved Hide resolved
test/gtest/uct/ib/test_devx.cc Outdated Show resolved Hide resolved
test/gtest/uct/ib/test_devx.cc Outdated Show resolved Hide resolved
test/gtest/uct/ib/test_devx.cc Outdated Show resolved Hide resolved
test/gtest/uct/ib/test_devx.cc Outdated Show resolved Hide resolved
destroy_memh(memh);

/* After release of memh, associated UMR mkey is moved back to mkey pool */
ASSERT_EQ(check_xgvmi() ? 1 : 0, ucs_list_length(&md()->umr.mkey_pool));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is it needed to check?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here I wanted to check that releasing of memory handle triggers invalidation of associated UMR mkey, that must be returned to the UMR mkey pool. Again, we check it for both flows, truly-xgvmi and fake-xgvmi devices

test/gtest/uct/ib/test_devx.cc Outdated Show resolved Hide resolved
test/gtest/uct/ib/test_devx.cc Outdated Show resolved Hide resolved
test/gtest/uct/ib/test_devx.cc Outdated Show resolved Hide resolved
@iyastreb iyastreb dismissed stale reviews from tvegas1 and brminich via 0603a4a May 6, 2024 13:52
@yosefe
Copy link
Contributor

yosefe commented May 19, 2024

/azp run UCX PR

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

src/uct/ib/base/ib_md.c Outdated Show resolved Hide resolved
test/gtest/uct/ib/test_devx.cc Outdated Show resolved Hide resolved
test/gtest/uct/ib/test_devx.cc Outdated Show resolved Hide resolved
test/gtest/uct/ib/test_devx.cc Outdated Show resolved Hide resolved
test/gtest/uct/ib/test_devx.cc Outdated Show resolved Hide resolved
test/gtest/uct/ib/test_devx.cc Outdated Show resolved Hide resolved
test/gtest/uct/ib/test_devx.cc Outdated Show resolved Hide resolved
test/gtest/uct/ib/test_devx.cc Show resolved Hide resolved
test/gtest/uct/ib/test_devx.cc Outdated Show resolved Hide resolved
* ones, given jitter we check that it's at least 50 times faster */
ASSERT_GT(export_create_time / 50, export_reuse_time);
ASSERT_GT(import_create_time / 50, import_reuse_time);
UCS_TEST_MESSAGE << "Export create time: " << export_create_time;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO we can print it in a more compact way + convert to milliseconds

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, now it's in this format:

[     INFO ] Create 1000 UMR mkeys, export: 397.802 ms, import: 215.757 ms       
[     INFO ] Reuse 1000 UMR mkeys, export: 1.784 ms, import: 0.421 ms                                            

EXPECT_LT(export_reuse_time, export_create_time / 10);

char buf[1024] = {};
sprintf(buf, "Create %zu UMR mkeys, export: %.3f ms, import: %.3f ms",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

snprintf / ucs_snprintf_safe

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure

MEMH_COUNT, ucs_time_to_msec(export_create_time),
ucs_time_to_msec(import_create_time));
UCS_TEST_MESSAGE << buf;
sprintf(buf, "Reuse %zu UMR mkeys, export: %.3f ms, import: %.3f ms",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

snprintf / ucs_snprintf_safe

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@yosefe yosefe enabled auto-merge May 26, 2024 12:10
@yosefe yosefe merged commit bb67c18 into openucx:master May 27, 2024
142 checks passed
@iyastreb iyastreb deleted the uct/gtest/umr-xgvmi branch May 27, 2024 08:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants