[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

bratpiorka · 2025-03-21T15:51:41Z

include/umf/memory_pool_ops.h

    /// @param queryType type of the query to be performed.
    ///
    /// @return umf_result_t result of the control operation.
    ///
    umf_result_t (*ctl)(void *hPool, int operationType, const char *name,
-                        void *arg, umf_ctl_query_type_t queryType);
+                        void *arg, size_t size, umf_ctl_query_type_t queryType);


hPool -> pool to be consistent across the file

bratpiorka · 2025-03-24T17:21:50Z

src/memory_pool.c

+                                        const char *extra_name,
+                                        umf_ctl_query_type_t queryType) {
+    (void)indexes, (void)source, (void)size, (void)ctx;
+    assert(queryType == CTL_QUERY_WRITE && "DO NOT SUPPORTED QUERY TYPE");


not supported query type (without "do" + small letters)

bratpiorka · 2025-03-24T17:22:43Z

src/memory_pool.c


 #include "base_alloc_global.h"
 #include "memory_pool_internal.h"
 #include "memory_provider_internal.h"
 #include "provider_tracking.h"

+enum {
+    UMF_DEFAULT_SIZE = 100,


we use defines or static consts

Done, I used #define

bratpiorka · 2025-03-24T17:24:17Z

src/memory_pool.c

+                break;
+            }
+        }
+    }


what if we run out of default entries?
what if arg len > default len?

I've added error handling

bratpiorka · 2025-03-24T17:27:46Z

src/pool/pool_disjoint_ctl.h

+    disjoint_pool_t *pool = (disjoint_pool_t *)ctx;
+    if (strstr(extra_name, pool->params.name) != NULL) {
+        strncpy(pool->params.name, (char *)arg,
+                sizeof(pool->params.name) / sizeof(pool->params.name[0]) - 1);


I don't get why we have a div here - could you comment?

Assuming that char is 1 byte, it's unnecessary

bratpiorka · 2025-03-24T17:32:58Z

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,
-                    &ipc_enabled);
+                    &ipc_enabled, 0); // Some handlers omit size


I don't get it - is size always required? is it a mandatory arg? when I should set it?

Added description that is an optional argument

bratpiorka · 2025-03-24T17:34:13Z

test/pools/disjoint_pool.cpp

@@ -12,6 +12,7 @@
 #include "provider.hpp"
 #include "provider_null.h"
 #include "provider_trace.h"
+#include "umf/memory_pool.h"


this should be #include <memory_pool.h>

I removed it completely

bratpiorka · 2025-03-24T17:34:28Z

test/pools/disjoint_pool_ctl.cpp

+#include <umf/memory_pool.h>
+#include <umf/pools/pool_disjoint.h>
+
+#include "base.hpp"


cleanup includes

bratpiorka · 2025-03-24T17:35:25Z

test/pools/disjoint_pool_ctl.cpp

+    ASSERT_EQ(ret, UMF_RESULT_SUCCESS);
+
+    const char *val = "disjoint_new_name";
+    umfCtlSet("umf.pool.default.disjoint.name", NULL, (void *)val, strlen(val));


do this func return any result if succeeds? we should assert this

Added macro to handle success

bratpiorka · 2025-03-24T17:35:45Z

test/pools/disjoint_pool_ctl.cpp

+
+    umf_memory_pool_handle_t hPool;
+    umf_disjoint_pool_params_handle_t params;
+    umfDisjointPoolParamsCreate(&params);


add asserts for results

bratpiorka · 2025-03-25T09:30:15Z

include/umf/memory_pool_ops.h

@@ -135,12 +135,20 @@ typedef struct umf_memory_pool_ops_t {
    /// @param operationType type of the operation to be performed.
    /// @param name name associated with the operation.
    /// @param arg argument for the operation.
+    /// @param size size of the argument.


add: may be optional - please check spec for path

bratpiorka · 2025-03-25T09:33:03Z

include/umf/base.h


 ///
 /// @brief Set value of a specified attribute at the given name.
 /// @param name name of an attribute to be set
 /// @param ctx pointer to the pool or the provider
 /// @param arg [in] pointer to the value that will be set
+/// @param size [in] size of the value


set + get: ctx - should be null for default + test

bratpiorka · 2025-03-25T09:35:17Z

test/pools/disjoint_pool_ctl.cpp

+    ASSERT_EQ(ret, UMF_RESULT_SUCCESS);
+
+    const char *val = "disjoint_new_name";
+    umfCtlSet("umf.pool.default.disjoint.name", NULL, (void *)val, strlen(val));


where do you test get?

lplewa

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

lplewa · 2025-03-25T15:21:07Z

src/ctl/ctl.c

@@ -416,11 +402,14 @@ int ctl_query(struct ctl *ctl, void *ctx, umf_ctl_query_source_t source,
        errno = EINVAL;
        goto out;
    }
+    const char *extra_name = &name[0];
+    if (strstr(extra_name, "by_handle") != NULL) {
+        extra_name = &name[0] + name_offset;


Please do not include UMF specyfic soultions in ctl* files.

lplewa · 2025-03-25T15:23:08Z

src/memory_pool.c

+                                        const char *extra_name,
+                                        umf_ctl_query_type_t queryType) {
+    (void)indexes, (void)source, (void)size, (void)ctx;
+    assert(queryType == CTL_QUERY_WRITE && "not supported query type");


This is not an assert. This sould return a correct error code to the user.

read should work

lplewa · 2025-03-25T15:30:07Z

src/memory_pool.c

@@ -99,6 +135,15 @@ static umf_result_t umfPoolCreateInternal(const umf_memory_pool_ops_t *ops,
        goto err_pool_init;
    }

+    // Set default property "name" to pool if exists
+    for (int i = 0; i < UMF_DEFAULT_SIZE; i++) {
+        if (CTL_DEFAULT_ENTRIES[i][0] != '\0') {


You should only call it for entries with matching name, not all.

Now, should be fine

lplewa · 2025-03-25T15:33:39Z

src/pool/pool_disjoint_ctl.h

@@ -0,0 +1,61 @@
+/*
+ * Copyright (C) 2025 Intel Corporation


imho this should not be a .h files

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

lplewa · 2025-03-26T13:52:55Z

src/ctl/ctl.c

+    if (name == NULL || arg == NULL) {
+        return UMF_RESULT_ERROR_INVALID_ARGUMENT;
+    }
+    // using ctx is disallowed for default settings


There should not be any assumptions about ctl tree in ctl.c file

I moved UmfCtl* functions to libumf.c

lplewa · 2025-03-26T13:56:02Z

src/memory_pool.c

+                                        umf_ctl_query_type_t queryType) {
+    (void)indexes, (void)source, (void)size, (void)ctx;
+
+    if (queryType == CTL_QUERY_WRITE) {


Thread safety. you can add multithreaded tests too.

What if user set default multipletimes, you should overide entry in table, not add another one.

lplewa · 2025-03-26T13:57:09Z

src/memory_pool.c

+    // Set default property "name" to pool if exists
+    for (int i = 0; i < UMF_DEFAULT_SIZE; i++) {
+        if (CTL_DEFAULT_ENTRIES[i][0] != '\0' &&
+            strstr(CTL_DEFAULT_ENTRIES[i], umfPoolGetName(pool))) {


Please cache umfPoolName, as you can change it latter on with ctl call, and all following default entries will not work

Now should work as designed, I've added test for that.

lplewa · 2025-03-26T16:34:24Z

include/umf/memory_pool.h

+/// @param pool handle to the memory pool
+/// @return pointer to a string containing the name of the \p pool
+///
+const char *umfPoolGetName(umf_memory_pool_handle_t pool);


It should return pool type name if pool handle is null, and pool specific name if handle is not null.

This is wording we should update provider ops too - ATM all providers ignore provider handle in getName function, but we should document it too.

This allows us always check default provider/pool name even if it was changed.

lplewa · 2025-03-26T16:47:33Z

src/ctl/ctl.h

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


if you move ctl struct to header you should remove this function, and update other places.

lplewa · 2025-03-26T16:50:59Z

src/memory_pool.c

+                                        umf_ctl_index_utlist_t *indexes,
+                                        const char *extra_name,
+                                        umf_ctl_query_type_t queryType) {
+    (void)indexes, (void)source, (void)size, (void)ctx;


Wait - you introduced size parameter exactly to be able precisely handle this function, so you can handle parameters with any size, and you are not using it here.

Please update this function to handle parameters any size. Please add test including arguments of different size and type.

lplewa · 2025-03-26T16:59:37Z

src/pool/pool_disjoint.c

+#include <ctl/ctl.h>
+
+/* Disjoint pool CTL implementation */
+struct ctl disjoint_ctl_root;
+static UTIL_ONCE_FLAG ctl_initialized = UTIL_ONCE_FLAG_INIT;
+
+static int CTL_READ_HANDLER(name)(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 queryType) {
+    (void)source, (void)indexes, (void)queryType, (void)size, (void)extra_name;
+    disjoint_pool_t *pool = (disjoint_pool_t *)ctx;
+
+    if (arg == NULL) {
+        return -1;
+    }
+
+    strncpy(pool->params.name, (char *)arg, sizeof(pool->params.name) - 1);
+    return 0;
+}
+
+static const umf_ctl_node_t CTL_NODE(disjoint)[] = {CTL_LEAF_RO(name),
+                                                    CTL_NODE_END};
+
+static void initialize_disjoint_ctl(void) {
+    CTL_REGISTER_MODULE(&disjoint_ctl_root, disjoint);
+}
+
+umf_result_t disjoint_pool_ctl(void *hPool, int operationType, const char *name,
+                               void *arg, size_t size,
+                               umf_ctl_query_type_t queryType) {
+    (void)operationType, (void)queryType;
+    utils_init_once(&ctl_initialized, initialize_disjoint_ctl);
+
+    const char *prefix = "disjoint.";
+    const char *name_wo_prefix = strstr(name, "disjoint.");
+
+    // Check if the name has the prefix
+    if ((name_wo_prefix = strstr(name, prefix)) == NULL) {
+        return UMF_RESULT_ERROR_INVALID_ARGUMENT;
+    }
+    return ctl_query(&disjoint_ctl_root, hPool, CTL_QUERY_PROGRAMMATIC,
+                     name_wo_prefix, CTL_QUERY_READ, arg, size);
+}


Please add test if both strings works
umf.pool.byHandle.name
umf.pool.default. disjoint.name

imho it dont.

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

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

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 from d506e23 to f9cbcca Compare March 26, 2025 11:00

Add function to get pool name

8401c15

KFilipek force-pushed the ctl-default branch 2 times, most recently from 94ca030 to a8a7fe8 Compare March 26, 2025 11:45

KFilipek force-pushed the ctl-default branch 3 times, most recently from 864f888 to ac65d2f Compare March 26, 2025 13:43

KFilipek requested review from lplewa and lukaszstolarczuk March 26, 2025 13:49

lplewa requested changes Mar 26, 2025

View reviewed changes

KFilipek force-pushed the ctl-default branch from ac65d2f to 495ba7a Compare March 26, 2025 17:07

[CTL] Add support for defaults

e5ffafa

KFilipek force-pushed the ctl-default branch from 495ba7a to e5ffafa Compare March 27, 2025 09:48

		@@ -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

Are you sure you want to change the base?

[CTL] Add support for defaults #1216

Conversation

KFilipek commented Mar 21, 2025 • edited Loading

[CTL] Add support for defaults

Description

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KFilipek Mar 26, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bratpiorka Mar 25, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lplewa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KFilipek commented Mar 21, 2025 •

edited

Loading

KFilipek Mar 26, 2025 •

edited

Loading

bratpiorka Mar 25, 2025 •

edited

Loading