-
-
Notifications
You must be signed in to change notification settings - Fork 310
Make H5FL package threadsafe #5195
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
base: develop
Are you sure you want to change the base?
Conversation
Signed-off-by: Quincey Koziol <[email protected]>
Signed-off-by: Quincey Koziol <[email protected]>
…tocol Signed-off-by: Quincey Koziol <[email protected]>
Signed-off-by: Quincey Koziol <[email protected]>
Signed-off-by: Quincey Koziol <[email protected]>
Signed-off-by: Quincey Koziol <[email protected]>
…protocol Signed-off-by: Quincey Koziol <[email protected]>
Signed-off-by: Quincey Koziol <[email protected]>
Signed-off-by: Quincey Koziol <[email protected]>
@qkoziol the conflicts are Makefile.ams that are gone in develop since autotools support has been removed there. Do we need to fix the conflicts, or is this PR to be superseeded by another one that will be coming? |
I have been waiting for the CMake builds to stabilize, which I believe we're at. I'll update this to resolve the conflicts this week. |
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.
Pull Request Overview
Make the H5FL free-list package threadsafe by introducing DLFTT-aware mutexes, atomic size_t wrappers, deferred global initialization macros, and initializing the free-list interface at library startup. Tests are expanded to verify free-list behavior under concurrency with new setup routines.
- Introduce
H5_global_t
andH5_GLOBAL_INIT
for deferred init of globals - Add
H5TS_dlftt_mutex_t
and atomicsize_t
wrappers for safe concurrent access - Initialize the free-list package at startup and extend tests (
tts_h5fl
,test_misc35
) for multithreading
Reviewed Changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
test/ttsafe.h | Declare tts_h5fl test routine |
test/ttsafe.c | Register tts_h5fl in the ttsafe test suite |
test/tmisc.c | Extend test_misc35 to check free-list sizes pre/mid/post |
test/h5test.h | Add declaration for h5_setup_local_rand |
test/h5test.c | Implement h5_setup_local_rand for deterministic seeding |
test/Makefile.am | Add ttsafe_h5fl.c to ttsafe_SOURCES |
test/CMakeLists.txt | Include ttsafe_h5fl.c in CMake ttsafe_SOURCES |
src/Makefile.am | Include H5TSdlftt_mutex.c in library build |
src/CMakeLists.txt | Add H5TSdlftt_mutex.c to CMake H5TS_SOURCES |
src/H5private.h | Define H5_global_t and H5_GLOBAL_INIT macros |
src/H5TSprivate.h | Add atomic_size_t , H5TS_dlftt_mutex_t , and wrappers |
src/H5TSpkg.h | Expose H5TS__get_dlftt prototype if not already defined |
src/H5TSint.c | Define and initialize the bootstrap DLFTT mutex |
src/H5TSdlftt_mutex.h | Inline DLFTT mutex acquire/release implementations |
src/H5TSdlftt_mutex.c | Implement DLFTT mutex init/destroy routines |
src/H5TSatomic.h | Add inline atomic_size_t load/store/add/sub functions |
src/H5TSatomic.c | Implement H5TS_atomic_init_size_t and destroy functions |
src/H5FLprivate.h | Embed H5_global_t and DLFTT mutex into free-list structs |
src/H5.c | Call H5FL_init() in H5_init_library for concurrency builds |
Comments suppressed due to low confidence (3)
test/ttsafe.c:176
- [nitpick] Using a leading '-' in the test name may unintentionally disable the test or confuse the test harness. Rename it to "h5fl" and rely on the build-time guard to skip the test when concurrency is disabled.
AddTest("-h5fl", tts_h5fl, NULL, NULL, NULL, 0, "Multithreaded H5FL package");
test/h5test.h:1222
- [nitpick] The Doxygen comment for h5_setup_local_rand lacks \param tags for
test_name
andpredefined_seed
. Add parameter documentation to fully describe the function's inputs.
* \return none
src/H5private.h:1750
- [nitpick] The field name
init
is somewhat ambiguous. Consider renaming it toinitialized
to more clearly indicate its boolean purpose.
bool init; /* Whether the global has been initialized */
/* Threadsafe package testing routines */ | ||
void tts_h5fl(void *); |
Copilot
AI
Jun 11, 2025
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.
The declaration of tts_h5fl is not guarded by H5_HAVE_CONCURRENCY, leading to a mismatch with its registration in test/ttsafe.c. Wrap this under the same H5_HAVE_CONCURRENCY guard to avoid undefined references when concurrency is disabled.
/* Threadsafe package testing routines */ | |
void tts_h5fl(void *); | |
/* Threadsafe package testing routines */ | |
#ifdef H5_HAVE_CONCURRENCY | |
void tts_h5fl(void *); | |
#endif /* H5_HAVE_CONCURRENCY */ |
Copilot uses AI. Check for mistakes.
size_t ret_value; | ||
|
||
/* Lock mutex that protects the "atomic" value */ | ||
H5TS_mutex_lock(&obj->mutex); |
Copilot
AI
Jun 11, 2025
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.
The return value of H5TS_mutex_lock is ignored in these atomic operations. It’s safer to check for lock/unlock failures to avoid potential deadlocks or inconsistent state if the mutex operations fail.
Copilot uses AI. Check for mistakes.
Signed-off-by: Quincey Koziol <[email protected]>
Signed-off-by: Quincey Koziol <[email protected]>
Signed-off-by: Quincey Koziol <[email protected]>
Signed-off-by: Quincey Koziol <[email protected]>
Removed unnecessary temporary variable. Signed-off-by: Quincey Koziol <[email protected]>
Don't read MTIME/MTIME_NEW messages when the object header is deserialized. Signed-off-by: Quincey Koziol <[email protected]>
Remove unneccesary temporary variable and simplify buffer size parameter to H5C__reconstruct_cache_entry(). Signed-off-by: Quincey Koziol <[email protected]>
Library:
Added mutex that obeys disable-locking-for-this-thread (DLFTT) semantics, to enable threadsafe data structures within the library that "suspend" locking when a thread has exclusive ownership of the library. (in src/H5TSdlftt_mutex.[ch])
Initialize the threadsafe aspects of the H5FL package at library startup, when concurrency is enabled. (in src/H5.c)
Added new H5_global_t type, with H5_GLOBAL_INIT macro, to hold and initialize threadsafe globals with complex initialization that needs to be performed dynamically at runtime. (in src/H5private.h)
Added wrapper for atomic size_t variables. (in src/H5TSprivate.h and src/H5TSatomic.c)
Added new H5TS_bootstrap_g DLFTT mutex in src/H5TSint.c to support new threadsafe globals. (in src/H5TSint.c)
H5FL package changes: (in src/H5FL.c and src/H5FLprivate.h)
Testing:
Added new h5_setup_local_rand() support routine to initialize h5_local_rand() with gettimeofday() or a predefined seed value.
Expand test/tmisc.c, test_misc35(), to cover additional cases.
Added tts_h5fl() to ttsafe test, skipped by default for non-concurrency builds.
The tts_h5fl() testing generates multiple random test vectors of operations for each type of free list (regular, block, factory, and array) and then spawns threads that randomly chooses a set of test vectors to execute concurrently.