Skip to content

Split initialize function and add post-initialize phase #1467

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 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 21 additions & 3 deletions .github/workflows/reusable_compatibility.yml
Original file line number Diff line number Diff line change
Expand Up @@ -99,8 +99,16 @@ jobs:
UMF_LOG: level:warning;flush:debug;output:stderr;pid:no
LD_LIBRARY_PATH: ${{github.workspace}}/latest_version/build/lib/
run: |
ctest --verbose -E test_memoryProvider
ctest --verbose -E "test_memoryProvider|test_disjoint_pool"

- name: Run disabled tests individually with latest UMF libs (warnings enabled)
working-directory: ${{github.workspace}}/tag_version/build
env:
UMF_LOG: level:warning;flush:debug;output:stderr;pid:no
LD_LIBRARY_PATH: ${{github.workspace}}/latest_version/build/lib/
run: |
test/test_memoryProvider --gtest_filter="-*Trace"
test/test_disjoint_pool --gtest_filter="-test.internals"

# Browse all folders in the examples directory, build them using the
# latest UMF version, and run them, excluding those in the exclude list.
Expand Down Expand Up @@ -225,8 +233,10 @@ jobs:
env:
UMF_LOG: level:warning;flush:debug;output:stderr;pid:no
run: |
$env:UMF_LOG="level:warning;flush:debug;output:stderr;pid:no"
cp ${{github.workspace}}/latest_version/build/bin/Debug/umf.dll ${{github.workspace}}/tag_version/build/bin/Debug/umf.dll
ctest -C Debug --verbose -E test_memoryProvider
ctest -C Debug --verbose -E "test_memoryProvider|test_disjoint_pool"


# Browse all folders in the examples directory, build them using the
# latest UMF version, and run them, excluding those in the exclude list.
Expand Down Expand Up @@ -368,8 +378,16 @@ jobs:
UMF_LOG: level:warning;flush:debug;output:stderr;pid:no
LD_LIBRARY_PATH: ${{github.workspace}}/latest_version/build/lib/
run: |
ctest --verbose -E test_memoryProvider
ctest --verbose -E "test_memoryProvider|test_disjoint_pool"

- name: Run disabled tests individually with latest UMF libs (warnings enabled)
working-directory: ${{github.workspace}}/tag_version/build
env:
UMF_LOG: level:warning;flush:debug;output:stderr;pid:no
LD_LIBRARY_PATH: ${{github.workspace}}/latest_version/build/lib/
run: |
test/test_memoryProvider --gtest_filter="-*Trace"
test/test_disjoint_pool --gtest_filter="-test.internals"

# Browse all folders in the examples directory, build them using the
# latest UMF version, and run them, excluding those in the exclude list.
Expand Down
22 changes: 22 additions & 0 deletions include/umf/memory_pool_ops.h
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,9 @@ typedef struct umf_memory_pool_ops_t {

///
/// @brief Initializes memory pool.
/// /details
/// * The memory pool implementation *must* allocate the memory pool structure
/// and return it by the \p pool parameter.
/// @param provider memory provider that will be used for coarse-grain allocations.
/// @param params pool-specific params, or NULL for defaults
/// @param pool [out] returns pointer to the pool
Expand Down Expand Up @@ -191,6 +194,25 @@ typedef struct umf_memory_pool_ops_t {
/// failure.
///
umf_result_t (*ext_trim_memory)(void *pool, size_t minBytesToKeep);

///
Copy link
Contributor

Choose a reason for hiding this comment

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

add a comment above "The following operations were added in ops version 1.1"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rebased.

/// @brief Post-initializes and set up memory pool.
Copy link
Contributor

Choose a reason for hiding this comment

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

add "this function must be implemented if the pool/provider supports CTL that overrides defaults"

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Even with this clarification, for me it is not clear why we need a separate function.

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've added a short description what is and what for.

/// Post-construction hook for memory pools, enabling advanced or deferred setup that cannot
/// be done in the initial allocation phase (e.g. setting defaults from CTL).
///
/// \details
/// * This function *must* be implemented if the pool/provider supports CTL that overrides defaults.
/// * This function *must* free any resources allocated in the function.
/// * This function *must* be called after the memory pool has been allocated in initialize function
Copy link
Contributor

Choose a reason for hiding this comment

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

This function *must* be called after the memory pool has been allocated in initialize function

Must be called by whom? If UMF calls it, then the comment is missleading.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's about the function order. Generally, this function is the same case as initialize(), it's also invoked in the same way - internally.

/// and is used to perform any additional setup required by the memory pool.
/// * This function *may* be used to set up any additional resources required by the memory pool.
/// * This function *may* be used to set up default values for the memory pool parameters set up by CTL.
///
/// @param pool pointer to the pool
/// @return UMF_RESULT_SUCCESS on success or appropriate error code on failure
///
umf_result_t (*ext_post_initialize)(void *pool);

} umf_memory_pool_ops_t;

#ifdef __cplusplus
Expand Down
9 changes: 9 additions & 0 deletions include/umf/memory_provider_ops.h
Original file line number Diff line number Diff line change
Expand Up @@ -322,6 +322,15 @@ typedef struct umf_memory_provider_ops_t {
void *provider, umf_memory_property_id_t memory_property_id,
size_t *size);

/// @brief Post-initializes memory provider.
/// Post-construction hook for memory pools, enabling advanced or deferred setup that cannot
/// be done in the initial allocation phase (e.g. setting defaults from CTL).
///
/// @param provider pointer to the provider
/// @return UMF_RESULT_SUCCESS on success or appropriate error code on failure.
///
umf_result_t (*ext_post_initialize)(void *provider);

} umf_memory_provider_ops_t;

#ifdef __cplusplus
Expand Down
22 changes: 18 additions & 4 deletions src/memory_pool.c
Original file line number Diff line number Diff line change
Expand Up @@ -463,6 +463,11 @@ static umf_result_t umfDefaultTrimMemory(void *provider,
return UMF_RESULT_ERROR_NOT_SUPPORTED;
}

static umf_result_t umfDefaultExtPostInitialize(void *pool) {
(void)pool;
return UMF_RESULT_SUCCESS;
}

// logical sum (OR) of all umf_pool_create_flags_t flags
static const umf_pool_create_flags_t UMF_POOL_CREATE_FLAG_ALL =
UMF_POOL_CREATE_FLAG_OWN_PROVIDER | UMF_POOL_CREATE_FLAG_DISABLE_TRACKING;
Expand Down Expand Up @@ -495,7 +500,6 @@ static umf_result_t umfPoolCreateInternal(const umf_memory_pool_ops_t *ops,
}

umf_result_t ret = UMF_RESULT_SUCCESS;

umf_memory_pool_ops_t compatible_ops;
if (ops->version != UMF_POOL_OPS_VERSION_CURRENT) {
LOG_WARN("Memory Pool ops version \"%d\" is different than the current "
Expand All @@ -504,8 +508,8 @@ static umf_result_t umfPoolCreateInternal(const umf_memory_pool_ops_t *ops,

// Create a new ops compatible structure with the current version
memset(&compatible_ops, 0, sizeof(compatible_ops));
if (UMF_MINOR_VERSION(ops->version) == 0) {
LOG_INFO("Detected 1.0 version of Memory Pool ops, "
if (ops->version < UMF_MAKE_VERSION(1, 1)) {
LOG_INFO("Detected 1.0 version or below of Memory Pool ops, "
"upgrading to current version");
memcpy(&compatible_ops, ops,
offsetof(umf_memory_pool_ops_t, ext_trim_memory));
Expand Down Expand Up @@ -547,13 +551,17 @@ static umf_result_t umfPoolCreateInternal(const umf_memory_pool_ops_t *ops,
pool->ops.ext_trim_memory = umfDefaultTrimMemory;
}

if (NULL == pool->ops.ext_post_initialize) {
pool->ops.ext_post_initialize = umfDefaultExtPostInitialize;
}

if (NULL == utils_mutex_init(&pool->lock)) {
LOG_ERR("Failed to initialize mutex for pool");
ret = UMF_RESULT_ERROR_UNKNOWN;
goto err_lock_init;
}

ret = ops->initialize(pool->provider, params, &pool->pool_priv);
ret = pool->ops.initialize(pool->provider, params, &pool->pool_priv);
if (ret != UMF_RESULT_SUCCESS) {
goto err_pool_init;
}
Expand All @@ -579,6 +587,12 @@ static umf_result_t umfPoolCreateInternal(const umf_memory_pool_ops_t *ops,
}
}

ret = pool->ops.ext_post_initialize(pool->pool_priv);
if (ret != UMF_RESULT_SUCCESS) {
LOG_ERR("Failed to post-initialize pool");
goto err_pool_init;
}

*hPool = pool;
pools_by_name_add(pool);

Expand Down
17 changes: 17 additions & 0 deletions src/memory_provider.c
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,11 @@ static umf_result_t umfDefaultCloseIPCHandle(void *provider, void *ptr,
return UMF_RESULT_ERROR_NOT_SUPPORTED;
}

static umf_result_t umfDefaultPostInitialize(void *provider) {
(void)provider;
return UMF_RESULT_ERROR_NOT_SUPPORTED;
}

static umf_result_t
umfDefaultCtlHandle(void *provider, umf_ctl_query_source_t operationType,
const char *name, void *arg, size_t size,
Expand Down Expand Up @@ -183,6 +188,10 @@ void assignOpsExtDefaults(umf_memory_provider_ops_t *ops) {
ops->ext_get_allocation_properties_size =
umfDefaultGetAllocationPropertiesSize;
}

if (!ops->ext_post_initialize) {
ops->ext_post_initialize = umfDefaultPostInitialize;
}
}

void assignOpsIpcDefaults(umf_memory_provider_ops_t *ops) {
Expand Down Expand Up @@ -310,6 +319,14 @@ umf_result_t umfMemoryProviderCreate(const umf_memory_provider_ops_t *ops,

provider->provider_priv = provider_priv;

ret = provider->ops.ext_post_initialize(provider_priv);
if (ret != UMF_RESULT_SUCCESS && ret != UMF_RESULT_ERROR_NOT_SUPPORTED) {
LOG_ERR("Failed to post-initialize provider");
provider->ops.finalize(provider_priv);
umf_ba_global_free(provider);
return ret;
}

*hProvider = provider;

const char *provider_name = NULL;
Expand Down
15 changes: 10 additions & 5 deletions src/pool/pool_disjoint.c
Original file line number Diff line number Diff line change
Expand Up @@ -758,6 +758,14 @@ umf_result_t disjoint_pool_initialize(umf_memory_provider_handle_t provider,
disjoint_pool->provider = provider;
disjoint_pool->params = *dp_params;

*ppPool = (void *)disjoint_pool;

return UMF_RESULT_SUCCESS;
}

umf_result_t disjoint_pool_post_initialize(void *ppPool) {
disjoint_pool_t *disjoint_pool = (disjoint_pool_t *)ppPool;

disjoint_pool->known_slabs = critnib_new(free_slab, NULL);
if (disjoint_pool->known_slabs == NULL) {
goto err_free_disjoint_pool;
Expand Down Expand Up @@ -816,13 +824,11 @@ umf_result_t disjoint_pool_initialize(umf_memory_provider_handle_t provider,
}

umf_result_t ret = umfMemoryProviderGetMinPageSize(
provider, NULL, &disjoint_pool->provider_min_page_size);
disjoint_pool->provider, NULL, &disjoint_pool->provider_min_page_size);
if (ret != UMF_RESULT_SUCCESS) {
disjoint_pool->provider_min_page_size = 0;
}

*ppPool = (void *)disjoint_pool;

return UMF_RESULT_SUCCESS;

err_free_buckets:
Expand All @@ -841,7 +847,6 @@ umf_result_t disjoint_pool_initialize(umf_memory_provider_handle_t provider,

err_free_disjoint_pool:
umf_ba_global_free(disjoint_pool);

return UMF_RESULT_ERROR_OUT_OF_HOST_MEMORY;
}

Expand Down Expand Up @@ -1199,7 +1204,7 @@ static umf_memory_pool_ops_t UMF_DISJOINT_POOL_OPS = {
.get_name = disjoint_pool_get_name,
.ext_ctl = disjoint_pool_ctl,
.ext_trim_memory = disjoint_pool_trim_memory,
};
.ext_post_initialize = disjoint_pool_post_initialize};
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: add ',' after the last member

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Initializer list doesn't require it; it's not a struct declaration. Sometimes ends with ,, sometimes not (vide defaults structure in the same file). I will leave it as is, if it's not a problem.


const umf_memory_pool_ops_t *umfDisjointPoolOps(void) {
return &UMF_DISJOINT_POOL_OPS;
Expand Down
Loading
Loading