Skip to content

Conversation

@edgargabriel
Copy link
Member

comparing ipc handles might not be always just a memcmp of the two handles. Introduce an abstraction for this functionality. Use the memcmp function that was used so far in the cuda and ze component, but use only certain parts of the ipc handle in rocm.

@edgargabriel edgargabriel changed the title accelerator: introduce compare_ipc_handles fnction accelerator: introduce compare_ipc_handles fctn Feb 20, 2024
int *pid_2 = (int *)&handle_2[pos];

/* Note: using pid_1 != pid_2 since we need to return 0 if identical */
return (memcmp(handle_1, handle_2, 32) && (*pid_1 != *pid_2));
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure about this - should it be memcmp(handle_1, handle_2, 32) || (*pid_1 != *pid_2) instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think you might be right, I hate these brain twister. I will write down the decision matrix to check

* and the process ID for comparison.
* We definitily need to exclude the offset component in the comparison.
*/
int pos = 32 + 2*sizeof(size_t);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit static const int POS = (int) 32 + 2 * sizeof(size_t); - maybe also need a const for 32.

@edgargabriel edgargabriel changed the title accelerator: introduce compare_ipc_handles fctn accelerator: introduce compare_ipc_handles fnct Feb 20, 2024
@edgargabriel edgargabriel force-pushed the topic/accelerator-ipc-handle-compare branch from 5dc1328 to 9251cd5 Compare February 20, 2024 18:28
@edgargabriel
Copy link
Member Author

@wenduwan thank you for your review, can you please have another look? I think I incorporated both of your comments.

int *pid_1 = (int *)&handle_1[pos];
int *pid_2 = (int *)&handle_2[pos];

return (memcmp(handle_1, handle_2, 32) + (*pid_1 == *pid_2 ? 0 : 1));
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah this might not work because -1 + 1 = 0

Copy link
Member Author

Choose a reason for hiding this comment

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

:-( you are unfortunately right again. Thanks for catching it!

Copy link
Contributor

@wenduwan wenduwan Feb 20, 2024

Choose a reason for hiding this comment

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

A more readable way can be

...
    if (*pid_1 != *pid_2) return *pid_1 > *pid_2 ? 1 : -1;
    return memcmp(handle_1, handle_2, 32);
...

Copy link
Member Author

Choose a reason for hiding this comment

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

that is actually a good idea, I made just one minor change, thanks!

comparing ipc handles might not be always just a memcmp of the two
handles. Introduce an abstraction for this functionality. Use the memcmp
function that was used so far in the cuda and ze component, but use only
certain parts of the ipc handle in rocm.

Signed-off-by: Edgar Gabriel <[email protected]>
@edgargabriel edgargabriel force-pushed the topic/accelerator-ipc-handle-compare branch from 9251cd5 to 4002fbc Compare February 20, 2024 18:43
@edgargabriel edgargabriel merged commit ede12d6 into open-mpi:main Feb 20, 2024
@edgargabriel edgargabriel deleted the topic/accelerator-ipc-handle-compare branch July 12, 2024 12:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants