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

prov/efa: Add unit tests for efa_msg #10632

Merged
merged 1 commit into from
Dec 13, 2024
Merged

Conversation

jiaxiyan
Copy link
Contributor

@jiaxiyan jiaxiyan commented Dec 12, 2024

Add unit tests for efa_msg. Use FI_EP_RDM as ep type and override efa_msg_ops.

@jiaxiyan jiaxiyan requested a review from a team December 12, 2024 20:45
prov/efa/test/efa_unit_test_msg.c Outdated Show resolved Hide resolved
prov/efa/test/efa_unit_test_msg.c Outdated Show resolved Hide resolved
cmocka_unit_test_setup_teardown(test_efa_msg_fi_send, efa_unit_test_mocks_setup, efa_unit_test_mocks_teardown),
cmocka_unit_test_setup_teardown(test_efa_msg_fi_sendv, efa_unit_test_mocks_setup, efa_unit_test_mocks_teardown),
cmocka_unit_test_setup_teardown(test_efa_msg_fi_sendmsg, efa_unit_test_mocks_setup, efa_unit_test_mocks_teardown),
cmocka_unit_test_setup_teardown(test_efa_msg_fi_senddata, efa_unit_test_mocks_setup, efa_unit_test_mocks_teardown),
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you want to add tests for inject* calls?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure how to test the inject as I am using FI_EP_DGRAM as the ep type right now.

Copy link
Contributor

@shijin-aws shijin-aws Dec 12, 2024

Choose a reason for hiding this comment

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

easy thing: use FI_EP_RDM, but override your ep function table before you making the send/recv call.

https://github.com/ofiwg/libfabric/blob/main/prov/efa/src/rdm/efa_rdm_ep_fiops.c#L639C2-L640C32

// get ep from your ep_fid
(*ep)->msg = &efa_msg_ops;
(*ep)->rma = &efa_rma_ops;

Copy link
Contributor

Choose a reason for hiding this comment

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

We should finally make DGRAM support inject after onboarding FI_CONTEXT2, you can do this override before that happens.

Copy link
Contributor

@shijin-aws shijin-aws Dec 13, 2024

Choose a reason for hiding this comment

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

You need to make efa_msg_ops as an external struct for your unit test to declare

Use FI_EP_RDM as ep type and override efa_msg_ops.

Signed-off-by: Jessie Yang <[email protected]>
Copy link
Contributor

@shijin-aws shijin-aws left a comment

Choose a reason for hiding this comment

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

Thanks!

@shijin-aws
Copy link
Contributor

Ideally we should add some more validation in the rdma-core level to make sure sge, imm data are set correctly, but that is likely beyond the scope of this PR. We will start from this one and explore that further validation as follow-ups.

@shijin-aws shijin-aws merged commit de520c2 into ofiwg:main Dec 13, 2024
13 checks passed
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.

2 participants