Add the umfPoolCreateEx internal function

[kswiecicki]

... that creates a pool owning the provided memory provider.

[igchor]

FYI, this (or something similar) is needed if we want to implement umfCreateMemoryPoolFromMemspace.

[vinser52]

I understand that this function is needed to implement umfCreateMemoryPoolFromMemspace function, but why should it be a public interface?

[bratpiorka]

I would prefer a function that combines pool and provider params so the provider would be both created and destroyed with the pool. Something like:

umfPoolCreateEx(const struct umf_memory_pool_ops_t *pool_ops, void *pool_params,
const struct umf_memory_provider_ops_t *provider_ops, void *provider_params,
umf_memory_pool_handle_t *hPool);

with this API we don't expose the handle to the provider to the user at all so it is obvious who is managing and controlling the live of the provider.

[vinser52]

I would prefer a function that combines pool and provider params so the provider would be both created and destroyed with the pool. Something like:

umfPoolCreateEx(const struct umf_memory_pool_ops_t *pool_ops, void *pool_params,
const struct umf_memory_provider_ops_t *provider_ops, void *provider_params,
umf_memory_pool_handle_t *hPool);

with this API we don't expose the handle to the provider to the user at all so it is obvious who is managing and controlling the live of the provider.

It just proves the point that every time we want to extend public API we need to discuss it first. In that particular case, I think we can keep the proposed function internal. At least as a first step - there are no immediate need to make it public.

[bratpiorka]

Yes, I agree that we shouldn't expose this in public API if this is not needed. But also I think that we should follow the RAII concept - if the pool is an owner of the provider, the handle of provider should be hidden so no one could destroy the provider in the background

[igchor]

Yes, I think both comments make sense. We can make it private for now and use the signature that Rafal suggested. However, we will need to expose it once we want to also expose memory_target_ops.

[vinser52]

Will you add the unit test?

[bratpiorka]

Note that with recent Łukasz D. changes there is no need to use "enum" and "struct" keywords when you reference umf_result_t etc. Please remove them.

[kswiecicki]

Will you add the unit test?

I've added test that checks whether poolCreateEx creates a valid pool internally and some basic successful pool creation, param fail tests.
@vinser52 @bratpiorka what do you think about adding AddressSanitizer or Valgrind for UMF testing? I could use it to check whether poolCreateEx appropriately destroyed the internal memory provider.

[igchor]

Will you add the unit test?

I've added test that checks whether poolCreateEx creates a valid pool internally and some basic successful pool creation, param fail tests. @vinser52 @bratpiorka what do you think about adding AddressSanitizer or Valgrind for UMF testing? I could use it to check whether poolCreateEx appropriately destroyed the internal memory provider.

I think you could try enabling sanitizer, it should be pretty easy (just need to add flag to the compilation). And you can even reuse logic from UR: https://github.com/oneapi-src/unified-runtime/blob/4424195c7d5d990901f7343fa9e7ac500b03e38d/cmake/helpers.cmake#L45

[bratpiorka]

Will you add the unit test?

I've added test that checks whether poolCreateEx creates a valid pool internally and some basic successful pool creation, param fail tests. @vinser52 @bratpiorka what do you think about adding AddressSanitizer or Valgrind for UMF testing? I could use it to check whether poolCreateEx appropriately destroyed the internal memory provider.

I think you could try enabling sanitizer, it should be pretty easy (just need to add flag to the compilation). And you can even reuse logic from UR: https://github.com/oneapi-src/unified-runtime/blob/4424195c7d5d990901f7343fa9e7ac500b03e38d/cmake/helpers.cmake#L45

We had plans to add various sanitizers - look for them in our JIRA (UMFW-130). If you want to take care of these please do it in separate PR.

[kswiecicki]

Will you add the unit test?

I've added test that checks whether poolCreateEx creates a valid pool internally and some basic successful pool creation, param fail tests. @vinser52 @bratpiorka what do you think about adding AddressSanitizer or Valgrind for UMF testing? I could use it to check whether poolCreateEx appropriately destroyed the internal memory provider.

I think you could try enabling sanitizer, it should be pretty easy (just need to add flag to the compilation). And you can even reuse logic from UR: https://github.com/oneapi-src/unified-runtime/blob/4424195c7d5d990901f7343fa9e7ac500b03e38d/cmake/helpers.cmake#L45

We had plans to add various sanitizers - look for them in our JIRA (UMFW-130). If you want to take care of these please do it in separate PR.

I've assigned myself for this task.