Andrew Stubbs wrote:
This patch introduces a new custom memory allocator for use with pinned
memory (in the case where the Cuda allocator isn't available). In future,
this allocator will also be used for Managed Memory. Both memories are
incompatible with the system malloc because allocated memory cannot share a
page with memory allocated for other purposes.
This means that small allocations will no longer consume an entire page of
pinned memory. Unfortunately, it also means that pinned memory pages will
never be unmapped (although they may be reused). This isn't a technical
limitation; the "free" algorithm could be extended in future, if needed.
I noticed that we now have two allocators in libgomp:
* basic-allocator.c - to handle low-latency memory (nvptx, gcn)
which only gets used as:
- config/gcn/allocator.c:#include "../../basic-allocator.c"
- config/nvptx/allocator.c:#include "../../basic-allocator.c"
* simple-allocator.c (of this patch)
which gets always build but only called in
config/linux/allocator.c
* * *
basic-allocator.c's functions are mostly 'static', however,
basic_alloc_init is not - for no good reason.
Hence, __gcn_lowlat_init 'T' in the .o file.
As you touch basic-allocator.c, can you mark 'basic_alloc_init'
as 'static'? (aka, e.g., __gcn_lowlat_init in allocate.o).
* * *
For simple-allocator.c, the use is quite similar - but:
* It is always build, not only when config/linux/ is
build
- This pointlessly increases the size of the .a file
and might cause follow-up issues
- It uses atomics - but those are not protected by '#if'
I fear that will break on some system that don't have it.
(For the last item, I am not even sure whether all GNU Linux
systems have it only nearly all.)
Side remark:
At least when building as an .a file, we currently pollute
quite some namespaces (besides gomp_, omp_, GOMP_, acc_,
goacc_, GOACC_). Namely:
build_indirect_map
priority_queue_find (priority_..., prio_splay_...)
__reduction_lock
splay_tree_...
And your patch adds on top:
simple_alloc
simple_alloc_init_context
simple_alloc_register_memory
simple_alloc_splay_tree_foreach
simple_alloc_splay_tree_foreach_lazy
simple_alloc_splay_tree_insert
simple_alloc_splay_tree_lookup
simple_alloc_splay_tree_lookup_node
simple_alloc_splay_tree_remove
simple_free
simple_realloc
I think we should avoid this and should put it into
the gomp_ namespace - assuming that we don't make
simple_* static and include it into config/linux/allocate.c
[Due to visibility, that should* not an issue for dynamic
linking, but for static linking ...
Disclaimer: No idea whether hiding works on all DSO systems.]
* * *
+ if (!pagesize)
+ pagesize = sysconf(_SC_PAGE_SIZE);
+
If I apply the patch, I see a tailing space (' ') in the
otherwise empty line.
Sorry for only spotting the build-related issue only now.
Otherwise, the patch LGTM.
Thanks,
Tobias
