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

UCT/API: Add uct_rkey_compare() function #9297

Merged
merged 1 commit into from
Sep 1, 2023

Conversation

tvegas1
Copy link
Contributor

@tvegas1 tvegas1 commented Aug 17, 2023

What

Add uct_rkey_compare() API, relates to #9289.

Why ?

We need to compare remote keys to deduplicate received remote keys, > 0, = 0 or < 0.

How ?

Usage:

uct_rkey_compare_params_t params = {};
status = uct_rkey_compare(context, rkey1, rkey2, &params, &result);

@tvegas1 tvegas1 added the API label Aug 17, 2023
brminich
brminich previously approved these changes Aug 17, 2023
/**
* Register the memory region so its remote access key would likely be
* equal to remote access keys received from other peers, when compared
* with @ref uct_rkey_compare. This flag is best-effort. When remote access
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest the following to solve docs build problem

Suggested change
* with @ref uct_rkey_compare. This flag is best-effort. When remote access
* with @a uct_rkey_compare. This flag is best-effort. When remote access

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

@brminich
Copy link
Contributor

@yosefe, @shamisp - pls take a look

brminich
brminich previously approved these changes Aug 22, 2023
yosefe
yosefe previously approved these changes Aug 22, 2023
* with @a uct_rkey_compare. This flag is a best-effort hint. When remote
* access keys received from different peers are compared equal, they can
* be used interchangeably, avoiding the need to keep all of them in
* memory.
Copy link
Contributor

Choose a reason for hiding this comment

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

All the comment for UCP API are applicable here.

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

/**
* @ingroup UCT_MD
*
* @brief Compare two remote keys
Copy link
Contributor

Choose a reason for hiding this comment

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

this is not a sentence

Copy link
Contributor Author

Choose a reason for hiding this comment

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

replaced by This routine compares two remote keys.

* semantic is arbitrary and implementation dependent, nevertheless it's
* guaranteed to be consistent (transitive and antisymmetric). It can be used,
* for example, to perform an efficient binary search in a sorted array of
* remote keys.
Copy link
Contributor

Choose a reason for hiding this comment

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

the same comments as in UCP code review

Copy link
Contributor Author

Choose a reason for hiding this comment

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

replaced by shorter paragraph.

* @param[in] params Additional parameters for comparison
* @param[out] result Result of the comparison
*
* @return Error code or UCS_OK if result contains the comparison result
Copy link
Contributor

Choose a reason for hiding this comment

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

What UCP will do if error returned ? What does unsupported means ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The proposal is to have UCP immediately return that error.
The unsupported below is to be replaced by actual implementation after this PR is merged.

Copy link
Contributor

Choose a reason for hiding this comment

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

what UCP will return if underlaying layer reports unsuported

Copy link
Contributor Author

Choose a reason for hiding this comment

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

please check UCP side response: #9289 (comment)

Copy link
Contributor

@shamisp shamisp left a comment

Choose a reason for hiding this comment

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

please see above comments

@tvegas1 tvegas1 dismissed stale reviews from yosefe and brminich via c498968 August 25, 2023 06:50
yosefe
yosefe previously approved these changes Aug 25, 2023
brminich
brminich previously approved these changes Aug 28, 2023
* @brief This routine compares two remote keys.
*
* It sets the @a result argument to < 0 if rkey1 is lower than rkey2, 0 if they
* are equal or > 0 if rkey1 is greater than rkey2. It can be used for sorting.
Copy link
Contributor

Choose a reason for hiding this comment

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

E.g. the result value can be used for sorting remote keys

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

@tvegas1 tvegas1 dismissed stale reviews from brminich and yosefe via e0ca269 August 30, 2023 06:58
@tvegas1 tvegas1 requested a review from shamisp August 30, 2023 07:05
shamisp
shamisp previously approved these changes Aug 30, 2023
yosefe
yosefe previously approved these changes Aug 31, 2023
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.

pls squash

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.

@tvegas1 next time pls use a merge commit instead of force-push

@yosefe yosefe merged commit 73a4870 into openucx:master Sep 1, 2023
113 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants