Skip to content

Commit

Permalink
Fix incorrect lock counting in virtual locks
Browse files Browse the repository at this point in the history
  • Loading branch information
@mattjala
mattjala committed Oct 31, 2024
1 parent 73a83e8 commit ced5c14
Show file tree
Hide file tree
Showing 4 changed files with 51 additions and 17 deletions.
33 changes: 20 additions & 13 deletions src/H5I.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2068,17 +2068,16 @@ herr_t H5I_vlock_enter(hid_t id) {
/* If this attempt fails, this is undone by assignment of mod_info_k */
mod_info_k.lock_count++;

if (mod_info_k.lock_count <= 0) {
if (mod_info_k.lock_count <= 0)
HGOTO_DONE(FAIL);
}

/* If ID info was concurrently modified, restart and check again */
} while (!atomic_compare_exchange_strong(&(id_info_ptr->k), &info_k, mod_info_k));

/* Validity check */
if ((size_t) mod_info_k.lock_count > mod_info_k.app_count) {
HGOTO_DONE(FAIL);
}

assert(mod_info_k.lock_count > 0);
/* Ensure that ID is not being used more times than app ref count allows */
assert(mod_info_k.lock_count <= (int) mod_info_k.app_count + mod_info_k.app_unlocks );

done:
FUNC_LEAVE_NOAPI(ret_value);
Expand Down Expand Up @@ -2122,10 +2121,9 @@ herr_t H5I_vlock_exit(hid_t id) {
HGOTO_DONE(SUCCEED);

/* Get ID info */
if ((id_info_ptr = H5I__find_id(id)) == NULL) {
if ((id_info_ptr = H5I__find_id(id)) == NULL)
/* Assume ID was released during the course of the API routine */
HGOTO_DONE(SUCCEED);
}

/* Ensure that we have exlusive write access to the ID */
do {
Expand All @@ -2137,21 +2135,30 @@ herr_t H5I_vlock_exit(hid_t id) {
continue;
}

if (info_k.lock_count == 0) {
/* Lock count is already 0, no need to decrement */
HGOTO_DONE(SUCCEED);
}

/* Update the lock count and check for consistency with ID ref count */
mod_info_k = info_k;

mod_info_k.lock_count--;
if (mod_info_k.app_unlocks > 0) {
/* A routine release an application-level reference, and H5I already handled the vlock release */
mod_info_k.app_unlocks--;
} else {
mod_info_k.lock_count--;
}

if (mod_info_k.lock_count < 0)
HGOTO_DONE(FAIL);

/* If ID info was concurrently modified, restart and check again */
} while (!atomic_compare_exchange_strong(&(id_info_ptr->k), &info_k, mod_info_k));

/* Validity check */
if ((size_t) mod_info_k.lock_count > mod_info_k.app_count)
HGOTO_DONE(FAIL);

/* If app count is zero, ID was released and we don't need to worry about the lock count.
* Otherwise, ensure that the ID wasn't used more times than ref count allows */
assert(mod_info_k.app_count == 0 ||( mod_info_k.lock_count <= ((int) mod_info_k.app_count) + mod_info_k.app_unlocks) );
done:
FUNC_LEAVE_NOAPI(ret_value);
} /* H5I_vlock_exit() */
Expand Down
33 changes: 29 additions & 4 deletions src/H5Iint.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2859,6 +2859,7 @@ H5I__mark_node(void *_info, void H5_ATTR_UNUSED *key, void *_udata)

#if H5_HAVE_VIRTUAL_LOCK
mod_info_k.lock_count = info_k.lock_count;
mod_info_k.app_unlocks = info_k.app_unlocks;
#endif /* H5_HAVE_VIRTUAL_LOCK */

/* We don't want multiple threads trying to either realize or dispose of the
Expand Down Expand Up @@ -2909,6 +2910,7 @@ H5I__mark_node(void *_info, void H5_ATTR_UNUSED *key, void *_udata)
assert(info_k.have_global_mutex == mod_info_k.have_global_mutex);
#if H5_HAVE_VIRTUAL_LOCK
assert(info_k.lock_count == mod_info_k.lock_count);
assert(info_k.app_unlocks == mod_info_k.app_unlocks);
#endif /* H5_HAVE_VIRTUAL_LOCK */
#endif /* JRM */

Expand Down Expand Up @@ -3094,6 +3096,7 @@ H5I__mark_node(void *_info, void H5_ATTR_UNUSED *key, void *_udata)

#if H5_HAVE_VIRTUAL_LOCK
mod_info_k.lock_count = 0;
mod_info_k.app_unlocks = 0;
#endif /* H5_HAVE_VIRTUAL_LOCK */
} else {

Expand All @@ -3108,6 +3111,7 @@ H5I__mark_node(void *_info, void H5_ATTR_UNUSED *key, void *_udata)

#if H5_HAVE_VIRTUAL_LOCK
mod_info_k.lock_count = info_k.lock_count;
mod_info_k.app_unlocks = info_k.app_unlocks;
#endif /* H5_HAVE_VIRTUAL_LOCK */
}

Expand Down Expand Up @@ -4067,6 +4071,7 @@ H5I_subst(hid_t id, const void *new_object)
mod_info_k.object = new_object;
#if H5_HAVE_VIRTUAL_LOCK
mod_info_k.lock_count = info_k.lock_count;
mod_info_k.app_unlocks = info_k.app_unlocks;
#endif /* H5_HAVE_VIRTUAL_LOCK */
if ( atomic_compare_exchange_strong(&(id_info_ptr->k), &info_k, mod_info_k) ) {

Expand Down Expand Up @@ -4840,6 +4845,7 @@ H5I__remove_common(H5I_type_info_t *type_info_ptr, hid_t id)

#if H5_HAVE_VIRTUAL_LOCK
mod_info_k.lock_count = 0;
mod_info_k.app_unlocks = 0;
#endif /* H5_HAVE_VIRTUAL_LOCK */
if ( atomic_compare_exchange_strong(&(id_info_ptr->k), &info_k, mod_info_k) ) {

Expand Down Expand Up @@ -5270,7 +5276,15 @@ H5I__dec_ref(hid_t id, void **request, hbool_t app)
mod_info_k.have_global_mutex = FALSE;

#if H5_HAVE_VIRTUAL_LOCK
mod_info_k.lock_count = info_k.lock_count;
if (app && !H5I_is_default_id(id)) {
/* Application-visible reference released, mirror that in the virtual lock */
mod_info_k.app_unlocks = info_k.app_unlocks + 1;
mod_info_k.lock_count = (info_k.lock_count > 0) ? info_k.lock_count - 1 : mod_info_k.lock_count;
} else {
/* Internal reference released, virtual lock unaffected */
mod_info_k.lock_count = info_k.lock_count;
mod_info_k.app_unlocks = info_k.app_unlocks;
}
#endif /* H5_HAVE_VIRTUAL_LOCK */
if ( info_k.count > 1 ) {

Expand Down Expand Up @@ -5303,6 +5317,7 @@ H5I__dec_ref(hid_t id, void **request, hbool_t app)

#if H5_HAVE_VIRTUAL_LOCK
mod_info_k.lock_count = 0;
mod_info_k.app_unlocks = 0;
#endif /* H5_HAVE_VIRTUAL_LOCK */
marked_for_deletion = TRUE;
}
Expand Down Expand Up @@ -5396,7 +5411,8 @@ H5I__dec_ref(hid_t id, void **request, hbool_t app)
mod_info_k.have_global_mutex = ((have_global_mutex) || (! cls_is_mt_safe));

#if H5_HAVE_VIRTUAL_LOCK
mod_info_k.lock_count = info_k.lock_count;
mod_info_k.app_unlocks = 0;
mod_info_k.lock_count = 0;
#endif /* H5_HAVE_VIRTUAL_LOCK */
/* We want to call the free function, and then mark the id for deletion.
* Since we can't roll this action back, we need exclusive access to the
Expand Down Expand Up @@ -5446,6 +5462,7 @@ H5I__dec_ref(hid_t id, void **request, hbool_t app)
assert(info_k.have_global_mutex == mod_info_k.have_global_mutex);
#if H5_HAVE_VIRTUAL_LOCK
assert(info_k.lock_count == mod_info_k.lock_count);
assert(info_k.app_unlocks == mod_info_k.app_unlocks);
#endif /* H5_HAVE_VIRTUAL_LOCK */
#endif /* JRM */

Expand Down Expand Up @@ -5504,6 +5521,7 @@ H5I__dec_ref(hid_t id, void **request, hbool_t app)

#if H5_HAVE_VIRTUAL_LOCK
mod_info_k.lock_count = 0;
mod_info_k.app_unlocks = 0;
#endif /* H5_HAVE_VIRTUAL_LOCK */
marked_for_deletion = TRUE;

Expand Down Expand Up @@ -6423,6 +6441,7 @@ H5I_inc_ref_internal(hid_t id, hbool_t app_ref)

#if H5_HAVE_VIRTUAL_LOCK
mod_info_k.lock_count = info_k.lock_count;
mod_info_k.app_unlocks = info_k.app_unlocks;
#endif /* H5_HAVE_VIRTUAL_LOCK */
if ( app_ref ) {

Expand Down Expand Up @@ -7100,6 +7119,7 @@ H5I__iterate_cb(void *_item, void H5_ATTR_UNUSED *_key, void *_udata)

#if H5_HAVE_VIRTUAL_LOCK
mod_info_k.lock_count = info_k.lock_count;
mod_info_k.app_unlocks = info_k.app_unlocks;
#endif /* H5_HAVE_VIRTUAL_LOCK */
/* We want to ensure that no other thread inside H5I does anything with
* the object while we call the user_func on the objec on the object.
Expand Down Expand Up @@ -7152,6 +7172,7 @@ H5I__iterate_cb(void *_item, void H5_ATTR_UNUSED *_key, void *_udata)
assert(info_k.have_global_mutex == mod_info_k.have_global_mutex);
#if H5_HAVE_VIRTUAL_LOCK
assert(info_k.lock_count == mod_info_k.lock_count);
assert(info_k.app_unlocks == mod_info_k.app_unlocks);
#endif /* H5_HAVE_VIRTUAL_LOCK */
#endif /* JRM */

Expand Down Expand Up @@ -8047,6 +8068,7 @@ H5I__find_id(hid_t id)

#if H5_HAVE_VIRTUAL_LOCK
mod_info_k.lock_count = info_k.lock_count;
mod_info_k.app_unlocks = info_k.app_unlocks;
#endif /* H5_HAVE_VIRTUAL_LOCK */
if ( ! atomic_compare_exchange_strong(&(id_info_ptr->k), &info_k, mod_info_k ) ) {

Expand Down Expand Up @@ -8089,6 +8111,7 @@ H5I__find_id(hid_t id)

#if H5_HAVE_VIRTUAL_LOCK
assert(info_k.lock_count == mod_info_k.lock_count);
assert(info_k.app_unlocks == mod_info_k.app_unlocks);
#endif /* H5_HAVE_VIRTUAL_LOCK */
#endif /* JRM */

Expand Down Expand Up @@ -9411,8 +9434,10 @@ H5I__new_mt_id_info(hid_t id, unsigned count, unsigned app_count, const void * o
new_k.do_not_disturb = FALSE;
new_k.is_future = is_future;
new_k.have_global_mutex = FALSE;


#if H5_HAVE_VIRTUAL_LOCK
new_k.lock_count = 0;
new_k.app_unlocks = 0;
#endif

atomic_fetch_add(&(H5I_mt_g.H5I__new_mt_id_info__num_calls), 1ULL);

Expand Down
1 change: 1 addition & 0 deletions src/H5Ipkg.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1400,6 +1400,7 @@ typedef struct H5I_mt_id_info_kernel_t {

#if H5_HAVE_VIRTUAL_LOCK
int lock_count; /* Virtual lock for this ID */
int app_unlocks; /* Number of times this ID has been unlocked by the application */
#endif /* H5_HAVE_VIRTUAL_LOCK */
} H5I_mt_id_info_kernel_t;

Expand Down
1 change: 1 addition & 0 deletions src/H5VL.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -492,6 +492,7 @@ H5VLunregister_connector(hid_t vol_id)
hid_t native_id = H5I_INVALID_HID;
herr_t ret_value = SUCCEED; /* Return value */
int dec_ref_ret = 0; /* Return value from H5I_dec_(app_)ref */

FUNC_ENTER_API_NO_MUTEX(FAIL, vol_id)
H5TRACE1("e", "i", vol_id);

Expand Down

0 comments on commit ced5c14

Please sign in to comment.