-
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
UCP/CORE: Fix memory cache lookup return value handling #7737
Conversation
} | ||
if (ucs_likely(status == UCS_ERR_NO_ELEM)) { | ||
goto out_host_mem; | ||
} else { |
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.
remove else?
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.
fixed and force-pushed (the PR is small enough)
@dmitrygx the problem was extra detect overhead when not needed? |
8610669
to
2e90f60
Compare
src/ucp/core/ucp_context.h
Outdated
|
||
if ((mem_info->type != UCS_MEMORY_TYPE_UNKNOWN) && | ||
(mem_info->sys_dev != UCS_SYS_DEVICE_ID_UNKNOWN)) { | ||
return; |
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.
maybe move line 517 inside this if and inverse it's condition
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.
done
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.
maybe remove this return?
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.
done
src/ucp/core/ucp_context.h
Outdated
* memory domains. In any case, the memory type cache is not expected to | ||
* return HOST memory type. | ||
*/ | ||
ucs_assert(mem_info->type != UCS_MEMORY_TYPE_HOST); |
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.
maybe move this assert to line 505
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.
good catch, done
yes, if it returns also, it fixes incorrect assertion:
|
src/ucp/core/ucp_context.h
Outdated
|
||
if ((mem_info->type != UCS_MEMORY_TYPE_UNKNOWN) && | ||
(mem_info->sys_dev != UCS_SYS_DEVICE_ID_UNKNOWN)) { | ||
return; |
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.
maybe remove this return?
src/ucp/core/ucp_context.h
Outdated
goto out_host_mem; | ||
} | ||
|
||
ucs_assertv((status == UCS_OK) || (status == UCS_ERR_UNSUPPORTED), |
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.
why would we get UCS_ERR_UNSUPPORTED?
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.
I've updated PR to return UCS_ERR_UNSUPPORTED
in case of MEMTYPE is disabled to distinguish between no element (HOST mem buffer) or always use memory detection (slowpath)
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.
what is the advantage? it adds another branch (line 507)
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.
what is the advantage? it adds another branch (line 507)
it fixes the problem with memory type detection if MEMTYPE cache wasn't enabled - we should go to memory detection through MDs. without this check - we assumed that memory type is HOST.
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.
interesting...
maybe we should set mem_info->type=UNKNOWN in this case?
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.
Actually it would be weird to set mem_info in case of error return,
so guess better restore an equivalent of the prev logic where we checked if memtype_cache was enabled in the first place. keep it UNSUPPORTED
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.
so guess better restore an equivalent of the prev logic where we checked if memtype_cache was enabled in the first place. keep it UNSUPPORTED
do you mean checking status == UCS_ERR_UNSUPPORTED
and doing memory detection in this case?
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
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.
done
src/ucp/core/ucp_context.h
Outdated
if ((status == UCS_ERR_UNSUPPORTED) || | ||
(mem_info->type == UCS_MEMORY_TYPE_UNKNOWN) || | ||
(mem_info->sys_dev == UCS_SYS_DEVICE_ID_UNKNOWN)) { | ||
if (ucs_unlikely((status == UCS_ERR_UNSUPPORTED) || |
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.
- pls simplify, i think we can avoid "goto" now
- host memory is the fast path and should be checked first
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.
host memory is the fast path and should be checked first
done
pls simplify, i think we can avoid "goto" now
can't remove goto
- two places use it - and can't merge them into single if
statement, because context->num_mem_type_detect_mds == 0
must be checked before calling ucs_memtype_cache_lookup()
bd42ac7
to
8659677
Compare
What
Fix memory cache lookup return value handling.
Why ?
The return value could be either
UCS_OK
orUCS_ERR_NO_ELEM
.How ?