-
-
Notifications
You must be signed in to change notification settings - Fork 428
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Alloc membind #2923
Alloc membind #2923
Conversation
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove non-used code.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
6524d7c
to
d9b91d1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
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.