-
Notifications
You must be signed in to change notification settings - Fork 3
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
Support multi-threaded H5VL operations #7
Support multi-threaded H5VL operations #7
Conversation
This requires a threadsafe H5SL or H5SL replacement to be threadsafe.
@@ -0,0 +1,52 @@ | |||
/* Test */ |
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 file was added to support building the API tests with autotools
SetTestNoCleanup(); | ||
} else if ((strcmp(*argv, "-maxthreads") == 0) || (strcmp(*argv, "-t") == 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.
These changes to testframe are taken from Jordan's changes to the test framework in the upstream.
@@ -42,137 +42,6 @@ static const char *FILENAME[] = {"vol_test_file", NULL}; | |||
#define N_ELEMENTS 10 | |||
#define NAME_LEN 512 | |||
|
|||
/* A VOL class struct to verify registering optional operations */ |
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.
Definitions for the dynamic registration VOL were moved upwards into h5test to allow the MT VL tests to test dynamic op registration in parallel. It would probably be better to move them into a new header file only used by the MT VL tests and vol.c, but I ran into some issues implementing that so I've left it like this for now.
@@ -5914,7 +5914,7 @@ test_misc35(void) | |||
ret = H5get_free_list_sizes(®_size_start, &arr_size_start, &blk_size_start, &fac_size_start); | |||
CHECK(ret, FAIL, "H5get_free_list_sizes"); | |||
|
|||
#if !defined H5_NO_FREE_LISTS && !defined H5_USING_MEMCHECKER |
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.
Free lists aren't threadsafe, so don't check them when using multithreading.
@@ -1,3 +1,5 @@ | |||
HDF5 version 1.14.2 released on 2023-08-11 |
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.
Already below past the dashed-line separator
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.
One of the h5repack tests parses the current version of HDF5 from the third word in the README file and breaks if this is moved.
configure.ac
Outdated
## TODO - Probably this should be reworked or moved somewhere else | ||
|
||
# First arg has name and value. If first in undef, then non-empty second value is default | ||
m4_define([SETUP_API_TEST_VAR], |
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.
What are these macros for? Reaching for m4 macros is probably very overkill
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.
SETUP_API_TEST_VAR
is a helper to set up macros if an argument is provided at configuration time. If not, then they're either set to a default value or not defined. It's used for the new variables introduced to handle building the API tests with autotools.
@@ -27,7 +27,7 @@ | |||
|
|||
/* Macros for turning off free lists in the library */ | |||
/*#define H5_NO_FREE_LISTS*/ | |||
#if defined H5_NO_FREE_LISTS || defined H5_USING_MEMCHECKER | |||
#if defined H5_NO_FREE_LISTS || defined H5_USING_MEMCHECKER || defined H5_HAVE_MULTITHREAD |
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 was the approach that I suggested, though @jrmainzer had some concerns about going that way, so we may want to consider something different.
test/API/H5_api_test.h
Outdated
* | ||
* TBD: Note that it invokes another macro-defined routine...which sets up threadlocal info... | ||
* */ | ||
#define DECLARE_MT_API_TEST_FUNC_OUTER(func) \ |
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 feels like a very over-engineered macro solution to a situation which would probably be better handled by just having logic in the testing framework itself to manage threads. I'd suggest that we consider a different approach where an individual test or the whole test program can signal that it would prefer for the testing framework to manage threads.
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.
Would it be reasonable to add a new parameter to AddTest
to control whether a test is performed in multiple threads? Since the test parameters are opaque to the test framework, I don't think there are many other reasonable ways to do it. We could maybe check the test name for an mt_
prefix or something like that, but that seems opaque and easy to forget when adding new tests later on.
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.
I'm thinking something like that, yes, though maybe give some consideration towards if there is a more generic way to extend AddTest
so that we can reduce the need to go and update calls to it everywhere whenever we need a new testing parameter. I think it will simply be more maintainable to have the testing framework handle threads when asked for, rather than using an ad-hoc macro-based solution. Any time you find that you're reaching for macros to basically implement an entire function, it should usually tell you right away that there's likely a better solution.
@@ -1185,6 +1194,7 @@ H5CX_set_dcpl(hid_t dcpl_id) | |||
H5CX_node_t **head = NULL; /* Pointer to head of API context list */ | |||
|
|||
FUNC_ENTER_NOAPI_NOINIT_NOERR | |||
H5_API_LOCK |
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.
As much as I hate the proliferation of the FUNC_ENTER/LEAVE macros, it may make sense to have a pair for functions like this that still need to acquire the lock immediately on entry and release it right before exit. Otherwise, the additional function prologue/epilogue macros become a burden and create opportunities for accidentally missing one or getting them in the wrong order.
Also consider that some of the new FUNC_ENTER macros that were added in this branch previously acquire the lock in the middle of operations performed by the whole macro, e.g.
#define FUNC_ENTER_API(err) \
{ \
{ \
hbool_t api_ctx_pushed = FALSE; \
\
FUNC_ENTER_API_COMMON \
**FUNC_ENTER_API_THREADSAFE**; \
FUNC_ENTER_API_INIT(err); \
FUNC_ENTER_API_PUSH(err); \
/* Clear thread error stack entering public functions */ \
H5E_clear_stack(NULL); \
{
Have you checked to see if there may be some places where H5_API_LOCK
occurs too late, when something in the FUNC_ENTER macro preceding it may have needed a lock?
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.
H5E
operations in enter/leave macros should be safe since error stacks are threadlocal. Same for pushing nodes onto and popping nodes off of the context stack.
It looks like a double initialization from FUNC_ENTER_API_INIT()
is possible right now. That should be changed to use a compare and swap on the global initialization tracking structure, which should also be converted to an atomic.
The code stack push/pop probably isn't multi-threadsafe, but since these were removed in 1.14.5, I don't think we need to worry about supporting them.
57904e2
to
c599c85
Compare
acbb10e
to
2d552f8
Compare
2d552f8
to
2d124c5
Compare
This PR has been broken up into the following cumulative PRs: API Test & Test Framework Changes - #8 |
Support multi-threading in the H5VL module (H5VL.c, H5VLcallback.c, H5VLdyn_ops.c, and H5VLint.c)
The global mutex is no longer acquired upon entry to H5VL API routines. The high-level outline for the changes is in the H5VL threadsafety document, but the major changes are:
H5_API_LOCK
/H5_API_UNLOCK
(Native VOL registration, routines in H5T/H5S, etc.)Push global lock acquisition down to VOL layer for most VOL-using API routines
For routines that bottom out in a VOL operation (Most major API routines in H5A, H5F, H5D, H5G, H5O, H5L, H5Tcommit), the global mutex is now only acquired right before VOL callback invocation, and only if the active VOL connector (or next active VOL connector in the stack) does not have the threadsafe capability flag.
API Tests now support multi-threaded execution via the -maxthreads option.
Concurrent test execution is supported by having each thread target a different file depending on a test-assigned thread index. For example, when running the API tests with 16 threads, each thread will operate on one of "000H5_api_test.h5" through "015H5_api_test.h5".
prefix_filename
now prepends this thread prefix along with thetest_path_prefix
, so any files created within specific tests are also thread-specific.To support this, two pieces of information are stored in each each thread: the test-assigned thread index, and the final filename for the thread (test path prefix + thread index + base filename).
To avoid excessive file creation and deletion, the thread-specifics API test containers are created once before API tests are run and cleaned up after all tests are complete.
To keep separate tests independent, the threads are created and joined during the invocation of each individual test. This is implemented with a pair of macros. If the library is built with
H5_HAVE_MULTITHREAD
, then every API test routine<api_test_func>()
has two multi-threaded counterparts:<api_test_func>_thread_outer()
, which handles the spawning and joining of threads, and<api_test_func>_thread_inner()
, which sets up thread local information before running the original test. At AddTest time, each API test function which supports being run in multiple threads should be added asMT_API_TEST_FUNC_OUTER(<api_test_func>)
, which is either the_thread_outer()
counterpart or the original test routine, depending on whether multithread is enabled.The inner and outer multithread routines are created for each test routine by another macro,
MULTI_DEFINE
.Add dedicated tests for multithreaded H5VL behavior in testmthdf5
The individual tests are implemented in
mt_vl_test.c
.Add MT Native Wrapper VOL and MT Passthru Wrapper VOL
These connectors are used to test the H5VL module's ability manage concurrent thread entry into a VOL connector/back into the VOL layer from a passthrough connector. The MT Native Wrapper is a connector with the threadsafe flag enabled, which simply grabs the global mutex before invoking the native VOL operation. The MT Passthru Wrapper is a clone of the passthrough VOL with the threadsafe flag enabled.
Add an option to build the API tests with Autotools
Misc
Virtual locks for IDs were originally part of this, but have been separated from this PR and should come in afterwards - mattjala#7