Project

General

Profile

Misc #13514

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

Added by normalperson (Eric Wong) over 2 years ago. Updated over 2 years ago.

Status:
Open
Priority:
Normal
Assignee:
-
[ruby-core:80889]

Description

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.


Files

History

Updated by ko1 (Koichi Sasada) over 2 years ago

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?

Updated by normalperson (Eric Wong) over 2 years ago

ko1@atdot.net 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.

Updated by ko1 (Koichi Sasada) over 2 years ago

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

Updated by ko1 (Koichi Sasada) over 2 years ago

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

Updated by normalperson (Eric Wong) over 2 years ago

SASADA Koichi ko1@atdot.net 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/20170427033423.19856-1-e@80x24.org/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.

Updated by normalperson (Eric Wong) over 2 years ago

Eric Wong normalperson@yhbt.net wrote:

This is my work-in-progress patch:

https://80x24.org/spew/20170427033423.19856-1-e@80x24.org/raw

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

Nevermind, fixed:

https://80x24.org/spew/20170427034243.22272-1-e@80x24.org/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...

Updated by ko1 (Koichi Sasada) over 2 years ago

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

https://80x24.org/spew/20170427034243.22272-1-e@80x24.org/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

Updated by normalperson (Eric Wong) over 2 years ago

SASADA Koichi ko1@atdot.net wrote:

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

https://80x24.org/spew/20170427034243.22272-1-e@80x24.org/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/20170427034243.22272-1-e@80x24.org/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...

Updated by mame (Yusuke Endoh) over 2 years ago

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.

Updated by normalperson (Eric Wong) over 2 years ago

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...

Also available in: Atom PDF