-
Notifications
You must be signed in to change notification settings - Fork 407
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
Conversation
test/gtest/uct/ib/test_devx.cc
Outdated
destroy_memh(imported_memh2); | ||
destroy_memh(exported_memh); | ||
} | ||
|
||
UCT_INSTANTIATE_IB_TEST_CASE(test_devx); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure
There was a problem hiding this 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
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)); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
/azp run UCX PR |
Azure Pipelines successfully started running 1 pipeline(s). |
test/gtest/uct/ib/test_devx.cc
Outdated
* 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; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
test/gtest/uct/ib/test_devx.cc
Outdated
EXPECT_LT(export_reuse_time, export_create_time / 10); | ||
|
||
char buf[1024] = {}; | ||
sprintf(buf, "Create %zu UMR mkeys, export: %.3f ms, import: %.3f ms", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
snprintf / ucs_snprintf_safe
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure
test/gtest/uct/ib/test_devx.cc
Outdated
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", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
snprintf / ucs_snprintf_safe
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
480d07a
to
5873fa1
Compare
What
Few unit tests for XGVMI UMR approach to check that:
We introduce new config option UCX_XGVMI_UMR_ENABLE option as a part of this commit, which enables XGVMI UMR workflow.