Alloc membind #2923
Alloc membind #2923
Conversation
biddisco
force-pushed
the
alloc_membind
branch
3 times, most recently
from
c14fa15
to
489d456
Compare
| hpx_hwloc_bitmap_wrapper const* bmp); | ||
|
|
||
| // the raw bitmap object | ||
| hwloc_bitmap_t bmp_; |
There was a problem hiding this comment.
Please make the bmp_ member private.
| @@ -41,6 +42,35 @@ namespace hpx { namespace resource | |||
|
|
|||
| namespace hpx { namespace threads | |||
| { | |||
| struct hpx_hwloc_bitmap_wrapper { | |||
There was a problem hiding this comment.
This class will things make blow up if an instance of it is copied:
hpx_hwloc_bitmap_wrapper hwlw1 (...);
hpx_hwloc_bitmap_wrapper hwlw2 = hwlw1;
which will cause a double-free.
Please add proper copy constructor/assignment operator or alternatively proper move semantics.
There was a problem hiding this comment.
Alternatively you can make the class non-copyable (and non-movable).
| /// \brief Please see hwloc documentation for the corresponding | ||
| /// enums HWLOC_MEMBIND_XXX | ||
| enum hpx_hwloc_membind_policy : int { | ||
| HPX_MEMBIND_DEFAULT = 0, |
There was a problem hiding this comment.
Please use the original hwloc constants to initialize the values of this enumerator. I also think that there is no need to prefix the enumerator value names with HPX_. Lastly, we usually use ALL_CAPTIAL_NAMES for macros only.
| static_cast<unsigned int>(pu_obj->os_index)); | ||
| } | ||
| } | ||
| // std::cout << "mask_to_bitmap mask_cref is " << mask; |
| if (hwloc_bitmap_isset(bitmap, idx) != 0) | ||
| set(mask, detail::get_index(pu_obj)); | ||
| } | ||
| // std::cout << "bitmap_to_mask mask_cref is " << mask; |
There was a problem hiding this comment.
Same here, please remove the non-used code sections.
|
@biddisco Do you plan to work on this further? |
|
Yes of course. All my work is based on this. |
Great.
No worries, I was just wondering while going through all tickets and PRs. |
biddisco
force-pushed
the
alloc_membind
branch
from
489d456
to
6524d7c
Compare
| @@ -92,6 +93,15 @@ namespace hpx { namespace threads | |||
| } | |||
|
|
|||
| /////////////////////////////////////////////////////////////////////////// | |||
| std::ostream& operator<<(std::ostream& os, hpx_hwloc_bitmap_wrapper const* bmp) | |||
| { | |||
| char buffer[256]; | |||
There was a problem hiding this comment.
Is this buffer large enough for a large number of PUs? Is the generated string hex formatted or binary?
There was a problem hiding this comment.
yes, it's a hex output. 256 chars should be more than enough for any multi-core chips we see for the foreseeable future.
|
Should we also make HWLOC mandatory, remove the abstract topology base class, and remove the no-topology placeholder class? |
|
I think we should, but it should appear as a separate PR. I will create an issue for it. hwloc 2.0 is due to be released soon, we should target that API and timeframe. I would like to continue with this PR independently of that work and have it merged and ready before attempting the hwloc refactoring. |
Fine by me, would you mind addressing/responding to the other comments before that? |
In order to allocate memory and bind it to numa domains using hwloc binding options we need a numa mask that wraps or is compatible with with hwloc bitmap.
biddisco
force-pushed
the
alloc_membind
branch
from
6524d7c
to
d9b91d1
Compare
|
FYI, I think this broke the Clang 5.0 build on POWER8. see http://ktau.nic.uoregon.edu:8020/builders/ppc64le-clang5-release/builds/62/steps/Compile%20HPX/logs/stdio for errors. Specifically: /home/users/khuck/buildbot/slaves/phylanx/ppc64le-clang5-release/build/tools/buildbot/src/hpx/src/runtime/threads/policies/hwloc_topology_info.cpp:1172:35: error: use of undeclared identifier 'HWLOC_OBJ_NUMANODE'; did you mean 'HWLOC_OBJ_NODE'? |
|
This morning I added a #define check for hwloc version so that older hwloc installations will disable the new feature. |
|
@biddisco the worst that can happen is that the feature in HPX gets disabled even if its theoretically supported in hwloc. I don't think this is a problem. |
This provides an interface to the hwloc_alloc_membind function and requires a numa nodeset to be passed that the memory is bound to.
This in turn requires a nodeset and rather than using a mask and converting to and from hwloc types, it is less error prone to use an hwloc bitmap object wrapped in a c++ holder to manage deletion.