Project

General

Profile

Bug #14945

[PATCH] thread.c (blocking_region_end): clear ubf before unregister_ubf_list

Added by normalperson (Eric Wong) 19 days ago. Updated 8 days ago.

Status:
Closed
Priority:
Normal
Assignee:
-
Target version:
-
[ruby-core:88141]

Description

thread.c (blocking_region_end): clear ubf before unregister_ubf_list

If we keep ubf set after unregistering, there is a window for
other threads (including timer thread) to put this thread back
on the ubf_list right away. Entering ubf_list unexpectedly
after GVL acquisition may cause spurious wakeup and trigger
unexpected behavior.

Finally, clear ubf before acquiring GVL, to since ubf is useless
during GVL acquisition anyways and we don't want to waste cycles
in other threads calling ubf for useless work.

I found this bug while rewriting GVL to handle timer-thread duty
and eliminate separate timer thread.

Associated revisions

Revision 856bd77a
Added by normal 19 days ago

thread.c (blocking_region_end): clear ubf before unregister_ubf_list

If we keep ubf set after unregistering, there is a window for
other threads (including timer thread) to put this thread back
on the ubf_list right away. Entering ubf_list unexpectedly
after GVL acquisition may cause spurious wakeup and trigger
unexpected behavior.

Finally, clear ubf before acquiring GVL, to since ubf is useless
during GVL acquisition anyways and we don't want to waste cycles
in other threads calling ubf for useless work.

[Bug #14945]

git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/trunk@64083 b2dd03c8-39d4-4d8f-98ff-823fe69b080e

Revision 64083
Added by normalperson (Eric Wong) 19 days ago

thread.c (blocking_region_end): clear ubf before unregister_ubf_list

If we keep ubf set after unregistering, there is a window for
other threads (including timer thread) to put this thread back
on the ubf_list right away. Entering ubf_list unexpectedly
after GVL acquisition may cause spurious wakeup and trigger
unexpected behavior.

Finally, clear ubf before acquiring GVL, to since ubf is useless
during GVL acquisition anyways and we don't want to waste cycles
in other threads calling ubf for useless work.

[Bug #14945]

Revision 86d35a6b
Added by normal 16 days ago

thread_pthread.c (unregister_ubf_list): assert unblock.func is unset

We must not allow reentry into ubf_list_head once we delete
ourselves, otherwise we could hang in there forever.

[Bug #14945]

git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/trunk@64134 b2dd03c8-39d4-4d8f-98ff-823fe69b080e

Revision 64134
Added by normalperson (Eric Wong) 16 days ago

thread_pthread.c (unregister_ubf_list): assert unblock.func is unset

We must not allow reentry into ubf_list_head once we delete
ourselves, otherwise we could hang in there forever.

[Bug #14945]

History

#1 Updated by normalperson (Eric Wong) 19 days ago

  • Status changed from Open to Closed

Applied in changeset trunk|r64083.


thread.c (blocking_region_end): clear ubf before unregister_ubf_list

If we keep ubf set after unregistering, there is a window for
other threads (including timer thread) to put this thread back
on the ubf_list right away. Entering ubf_list unexpectedly
after GVL acquisition may cause spurious wakeup and trigger
unexpected behavior.

Finally, clear ubf before acquiring GVL, to since ubf is useless
during GVL acquisition anyways and we don't want to waste cycles
in other threads calling ubf for useless work.

[Bug #14945]

#2 [ruby-core:88218] Updated by normalperson (Eric Wong) 16 days ago

https://bugs.ruby-lang.org/issues/14945

Actually, r64083 may not be completely safe, either, as
the list_empty checks are dangerous, at least w/o GVL.

However, this order must be enforced:

unblock_function_clear
unregister_ubf_list

Working on fixes...

#3 [ruby-core:88340] Updated by ko1 (Koichi Sasada) 8 days ago

:+1:

One off topic question. list_empty() is thread-safe?

#4 [ruby-core:88341] Updated by ko1 (Koichi Sasada) 8 days ago

ko1 (Koichi Sasada) wrote:

One off topic question. list_empty() is thread-safe?

Sorry it should be safe (I misread as other operation. sorry).

#5 [ruby-core:88351] Updated by normalperson (Eric Wong) 8 days ago

ko1@atdot.net wrote:

ko1 (Koichi Sasada) wrote:

One off topic question. list_empty() is thread-safe?

Sorry it should be safe (I misread as other operation. sorry).

Right, good question, though... for register_ubf_list
and unregister_ubf_list, they are safe and I can commit
below patch to clarify.

However, list_empty in ubf_threads_empty I'm not 100% sure
about...

 diff --git a/thread_pthread.c b/thread_pthread.c
 index 29805ef2df..febec0c1d1 100644
 --- a/thread_pthread.c
 +++ b/thread_pthread.c
 @@ -1125,6 +1125,15 @@ register_ubf_list(rb_thread_t *th)
 {
 struct list_node *node = &th->native_thread_data.ubf_list;

 +    /*
 +     * list_empty check is safe here without ubf_list_lock held
 +     * because th->interrupt_lock is already held by
 +     * rb_threadptr_interrupt_common.
 +     *
 +     *  rb_threadptr_interrupt_common
 +     *    ubf_select (== th->unblock.func)
 +     *      register_ubf_list (this function)
 +     */
 if (list_empty((struct list_head*)node)) {
 rb_native_mutex_lock(&ubf_list_lock);
    list_add(&ubf_list_head, node);
 @@ -1141,6 +1150,11 @@ unregister_ubf_list(rb_thread_t *th)
 /* we can't allow re-entry into ubf_list_head */
 VM_ASSERT(th->unblock.func == 0);

 +    /*
 +     * list_empty check is safe here without ubf_list_lock held
 +     * because we already cleared th->unblock.func while
 +     * th->interrupt_lock was held.
 +     */
 if (!list_empty((struct list_head*)node)) {
 rb_native_mutex_lock(&ubf_list_lock);
    list_del_init(node);

Also available in: Atom PDF