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

UCP/API: Add recv_info to ucp_request_param_t #5919

Merged
merged 1 commit into from
Nov 26, 2020

Conversation

brminich
Copy link
Contributor

What

Add recv_info to ucp_request_param_t, so that *recv_nbx function can get receive data details in case of immediate completion.

Why ?

If operation completes immediately the callback is not invoked. Need to provide the length (and other info) of received data to the user.

* in case of immediate completion of receive operation.
*/
union {
size_t *length;
Copy link
Member

Choose a reason for hiding this comment

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

Do we need uint32_t mask here to show which fields were set by UCP?
e.g. I'd not expect that tag is updated by UCP AM API, but a user could specify it (e.g. user shares the same request params for TAG and AM operations)

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 do not think so. This is somewhat similar to cb field of this struct

Copy link
Member

Choose a reason for hiding this comment

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

Ah, this is union
I see

@brminich
Copy link
Contributor Author

@yosefe, @shamisp, @tonycurtis, can you please take a look?

* in case of immediate completion of receive operation.
*/
union {
size_t *length;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please add description for each one of the fields. I guess user has to allocate the memory and then it is updated in function return ?

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added

*/
union {
size_t *length;
ucp_tag_recv_info_t *tag;
Copy link
Contributor

Choose a reason for hiding this comment

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

variable name tag is very confusing. Why not info , seems like this is the var name used everywhere else.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

to avoid double info, because it is a part of recv_info union, so it will be recv_info.tag_info

Copy link
Contributor

Choose a reason for hiding this comment

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

FYI, we use info everywhere else, so I have no issue with double naming. tag_info is double-double naming :) If you have strong preference towards tag_info I can accept it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@shamisp
Copy link
Contributor

shamisp commented Nov 21, 2020

👍

@yosefe
Copy link
Contributor

yosefe commented Nov 21, 2020

bot:pipe:retest

@brminich
Copy link
Contributor Author

@tonycurtis, can you please review?

Copy link
Contributor

@tonycurtis tonycurtis left a comment

Choose a reason for hiding this comment

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

1423: in the case of an immediate...
1423: The user has to ...
1432: previous comment split into 2 sentences, should be consistent here
2827: value to which it points ... of the received...
3396: ditto

@brminich
Copy link
Contributor Author

1423: in the case of an immediate...
1423: The user has to ...
1432: previous comment split into 2 sentences, should be consistent here
2827: value to which it points ... of the received...
3396: ditto

fixed

@brminich
Copy link
Contributor Author

@tonycurtis, do you have other comments?

@yosefe
Copy link
Contributor

yosefe commented Nov 25, 2020

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@brminich
Copy link
Contributor Author

bot:pipe:retest

@brminich
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@brminich brminich merged commit 91a84fc into openucx:master Nov 26, 2020
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.

5 participants