https://bugs.ruby-lang.org/https://bugs.ruby-lang.org/favicon.ico?17113305112017-04-26T23:17:51ZRuby Issue Tracking SystemRuby master - Feature #13514: [PATCH] thread_pthread.c (native_sleep): preserve old unblock functionhttps://bugs.ruby-lang.org/issues/13514?journal_id=644982017-04-26T23:17:51Zko1 (Koichi Sasada)
<ul></ul><p>Do you expect such situation?</p>
<p>(1) run ruby code # acquiring GVL<br>
(2) run func on without_gvl() # releasing GVL<br>
(3) run func on with_gvl() # re-acquire GVL<br>
(4) run func on without_gvl() # releasing GVL <- here</p>
<p>I agree. It has a problem. But we should save UBF at <code>with_gvl()</code> function, I assume.<br>
And checking <code>thread.c</code>, it is saved.</p>
<p>What situation do you assume?</p> Ruby master - Feature #13514: [PATCH] thread_pthread.c (native_sleep): preserve old unblock functionhttps://bugs.ruby-lang.org/issues/13514?journal_id=645002017-04-27T00:09:54Znormalperson (Eric Wong)normalperson@yhbt.net
<ul></ul><p><a href="mailto:ko1@atdot.net" class="email">ko1@atdot.net</a> wrote:</p>
<blockquote>
<p>Issue <a class="issue tracker-2 status-1 priority-4 priority-default" title="Feature: [PATCH] thread_pthread.c (native_sleep): preserve old unblock function (Open)" href="https://bugs.ruby-lang.org/issues/13514">#13514</a> has been updated by ko1 (Koichi Sasada).</p>
<p>Do you expect such situation?</p>
</blockquote>
<p>Not right now. I am looking at making changes related to<br>
threading, and I noticed this weirdness.</p>
<blockquote>
<p>(1) run ruby code # acquiring GVL<br>
(2) run func on without_gvl() # releasing GVL<br>
(3) run func on with_gvl() # re-acquire GVL<br>
(4) run func on without_gvl() # releasing GVL <- here</p>
<p>I agree. It has a problem. But we should save UBF at <code>with_gvl()</code> function, I assume.<br>
And checking <code>thread.c</code>, it is saved.</p>
</blockquote>
<p>Yes, I do not believe there is a problem in current code.<br>
For lack of better explanation, it seems wrong to lose<br>
any existing values.</p>
<blockquote>
<p>What situation do you assume?</p>
</blockquote>
<p>I am looking to replace lock_func in thread_sync.c with<br>
native_sleep or similar. This is to reduce Mutex size and<br>
complexity by using a similar method to what I did in r52332<br>
with ccan/list</p>
<p>("variable.c: additional locking around autoload")</p>
<p>It is compatible with current GVL 1:1 threading,<br>
but I would like to support M:N threading, eventually.</p>
<p>Thanks for looking at this.</p> Ruby master - Feature #13514: [PATCH] thread_pthread.c (native_sleep): preserve old unblock functionhttps://bugs.ruby-lang.org/issues/13514?journal_id=645012017-04-27T03:22:15Zko1 (Koichi Sasada)
<ul></ul><p>On 2017/04/27 8:58, Eric Wong wrote:</p>
<blockquote>
<p>I am looking to replace lock_func in thread_sync.c with<br>
native_sleep or similar. This is to reduce Mutex size and<br>
complexity by using a similar method to what I did in r52332<br>
with ccan/list</p>
<p>("variable.c: additional locking around autoload")</p>
<p>It is compatible with current GVL 1:1 threading,<br>
but I would like to support M:N threading, eventually.</p>
</blockquote>
<p>Sorry I didn't check r52332. Could you explain more about your technique<br>
you want to introduce into sync.c and why native_sleep() is not enough<br>
now? Or please propose with your patch.</p>
<p>I'm afraid that the assumptions for native_sleep() (and other functions)<br>
will be break and can't control.</p>
<p>--<br>
// SASADA Koichi at atdot dot net</p> Ruby master - Feature #13514: [PATCH] thread_pthread.c (native_sleep): preserve old unblock functionhttps://bugs.ruby-lang.org/issues/13514?journal_id=645022017-04-27T03:32:28Zko1 (Koichi Sasada)
<ul></ul><p>On 2017/04/27 12:13, SASADA Koichi wrote:</p>
<blockquote>
<p>I'm afraid that the assumptions for native_sleep() (and other functions)<br>
will be break and can't control.</p>
</blockquote>
<p>I don't mean we should keep current assumption. But we need to update<br>
assumptions synchronously.</p>
<p>--<br>
// SASADA Koichi at atdot dot net</p> Ruby master - Feature #13514: [PATCH] thread_pthread.c (native_sleep): preserve old unblock functionhttps://bugs.ruby-lang.org/issues/13514?journal_id=645032017-04-27T03:42:09Znormalperson (Eric Wong)normalperson@yhbt.net
<ul></ul><p>SASADA Koichi <a href="mailto:ko1@atdot.net" class="email">ko1@atdot.net</a> wrote:</p>
<blockquote>
<p>On 2017/04/27 8:58, Eric Wong wrote:</p>
<blockquote>
<p>I am looking to replace lock_func in thread_sync.c with<br>
native_sleep or similar. This is to reduce Mutex size and<br>
complexity by using a similar method to what I did in r52332<br>
with ccan/list</p>
<p>("variable.c: additional locking around autoload")</p>
<p>It is compatible with current GVL 1:1 threading,<br>
but I would like to support M:N threading, eventually.</p>
</blockquote>
<p>Sorry I didn't check r52332. Could you explain more about your technique<br>
you want to introduce into sync.c and why native_sleep() is not enough<br>
now? Or please propose with your patch.</p>
</blockquote>
<p>This is my work-in-progress patch:</p>
<pre><code>https://80x24.org/spew/20170427033423.19856-1-e@80x24.org/raw
</code></pre>
<p>I am still working on fixing the failing test (but I am<br>
distracted by another project).</p>
<blockquote>
<p>I'm afraid that the assumptions for native_sleep() (and other functions)<br>
will be break and can't control.</p>
</blockquote>
<p>Right, I checked all callers of native_sleep and do not believe<br>
they are affected by preserving unblock function.</p> Ruby master - Feature #13514: [PATCH] thread_pthread.c (native_sleep): preserve old unblock functionhttps://bugs.ruby-lang.org/issues/13514?journal_id=645042017-04-27T04:12:08Znormalperson (Eric Wong)normalperson@yhbt.net
<ul></ul><p>Eric Wong <a href="mailto:normalperson@yhbt.net" class="email">normalperson@yhbt.net</a> wrote:</p>
<blockquote>
<p>This is my work-in-progress patch:</p>
<p><a href="https://80x24.org/spew/20170427033423.19856-1-e@80x24.org/raw" class="external">https://80x24.org/spew/20170427033423.19856-1-e@80x24.org/raw</a></p>
<p>I am still working on fixing the failing test (but I am<br>
distracted by another project).</p>
</blockquote>
<p>Nevermind, fixed:</p>
<p><a href="https://80x24.org/spew/20170427034243.22272-1-e@80x24.org/raw" class="external">https://80x24.org/spew/20170427034243.22272-1-e@80x24.org/raw</a></p>
<p>(forgot to break out of list_for_each_safe loop in rb_mutex_unlock_th :x)</p>
<p>/me goes back to taking a break from computers...</p> Ruby master - Feature #13514: [PATCH] thread_pthread.c (native_sleep): preserve old unblock functionhttps://bugs.ruby-lang.org/issues/13514?journal_id=645052017-04-27T05:12:14Zko1 (Koichi Sasada)
<ul></ul><p>On 2017/04/27 12:57, Eric Wong wrote:</p>
<blockquote>
<p><a href="https://80x24.org/spew/20170427034243.22272-1-e@80x24.org/raw" class="external">https://80x24.org/spew/20170427034243.22272-1-e@80x24.org/raw</a></p>
</blockquote>
<p>Thank you. I understand the idea. My understanding is "Do not rely on<br>
native cond, but manage sleeping threads by ourselves (manage waiting<br>
queue)". It sound great.</p>
<p>However, I can't understand well about changing native_sleep(). Before<br>
native_sleep(), GVL was acquired and UBF is zero. What kind of sequence<br>
do you think which requires [Misc <a class="issue tracker-2 status-1 priority-4 priority-default" title="Feature: [PATCH] thread_pthread.c (native_sleep): preserve old unblock function (Open)" href="https://bugs.ruby-lang.org/issues/13514">#13514</a>]?</p>
<p>Thanks,<br>
Koichi</p>
<p>--<br>
// SASADA Koichi at atdot dot net</p> Ruby master - Feature #13514: [PATCH] thread_pthread.c (native_sleep): preserve old unblock functionhttps://bugs.ruby-lang.org/issues/13514?journal_id=645222017-04-27T17:51:56Znormalperson (Eric Wong)normalperson@yhbt.net
<ul></ul><p>SASADA Koichi <a href="mailto:ko1@atdot.net" class="email">ko1@atdot.net</a> wrote:</p>
<blockquote>
<p>On 2017/04/27 12:57, Eric Wong wrote:</p>
<blockquote>
<p><a href="https://80x24.org/spew/20170427034243.22272-1-e@80x24.org/raw" class="external">https://80x24.org/spew/20170427034243.22272-1-e@80x24.org/raw</a></p>
</blockquote>
<p>Thank you. I understand the idea. My understanding is "Do not rely on<br>
native cond, but manage sleeping threads by ourselves (manage waiting<br>
queue)". It sound great.</p>
</blockquote>
<p>Thank you for looking at it. I will open separate ticket once<br>
I am satisfied with it. All internal tests and rubyspec pass,<br>
but I need to review patrol thread logic, more.</p>
<blockquote>
<p>However, I can't understand well about changing native_sleep(). Before<br>
native_sleep(), GVL was acquired and UBF is zero. What kind of sequence<br>
do you think which requires [Misc <a class="issue tracker-2 status-1 priority-4 priority-default" title="Feature: [PATCH] thread_pthread.c (native_sleep): preserve old unblock function (Open)" href="https://bugs.ruby-lang.org/issues/13514">#13514</a>]?</p>
</blockquote>
<p>Again, I am really not sure what requires [Misc <a class="issue tracker-2 status-1 priority-4 priority-default" title="Feature: [PATCH] thread_pthread.c (native_sleep): preserve old unblock function (Open)" href="https://bugs.ruby-lang.org/issues/13514">#13514</a>],<br>
it does not feel correct to lose existing values...</p>
<p>Note how my Mutex size reduction patch at<br>
<a href="https://80x24.org/spew/20170427034243.22272-1-e@80x24.org/raw" class="external">https://80x24.org/spew/20170427034243.22272-1-e@80x24.org/raw</a><br>
still uses set_unblock_function and reset_unblock_function in<br>
rb_mutex_lock around native_sleep.</p>
<p>This is what the original code did with lock_func.</p>
<p>Maybe they are not necessary with native_sleep,<br>
since "make exam" passes, but I am also unsure why the old code<br>
in rb_mutex_lock used reset_unblock_function instead of zeroing<br>
UBF like native_sleep...</p> Ruby master - Feature #13514: [PATCH] thread_pthread.c (native_sleep): preserve old unblock functionhttps://bugs.ruby-lang.org/issues/13514?journal_id=646212017-05-01T08:41:21Zmame (Yusuke Endoh)mame@ruby-lang.org
<ul></ul><p>normalperson (Eric Wong) wrote:</p>
<blockquote>
<blockquote>
<p>However, I can't understand well about changing native_sleep(). Before<br>
native_sleep(), GVL was acquired and UBF is zero. What kind of sequence<br>
do you think which requires [Misc <a class="issue tracker-2 status-1 priority-4 priority-default" title="Feature: [PATCH] thread_pthread.c (native_sleep): preserve old unblock function (Open)" href="https://bugs.ruby-lang.org/issues/13514">#13514</a>]?</p>
</blockquote>
<p>Again, I am really not sure what requires [Misc <a class="issue tracker-2 status-1 priority-4 priority-default" title="Feature: [PATCH] thread_pthread.c (native_sleep): preserve old unblock function (Open)" href="https://bugs.ruby-lang.org/issues/13514">#13514</a>],<br>
it does not feel correct to lose existing values...</p>
</blockquote>
<p>Perhaps, I first introduced such a preservation code for rb_mutex_lock at r17435. But sorry, I cannot remeber the reason.</p>
<p>I remember that, at that time, there was a bug in deadlock detection that produces false positive. I think I introduced a code to preserve UBF as a symptomatic treatment.</p>
<p>Now, I agree with ko1. Indeed it looks unnecessary. If we remove the code and all tests pass, I vote for removal.</p> Ruby master - Feature #13514: [PATCH] thread_pthread.c (native_sleep): preserve old unblock functionhttps://bugs.ruby-lang.org/issues/13514?journal_id=646222017-05-01T09:32:53Znormalperson (Eric Wong)normalperson@yhbt.net
<ul></ul><p>OK, so I think my patch for [Feature <a class="issue tracker-2 status-5 priority-4 priority-default closed" title="Feature: [PATCH] reduce rb_mutex_t size from 160 to 80 bytes on 64-bit (Closed)" href="https://bugs.ruby-lang.org/issues/13517">#13517</a>] will be fine<br>
and this one can be dropped. I may add a return value for<br>
native_sleep to indicate it is interrupted, though...</p> Ruby master - Feature #13514: [PATCH] thread_pthread.c (native_sleep): preserve old unblock functionhttps://bugs.ruby-lang.org/issues/13514?journal_id=930262021-07-27T11:41:23Zhsbt (Hiroshi SHIBATA)hsbt@ruby-lang.org
<ul><li><strong>Tracker</strong> changed from <i>Misc</i> to <i>Feature</i></li></ul>