[CTL] Add support for defaults #1216

KFilipek · 2025-03-21T15:47:08Z

[CTL] Add support for defaults

Description

This PR introduces initial support for setting up default values.

Checklist

Code compiles without errors locally
All tests pass locally
CI workflows execute properly

src/ctl/ctl.c

src/memory_pool.c

src/pool/pool_disjoint_ctl.h

lukaszstolarczuk · 2025-03-24T15:42:37Z

test/pools/disjoint_pool_ctl.cpp

+    umf_os_memory_provider_params_handle_t os_memory_provider_params = NULL;
+    umf_memory_provider_ops_t *os_provider_ops = umfOsMemoryProviderOps();
+    if (os_provider_ops == NULL) {
+        GTEST_SKIP() << "OS memory provider is not supported!";


Instead of adding such skip msg, I guess we should add this test in CMake only when OS prov and disjoint pool are enabled.

It's what for we've added a dummy function.

yy? not sure what you mean

lukaszstolarczuk · 2025-03-24T15:49:14Z

test/ctl/ctl_api.cpp

@@ -35,7 +35,7 @@ TEST_F(test, ctl_by_handle_os_provider) {

    int ipc_enabled = 0xBAD;
    ret = umfCtlGet("umf.provider.by_handle.params.ipc_enabled", hProvider,


do we have any test for umfCtlGet that pass something else than 0?

As I see it's not a part of this PR

You're adding a new paramter and without testing values other than 0 it's not properly tested. At least mark in tests a TODO for future.

This callback discard completely size, so such test will cover completely nothing and will test nothing.

static int CTL_READ_HANDLER(ipc_enabled)(void *ctx, umf_ctl_query_source_t source, void *arg, size_t size, umf_ctl_index_utlist_t *indexes, const char *extra_name, umf_ctl_query_type_t query_type) { /* suppress unused-parameter errors */ (void)source, (void)indexes, (void)ctx, (void)extra_name, (void)query_type, (void)size; int *arg_out = arg; os_memory_provider_t *os_provider = (os_memory_provider_t *)ctx; *arg_out = os_provider->IPC_enabled; return 0; }

I'm reading your last comment as "size is useless" ;)

Let me re-phrase - whenever size is used, please add a test with various values (user may give some input we may not expect, e.g. on wrong assumptions on how this func works) or mark a TODO for adding tests in the future.

src/memory_pool.c

include/umf/memory_pool_ops.h

src/memory_pool.c

src/pool/pool_disjoint_ctl.h

test/ctl/ctl_api.cpp

test/pools/disjoint_pool.cpp

test/pools/disjoint_pool_ctl.cpp

include/umf/memory_pool_ops.h

include/umf/base.h

test/pools/disjoint_pool_ctl.cpp

lplewa

umf_result_t umfCtlExec(const char *name, void *ctx, void *arg); should also have arg size imho.

src/ctl/ctl.c

src/memory_pool.c

src/pool/pool_disjoint_ctl.h

lukaszstolarczuk · 2025-03-26T09:48:27Z

test/ctl/ctl_api.cpp

@@ -35,7 +35,7 @@ TEST_F(test, ctl_by_handle_os_provider) {

    int ipc_enabled = 0xBAD;
    ret = umfCtlGet("umf.provider.by_handle.params.ipc_enabled", hProvider,


You're adding a new paramter and without testing values other than 0 it's not properly tested. At least mark in tests a TODO for future.

lukaszstolarczuk · 2025-03-26T09:49:31Z

test/pools/disjoint_pool_ctl.cpp

+    umf_os_memory_provider_params_handle_t os_memory_provider_params = NULL;
+    umf_memory_provider_ops_t *os_provider_ops = umfOsMemoryProviderOps();
+    if (os_provider_ops == NULL) {
+        GTEST_SKIP() << "OS memory provider is not supported!";


yy? not sure what you mean

test/ctl/ctl_api.cpp

include/umf/base.h

KFilipek · 2025-04-28T10:38:33Z

Please, re-review.

src/memory_pool.c

include/umf/memory_provider_ops.h

src/memory_provider.c

src/pool/pool_disjoint.c

test/ctl/ctl_api.cpp

test/pools/disjoint_pool.cpp

test/pools/disjoint_pool_ctl.cpp

bratpiorka · 2025-04-29T09:27:42Z

@KFilipek please work with other reviewers to close all conversations

test/ctl/ctl_api.cpp

Copilot

Pull Request Overview

This PR introduces support for default values in the CTL interface and updates the CTL query API across the codebase by adding an extra size parameter. Key changes include updates to unit tests to verify default value handling (including multithreaded tests), modifications to CTL handler APIs in providers, pools, and CTL modules, and corresponding documentation updates.

Reviewed Changes

Copilot reviewed 21 out of 23 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
test/ctl/ctl_unittest.cpp	Added extra size parameter to ctl_query calls in unit tests.
test/ctl/ctl_api.cpp	Extended tests with default handling and updated query template.
test/ctl/ctl_debug.c	Updated CTL handlers with new extra size parameter.
src/provider/.c & src/provider/.h	Updated CTL query signatures to include the size parameter.
src/pool/*.c	Adjusted CTL query calls and added default value integration.
src/memory_provider.c, src/memory_pool.c	Updated CTL API use and logic for setting defaults.
src/libumf.c, src/ctl/ctl.[ch]	Updated CTL API function signatures and internal query logic.
include/umf/*.h	Updated documentation and API prototypes with new size parameter.
benchmark/benchmark_umf.hpp	Minor update to supply size parameter for CTL queries.

Files not reviewed (2)

src/libumf.def: Language not supported
test/CMakeLists.txt: Language not supported

Comments suppressed due to low confidence (1)

src/memory_pool.c:160

Using strstr to match the default entry with the result of ops->get_name(NULL) may lead to unintended substring matches. Consider using a strict equality comparison to correctly detect a matching default property.

if (CTL_DEFAULT_ENTRIES[i][0] != '\0' && strstr(CTL_DEFAULT_ENTRIES[i], ops->get_name(NULL))) {

test/ctl/ctl_api.cpp

AI suggested Co-authored-by: Copilot <[email protected]>

include/umf/memory_pool_ops.h

lplewa · 2025-05-09T15:41:05Z

src/ctl/ctl.h

+    umf_ctl_node_t root[CTL_MAX_ENTRIES];
+    int first_free;
+};
+
 struct ctl *ctl_new(void);


src/libumf.c

src/memory_pool.c

src/pool/pool_disjoint.c

lukaszstolarczuk · 2025-05-19T09:05:22Z

continued in #1320

KFilipek added the enhancement New feature or request label Mar 21, 2025

KFilipek self-assigned this Mar 21, 2025

KFilipek linked an issue Mar 21, 2025 that may be closed by this pull request

CTL - setting global namespace #1058

Open

KFilipek force-pushed the ctl-default branch 5 times, most recently from 2ec6659 to 408f2c4 Compare March 24, 2025 08:16

KFilipek marked this pull request as ready for review March 24, 2025 09:43

KFilipek requested a review from a team as a code owner March 24, 2025 09:43

KFilipek force-pushed the ctl-default branch from 408f2c4 to 13bdb89 Compare March 24, 2025 15:35

lukaszstolarczuk reviewed Mar 24, 2025

View reviewed changes

bratpiorka requested changes Mar 24, 2025

View reviewed changes

bratpiorka reviewed Mar 25, 2025

View reviewed changes

include/umf/memory_pool_ops.h Outdated Show resolved Hide resolved

bratpiorka reviewed Mar 25, 2025

View reviewed changes

include/umf/base.h Outdated Show resolved Hide resolved

bratpiorka reviewed Mar 25, 2025

View reviewed changes

test/pools/disjoint_pool_ctl.cpp Outdated Show resolved Hide resolved

KFilipek force-pushed the ctl-default branch 4 times, most recently from b7324ed to 8e3a2fc Compare March 25, 2025 13:00

lplewa requested changes Mar 25, 2025

View reviewed changes

src/ctl/ctl.c Outdated Show resolved Hide resolved

src/memory_pool.c Outdated Show resolved Hide resolved

src/memory_pool.c Outdated Show resolved Hide resolved

src/pool/pool_disjoint_ctl.h Outdated Show resolved Hide resolved

KFilipek force-pushed the ctl-default branch 2 times, most recently from 338947a to 29380ba Compare March 25, 2025 16:02

KFilipek requested review from lukaszstolarczuk and bratpiorka March 25, 2025 16:02

lukaszstolarczuk reviewed Mar 26, 2025

View reviewed changes

KFilipek force-pushed the ctl-default branch 4 times, most recently from a8a7fe8 to 56b5408 Compare March 26, 2025 11:57

KFilipek force-pushed the ctl-default branch 3 times, most recently from 33ceeeb to 77692e1 Compare April 17, 2025 09:59

KFilipek requested a review from lplewa April 17, 2025 09:59

KFilipek force-pushed the ctl-default branch 4 times, most recently from 0c02aaa to f7c7900 Compare April 20, 2025 09:02

bratpiorka requested changes Apr 29, 2025

View reviewed changes

KFilipek force-pushed the ctl-default branch from f7c7900 to d96f3f2 Compare May 5, 2025 13:29

bratpiorka reviewed May 6, 2025

View reviewed changes

test/ctl/ctl_api.cpp Outdated Show resolved Hide resolved

KFilipek force-pushed the ctl-default branch from d96f3f2 to 80e82eb Compare May 6, 2025 14:59

KFilipek requested a review from bratpiorka May 6, 2025 14:59

[CTL] Add support for defaults

63c8f9e

KFilipek force-pushed the ctl-default branch from 80e82eb to 63c8f9e Compare May 7, 2025 09:34

KFilipek requested a review from Copilot May 8, 2025 08:13

Copilot AI reviewed May 8, 2025

View reviewed changes

test/ctl/ctl_api.cpp Outdated Show resolved Hide resolved

Update test/ctl/ctl_api.cpp

7427d25

AI suggested Co-authored-by: Copilot <[email protected]>

lplewa requested changes May 9, 2025

View reviewed changes

KFilipek force-pushed the ctl-default branch from 1d45bdf to b1b73d0 Compare May 13, 2025 15:33

Fixes after latest review

b30bcb8

KFilipek force-pushed the ctl-default branch from b1b73d0 to b30bcb8 Compare May 14, 2025 10:10

Drop prefix verification

892c8d5

KFilipek requested a review from lplewa May 15, 2025 15:14

Fix deadlock in default handler

7ac2d2c

KFilipek force-pushed the ctl-default branch from 0fccdf0 to 7ac2d2c Compare May 16, 2025 09:27

KFilipek closed this May 19, 2025

		@@ -35,7 +35,7 @@ TEST_F(test, ctl_by_handle_os_provider) {

		int ipc_enabled = 0xBAD;
		ret = umfCtlGet("umf.provider.by_handle.params.ipc_enabled", hProvider,

[CTL] Add support for defaults #1216

[CTL] Add support for defaults #1216

Uh oh!

Conversation

KFilipek commented Mar 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

[CTL] Add support for defaults

Description

Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KFilipek Mar 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lplewa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

KFilipek commented Apr 28, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bratpiorka commented Apr 29, 2025

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

KFilipek commented Mar 21, 2025 •

edited

Loading

KFilipek Mar 26, 2025 •

edited

Loading