common/ofi: Add comments for the import monitor code #9387
Conversation
|
Adding some comments to make the code more understandable. |
opal/mca/common/ofi/common_ofi.c
Outdated
|
|
||
| /* | ||
| * These no-op functions are necessary since libfabric does not allow null | ||
| * function pointers here or else it will segfault. |
There was a problem hiding this comment.
drop the "or else it will segfault". The fact that it doesn't allow NULL pointers is sufficient.
| #endif /* OPAL_OFI_IMPORT_MONITOR_SUPPORT */ | ||
|
|
||
| OPAL_DECLSPEC int opal_common_ofi_init(void) | ||
| { |
There was a problem hiding this comment.
All functions that do interesting work should have a doxygen style header.
You should call out things like what this function is doing and why. At some point, you should explicitly say that you're injecting a monitor into Libfabric so that Libfabric can use Open MPI's memory hooks as its memory monitor, rather than trying to use its own.
There was a problem hiding this comment.
I had a comment in the header file for the function, but it isn't doxygen style, I'll update it and give it more information
opal/mca/common/ofi/common_ofi.c
Outdated
| int ret; | ||
|
|
||
| /* | ||
| * Global refcnt and boolean are used to ensure we only initialize once |
There was a problem hiding this comment.
This pattern is well established and doesn't need a comment about why its here. However, you do need to explain how this fits in with the thread safety requirements of this function (and document those requirements in the function header).
There was a problem hiding this comment.
I think to make it easier for users to call this function, I'll add a new opal_mutex_t lock and lock around the whole function.
There was a problem hiding this comment.
why is that the right thing to do? That has its own risks, especially around initialization of the mutex in the initialization function you're currently writing. Just get the definition right.
There was a problem hiding this comment.
There's already a common mutex lock that is being used, I think it's fine to re-use that
| } | ||
|
|
||
| /* | ||
| * This cache object doesn't do much, but is necessary for the API to work. |
There was a problem hiding this comment.
this doesn't really help the reader. Bigger questions would be "Why 1.13" and "why "mr_cache"". Those are the things you should comment on.
opal/mca/common/ofi/common_ofi.c
Outdated
| OPAL_DECLSPEC int opal_common_ofi_fini(void) | ||
| { | ||
| /* | ||
| * Global refcnt and boolean are used to ensure we only finalize once |
There was a problem hiding this comment.
you don't need to do this comment.
For thread safety, we have the init/fini functions locked under the common ofi lock. Signed-off-by: William Zhang <[email protected]>
Signed-off-by: William Zhang <[email protected]>
|
Updated comments, added locking to the init/fini function since I don't think it was actually thread safe previously. Reused the common ofi mutex lock for this. |
|
Replaced by #9441 |
2 participants
Signed-off-by: William Zhang [email protected]