[CTL] Add support for defaults #1320

KFilipek · 2025-05-19T09:01:21Z

Description

Checklist

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

lukaszstolarczuk · 2025-05-19T09:43:47Z

src/pool/pool_disjoint.c

+const char *disjoint_pool_get_name(void *pool) {
+    disjoint_pool_t *hPool = (disjoint_pool_t *)pool;
+    if (pool == NULL) {
+        return "disjoint";


can't we use the DEFAULT_NAME here?

lukaszstolarczuk · 2025-05-19T09:45:31Z

src/pool/pool_disjoint.c

    };

-    *hParams = params;
+    // Find default name and update params name


what do you mean by "Find"?

lukaszstolarczuk · 2025-05-19T09:52:14Z

src/memory_pool.c

@@ -168,6 +245,11 @@ umf_result_t umfPoolGetMemoryProvider(umf_memory_pool_handle_t hPool,
    return UMF_RESULT_SUCCESS;
 }

+const char *umfPoolGetName(umf_memory_pool_handle_t pool) {
+    UMF_CHECK(pool != NULL, NULL);


hmm not sure if I get this right - if pool == NULL we return NULL, and not the default pool name...?

umfPoolGetName is an API function that needs a context.
I extended function to works in case when get_name callback is not set.

lukaszstolarczuk · 2025-05-19T09:52:39Z

include/umf/memory_pool_ops.h

+    /// @brief Retrieves the name of the disjoint memory pool [optional]
+    /// @param pool pointer to the memory pool or NULL value
+    /// @return A constant character string representing the pool's name.
+    ///         Returns "disjoint" if `pool` is NULL, otherwise returns the


should we mention disjoint here? did you mean return default pool name?

lukaszstolarczuk · 2025-05-19T09:53:50Z

include/umf/memory_pool_ops.h

+                        void *arg, size_t size, umf_ctl_query_type_t queryType);
+
+    ///
+    /// @brief Retrieves the name of the disjoint memory pool [optional]


is it only for disjoint pool?

lukaszstolarczuk · 2025-05-19T10:59:47Z

test/ctl/ctl_api.cpp

+        umfDisjointPoolParamsDestroy(params);
+    } catch (Pool::CtlException &e) {
+        umfDisjointPoolParamsDestroy(params);
+        GTEST_SKIP() << e.what();


why skip, can't we fail?

This approach is commonly used in our tests:

ctest --verbose | grep SKIP | wc -l 295

lukaszstolarczuk · 2025-05-19T11:05:54Z

test/pools/disjoint_pool.cpp

+    umf_memory_provider_handle_t provider_handle = nullptr;
+    umf_memory_pool_handle_t pool = NULL;
+
+    struct memory_provider : public umf_test::provider_base_t {


do we need this struct in this test?

This should be reworked separately. It follows the approach used in the disjoint pool tests.

lukaszstolarczuk · 2025-05-19T11:06:28Z

test/pools/disjoint_pool.cpp

+    res =
+        umfPoolCreate(umfDisjointPoolOps(), provider_handle, params, 0, &pool);
+    EXPECT_EQ(res, UMF_RESULT_SUCCESS);
+    const char *name = umfPoolGetName(pool);


can we also test umfPoolGetName(null here?

No, umfPoolGetName() must take a valid pointer to the pool.

lukaszstolarczuk · 2025-05-19T11:07:02Z

test/pools/disjoint_pool_ctl.cpp

+        auto res = umfPoolCreate(m_poolOps, m_provider, m_params, 0, &m_pool);
+        if (res != UMF_RESULT_SUCCESS) {
+            m_pool = nullptr;
+            LOG_ERR("Failed to create memory pool");


not sure if LOG in test is expected

Done.
Removed.

lukaszstolarczuk · 2025-05-19T11:09:57Z

test/pools/disjoint_pool_ctl.cpp

+using umf_test::test;
+using namespace umf_test;
+
+#define RETURN_SUCCESS(ret) ASSERT_EQ(ret, UMF_RESULT_SUCCESS)


if you really want this macro to be here, perhaps rename to ASSERT_SUCCESS...?

lplewa · 2025-05-19T11:07:49Z

src/libumf.c

+
+umf_result_t umfCtlGet(const char *name, void *ctx, void *arg, size_t size) {
+    if (name == NULL || arg == NULL) {
+        return UMF_RESULT_ERROR_INVALID_ARGUMENT;
+    }
+    return ctl_query(NULL, ctx, CTL_QUERY_PROGRAMMATIC, name, CTL_QUERY_READ,
+                     arg, size);
+}
+
+umf_result_t umfCtlSet(const char *name, void *ctx, void *arg, size_t size) {
+    // Context can be NULL when setting defaults
+    if (name == NULL || arg == NULL || size == 0) {
+        return UMF_RESULT_ERROR_INVALID_ARGUMENT;
+    }
+
+    return ctl_query(NULL, ctx, CTL_QUERY_PROGRAMMATIC, name, CTL_QUERY_WRITE,
+                     arg, size)
+               ? UMF_RESULT_ERROR_UNKNOWN
+               : UMF_RESULT_SUCCESS;
+}
+
+umf_result_t umfCtlExec(const char *name, void *ctx, void *arg, size_t size) {
+    if (name == NULL || arg == NULL || ctx == NULL) {
+        return UMF_RESULT_ERROR_INVALID_ARGUMENT;
+    }
+    return ctl_query(NULL, ctx, CTL_QUERY_PROGRAMMATIC, name,
+                     CTL_QUERY_RUNNABLE, arg, size)
+               ? UMF_RESULT_ERROR_UNKNOWN
+               : UMF_RESULT_SUCCESS;
+}


there is chaos in this inval check - please add comment per each function why we not check for null.

For sure exec should accept null arg.

lplewa · 2025-05-19T11:08:48Z

src/memory_pool.c

+    if (ctx && strstr(extra_name, "default")) {
+        utils_mutex_unlock(&ctl_mtx);
+        return UMF_RESULT_ERROR_INVALID_ARGUMENT;
+    }


Do we need this strstr? If yes what we are looking for?

lplewa · 2025-05-19T11:11:16Z

src/memory_provider.c

@@ -22,13 +22,13 @@
 #include "utils_assert.h"



Where is support for defaults for memory providers? I see only implementation for pools, and we need both

Should be splitted between 2 PR's

lplewa · 2025-05-19T11:14:13Z

include/umf/memory_pool_ops.h

+    /// @brief Retrieves the name of the disjoint memory pool [optional]
+    /// @param pool pointer to the memory pool or NULL value
+    /// @return A constant character string representing the pool's name.
+    ///         Returns "disjoint" if `pool` is NULL, otherwise returns the
+    ///         configured name of the specific pool instance.
+    ///


Please document requirements of interface - not how this function behaviors for disjoint pool

lplewa · 2025-05-19T11:23:12Z

src/ctl/ctl.h

+/*
+ * Declaration of a new RW leaf. This type of RW leaf doesn't require parsing
+ * of the argument. The argument is passed directly to the read/write callback.
+ */
+#define CTL_LEAF_RW_no_arg(name, ...)                                          \
+    {                                                                          \
+        CTL_STR(name), CTL_NODE_LEAF,                                          \
+            {CTL_READ_HANDLER(name, __VA_ARGS__),                              \
+             CTL_WRITE_HANDLER(name, __VA_ARGS__), NULL, NULL},                \
+            NULL, NULL                                                         \
+    }
+


Why we need this?

As discussed, done.

lukaszstolarczuk · 2025-05-19T11:28:44Z

test/ctl/ctl_unittest.cpp

@@ -21,35 +21,35 @@ TEST_F(test, ctl_debug_read_from_string) {

    int value = 0;
    ctl_query(ctl_handler, NULL, CTL_QUERY_PROGRAMMATIC,
-              "debug.heap.alloc_pattern", CTL_QUERY_READ, &value);
+              "debug.heap.alloc_pattern", CTL_QUERY_READ, &value, 0);


my comment from the prev. PR is not resolved:

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

e.g. here you can simply add a repeated queries with non-zero values

bratpiorka · 2025-05-19T12:04:40Z

src/ctl/ctl.c

                                   const char *extra_name,
                                   umf_ctl_query_type_t query_type) {
    (void)extra_name, (void)query_type;
    assert(n != NULL);
    assert(n->cb[CTL_QUERY_RUNNABLE] != NULL);
    assert(MAX_CTL_QUERY_TYPE != query_type);
-    return n->cb[CTL_QUERY_RUNNABLE](ctx, source, arg, indexes, NULL,
-                                     MAX_CTL_QUERY_TYPE);
+    return n->cb[CTL_QUERY_RUNNABLE](ctx, source, arg, size, indexes,


extra name is used so remove (void)extra_name few lines above

bratpiorka · 2025-05-19T12:06:39Z

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)ctx;


ctx is used in if()

bratpiorka · 2025-05-19T12:09:05Z

src/pool/pool_disjoint.c

+                                  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;


size is used

bratpiorka · 2025-05-19T12:09:52Z

src/pool/pool_disjoint.c

+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;


queryType is used

bratpiorka · 2025-05-19T12:15:16Z

test/pools/disjoint_pool.cpp

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


use <> and move up

Add function to get pool name

136dc39

KFilipek self-assigned this May 19, 2025

KFilipek requested a review from a team as a code owner May 19, 2025 09:01

KFilipek added the enhancement New feature or request label May 19, 2025

lukaszstolarczuk mentioned this pull request May 19, 2025

[CTL] Add support for defaults #1216

Closed

3 tasks

bratpiorka requested review from lplewa and bratpiorka May 19, 2025 09:28

lukaszstolarczuk reviewed May 19, 2025

View reviewed changes

lplewa requested changes May 19, 2025

View reviewed changes

lukaszstolarczuk reviewed May 19, 2025

View reviewed changes

bratpiorka reviewed May 19, 2025

View reviewed changes

KFilipek force-pushed the ctl-default branch 6 times, most recently from 3dbf762 to c43ddc7 Compare May 22, 2025 14:37

KFilipek requested review from lplewa, lukaszstolarczuk and bratpiorka May 22, 2025 14:37

KFilipek force-pushed the ctl-default branch 2 times, most recently from e7f77ee to c28fe62 Compare May 22, 2025 15:37

[CTL] Add support for defaults

65c2e84

KFilipek force-pushed the ctl-default branch from c28fe62 to 65c2e84 Compare May 22, 2025 17:01

[CTL] Add support for defaults #1320

Are you sure you want to change the base?

[CTL] Add support for defaults #1320

Conversation

KFilipek commented May 19, 2025 • edited Loading

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

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

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 May 19, 2025 •

edited

Loading