- 
                Notifications
    
You must be signed in to change notification settings  - Fork 490
 
SRC/UCP/CORE: set FLAG_LOCK for allocated buffers when NONBLOCK is not passed #10830
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
base: master
Are you sure you want to change the base?
Conversation
…t passed to ucp_mem_map Signed-off-by: Roie Danino <rdanino@nvidia.com>
| flags |= UCT_MD_MEM_FLAG_NONBLOCK; | ||
| } else if (params->flags & UCP_MEM_MAP_LOCK) { | ||
| } else if ((params->flags & UCP_MEM_MAP_LOCK) || | ||
| (params->flags & UCP_MEM_MAP_ALLOCATE)) { | 
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'm not sure that would always work.
It leads to usage of reg_block_md_map (https://github.com/roiedanino/ucx/blob/reg-nb/src/ucp/core/ucp_mm.c#L866)
which is populated here.
But if UCX_REG_NONBLOCK_MEM_TYPES is empty (as default), that map would be empty.
@yosefe wdyt?
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.
should be ok, because it just adds context->reg_block_md_map[mem_type] to the existing reg_map.
but what does it fix? Why would we need this change?
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.
The default for
UCX_REG_NONBLOCK_MEM_TYPESis "host,cuda-managed" its not really empty
it is default only for grace
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 my bad
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.
The issue is on grace machines the default is not empty and then even if NONBLOCK flag wasn't passed it would still use NONBLOCK registration (unless LOCK flag was set on)
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.
👍🏼
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.
Meaning, the change will have effect only when UCX_REG_NONBLOCK_MEM_TYPES has a non-empty value.
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.
but we have UCP_MEM_MAP_LOCK for those who want blocking behavior?
otherwise UCX may decide what is better
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.
There is more to it.. we need to cancel all effects of REG_NONBLOCK_MEM_TYPES in case of memory allocation. Here, we create another effect (passing UCT lock flag) and we don't prevent the registration md map filtering - that limits registration only to MD that support NONBLOCK registration.
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.
that was the intent to use ODP on GH machines by default
The issue of  ODP+ DPP is being addressed here #10803
What?
Set
UCT_MD_MEM_FLAG_LOCKflag when user is allocating a buffer using ucp_mem_map without passingUCP_MEM_MAP_NONBLOCKflagWhy?
Otherwise, non-blocking registration (ODP) can still happen in
ucp_memh_register_internal