Skip to content

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

SRC/UCP/CORE: set FLAG_LOCK for allocated buffers when NONBLOCK is not passed #10830

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/ucp/core/ucp_mm.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -319,7 +319,8 @@ ucp_mem_map_params2uct_flags(const ucp_context_h context,
if (params->field_mask & UCP_MEM_MAP_PARAM_FIELD_FLAGS) {
if (params->flags & UCP_MEM_MAP_NONBLOCK) {
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)) {
Copy link
Contributor

@gleon99 gleon99 Aug 28, 2025

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?

Copy link
Contributor

@brminich brminich Aug 28, 2025

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?

Copy link
Contributor

@brminich brminich Aug 28, 2025

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_TYPES is "host,cuda-managed" its not really empty

it is default only for grace

Copy link
Contributor Author

@roiedanino roiedanino Aug 28, 2025

Choose a reason for hiding this comment

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

Yes my bad

Copy link
Contributor Author

@roiedanino roiedanino Aug 28, 2025

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)

Copy link
Contributor

@gleon99 gleon99 Aug 28, 2025

Choose a reason for hiding this comment

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

👍🏼

Copy link
Contributor

@gleon99 gleon99 Aug 28, 2025 •
edited
Loading

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.

Copy link
Contributor

@brminich brminich Aug 28, 2025

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

Copy link
Contributor

@yosefe yosefe Aug 28, 2025

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.

Copy link
Contributor

@brminich brminich Aug 28, 2025

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

flags |= UCT_MD_MEM_FLAG_LOCK;
}

Expand Down
Loading