Feature #13514: [PATCH] thread_pthread.c (native_sleep): preserve old unblock function

• Status: Open
• Assignee: -
• Target version: -

[normalperson (Eric Wong)]

Do not blindly clobber UBF if one exists, emulating the
behavior of the set_unblock_function and reset_unblock_function
pair.

I think the native_sleep implementation in thread_win32.c,
can use a similar change; but I do not run non-Free software
so I cannot test.

I'm pretty sure this is correct, and will commit in a few days.
On the other hand, I'm not sure if anybody is affected by this.
If it's OK, somebody should also update thread_win32.c since I'm
not comfortable doing so without being able to test.

[ko1 (Koichi Sasada)]

Do you expect such situation?

(1) run ruby code # acquiring GVL
(2) run func on without_gvl() # releasing GVL
(3) run func on with_gvl() # re-acquire GVL
(4) run func on without_gvl() # releasing GVL <- here

I agree. It has a problem. But we should save UBF at with_gvl() function, I assume.
And checking thread.c, it is saved.

What situation do you assume?

[normalperson (Eric Wong)]

[e-mail] wrote:

Issue #13514 has been updated by ko1 (Koichi Sasada).

Do you expect such situation?

Not right now. I am looking at making changes related to
threading, and I noticed this weirdness.

(1) run ruby code # acquiring GVL
(2) run func on without_gvl() # releasing GVL
(3) run func on with_gvl() # re-acquire GVL
(4) run func on without_gvl() # releasing GVL <- here

I agree. It has a problem. But we should save UBF at with_gvl() function, I assume.
And checking thread.c, it is saved.

Yes, I do not believe there is a problem in current code.
For lack of better explanation, it seems wrong to lose
any existing values.

What situation do you assume?

I am looking to replace lock_func in thread_sync.c with
native_sleep or similar. This is to reduce Mutex size and
complexity by using a similar method to what I did in r52332
with ccan/list

("variable.c: additional locking around autoload")

It is compatible with current GVL 1:1 threading,
but I would like to support M:N threading, eventually.

Thanks for looking at this.

[ko1 (Koichi Sasada)]

On 2017/04/27 8:58, Eric Wong wrote:

I am looking to replace lock_func in thread_sync.c with
native_sleep or similar. This is to reduce Mutex size and
complexity by using a similar method to what I did in r52332
with ccan/list

("variable.c: additional locking around autoload")

It is compatible with current GVL 1:1 threading,
but I would like to support M:N threading, eventually.

Sorry I didn't check r52332. Could you explain more about your technique
you want to introduce into sync.c and why native_sleep() is not enough
now? Or please propose with your patch.

I'm afraid that the assumptions for native_sleep() (and other functions)
will be break and can't control.

--
// SASADA Koichi at atdot dot net

[ko1 (Koichi Sasada)]

On 2017/04/27 12:13, SASADA Koichi wrote:

I'm afraid that the assumptions for native_sleep() (and other functions)
will be break and can't control.

I don't mean we should keep current assumption. But we need to update
assumptions synchronously.

--
// SASADA Koichi at atdot dot net

[normalperson (Eric Wong)]

SASADA Koichi [e-mail] wrote:

On 2017/04/27 8:58, Eric Wong wrote:

I am looking to replace lock_func in thread_sync.c with
native_sleep or similar. This is to reduce Mutex size and
complexity by using a similar method to what I did in r52332
with ccan/list

("variable.c: additional locking around autoload")

It is compatible with current GVL 1:1 threading,
but I would like to support M:N threading, eventually.

Sorry I didn't check r52332. Could you explain more about your technique
you want to introduce into sync.c and why native_sleep() is not enough
now? Or please propose with your patch.

This is my work-in-progress patch:

https://80x24.org/spew/[e-mail]/raw

I am still working on fixing the failing test (but I am
distracted by another project).

I'm afraid that the assumptions for native_sleep() (and other functions)
will be break and can't control.

Right, I checked all callers of native_sleep and do not believe
they are affected by preserving unblock function.

[normalperson (Eric Wong)]

Eric Wong [e-mail] wrote:

This is my work-in-progress patch:

https://80x24.org/spew/[e-mail]/raw

I am still working on fixing the failing test (but I am
distracted by another project).

Nevermind, fixed:

https://80x24.org/spew/[e-mail]/raw

(forgot to break out of list_for_each_safe loop in rb_mutex_unlock_th :x)

/me goes back to taking a break from computers...

[ko1 (Koichi Sasada)]

On 2017/04/27 12:57, Eric Wong wrote:

https://80x24.org/spew/[e-mail]/raw

Thank you. I understand the idea. My understanding is "Do not rely on
native cond, but manage sleeping threads by ourselves (manage waiting
queue)". It sound great.

However, I can't understand well about changing native_sleep(). Before
native_sleep(), GVL was acquired and UBF is zero. What kind of sequence
do you think which requires [Misc #13514]?

Thanks,
Koichi

--
// SASADA Koichi at atdot dot net

[normalperson (Eric Wong)]

SASADA Koichi [e-mail] wrote:

On 2017/04/27 12:57, Eric Wong wrote:

https://80x24.org/spew/[e-mail]/raw

Thank you. I understand the idea. My understanding is "Do not rely on
native cond, but manage sleeping threads by ourselves (manage waiting
queue)". It sound great.

Thank you for looking at it. I will open separate ticket once
I am satisfied with it. All internal tests and rubyspec pass,
but I need to review patrol thread logic, more.

However, I can't understand well about changing native_sleep(). Before
native_sleep(), GVL was acquired and UBF is zero. What kind of sequence
do you think which requires [Misc #13514]?

Again, I am really not sure what requires [Misc #13514],
it does not feel correct to lose existing values...

Note how my Mutex size reduction patch at
https://80x24.org/spew/[e-mail]/raw
still uses set_unblock_function and reset_unblock_function in
rb_mutex_lock around native_sleep.

This is what the original code did with lock_func.

Maybe they are not necessary with native_sleep,
since "make exam" passes, but I am also unsure why the old code
in rb_mutex_lock used reset_unblock_function instead of zeroing
UBF like native_sleep...

[mame (Yusuke Endoh)]

normalperson (Eric Wong) wrote:

However, I can't understand well about changing native_sleep(). Before
native_sleep(), GVL was acquired and UBF is zero. What kind of sequence
do you think which requires [Misc #13514]?

Again, I am really not sure what requires [Misc #13514],
it does not feel correct to lose existing values...

Perhaps, I first introduced such a preservation code for rb_mutex_lock at r17435. But sorry, I cannot remeber the reason.

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.

Now, I agree with ko1. Indeed it looks unnecessary. If we remove the code and all tests pass, I vote for removal.

[normalperson (Eric Wong)]

OK, so I think my patch for [Feature #13517] will be fine
and this one can be dropped. I may add a return value for
native_sleep to indicate it is interrupted, though...

[hsbt (Hiroshi SHIBATA)]

• Tracker changed from Misc to Feature