Skip to content

[CTL] Add disjoint pool allocation balance counter #1334

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

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

Conversation

KFilipek
Copy link
Contributor

Description

Checklist

  • Code compiles without errors locally
  • All tests pass locally
  • CI workflows execute properly
  • CI workflows, not executed per PR (e.g. Nightly), 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
  • All newly added source files have a license
  • All newly added source files are referenced in CMake files
  • Logger (with debug/info/... messages) is used
  • All API changes are reflected in docs and def/map files, and are tested

@KFilipek KFilipek self-assigned this May 28, 2025
@KFilipek KFilipek requested a review from a team as a code owner May 28, 2025 10:15
@KFilipek KFilipek added the enhancement New feature or request label May 28, 2025
@KFilipek KFilipek force-pushed the ctl-disjoint_counters branch from 6c4e59a to ba5c390 Compare May 28, 2025 10:17
static const umf_ctl_node_t CTL_NODE(disjoint)[] = {CTL_LEAF_RW(name),
CTL_NODE_END};
// CTL: allocation counters
atomic_int counter_alloc_balance;
Copy link
Contributor

Choose a reason for hiding this comment

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

C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.43.34808\include\vcruntime_c11_stdatomic.h(16,1): error C1189: #error:  "C atomics require C11 or later" [D:\a\unified-memory-framework\unified-memory-framework\build\src\umf.vcxproj]
  (compiling source file '../../src/pool/pool_disjoint.c')

https://github.com/oneapi-src/unified-memory-framework/actions/runs/15297591937/job/43030445792?pr=1334

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you should use our utils from utils_concurrency.h.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@KFilipek KFilipek force-pushed the ctl-disjoint_counters branch from ba5c390 to 3bf6fd7 Compare May 28, 2025 11:42
@@ -745,7 +761,7 @@ umf_result_t disjoint_pool_initialize(umf_memory_provider_handle_t provider,
void *disjoint_pool_malloc(void *pool, size_t size) {
disjoint_pool_t *hPool = (disjoint_pool_t *)pool;
void *ptr = disjoint_pool_allocate(hPool, size);

atomic_fetch_add(&counter_alloc_balance, 1);
Copy link
Contributor

Choose a reason for hiding this comment

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

use utils_atomic_increment_u64

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@KFilipek KFilipek force-pushed the ctl-disjoint_counters branch from 3bf6fd7 to 9dce66e Compare June 6, 2025 08:51
@KFilipek KFilipek requested review from bratpiorka and ldorau June 6, 2025 09:25
@KFilipek KFilipek force-pushed the ctl-disjoint_counters branch from 9dce66e to 0609557 Compare June 6, 2025 10:14
Copy link
Contributor

@pbalcer pbalcer left a comment

Choose a reason for hiding this comment

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

it might also be useful to store total allocated size, not just the number of allocations.

static const umf_ctl_node_t CTL_NODE(disjoint)[] = {CTL_LEAF_RW(name),
CTL_NODE_END};
// CTL: allocation counters
uint64_t allocation_balance = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

static

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -33,6 +33,7 @@ static char *DEFAULT_NAME = "disjoint";
struct ctl disjoint_ctl_root;
static UTIL_ONCE_FLAG ctl_initialized = UTIL_ONCE_FLAG_INIT;

// CTL: name attribute
Copy link
Contributor

Choose a reason for hiding this comment

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

isn't this something we could implement generically for any pool implementation in the umfPoolAlloc/umfPoolFree interface?

// CTL: allocation counters
uint64_t allocation_balance = 0;

static int CTL_READ_HANDLER(allocation_balance)(
Copy link
Contributor

Choose a reason for hiding this comment

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

we should have docs that list all ctl interfaces.

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 will prepare the docs after this PR, which we discussed previously (with @lplewa @bratpiorka).

@KFilipek KFilipek force-pushed the ctl-disjoint_counters branch 5 times, most recently from cef102e to d30e9f9 Compare June 6, 2025 12:06
@KFilipek KFilipek force-pushed the ctl-disjoint_counters branch from d30e9f9 to 2c7329a Compare June 6, 2025 12:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants