MEMHEAP/BASE: removed hard segments limit #10478

hoopoepg · 2022-06-17T11:52:39Z

enabled dynamic segment's info allocation
removed hard limit for segments count

oshmem/mca/memheap/base/memheap_base_static.c

oshmem/mca/memheap/base/memheap_base_alloc.c

oshmem/mca/spml/ucx/spml_ucx.c

oshmem/mca/spml/ucx/spml_ucx.h

brminich · 2022-06-20T16:36:05Z

oshmem/mca/memheap/base/memheap_base_alloc.c

+        return NULL;
+    }
+
+    if (map->n_segments >= map->capacity) {


this conflicts with the assert above (line 109)

this conflicts with the assert above (line 109)

no: when capacity == 0 n_segments must be 0 too, in other cases must be (n_segments < capacity). assert is not strongly valid but covers cases when memory corruption happened

why not checking for 0 here instead of comparing n_segments and capacity?
currently it looks quite confusing

brminich · 2022-06-20T16:37:31Z

oshmem/mca/memheap/base/memheap_base_static.c

+                MEMHEAP_ERROR("failed to allocate segment");
+                return OSHMEM_ERR_OUT_OF_RESOURCE;


plz make a cleanup of already allocated ones

plz make a cleanup of already allocated ones

ok, done

oshmem/mca/memheap/base/memheap_base_static.c

hoopoepg · 2022-06-21T10:23:00Z

@yosefe ok to squash?

yosefe · 2022-06-29T12:46:39Z

oshmem/mca/memheap/base/memheap_base_alloc.c

+    if (map->n_segments == map->capacity) {
+        capacity = opal_min(mca_memheap_base_max_segments,
+                            opal_max(map->capacity * 2,
+                                     MCA_MEMHEAP_MAX_SEGMENTS));


i think need to rename MCA_MEMHEAP_MAX_SEGMENTS to MCA_MEMHEAP_MIN_SEGMENTS
since this is actually the initial/minimal value of number of segments

yosefe · 2022-06-29T12:47:15Z

oshmem/mca/memheap/base/memheap_base_frame.c

@@ -36,6 +36,7 @@ int mca_memheap_base_key_exchange = 1;
 opal_list_t mca_memheap_base_components_opened = {{0}};
 int mca_memheap_base_already_opened = 0;
 mca_memheap_map_t mca_memheap_base_map = {{{{0}}}};
+int mca_memheap_base_max_segments = MCA_MEMHEAP_MAX_SEGMENTS;


why need to limit it by default to 32?

why need to limit it by default to 32?

used as allowed number of segments to allocate, when limit is reached - warning prompted

let's rename it (and respective configuration) to something like mca_memheap_num_segments_warn
since it's not a hard limit anymore, but just threshold for showing a warning

done, macro is removed, variable is renamed

yosefe · 2022-06-29T12:47:45Z

oshmem/mca/memheap/base/memheap_base_frame.c

@@ -59,6 +60,13 @@ static int mca_memheap_base_register(mca_base_register_flag_t flags)
                          MCA_BASE_VAR_SCOPE_LOCAL,
                          &mca_memheap_base_config.device_nic_mem_seg_size);

+    mca_base_var_register("oshmem", "memheap", "base", "max_segments",
+                          "Maximum number of segments to register per process",


"Maximum number of shared data segments per process",

"Maximum number of shared data segments per process",

updated

yosefe · 2022-06-29T12:49:47Z

oshmem/mca/memheap/base/memheap_base_static.c

-        void* start;
-        void* end;
-    } mem_segs[MCA_MEMHEAP_MAX_SEGMENTS];
+    memheap_static_segment_t *mem_segs;


maybe just allocate mca_memheap_base_max_segments up front?
this struct is small enough anyway

maybe just allocate mca_memheap_base_max_segments up front?
this struct is small enough anyway

removed static segments allocation

hoopoepg · 2022-07-04T15:46:49Z

@yosefe ok to squash?

hoopoepg · 2022-07-25T06:46:23Z

@yosefe ???

awlauria · 2022-08-02T16:38:34Z

@yosefe @janjust please review

yosefe · 2022-08-11T09:53:17Z

oshmem/mca/memheap/base/memheap_base_frame.c

@@ -36,6 +36,7 @@ int mca_memheap_base_key_exchange = 1;
 opal_list_t mca_memheap_base_components_opened = {{0}};
 int mca_memheap_base_already_opened = 0;
 mca_memheap_map_t mca_memheap_base_map = {{{{0}}}};
+int mca_memheap_base_max_segments = MCA_MEMHEAP_MAX_SEGMENTS;


let's rename it (and respective configuration) to something like mca_memheap_num_segments_warn
since it's not a hard limit anymore, but just threshold for showing a warning

oshmem/mca/memheap/base/memheap_base_alloc.c

oshmem/mca/memheap/base/memheap_base_frame.c

oshmem/mca/spml/ucx/spml_ucx.c

yosefe · 2022-08-11T13:01:25Z

oshmem/mca/memheap/base/memheap_base_frame.c

@@ -61,11 +61,12 @@ static int mca_memheap_base_register(mca_base_register_flag_t flags)
                          &mca_memheap_base_config.device_nic_mem_seg_size);

    mca_base_var_register("oshmem", "memheap", "base", "max_segments",
-                          "Maximum number of shared data segments per process",
+                          "Display a warning if the number of segments of "
+                          "shared heap exceeds this value",


minor:
Display a warning if the number of segments of the shared memheap exceeds this value"

yosefe · 2022-08-18T07:31:28Z

@hoopoepg pls squash

- enabled dynamic segment's info allocation - removed hard limit for segments count - fixed error handling on no enough memory - refactpring for static segments initialization: removed middle segment allocation - fixed poterntial issue in spml_ucx - incorrect error handling which may lead to crash Signed-off-by: Sergey Oblomov <sergeyo@nvidia.com>

hoopoepg force-pushed the topic/oshmem-dynamic-segment-allocation branch from 9b5d882 to ad59721 Compare June 17, 2022 11:53

yosefe reviewed Jun 19, 2022

View reviewed changes

hoopoepg force-pushed the topic/oshmem-dynamic-segment-allocation branch from f0200bc to 8309ddf Compare June 20, 2022 10:15

brminich reviewed Jun 20, 2022

View reviewed changes

brminich approved these changes Jun 21, 2022

View reviewed changes

hoopoepg force-pushed the topic/oshmem-dynamic-segment-allocation branch from 4aa50cb to 87fd4b4 Compare June 27, 2022 12:33

yosefe reviewed Jun 29, 2022

View reviewed changes

awlauria requested a review from janjust August 2, 2022 16:38

yosefe reviewed Aug 11, 2022

View reviewed changes

hoopoepg force-pushed the topic/oshmem-dynamic-segment-allocation branch from 0a8c5e3 to 6dc1101 Compare August 11, 2022 12:52

yosefe reviewed Aug 11, 2022

View reviewed changes

yosefe approved these changes Aug 16, 2022

View reviewed changes

hoopoepg force-pushed the topic/oshmem-dynamic-segment-allocation branch from b516a4d to 1b26bbd Compare August 18, 2022 07:37

yosefe approved these changes Aug 21, 2022

View reviewed changes

yosefe merged commit 26742ec into open-mpi:main Aug 21, 2022

This was referenced Aug 24, 2022

MEMHEAP/BASE: removed hard segments limit - v5.0 #10711

Merged

MEMHEAP/BASE: removed hard segments limit - v4.1 #10712

Merged

		MEMHEAP_ERROR("failed to allocate segment");
		return OSHMEM_ERR_OUT_OF_RESOURCE;

MEMHEAP/BASE: removed hard segments limit #10478

MEMHEAP/BASE: removed hard segments limit #10478

Uh oh!

Conversation

hoopoepg commented Jun 17, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

hoopoepg commented Jun 21, 2022

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hoopoepg commented Jul 4, 2022

Uh oh!

hoopoepg commented Jul 25, 2022

Uh oh!

awlauria commented Aug 2, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yosefe commented Aug 18, 2022

Uh oh!

Uh oh!