-
Notifications
You must be signed in to change notification settings - Fork 423
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/RDMACM: fix uct_rdmacm_cm_cqs hash key #6495
Conversation
src/uct/ib/rdmacm/rdmacm_cm_ep.c
Outdated
@@ -192,6 +192,11 @@ static ucs_status_t uct_rdmacm_cm_create_dummy_qp(struct rdma_cm_id *id, | |||
return UCS_OK; | |||
} | |||
|
|||
static uint64_t uct_rdamcm_pd_get_key(const struct ibv_pd *pd) | |||
{ | |||
return (uintptr_t)pd ^ pd->handle; |
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 you pls add comment about container?
- maybe the key should be both pd and handle? and not a xor of them? there is some chance that pd1 and pd2 will be considered same because xor value would be the same. so need to make sure each of them is also compared.
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.
do you mean 86-bit key? maybe just set (or xor) the handle to hight 32 bits of 64-bit key initialized by the pd ptr?
return (uintptr_t)pd ^ ((uint64_t)pd->handle << 32);
or
uint64_t key64;
uint32_t *key32_p = &key64;
key32_p[0] = pd->handle;
key32_p[1] = (uint32_t)(uintptr_t)pd;
return key64;
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.
yes, i mean a struct of {pd, handle}
if we use xor, or reduce the pointer to 32 bit, it's not a 100% guarantee that we get the right PD, so could end up using CQ object with wrong PD
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.
just to clarify - should not pd->handle
guarantee this in normal case? I cant find any mention in docs usage examples or purposes of this handle. If yes, then it does not makes sence to me otherwise we can even do simpler:
return pd->handle ? pd->handle : pd;
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.
use GUID as key:
ibv_get_device_guid(struct ibv_device *device);
a355e6a
to
f4bf333
Compare
f4bf333
to
33fb22e
Compare
bot:pipe:retest |
@avildema, can we check some simple CS test inside container in CI somehow? |
Backport from master branch: openucx#6495 Signed-off-by: Evgeny Leksikov <evgenylek@mellanox.com>
What
fix uct_rdmacm_cm_cqs hash key
Why ?
pd->handle
is always0
in containerHow ?
make safer key