UCT/GGA: rkey resolution for many MDs #10236
Conversation
evgeny-leksikov
force-pushed
the
uct_gga_ep_put_get_mds_rkey
branch
from
fab25db
to
c86b266
Compare
|
/azp run |
|
Azure Pipelines successfully started running 4 pipeline(s). |
src/uct/ib/mlx5/gga/gga_mlx5.c
Outdated
| md_attr->rkey_packed_size = md_attr->exported_mkey_packed_size; | ||
| return UCS_OK; | ||
| } | ||
| static pthread_mutex_t uct_gga_mlx5_rkeys_locker = PTHREAD_MUTEX_INITIALIZER; |
There was a problem hiding this comment.
maybe use "_lock" or "_mutex" to conform with the codebase
Maybe store it in the component uct_gga_component?
There was a problem hiding this comment.
Maybe store it in the component uct_gga_component?
I also thought about it, but don't found much benefit, just more code related to new type/struct
|
|
||
| *rkey_p = (uintptr_t)rkey_handle; | ||
| *handle_p = NULL; | ||
| return UCS_OK; | ||
| } | ||
|
|
||
| static ucs_status_t | ||
| uct_gga_mlx5_md_put_rkey(uct_gga_mlx5_md_t *md, | ||
| const uct_gga_mlx5_rkey_hash_key_t key, |
There was a problem hiding this comment.
minor: since key is primitive const is redundant
There was a problem hiding this comment.
it's more for readability and clean API + uct_gga_mlx5_rkey_hash_key_t might be changed in future, so this place won't require changes
src/uct/ib/mlx5/gga/gga_mlx5.c
Outdated
|
|
||
| iter = kh_put(resolved_rkeys, &md->rkey_cache, key, &ret); | ||
| if (ret == UCS_KH_PUT_FAILED) { | ||
| ucs_error("md %p: failed to add rkey to hash", md); |
There was a problem hiding this comment.
here and in ucs_assert I propose to print key value
| return ucs_unlikely(iter == kh_end(&md->rkey_cache)) ? NULL : | ||
| kh_val(&md->rkey_cache, iter); |
There was a problem hiding this comment.
maybe:
| return ucs_unlikely(iter == kh_end(&md->rkey_cache)) ? NULL : | |
| kh_val(&md->rkey_cache, iter); | |
| return ucs_unlikely(iter == kh_end(&md->rkey_cache)) ? | |
| NULL : kh_val(&md->rkey_cache, iter); |
| @@ -192,57 +280,124 @@ static uct_component_t uct_gga_component = { | |||
| }; | |||
|
|
|||
| static UCS_F_ALWAYS_INLINE | |||
| void uct_gga_mlx5_rkey_trace(uct_ib_mlx5_md_t *md, | |||
| void uct_gga_mlx5_rkey_trace(uct_gga_mlx5_md_t *md, | |||
There was a problem hiding this comment.
minor: void can be on previous line like in other places
| } | ||
|
|
||
| attach_params.field_mask = 0; | ||
| pthread_mutex_lock(&uct_gga_mlx5_rkeys_lock); |
There was a problem hiding this comment.
Do we need to use the same mutex which is protecting the hash map?
If it's not necessary, then we may consider having static local mutex protecting only mem_attach call, for better scalability
What
rkey resolution for many MDs in GGA transport
Why ?
#9993 (comment)
How ?