-
Notifications
You must be signed in to change notification settings - Fork 490
Pass nodelay flag in ucp perftest #10894
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
Conversation
| size_t m_size; | ||
| ucp_device_request_t m_requests[CAPACITY]; | ||
| uint8_t m_pending[UCX_BITSET_SIZE(CAPACITY)]; | ||
| uint8_t m_pending[UCX_BITSET_SIZE(CAPACITY)]; |
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 revert
| status = ucp_device_put_single<level>(mem_list, mem_list_index, address, | ||
| remote_address, length, 0, &req); | ||
| remote_address, length, | ||
| UCP_DEVICE_FLAG_NODELAY, &req); |
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.
how did it pass CI?
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.
Hmm, it works fine on my side without this flag
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.
Can we keep only this change and exclude formatting changes?
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.
Did it worked for you with two nodes? With single node & cudaipc it will not go throw NIC so it will work without DB. (Maybe this is why CI passed?)
I can just add the flag and revert the rest. We need it to measure perf so I don't want to wait for PR with many changes that require review
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.
Right, I haven't tested with two nodes
Thanks for reducing the change
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.
It seems this PR only sets a flag, which I can probably just add in #10893
All the formatting changes are redundant, most of these functions are changed anyway in the PR above, and it will just cause merging issues
What?
Pass
UCP_DEVICE_FLAG_NODELAYflag when posting in UCP perftest.Why?
Test hangs without it.