enable tracker in ProxyLib by default #761

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

Jump to bottom

Merged

ldorau merged 5 commits into oneapi-src:main from bratpiorka:rrudnick_proxy_tracking_flag

Nov 12, 2024

Contributor

bratpiorka commented Sep 26, 2024 •

edited

Loading

Description

Enable Tracking Provider in the Proxy Lib by default.

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

bratpiorka force-pushed the rrudnick_proxy_tracking_flag branch from c6c79fb to 3636263 Compare

October 1, 2024 14:28

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 force-pushed the rrudnick_proxy_tracking_flag branch 2 times, most recently from 8393472 to 96e05e8 Compare

October 2, 2024 14:58

bratpiorka changed the title ~~add env to enable tracking prov in proxy lib~~ enable tracker in ProxyLib by default

bratpiorka force-pushed the rrudnick_proxy_tracking_flag branch from 96e05e8 to 1a7c33e Compare

October 3, 2024 08:16

ldorau requested changes

View reviewed changes

src/proxy_lib/proxy_lib.c Show resolved Hide resolved

test/ipc_os_prov_proxy.sh Outdated Show resolved Hide resolved

bratpiorka force-pushed the rrudnick_proxy_tracking_flag branch 3 times, most recently from 10a5b7f to 722ad17 Compare

October 10, 2024 06:38

bratpiorka force-pushed the rrudnick_proxy_tracking_flag branch 9 times, most recently from b61e0bf to 0c0e480 Compare

October 14, 2024 16:51

bratpiorka requested a review from ldorau

October 15, 2024 08:20

bratpiorka marked this pull request as ready for review

October 15, 2024 08:20

bratpiorka requested a review from a team as a code owner

October 15, 2024 08:20

bratpiorka requested a review from vinser52

October 15, 2024 08:21

ldorau requested changes

View reviewed changes

src/provider/provider_tracking.c Outdated Show resolved Hide resolved

test/CMakeLists.txt Outdated Show resolved Hide resolved

test/CMakeLists.txt Outdated Show resolved Hide resolved

test/CMakeLists.txt Outdated Show resolved Hide resolved

test/CMakeLists.txt Outdated Show resolved Hide resolved

vinser52 reviewed

View reviewed changes

CMakeLists.txt Show resolved Hide resolved

src/provider/provider_tracking.c Outdated Show resolved Hide resolved

bratpiorka force-pushed the rrudnick_proxy_tracking_flag branch from 0c0e480 to 180b06b Compare

October 18, 2024 07:58

bratpiorka marked this pull request as draft

October 18, 2024 09:32

bratpiorka force-pushed the rrudnick_proxy_tracking_flag branch 3 times, most recently from 5d76c37 to 9603d32 Compare

October 19, 2024 11:33

bratpiorka force-pushed the rrudnick_proxy_tracking_flag branch 2 times, most recently from dd22268 to 569a2c0 Compare

November 7, 2024 09:38

Contributor

ldorau commented Nov 7, 2024

@bratpiorka Rebase please

bratpiorka force-pushed the rrudnick_proxy_tracking_flag branch 4 times, most recently from 6ddabc9 to 536b05b Compare

November 7, 2024 13:16

vinser52 reviewed

View reviewed changes

src/provider/provider_tracking.c Outdated

+                  LOG_DEBUG("allocated %p, provider: %p, size: %zu", *ptr,
+                            (void *)p->hUpstream, size);
+                  // check if the allocation was already added to the tracker

Contributor

vinser52 Nov 7, 2024

this logic is not needed anymore, right?

Contributor Author

bratpiorka Nov 8, 2024

right, removed

src/provider/provider_tracking.c Show resolved Hide resolved

bratpiorka force-pushed the rrudnick_proxy_tracking_flag branch from 536b05b to ac7a88f Compare

November 8, 2024 08:32

bratpiorka requested review from vinser52 and ldorau

November 8, 2024 09:30

bratpiorka marked this pull request as ready for review

November 8, 2024 09:31

ldorau requested changes

View reviewed changes

src/provider/provider_devdax_memory.c Show resolved Hide resolved

ldorau approved these changes

View reviewed changes

vinser52 requested changes

View reviewed changes

src/provider/provider_tracking.c Show resolved Hide resolved

src/proxy_lib/proxy_lib.c Show resolved Hide resolved

PatKamin reviewed

View reviewed changes

test/ipc_os_prov_proxy.c Outdated

+                  size_t nbytes = 1;
+                  char *found = NULL;
+                  while (nbytes > 0 && found == NULL) {
+                      memset(buf, 0, nbytes); // erase previous data

Contributor

PatKamin Oct 21, 2024

You should erase the whole buf: nbytes -> size_buf

Contributor

ldorau Nov 12, 2024

Verified - it was fixed.

test/ipc_os_prov_proxy.c Outdated

+                  char *found = NULL;
+                  while (nbytes > 0 && found == NULL) {
+                      memset(buf, 0, nbytes); // erase previous data
+                      nbytes = read(fd, buf, size_buf);

Contributor

PatKamin Oct 21, 2024

Check for a read error: nbytes == -1

Contributor

ldorau Nov 12, 2024

Verified - it was fixed.

test/ipc_os_prov_proxy.c

+                  size_t size = 2137;
+                  void *ptr = malloc(size);
+                  if (ptr == NULL) {
+                      ret = -1;

Contributor

PatKamin Oct 21, 2024

Print an error

Contributor Author

bratpiorka Nov 12, 2024

done

Contributor

ldorau Nov 12, 2024

Verified.

test/ipc_os_prov_proxy.c Outdated

+                  }
+                  fprintf(stderr, "Allocated memory - %zu\n", size);
+                  unsigned long long val = 144;

Contributor

PatKamin Oct 21, 2024

Why not using size_t?

Contributor

ldorau Nov 12, 2024

Verified - it was fixed.

src/CMakeLists.txt

-              if(UMF_PROXY_LIB_ENABLED
-                 AND NOT UMF_LINK_HWLOC_STATICALLY
-                 AND NOT UMF_DISABLE_HWLOC)
+              if(UMF_PROXY_LIB_ENABLED)

Contributor

PatKamin Nov 8, 2024

AND UMF_BUILD_SHARED_LIBRARY?

Contributor Author

bratpiorka Nov 12, 2024

UMF_PROXY_LIB_ENABLED is set only in shared library mode, see https://github.com/oneapi-src/unified-memory-framework/pull/761/files#diff-1e7de1ae2d059d21e1dd75d5812d5a34b0222cef273b7c3a2af62eb747f9d20aR439

Contributor

ldorau Nov 12, 2024

Verified.

PatKamin reviewed

View reviewed changes

test/ipc_os_prov_proxy.c Outdated

+                  // read the "/proc/self/maps" file until the "libumf_proxy.so" of the maps
+                  // is found or EOF is reached.
+                  const size_t size_buf = 8192;

Contributor

PatKamin Nov 8, 2024

nit: uppercase constant

Contributor Author

bratpiorka Nov 12, 2024

done

Contributor

ldorau Nov 12, 2024

Verified.

test/ipc_os_prov_proxy.c Outdated

+                  ssize_t len =
+                      send(producer_socket, &ipc_handle_size, sizeof(ipc_handle_size), 0);
+                  if (len < 0) {
+                      fprintf(stderr, "[producer] ERROR: unable to send the message\n");

Contributor

PatKamin Nov 8, 2024

More descriptive: unable to send the ipc_handle_size to the consumer?

Contributor Author

bratpiorka Nov 12, 2024

done

Contributor

ldorau Nov 12, 2024

Verified.

test/ipc_os_prov_proxy.c Outdated

+                  // send the ipc_handle of ipc_handle_size to the consumer
+                  if (send(producer_socket, ipc_handle, ipc_handle_size, 0) < 0) {
+                      fprintf(stderr, "[producer] ERROR: unable to send the message\n");

Contributor

PatKamin Nov 8, 2024

More descriptive: unable to send the ipc_handle to the consumer?

Contributor Author

bratpiorka Nov 12, 2024

done

Contributor

ldorau Nov 12, 2024

Verified.

ldorau mentioned this pull request

Add threshold to proxy lib to call system allocator #883

Merged

4 tasks

bratpiorka added 5 commits

November 12, 2024 10:35


          enable tracker in ProxyLib by default

0d64d11


          add IPC test with proxy lib

57b96f9


          enable Proxy with static HWLOC

62d7b06


          disable Proxy Lib when UMF is build as static lib

05ddc12


          fix devdax_allocation_merge arg checks

09ee70e

bratpiorka force-pushed the rrudnick_proxy_tracking_flag branch from ac7a88f to 09ee70e Compare

November 12, 2024 09:36

bratpiorka requested review from PatKamin and vinser52

November 12, 2024 09:39

vinser52 approved these changes

View reviewed changes

ldorau merged commit f8bf899 into oneapi-src:main

74 checks passed

This was referenced Nov 12, 2024

Do not run tests under proxy lib if libumf is not a shared lib #890

Merged

Minor docs changes #893

Merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet