Skip to content

Make it possible to create Fixed Provider from pointer of UMF memory pool #1121

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.

Sign up for GitHub

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

Closed
wants to merge 4 commits into from
Closed

Make it possible to create Fixed Provider from pointer of UMF memory pool #1121

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
279f8cb
Add umfFixedMemoryProviderParamsSetFlags()
ldorau Feb 21, 2025
b4d91c3
Add umfPoolGetTrackingProvider()
ldorau Feb 21, 2025
719dd6f
Add trackerShrinkEntry() and trackerGrowEntry()
ldorau Feb 20, 2025
d6d8f60
Add tests for pool from pointer
ldorau Feb 21, 2025
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
17 changes: 16 additions & 1 deletion include/umf/providers/provider_fixed_memory.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (C) 2024 Intel Corporation
* Copyright (C) 2024-2025 Intel Corporation
*
* Under the Apache License v2.0 with LLVM Exceptions. See LICENSE.TXT.
* SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
Expand All @@ -18,6 +18,14 @@ extern "C" {
#define UMF_FIXED_RESULTS_START_FROM 4000
/// @endcond

/// @brief Fixed Memory Provider flags
typedef enum umf_fixed_memory_provider_flag {
UMF_FIXED_FLAG_DEFAULT = 0, ///< the default - no flags set
UMF_FIXED_FLAG_CREATE_FROM_POOL_PTR =
(1
<< 0), ///< The Fixed Memory Provider is created from the UMF pool's pointer
} umf_fixed_memory_provider_flag_t;

struct umf_fixed_memory_provider_params_t;

typedef struct umf_fixed_memory_provider_params_t
Expand All @@ -41,6 +49,13 @@ umf_result_t umfFixedMemoryProviderParamsCreate(
umf_result_t umfFixedMemoryProviderParamsSetMemory(
umf_fixed_memory_provider_params_handle_t hParams, void *ptr, size_t size);

/// @brief Set the memory region flags in params struct.
/// @param hParams [in] handle to the parameters of the Fixed Memory Provider.
/// @param flags [in] flags for the memory region.
/// @return UMF_RESULT_SUCCESS on success or appropriate error code on failure.
umf_result_t umfFixedMemoryProviderParamsSetFlags(
umf_fixed_memory_provider_params_handle_t hParams, unsigned int flags);

/// @brief Destroy parameters struct.
/// @param hParams [in] handle to the parameters of the Fixed Memory Provider.
/// @return UMF_RESULT_SUCCESS on success or appropriate error code on failure.
Expand Down
1 change: 1 addition & 0 deletions src/libumf.def
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,5 +134,6 @@ EXPORTS
umfFixedMemoryProviderOps
umfFixedMemoryProviderParamsCreate
umfFixedMemoryProviderParamsDestroy
umfFixedMemoryProviderParamsSetFlags
umfLevelZeroMemoryProviderParamsSetFreePolicy
umfLevelZeroMemoryProviderParamsSetDeviceOrdinal
1 change: 1 addition & 0 deletions src/libumf.map
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,7 @@ UMF_0.11 {
umfFixedMemoryProviderOps;
umfFixedMemoryProviderParamsCreate;
umfFixedMemoryProviderParamsDestroy;
umfFixedMemoryProviderParamsSetFlags;
umfLevelZeroMemoryProviderParamsSetFreePolicy;
umfLevelZeroMemoryProviderParamsSetDeviceOrdinal;
} UMF_0.10;
16 changes: 16 additions & 0 deletions src/memory_pool.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,22 @@ umf_result_t umfPoolGetMemoryProvider(umf_memory_pool_handle_t hPool,
return UMF_RESULT_SUCCESS;
}

umf_result_t
umfPoolGetTrackingProvider(umf_memory_pool_handle_t hPool,
umf_memory_provider_handle_t *hProvider) {
if (!hPool || !hProvider) {
return UMF_RESULT_ERROR_INVALID_ARGUMENT;
}

if (hPool->flags & UMF_POOL_CREATE_FLAG_DISABLE_TRACKING) {
return UMF_RESULT_ERROR_INVALID_ARGUMENT;
}

*hProvider = umfMemoryProviderGetPriv(hPool->provider);

return UMF_RESULT_SUCCESS;
}

umf_result_t umfPoolCreate(const umf_memory_pool_ops_t *ops,
umf_memory_provider_handle_t provider, void *params,
umf_pool_create_flags_t flags,
Expand Down
4 changes: 4 additions & 0 deletions src/memory_pool_internal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,10 @@ typedef struct umf_memory_pool_t {
void *tag;
} umf_memory_pool_t;

umf_result_t
umfPoolGetTrackingProvider(umf_memory_pool_handle_t hPool,
umf_memory_provider_handle_t *hProvider);

#ifdef __cplusplus
}
#endif
Expand Down
79 changes: 76 additions & 3 deletions src/provider/provider_fixed_memory.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,22 +20,31 @@
#include "base_alloc_global.h"
#include "coarse.h"
#include "libumf.h"
#include "memory_pool_internal.h"
#include "provider_tracking.h"
#include "utils_common.h"
#include "utils_concurrency.h"
#include "utils_log.h"

#define TLS_MSG_BUF_LEN 1024

typedef struct fixed_memory_provider_t {
void *base; // base address of memory
size_t size; // size of the memory region
coarse_t *coarse; // coarse library handle
void *base; // base address of memory
size_t size; // size of the memory region
unsigned int flags; // flags of the memory region
coarse_t *coarse; // coarse library handle

// used only when the UMF_FIXED_FLAG_CREATE_FROM_POOL_PTR flag is set
size_t ptr_orig_size; // original size of the memory region in the tracker
umf_memory_provider_handle_t
trackingProvider; // tracking provider of the original memory pool
} fixed_memory_provider_t;

// Fixed Memory provider settings struct
typedef struct umf_fixed_memory_provider_params_t {
void *ptr;
size_t size;
unsigned int flags;
} umf_fixed_memory_provider_params_t;

typedef struct fixed_last_native_error_t {
Expand Down Expand Up @@ -83,6 +92,8 @@ static umf_result_t fixed_allocation_merge_cb(void *provider, void *lowPtr,

static umf_result_t fixed_initialize(void *params, void **provider) {
umf_result_t ret;
size_t ptr_orig_size = 0;
umf_memory_provider_handle_t trackingProvider = NULL;

if (params == NULL) {
return UMF_RESULT_ERROR_INVALID_ARGUMENT;
Expand All @@ -91,6 +102,30 @@ static umf_result_t fixed_initialize(void *params, void **provider) {
umf_fixed_memory_provider_params_t *in_params =
(umf_fixed_memory_provider_params_t *)params;

if (in_params->flags & UMF_FIXED_FLAG_CREATE_FROM_POOL_PTR) {
umf_memory_pool_handle_t pool = umfPoolByPtr(in_params->ptr);
if (pool == NULL) {
LOG_ERR("The UMF_FIXED_FLAG_CREATE_FROM_POOL_PTR flag is set, but "
"the given pointer does not belong to any UMF pool");
return UMF_RESULT_ERROR_INVALID_ARGUMENT;
}

ret = umfPoolGetTrackingProvider(pool, &trackingProvider);
if (ret != UMF_RESULT_SUCCESS) {
LOG_ERR("cannot get the tracking provider of the pool %p", pool);
return ret;
}

ret = trackerShrinkEntry(trackingProvider, in_params->ptr,
in_params->size, &ptr_orig_size);
if (ret != UMF_RESULT_SUCCESS) {
LOG_ERR(
"cannot shrink the allocation %p in the tracker to size %zu",
in_params->ptr, in_params->size);
return ret;
}
}

fixed_memory_provider_t *fixed_provider =
umf_ba_global_alloc(sizeof(*fixed_provider));
if (!fixed_provider) {
Expand Down Expand Up @@ -122,6 +157,9 @@ static umf_result_t fixed_initialize(void *params, void **provider) {

fixed_provider->base = in_params->ptr;
fixed_provider->size = in_params->size;
fixed_provider->flags = in_params->flags;
fixed_provider->ptr_orig_size = ptr_orig_size;
fixed_provider->trackingProvider = trackingProvider;

// add the entire memory as a single block
ret = coarse_add_memory_fixed(coarse, fixed_provider->base,
Expand All @@ -145,6 +183,20 @@ static umf_result_t fixed_initialize(void *params, void **provider) {
static void fixed_finalize(void *provider) {
fixed_memory_provider_t *fixed_provider = provider;
coarse_delete(fixed_provider->coarse);

if (fixed_provider->trackingProvider &&
(fixed_provider->ptr_orig_size >= fixed_provider->size)) {
umf_result_t ret = trackerGrowEntry(
fixed_provider->trackingProvider, fixed_provider->base,
fixed_provider->size, fixed_provider->ptr_orig_size);
if (ret != UMF_RESULT_SUCCESS) {
LOG_ERR("cannot grow the allocation %p in the tracker (from size "
"%zu to size %zu)",
fixed_provider->base, fixed_provider->size,
fixed_provider->ptr_orig_size);
}
}

umf_ba_global_free(fixed_provider);
}

Expand Down Expand Up @@ -333,5 +385,26 @@ umf_result_t umfFixedMemoryProviderParamsSetMemory(

hParams->ptr = ptr;
hParams->size = size;
hParams->flags = 0;

return UMF_RESULT_SUCCESS;
}

umf_result_t umfFixedMemoryProviderParamsSetFlags(
umf_fixed_memory_provider_params_handle_t hParams, unsigned int flags) {
if (hParams == NULL) {
LOG_ERR("Memory Provider params handle is NULL");
return UMF_RESULT_ERROR_INVALID_ARGUMENT;
}

if ((flags & UMF_FIXED_FLAG_CREATE_FROM_POOL_PTR) &&
(umfPoolByPtr(hParams->ptr) == NULL)) {
LOG_ERR("Cannot set the UMF_FIXED_FLAG_CREATE_FROM_POOL_PTR, because "
"the given pointer does not belong to any UMF pool");
return UMF_RESULT_ERROR_INVALID_ARGUMENT;
}

hParams->flags = flags;

return UMF_RESULT_SUCCESS;
}
111 changes: 111 additions & 0 deletions src/provider/provider_tracking.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -267,6 +267,58 @@ static umf_result_t trackingAllocationSplit(void *hProvider, void *ptr,
return ret;
}

// shrink (or remove) an entry in the tracker and return the totalSize of the original entry
umf_result_t trackerShrinkEntry(void *hProvider, void *ptr, size_t shrinkSize,
Copy link
Contributor

Choose a reason for hiding this comment

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

I do not think it is a good idea for several reasons:

  • Tracker contains info about actual memory chunks that correspond to the coarse grain allocations by the upstream memory providers. This function violates this rule.
  • We have split and merge functions but they notify the upstream memory provider first and work only if the upstream provider supports split/merge functionality.
  • Have you considered how IPC flow is affected by this logic? it might break IPC functionality.

Copy link
Contributor Author

@ldorau ldorau Feb 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

If we could restrict this functionality to using only the whole allocations, splitting and merging would not be needed. I will prepare an additional commit with those changes.

Copy link
Contributor Author

@ldorau ldorau Feb 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@vinser52 I have prepared one commit with this change (only the whole allocation can be used) - on the https://github.com/ldorau/unified-memory-framework/tree/Replace_trackerShrinkEntry_with_trackerRemoveEntry branch.

Copy link
Contributor Author

@ldorau ldorau Feb 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@vinser52 Just a brief explanation to paint the background for the later discussion:

This new functionality is needed for async API in UR/SYCL/LLVM (oneapi-src/unified-runtime#2180). We need to create a native pool based on the Fixed-size Memory Provider created from a user-provided buffer that comes from a USM pool based on Disjoint Pool with the L0 memory provider (see here) - @kswiecicki please correct me if I am wrong.

Using Disjoint Pool we cannot guarantee that we will receive from umfPoolMalloc() the whole allocation saved in the tracker and received from the L0 memory provider, so we have to be able to split/merge this allocation in the tracker, but L0 memory provider does not support splitting and merging, so we cannot notify the upstream provider (L0 memory provider in this case) and we have to add a new API to the tracker for this purpose ...

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 do not think it is a good idea for several reasons:

  • Tracker contains info about actual memory chunks that correspond to the coarse grain allocations by the upstream memory providers. This function violates this rule.

We have to create a nested UMF pool here, what causes that we have to add the same entry to the tracker for the second time. This seems to be the simplest and the fastest solution of this issue.

  • We have split and merge functions but they notify the upstream memory provider first and work only if the upstream provider supports split/merge functionality.

The upstream provider is L0 memory provider that does not support splitting and merging.

  • Have you considered how IPC flow is affected by this logic? it might break IPC functionality.

This feature is enabled only in the Fixed-size memory provider that does not support IPC at all yet.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, the pool handle param seems to be optional here: https://github.com/intel/llvm/pull/17117/files#diff-b4c22134fb720af1413e8c0d1a8c737092bae2a747ce73ca4d803773249eb81bR242.

Copy link
Contributor

Choose a reason for hiding this comment

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

In case of NULL pool handle the default pool is used. And UR knows the default pool handle

Copy link
Member

Choose a reason for hiding this comment

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

There is also a matter of UR queries: urUSMGetMemAllocInfo with https://github.com/intel/llvm/blob/sycl/unified-runtime/include/ur_api.h#L4230 I'm not sure if the spec for async says anything about this, but I think returning the underlying pool here would be a misleading.

Copy link
Member

Choose a reason for hiding this comment

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

@vinser52

Tracker contains info about actual memory chunks that correspond to the coarse grain allocations by the upstream memory providers. This function violates this rule.

We can't implement this feature (if we indeed need tracking capabilities) without changing the tracker behavior somehow. I do agree that we lose some information in the tracker with this approach. Now, only the fixed_provider knows everything about the original allocation, and to support IPC, I think the fixed_provider would have to forward calls to the original provider. (this might also apply to other functions, like get_recommended_page_size()).

If we extended the provider to allow multiple values to be kept on a stack for every key, as @lplewa proposed, we would keep all the information, but then the IPC implementation would need to have some extra logic to extract the correct provider. Also, merge/split implementation might become a bit complicated.

By the way, in the current implementation instead of adding trackerShrinkEntry/trackerShrinkGrow I think we could just add extra argument to trackerSplit/trackerMerge to specify whether to call upstream provider split/merge and use them.

Copy link
Contributor

Choose a reason for hiding this comment

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

We can't implement this feature (if we indeed need tracking capabilities) without changing the tracker behavior somehow.

I understand that we have to change somehow our tracking mechanism. Don't get me wrong I am not against of any changes, I just try to figure out the right way.

Now, only the fixed_provider knows everything about the original allocation, and to support IPC, I think the fixed_provider would have to forward calls to the original provider. (this might also apply to other functions, like get_recommended_page_size()).

That is what I am worried about. Today we are implementing the Fixed provider and we are UMF experts. Imagine tomorrow some external people want to implement a provider similar to the Fixed provider do we want to propose them that way or should we think of a solution that exposes as little as possible UMF internals (memory tracker, IPC implementation, etc.).

If we extended the provider to allow multiple values to be kept on a stack for every key, as @lplewa proposed, we would keep all the information, but then the IPC implementation would need to have some extra logic to extract the correct provider. Also, merge/split implementation might become a bit complicated.

If this extra logic that you have in mind will be encapsulated inside UMF it is fine, what we should avoid/decrease is delegating the challenges we have to the pool/provider implementers.

By the way, in the current implementation instead of adding trackerShrinkEntry/trackerShrinkGrow I think we could just add extra argument to trackerSplit/trackerMerge to specify whether to call upstream provider split/merge and use them.

I did not get the idea, did you mean trackingAllocationSplit/trackingAllocationMerge functions? If so, these functions implement the provider interface (umf_memory_provider_ops_t structure). To add a new parameter to these functions we have to change the interface of the memory provider.

size_t *totalSize) {
umf_result_t ret = UMF_RESULT_SUCCESS;
umf_tracking_memory_provider_t *provider =
(umf_tracking_memory_provider_t *)hProvider;

int r = utils_mutex_lock(&provider->hTracker->splitMergeMutex);
if (r) {
return UMF_RESULT_ERROR_UNKNOWN;
}

tracker_alloc_info_t *value = (tracker_alloc_info_t *)critnib_get(
provider->hTracker->alloc_segments_map, (uintptr_t)ptr);
if (!value) {
LOG_ERR("region for shrinking is not found in the tracker");
ret = UMF_RESULT_ERROR_INVALID_ARGUMENT;
goto err;
}
if (shrinkSize > value->size) {
LOG_ERR("requested size %zu to shrink exceeds the tracked size %zu",
shrinkSize, value->size);
ret = UMF_RESULT_ERROR_INVALID_ARGUMENT;
goto err;
}

if (shrinkSize < value->size) {
void *highPtr = (void *)(((uintptr_t)ptr) + shrinkSize);
size_t secondSize = value->size - shrinkSize;
ret = umfMemoryTrackerAdd(provider->hTracker, provider->pool, highPtr,
secondSize);
if (ret != UMF_RESULT_SUCCESS) {
LOG_ERR("failed to add the new shrunk region to the tracker, ptr = "
"%p, size = %zu, ret = %d",
highPtr, secondSize, ret);
goto err;
}
}

*totalSize = value->size;

void *erasedValue =
critnib_remove(provider->hTracker->alloc_segments_map, (uintptr_t)ptr);
assert(erasedValue == value);
umf_ba_free(provider->hTracker->alloc_info_allocator, erasedValue);

err:
utils_mutex_unlock(&provider->hTracker->splitMergeMutex);

return ret;
}

static umf_result_t trackingAllocationMerge(void *hProvider, void *lowPtr,
void *highPtr, size_t totalSize) {
umf_result_t ret = UMF_RESULT_ERROR_UNKNOWN;
Expand Down Expand Up @@ -353,6 +405,65 @@ static umf_result_t trackingAllocationMerge(void *hProvider, void *lowPtr,
return ret;
}

// grow (or add) an entry in the tracker to its original size
umf_result_t trackerGrowEntry(void *hProvider, void *ptr, size_t growSize,
size_t origSize) {
umf_result_t ret = UMF_RESULT_SUCCESS;
umf_tracking_memory_provider_t *provider =
(umf_tracking_memory_provider_t *)hProvider;

if (growSize > origSize) {
LOG_ERR("Invalid grow size %zu (larger than the original size %zu)",
growSize, origSize);
return UMF_RESULT_ERROR_INVALID_ARGUMENT;
}

int r = utils_mutex_lock(&provider->hTracker->splitMergeMutex);
if (r) {
return UMF_RESULT_ERROR_UNKNOWN;
}

void *highPtr = (void *)(((uintptr_t)ptr) + growSize);
tracker_alloc_info_t *highValue = NULL;

if (growSize < origSize) {
highValue = (tracker_alloc_info_t *)critnib_get(
provider->hTracker->alloc_segments_map, (uintptr_t)highPtr);
if (!highValue) {
LOG_ERR("cannot find the tracker entry to grow %p", highPtr);
ret = UMF_RESULT_ERROR_INVALID_ARGUMENT;
goto err;
}
if (growSize + highValue->size != origSize) {
LOG_ERR("Grow size plus the current size does not equal the "
"original size");
ret = UMF_RESULT_ERROR_INVALID_ARGUMENT;
goto err;
}
}

ret =
umfMemoryTrackerAdd(provider->hTracker, provider->pool, ptr, origSize);
if (ret != UMF_RESULT_SUCCESS) {
LOG_ERR("failed to add the new grown region to the tracker, ptr = %p, "
"size = %zu, ret = %d",
ptr, origSize, ret);
goto err;
}

if (growSize < origSize) {
void *erasedhighValue = critnib_remove(
provider->hTracker->alloc_segments_map, (uintptr_t)highPtr);
assert(erasedhighValue == highValue);
umf_ba_free(provider->hTracker->alloc_info_allocator, erasedhighValue);
}

err:
utils_mutex_unlock(&provider->hTracker->splitMergeMutex);

return ret;
}

static umf_result_t trackingFree(void *hProvider, void *ptr, size_t size) {
umf_result_t ret;
umf_result_t ret_remove = UMF_RESULT_ERROR_UNKNOWN;
Expand Down
6 changes: 6 additions & 0 deletions src/provider/provider_tracking.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,12 @@ void umfTrackingMemoryProviderGetUpstreamProvider(
umf_memory_provider_handle_t hTrackingProvider,
umf_memory_provider_handle_t *hUpstream);

umf_result_t trackerShrinkEntry(void *hProvider, void *ptr, size_t shrinkSize,
size_t *totalSize);

umf_result_t trackerGrowEntry(void *hProvider, void *ptr, size_t growSize,
size_t origSize);

#ifdef __cplusplus
}
#endif
Expand Down
Loading
Loading