Skip to content
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

enable tracker in ProxyLib by default #761

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

bratpiorka
Copy link
Contributor

@bratpiorka bratpiorka commented Sep 26, 2024

Description

Enable Tracking Provider in the Proxy Lib by default

It is currently disabled by default to not enable the Tracking Provider twice if both Proxy Lib and regular UMF is used in the same process (see #365 and fix #380).

However, in CAL scenarios where one process is using the Proxy Lib and the second uses IPC API for memory management, all the allocations in the first process have to be registered in the tracker.

This PR:

  • enables Tracking Provider in the Proxy Lib by default and adds test for mix of Proxy Lib and IPC API
  • changes IPC tests to use Pool (Scalable - requires TBB) instead of Provider API
  • adds 60 sec timeout for IPC tests

Checklist

  • Code compiles without errors locally
  • All tests pass locally
  • CI workflows execute properly
  • New tests added, especially if they will fail without my changes
  • Added/extended example(s) to cover this functionality
  • Extended the README/documentation
  • Logger (with debug/info/... messages) is used

@vinser52
Copy link
Contributor

vinser52 commented Oct 2, 2024

Just to note it here:

As we agreed yesterday, the proxy lib should ON the memory tracker for the proxy pool. In general memory tracker (as an essential part of UMF) should be always ON. For the use cases when tracking should be disabled for some reasons the umfPoolCreate already provides an option to disable memory tracking for a particular memory pool by specifying the UMF_POOL_CREATE_FLAG_DISABLE_TRACKING flag.

@bratpiorka bratpiorka force-pushed the rrudnick_proxy_tracking_flag branch 2 times, most recently from 8393472 to 96e05e8 Compare October 2, 2024 14:58
@bratpiorka bratpiorka changed the title add env to enable tracking prov in proxy lib enable tracker in ProxyLib by default Oct 2, 2024
src/proxy_lib/proxy_lib.c Show resolved Hide resolved
test/ipc_os_prov_proxy.sh Outdated Show resolved Hide resolved
@bratpiorka bratpiorka force-pushed the rrudnick_proxy_tracking_flag branch 3 times, most recently from 10a5b7f to 722ad17 Compare October 10, 2024 06:38
@bratpiorka bratpiorka force-pushed the rrudnick_proxy_tracking_flag branch 8 times, most recently from 602ccfc to b61e0bf Compare October 14, 2024 16:42
@bratpiorka bratpiorka marked this pull request as ready for review October 15, 2024 08:20
@bratpiorka bratpiorka requested a review from a team as a code owner October 15, 2024 08:20
Comment on lines +176 to +177
// the allocation was made by the ProxyLib so we only update the tracker
value->pool = p->pool;
Copy link
Contributor

Choose a reason for hiding this comment

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

So we assume that ProxyLib will not look for this pointer any more? Is it based on the fact that ProxyLib does not call umfPoolByPtr() in its source code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. Also the proxy lib would not free this allocation - this would be done by the user created Pool

Comment on lines +450 to +452
# TODO - fix ipc_file_prov and ipc_file_prov_fsdax tests
# add_umf_ipc_test(TEST ipc_file_prov) add_umf_ipc_test(TEST
# ipc_file_prov_fsdax)
Copy link
Contributor

Choose a reason for hiding this comment

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

An issue should be added for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ldorau I will wait for your fix #800 to be merged and I will remove this

Comment on lines -445 to +459
if(UMF_BUILD_GPU_TESTS AND UMF_BUILD_LEVEL_ZERO_PROVIDER)
if(UMF_BUILD_GPU_TESTS
AND UMF_BUILD_LEVEL_ZERO_PROVIDER
AND UMF_BUILD_LIBUMF_POOL_DISJOINT)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does it belong to this PR? What does it have in common?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have to use Disjoint Pool in GPU tests

@@ -453,6 +467,7 @@ if(LINUX)
providers/level_zero_helpers.cpp
LIBS
ze_loader
disjoint_pool
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does it belong to this PR? What does it have in common?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have to use Disjoint Pool in GPU tests

@@ -464,6 +479,7 @@ if(LINUX)
providers/level_zero_helpers.cpp
LIBS
ze_loader
disjoint_pool
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does it belong to this PR? What does it have in common?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have to use Disjoint Pool in GPU tests

# set UMF_PROXY_LIB_ENABLED
if(UMF_PROXY_LIB_BASED_ON_POOL STREQUAL SCALABLE)
if(UMF_LINK_HWLOC_STATICALLY)
message(
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does proxy lib require HWLOC to be linked dynamically?

if (value) {
assert(value->pool != p->pool);

LOG_DEBUG("ptr already exists in the tracker (added by Proxy Lib) - "
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please describe the case when memory already present in the tracker?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants