add memory properties API by bratpiorka · Pull Request #1301 · oneapi-src/unified-memory-framework

bratpiorka · 2025-05-08T10:29:10Z

add memory properties API

https://reviewable.io/reviews/bratpiorka/unified-memory-framework/3

lplewa

Please add tests too all umf pools
you should use poolfixtures.hpp
Including it's abstraction to create custom pools and providers.

lplewa · 2025-06-12T09:08:00Z

+static void do_umf_mem_props_benchmark(ze_context_handle_t context,
+                                       bool use_umf, alloc_t *allocs,
+                                       size_t num_allocs, size_t repeats) {
+    assert(context != NULL);
+
+    for (size_t r = 0; r < repeats * 10; ++r) {
+        for (size_t i = 0; i < num_allocs; ++i) {
+            if (use_umf) {
+                umf_memory_properties_handle_t props_handle = NULL;
+                umf_result_t res =
+                    umfGetMemoryPropertiesHandle(allocs[i].ptr, &props_handle);
+                (void)res;
+                assert(res == UMF_RESULT_SUCCESS);
+
+                umf_usm_memory_type_t type = UMF_MEMORY_TYPE_UNKNOWN;
+                res = umfGetMemoryProperty(
+                    props_handle, UMF_MEMORY_PROPERTY_POINTER_TYPE, &type);
+                assert(res == UMF_RESULT_SUCCESS);
+                if (type != UMF_MEMORY_TYPE_DEVICE) {
+                    fprintf(stderr,
+                            "error: unexpected alloc_props.type value: %d\n",
+                            type);
+                    exit(-1);


Please in final version add benchmark to new framework.

I removed the old version of the benchmark, but the new one should be done in a separate PR

lplewa · 2025-06-12T09:09:50Z

+///        be filled. NOTE: the type and size of the value depends on the
+///        memory property ID and should be checked in the documentation


api like this should include size parameter for safety

lplewa · 2025-06-12T09:12:08Z

+/// @brief Get the memory properties handle for a given pointer
+/// @param ptr pointer to the allocated memory
+/// @param props_handle [out] pointer to the memory properties handle
+/// @return UMF_RESULT_SUCCESS on success or appropriate error code on failure
+umf_result_t
+umfGetMemoryPropertiesHandle(const void *ptr,
+                             umf_memory_properties_handle_t *props_handle);


document ownership of this handle.

lplewa · 2025-06-12T09:17:10Z

+typedef struct umf_memory_properties_t {
+    // TODO alloc_info_t
+    void *ptr;
+    umf_memory_pool_handle_t pool;
+    umf_memory_provider_handle_t provider;
+    uint64_t id;
+    void *base;
+    size_t base_size;
+} umf_memory_properties_t;


if we have api to literaly read this structure - it should be defined in .c file

other files directly access fields of this struct so it cannot be moved

lplewa · 2025-06-12T09:17:48Z

+#include <umf/memory_provider.h>
+
+#if UMF_BUILD_LEVEL_ZERO_PROVIDER
+#include "ze_api.h"


i do not see a reason to include this file there

lplewa · 2025-06-12T09:18:49Z

+    /// @param value [out] pointer to the preallocated variable where the value will be stored
+    /// @return UMF_RESULT_SUCCESS on success or appropriate error code on failure,
+    ///         UMF_RESULT_ERROR_INVALID_ARGUMENT if memory_property_id is invalid or value is NULL,
+    ///         UMF_RESULT_ERROR_NOT_SUPPORTED if the property is not supported by this provider.


This is not true:
Please include tests to ensure that comment it true (as far i see we return EINVAL

Especially please include also test with custom user provided provider, which support extra property which is user defined.

comment changed
about user provider - this should be done by adding an example, not the test, and probably in a separate PR

Copilot

Pull Request Overview

This PR adds a comprehensive memory properties API to the unified memory framework, enabling users to query various properties of allocated memory such as memory type, base address, size, and provider-specific information.

Introduces memory properties API with two main functions: umfGetMemoryPropertiesHandle and umfGetMemoryProperty
Extends memory provider operations with ext_get_allocation_properties function
Adds comprehensive test coverage for the new memory properties functionality

Reviewed Changes

Copilot reviewed 37 out of 38 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
include/umf/base.h	Defines memory property types and handle structures
include/umf/experimental/memory_props.h	Public API for memory properties functionality
src/memory_props.c	Core implementation of memory properties API
src/provider/provider_tracking.c	Updates tracking provider to support memory properties
test/props/	Comprehensive test suite for memory properties API
Various provider files	Updates all providers to support new allocation properties interface

lukaszstolarczuk

LGTM

bratpiorka force-pushed the rrudnick_props_api branch 15 times, most recently from 92db71d to 56573df Compare May 16, 2025 13:42

bratpiorka force-pushed the rrudnick_props_api branch 15 times, most recently from a5c357c to 17f6335 Compare June 4, 2025 17:42

bratpiorka force-pushed the rrudnick_props_api branch 17 times, most recently from 96bb935 to 5015432 Compare June 11, 2025 12:19

lplewa requested changes Jun 12, 2025

View reviewed changes

bratpiorka force-pushed the rrudnick_props_api branch 4 times, most recently from 3d40afe to 7228b45 Compare June 12, 2025 12:01

Copilot AI reviewed Jul 22, 2025

View reviewed changes

Comment thread test/utils/cpp_helpers.hpp

Comment thread test/providers/provider_level_zero.cpp

Comment thread test/providers/provider_cuda.cpp

Comment thread src/provider/provider_tracking.c Outdated

KFilipek approved these changes Jul 22, 2025

View reviewed changes

lukaszstolarczuk mentioned this pull request Jul 23, 2025

add memory properties API #1263

Closed

lukaszstolarczuk reviewed Jul 23, 2025

View reviewed changes

lplewa approved these changes Aug 7, 2025

View reviewed changes

lukaszstolarczuk reviewed Aug 7, 2025

View reviewed changes

lukaszstolarczuk approved these changes Aug 7, 2025

View reviewed changes

add memory properties API

a7ca9f6

		/// be filled. NOTE: the type and size of the value depends on the
		/// memory property ID and should be checked in the documentation

Uh oh!

Conversation

bratpiorka commented May 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lplewa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bratpiorka Jul 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lukaszstolarczuk left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

bratpiorka commented May 8, 2025 •

edited

Loading

bratpiorka Jul 17, 2025 •

edited

Loading