Skip to content
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

Jump to bottom

[uma] Replace list of providers with explicit data and metadata providers. #641

Closed
wants to merge 3 commits into from
Closed

[uma] Replace list of providers with explicit data and metadata providers. #641

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d970216
[uma] simplify get_name
igchor Jun 20, 2023
5414e5a
[uma] replace list of providers with explicit
igchor Jun 20, 2023
eb8de02
[uma] extend error codes
igchor Jun 27, 2023
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
28 changes: 18 additions & 10 deletions source/common/uma_helpers.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ auto memoryProviderMakeUnique(Args &&...args) {
UMA_ASSIGN_OP(ops, T, get_min_page_size, UMA_RESULT_ERROR_UNKNOWN);
UMA_ASSIGN_OP(ops, T, purge_lazy, UMA_RESULT_ERROR_UNKNOWN);
UMA_ASSIGN_OP(ops, T, purge_force, UMA_RESULT_ERROR_UNKNOWN);
UMA_ASSIGN_OP_NORETURN(ops, T, get_name);
UMA_ASSIGN_OP(ops, T, get_name, "");

uma_memory_provider_handle_t hProvider = nullptr;
auto ret = umaMemoryProviderCreate(&ops, &argsTuple, &hProvider);
Expand All @@ -106,14 +106,16 @@ auto memoryProviderMakeUnique(Args &&...args) {
/// replaced by dtor). All arguments passed to this function are
/// forwarded to T::initialize().
template <typename T, typename... Args>
auto poolMakeUnique(uma_memory_provider_handle_t *providers,
size_t numProviders, Args &&...args) {
auto poolMakeUnique(uma_memory_provider_handle_t data_provider,
uma_memory_provider_handle_t metadata_provider,
Args &&...args) {
uma_memory_pool_ops_t ops;
auto argsTuple = std::make_tuple(std::forward<Args>(args)...);

ops.version = UMA_VERSION_CURRENT;
ops.initialize = [](uma_memory_provider_handle_t *providers,
size_t numProviders, void *params, void **obj) {
ops.initialize = [](uma_memory_provider_handle_t data_provider,
uma_memory_provider_handle_t metadata_provider,
void *params, void **obj) {
auto *tuple = reinterpret_cast<decltype(argsTuple) *>(params);
T *pool;

Expand All @@ -126,10 +128,11 @@ auto poolMakeUnique(uma_memory_provider_handle_t *providers,
*obj = pool;

try {
auto ret = std::apply(
&T::initialize,
std::tuple_cat(std::make_tuple(pool, providers, numProviders),
*tuple));
auto ret =
std::apply(&T::initialize,
std::tuple_cat(std::make_tuple(pool, data_provider,
metadata_provider),
*tuple));
if (ret != UMA_RESULT_SUCCESS) {
delete pool;
}
Expand All @@ -148,9 +151,14 @@ auto poolMakeUnique(uma_memory_provider_handle_t *providers,
UMA_ASSIGN_OP(ops, T, malloc_usable_size, ((size_t)0));
UMA_ASSIGN_OP_NORETURN(ops, T, free);
UMA_ASSIGN_OP(ops, T, get_last_result, UMA_RESULT_ERROR_UNKNOWN);
UMA_ASSIGN_OP(ops, T, get_data_memory_provider,
(uma_memory_provider_handle_t) nullptr);
UMA_ASSIGN_OP(ops, T, get_metadata_memory_provider,
(uma_memory_provider_handle_t) nullptr);

uma_memory_pool_handle_t hPool = nullptr;
auto ret = umaPoolCreate(&ops, providers, numProviders, &argsTuple, &hPool);
auto ret = umaPoolCreate(&ops, data_provider, metadata_provider, &argsTuple,
&hPool);
return std::pair<uma_result_t, pool_unique_handle_t>{
ret, pool_unique_handle_t(hPool, &umaPoolDestroy)};
}
Expand Down
22 changes: 14 additions & 8 deletions source/common/uma_pools/disjoint_pool.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -840,14 +840,12 @@ void DisjointPool::AllocImpl::printStats(bool &TitlePrinted,
}
}

uma_result_t DisjointPool::initialize(uma_memory_provider_handle_t *providers,
size_t numProviders,
DisjointPoolConfig parameters) {
if (numProviders != 1 || !providers[0]) {
return UMA_RESULT_ERROR_INVALID_ARGUMENT;
}

impl = std::make_unique<AllocImpl>(providers[0], parameters);
uma_result_t
DisjointPool::initialize(uma_memory_provider_handle_t data_provider,
uma_memory_provider_handle_t metadata_provider,
DisjointPoolConfig parameters) {
(void)metadata_provider;
impl = std::make_unique<AllocImpl>(data_provider, parameters);
return UMA_RESULT_SUCCESS;
}

Expand Down Expand Up @@ -920,6 +918,14 @@ enum uma_result_t DisjointPool::get_last_result(const char **ppMessage) {
return UMA_RESULT_ERROR_UNKNOWN;
}

uma_memory_provider_handle_t DisjointPool::get_data_memory_provider() {
return impl->getMemHandle();
}

uma_memory_provider_handle_t DisjointPool::get_metadata_memory_provider() {
return nullptr;
}

DisjointPool::DisjointPool() {}

// Define destructor for use with unique_ptr
Expand Down
7 changes: 5 additions & 2 deletions source/common/uma_pools/disjoint_pool.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,15 +60,18 @@ class DisjointPool {
class AllocImpl;
using Config = DisjointPoolConfig;

uma_result_t initialize(uma_memory_provider_handle_t *providers,
size_t numProviders, DisjointPoolConfig parameters);
uma_result_t initialize(uma_memory_provider_handle_t data_provider,
uma_memory_provider_handle_t metadata_provider,
DisjointPoolConfig parameters);
void *malloc(size_t size);
void *calloc(size_t, size_t);
void *realloc(void *, size_t);
void *aligned_malloc(size_t size, size_t alignment);
size_t malloc_usable_size(void *);
void free(void *ptr);
enum uma_result_t get_last_result(const char **ppMessage);
uma_memory_provider_handle_t get_data_memory_provider();
uma_memory_provider_handle_t get_metadata_memory_provider();

DisjointPool();
~DisjointPool();
Expand Down
13 changes: 8 additions & 5 deletions source/common/unified_memory_allocation/include/uma/base.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,13 +39,16 @@ enum uma_result_t {
UMA_RESULT_ERROR_POOL_SPECIFIC =
2, ///< A pool specific warning/error has been reported and can be
///< Retrieved via the umaPoolGetLastResult entry point.
UMA_RESULT_ERROR_MEMORY_PROVIDER_SPECIFIC =
3, ///< A provider specific warning/error has been reported and can be
UMA_RESULT_ERROR_METADATA_MEMORY_PROVIDER_SPECIFIC =
3, ///< A metadata provider specific warning/error has been reported and can be
///< Retrieved via the umaMemoryProviderGetLastResult entry point.
UMA_RESULT_ERROR_DATA_MEMORY_PROVIDER_SPECIFIC =
4, ///< A data provider specific warning/error has been reported and can be
///< Retrieved via the umaMemoryProviderGetLastNativeError entry point.
UMA_RESULT_ERROR_INVALID_ARGUMENT =
4, ///< Generic error code for invalid arguments
UMA_RESULT_ERROR_INVALID_ALIGNMENT = 5, /// Invalid alignment of an argument
UMA_RESULT_ERROR_NOT_SUPPORTED = 6, /// Operation not supported
5, ///< Generic error code for invalid arguments
UMA_RESULT_ERROR_INVALID_ALIGNMENT = 6, /// Invalid alignment of an argument
UMA_RESULT_ERROR_NOT_SUPPORTED = 7, /// Operation not supported

UMA_RESULT_ERROR_UNKNOWN = 0x7ffffffe ///< Unknown or internal error
};
Expand Down
32 changes: 16 additions & 16 deletions source/common/unified_memory_allocation/include/uma/memory_pool.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,17 +25,16 @@ struct uma_memory_pool_ops_t;
///
/// \brief Creates new memory pool.
/// \param ops instance of uma_memory_pool_ops_t
/// \param providers array of memory providers that will be used for coarse-grain allocations.
/// Should contain at least one memory provider.
/// \param numProvider number of elements in the providers array
/// \param data_provider memory provider that should be used for coarse-grain data allocation
/// \param metadata_provider [optional] memory provider that should be used for metadata allocations
/// \param params pointer to pool-specific parameters
/// \param hPool [out] handle to the newly created memory pool
/// \return UMA_RESULT_SUCCESS on success or appropriate error code on failure.
///
enum uma_result_t umaPoolCreate(struct uma_memory_pool_ops_t *ops,
uma_memory_provider_handle_t *providers,
size_t numProviders, void *params,
uma_memory_pool_handle_t *hPool);
uma_memory_provider_handle_t data_provider,
uma_memory_provider_handle_t metadata_provider,
Copy link
Contributor

Choose a reason for hiding this comment

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

it is a non-flexible API. As I remember in the past you had an idea of passing a list of memory providers with the lists of types. Have you considered it as an alternative.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, as I mentioned in the PR description we might want to accept a list of provides with types. However, for that we would need to define some properties for memory providers (so that the pool can choose the appropriate one) and we don't have those right now. I feel like it's better to start with a simpler API and evolve it over time if needed.

Copy link
Contributor

@vinser52 vinser52 Jun 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

My current feeling (and your comment confirms that) is that it is not the final API and furthermore it is specific to disjoint_pool implementation.
So why we cannot keep with the current approach when we pass just the list of memory providers and rely on the order of providers in that list?

I am worried because our goal was to upstream UMF as a part of UR but make API generic enough so that we can refer to it during discussions with other potential customers of the future standalone solution. Now it looks like we are making it USM (or disjoint_pool) specific.

Copy link
Member Author

Choose a reason for hiding this comment

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

We don't strictly need the interface to pass multiple memory providers to the memory pool right now. Even for metadata provider, it was mostly a 'future-looking' feature - the idea was that we could use it in DisjointPool (or any other) pool implementation, but we don't really use it right now (we just use regular new/delete).

As Piotr said, we should probably avoid implementing things we don't need to, perhaps for now, just having a single memory provider is enough - even without the metadata one (since we don;t have any implementation that would use it.)

I can revert the whole change that introduced the list of memory providers and just keep a single provider as it was originally. I would actually prefer that over defining which provider should be on position 0 or 1 in the list, etc. as that can change over time and we might silently break some (probably PoC) implementation silently - with function signature change, there would at least be a compilation error.

Copy link
Member Author

Choose a reason for hiding this comment

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

So, I think it's just a question: do we think that having separate data and metadata memory provider is useful (and generic enough) or we'd rather wait for an actual implementation that will want to leverage that distinction.

Copy link
Contributor

@vinser52 vinser52 Jun 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

We don't strictly need the interface to pass multiple memory providers to the memory pool right now. Even for metadata provider, it was mostly a 'future-looking' feature - the idea was that we could use it in DisjointPool (or any other) pool implementation, but we don't really use it right now (we just use regular new/delete).

I agree with that.

I can revert the whole change that introduced the list of memory providers and just keep a single provider as it was originally.

Sounds like we came up to the conclusion that the current pool creation API and the proposed one in this PR are not required by the current UR implementation. Because disjoint_pool does not use the second memory provider today. Do we want to extend disjoint_pool to use "metadata" memory provider? If so, then the pool creation API should be able to accept more than one memory provider. But I think that this PR should be independant from #640.

My current feeling is that #640 is more important and first we should address the native error handling use case. And I think error handling should not depend on how the pool is created. Otherwise, native error handling will depend on the Pool creation API and whenever we change the pool creation API the client code for native error handling is also impacted.

We can discuss it by call. In general, we agreed in the past that API and/or architectural changes should be discussed prior to actual code changes.

void *params, uma_memory_pool_handle_t *hPool);

///
/// \brief Destroys memory pool.
Expand Down Expand Up @@ -134,17 +133,18 @@ enum uma_result_t umaPoolGetLastResult(uma_memory_pool_handle_t hPool,
uma_memory_pool_handle_t umaPoolByPtr(const void *ptr);

///
/// \brief Retrieve memory providers associated with a given pool.
/// \brief Retrieve metadata memory provider associated with a given pool.
/// \param hPool specified memory pool
/// \param hProviders [out] pointer to an array of memory providers. If numProviders is not equal to or
/// greater than the real number of providers, UMA_RESULT_ERROR_INVALID_ARGUMENT is returned.
/// \param numProviders [in] number of memory providers to return
/// \param numProvidersRet pointer to the actual number of memory providers
/// \return UMA_RESULT_SUCCESS on success or appropriate error code on failure.
enum uma_result_t
umaPoolGetMemoryProviders(uma_memory_pool_handle_t hPool, size_t numProviders,
uma_memory_provider_handle_t *hProviders,
size_t *numProvidersRet);
/// \return handle to the metadata memory provider
uma_memory_provider_handle_t
umaPoolGetMetadataMemoryProvider(uma_memory_pool_handle_t hPool);

///
/// \brief Retrieve data memory provider associated with a given pool.
/// \param hPool specified memory pool
/// \return handle to the data memory provider
uma_memory_provider_handle_t
umaPoolGetDataMemoryProvider(uma_memory_pool_handle_t hPool);

#ifdef __cplusplus
}
Expand Down
14 changes: 8 additions & 6 deletions source/common/unified_memory_allocation/include/uma/memory_pool_ops.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,15 +27,15 @@ struct uma_memory_pool_ops_t {

///
/// \brief Intializes memory pool.
/// \param providers array of memory providers that will be used for coarse-grain allocations.
/// Should contain at least one memory provider.
/// \param numProvider number of elements in the providers array
/// \param data_provider memory provider that should be used for coarse-grain data allocation
/// \param metadata_provider [optional] memory provider that should be used for metadata allocations
/// \param params pool-specific params
/// \param pool [out] returns pointer to the pool
/// \return UMA_RESULT_SUCCESS on success or appropriate error code on failure.
enum uma_result_t (*initialize)(uma_memory_provider_handle_t *providers,
size_t numProviders, void *params,
void **pool);
enum uma_result_t (*initialize)(
uma_memory_provider_handle_t data_provider,
uma_memory_provider_handle_t metadata_provider, void *params,
void **pool);

///
/// \brief Finalizes memory pool
Expand All @@ -50,6 +50,8 @@ struct uma_memory_pool_ops_t {
size_t (*malloc_usable_size)(void *pool, void *ptr);
void (*free)(void *pool, void *);
enum uma_result_t (*get_last_result)(void *pool, const char **ppMessage);
uma_memory_provider_handle_t (*get_data_memory_provider)(void *pool);
uma_memory_provider_handle_t (*get_metadata_memory_provider)(void *pool);
};

#ifdef __cplusplus
Expand Down
6 changes: 3 additions & 3 deletions source/common/unified_memory_allocation/include/uma/memory_provider.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,8 @@ enum uma_result_t umaMemoryProviderFree(uma_memory_provider_handle_t hProvider,
///
/// \brief Retrieve string representation of the underlying provider specific
/// result reported by the last API that returned
/// UMA_RESULT_ERROR_MEMORY_PROVIDER_SPECIFIC. Allows for a provider
/// UMA_RESULT_ERROR_METADATA_MEMORY_PROVIDER_SPECIFIC or
/// UMA_RESULT_ERROR_DATA_MEMORY_PROVIDER_SPECIFIC. Allows for a provider
/// independent way to return a provider specific result.
///
/// \details
Expand Down Expand Up @@ -136,8 +137,7 @@ umaMemoryProviderPurgeForce(uma_memory_provider_handle_t hProvider, void *ptr,
/// \brief Retrive name of a given memory provider.
/// \param hProvider handle to the memory provider
/// \param ppName [out] pointer to a string containing name of the provider
void umaMemoryProviderGetName(uma_memory_provider_handle_t hProvider,
const char **ppName);
const char *umaMemoryProviderGetName(uma_memory_provider_handle_t hProvider);

#ifdef __cplusplus
}
Expand Down
2 changes: 1 addition & 1 deletion source/common/unified_memory_allocation/include/uma/memory_provider_ops.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ struct uma_memory_provider_ops_t {
size_t *pageSize);
enum uma_result_t (*purge_lazy)(void *provider, void *ptr, size_t size);
enum uma_result_t (*purge_force)(void *provider, void *ptr, size_t size);
void (*get_name)(void *provider, const char **ppName);
const char *(*get_name)(void *provider);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: looks like a change unrelated to this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, that's why I've put it into a separate commit, it's just a small improvement

};

#ifdef __cplusplus
Expand Down
90 changes: 36 additions & 54 deletions source/common/unified_memory_allocation/src/memory_pool.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,28 +21,18 @@ struct uma_memory_pool_t {
void *pool_priv;
struct uma_memory_pool_ops_t ops;

// Holds array of memory providers. All providers are wrapped
// by memory tracking providers (owned and released by UMA).
uma_memory_provider_handle_t *providers;
// All providers are wrapped by memory tracking providers (owned and released by UMA).
uma_memory_provider_handle_t data_provider;
uma_memory_provider_handle_t metadata_provider;

size_t numProviders;
};

static void
destroyMemoryProviderWrappers(uma_memory_provider_handle_t *providers,
size_t numProviders) {
for (size_t i = 0; i < numProviders; i++) {
umaMemoryProviderDestroy(providers[i]);
}

free(providers);
}

enum uma_result_t umaPoolCreate(struct uma_memory_pool_ops_t *ops,
uma_memory_provider_handle_t *providers,
size_t numProviders, void *params,
uma_memory_pool_handle_t *hPool) {
if (!numProviders || !providers) {
uma_memory_provider_handle_t data_provider,
uma_memory_provider_handle_t metadata_provider,
void *params, uma_memory_pool_handle_t *hPool) {
if (!data_provider) {
return UMA_RESULT_ERROR_INVALID_ARGUMENT;
}

Expand All @@ -54,27 +44,25 @@ enum uma_result_t umaPoolCreate(struct uma_memory_pool_ops_t *ops,

assert(ops->version == UMA_VERSION_CURRENT);

pool->providers =
calloc(numProviders, sizeof(uma_memory_provider_handle_t));
if (!pool->providers) {
ret = UMA_RESULT_ERROR_OUT_OF_HOST_MEMORY;
goto err_providers_alloc;
// Wrap each provider with memory tracking provider.
ret = umaTrackingMemoryProviderCreate(data_provider, pool,
&pool->data_provider);
if (ret != UMA_RESULT_SUCCESS) {
goto err_providers_wrap;
}

size_t providerInd = 0;
pool->numProviders = numProviders;

// Wrap each provider with memory tracking provider.
for (providerInd = 0; providerInd < numProviders; providerInd++) {
ret = umaTrackingMemoryProviderCreate(providers[providerInd], pool,
&pool->providers[providerInd]);
if (metadata_provider) {
ret = umaTrackingMemoryProviderCreate(metadata_provider, pool,
&pool->metadata_provider);
if (ret != UMA_RESULT_SUCCESS) {
goto err_providers_init;
goto err_providers_metadata_wrap;
}
} else {
pool->metadata_provider = NULL;
}

pool->ops = *ops;
ret = ops->initialize(pool->providers, pool->numProviders, params,
ret = ops->initialize(pool->data_provider, pool->metadata_provider, params,
&pool->pool_priv);
if (ret != UMA_RESULT_SUCCESS) {
goto err_pool_init;
Expand All @@ -84,17 +72,23 @@ enum uma_result_t umaPoolCreate(struct uma_memory_pool_ops_t *ops,
return UMA_RESULT_SUCCESS;

err_pool_init:
err_providers_init:
destroyMemoryProviderWrappers(pool->providers, providerInd);
err_providers_alloc:
if (pool->metadata_provider) {
umaMemoryProviderDestroy(pool->metadata_provider);
}
err_providers_metadata_wrap:
umaMemoryProviderDestroy(pool->data_provider);
err_providers_wrap:
free(pool);

return ret;
}

void umaPoolDestroy(uma_memory_pool_handle_t hPool) {
hPool->ops.finalize(hPool->pool_priv);
destroyMemoryProviderWrappers(hPool->providers, hPool->numProviders);
if (hPool->metadata_provider) {
umaMemoryProviderDestroy(hPool->metadata_provider);
}
umaMemoryProviderDestroy(hPool->data_provider);
free(hPool);
}

Expand Down Expand Up @@ -139,24 +133,12 @@ uma_memory_pool_handle_t umaPoolByPtr(const void *ptr) {
return umaMemoryTrackerGetPool(umaMemoryTrackerGet(), ptr);
}

enum uma_result_t
umaPoolGetMemoryProviders(uma_memory_pool_handle_t hPool, size_t numProviders,
uma_memory_provider_handle_t *hProviders,
size_t *numProvidersRet) {
if (hProviders && numProviders < hPool->numProviders) {
return UMA_RESULT_ERROR_INVALID_ARGUMENT;
}

if (numProvidersRet) {
*numProvidersRet = hPool->numProviders;
}

if (hProviders) {
for (size_t i = 0; i < hPool->numProviders; i++) {
umaTrackingMemoryProviderGetUpstreamProvider(
umaMemoryProviderGetPriv(hPool->providers[i]), hProviders + i);
}
}
uma_memory_provider_handle_t
umaPoolGetMetadataMemoryProvider(uma_memory_pool_handle_t hPool) {
return hPool->ops.get_metadata_memory_provider(hPool->pool_priv);
}

return UMA_RESULT_SUCCESS;
uma_memory_provider_handle_t
umaPoolGetDataMemoryProvider(uma_memory_pool_handle_t hPool) {
return hPool->ops.get_data_memory_provider(hPool->pool_priv);
}
5 changes: 2 additions & 3 deletions source/common/unified_memory_allocation/src/memory_provider.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,6 @@ umaMemoryProviderPurgeForce(uma_memory_provider_handle_t hProvider, void *ptr,
return hProvider->ops.purge_force(hProvider->provider_priv, ptr, size);
}

void umaMemoryProviderGetName(uma_memory_provider_handle_t hProvider,
const char **ppName) {
hProvider->ops.get_name(hProvider->provider_priv, ppName);
const char *umaMemoryProviderGetName(uma_memory_provider_handle_t hProvider) {
return hProvider->ops.get_name(hProvider->provider_priv);
}
Loading