[Tarantool-patches] [PATCH 1/1] fiber_cond: remove rlist_shift usages

[Tarantool-patches] [PATCH 1/1] fiber_cond: remove rlist_shift usages

[Vladislav Shpilevoy via Tarantool-patches]

Fiber_cond to pop fibers from the queue uses rlist_shift() +
fiber_wakeup() calls. The shift wasn't needed, because fibers are
linked with 'state' member, which is moved by fiber_wakeup()
anyway.

Closes #5855
---
Branch: http://github.com/tarantool/tarantool/tree/gerold103/gh-5855-optimize-fiber-cond
Issue: https://github.com/tarantool/tarantool/issues/5855

 src/lib/core/fiber_cond.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/lib/core/fiber_cond.c b/src/lib/core/fiber_cond.c
index 71bb2d04d..3624dffd4 100644
--- a/src/lib/core/fiber_cond.c
+++ b/src/lib/core/fiber_cond.c
@@ -84,7 +84,7 @@ fiber_cond_signal(struct fiber_cond *e)
 {
 	if (! rlist_empty(&e->waiters)) {
 		struct fiber *f;
-		f = rlist_shift_entry(&e->waiters, struct fiber, state);
+		f = rlist_first_entry(&e->waiters, struct fiber, state);
 		fiber_wakeup(f);
 	}
 }
@@ -94,7 +94,7 @@ fiber_cond_broadcast(struct fiber_cond *e)
 {
 	while (! rlist_empty(&e->waiters)) {
 		struct fiber *f;
-		f = rlist_shift_entry(&e->waiters, struct fiber, state);
+		f = rlist_first_entry(&e->waiters, struct fiber, state);
 		fiber_wakeup(f);
 	}
 }
-- 
2.24.3 (Apple Git-128)

Re: [Tarantool-patches] [PATCH 1/1] fiber_cond: remove rlist_shift usages

[Cyrill Gorcunov via Tarantool-patches]

On Mon, Mar 01, 2021 at 11:23:13PM +0100, Vladislav Shpilevoy wrote:
> @@ -94,7 +94,7 @@ fiber_cond_broadcast(struct fiber_cond *e)
>  {
>  	while (! rlist_empty(&e->waiters)) {
>  		struct fiber *f;
> -		f = rlist_shift_entry(&e->waiters, struct fiber, state);
> +		f = rlist_first_entry(&e->waiters, struct fiber, state);
>  		fiber_wakeup(f);
>  	}
>  }

The fiber_wakeup ignores

	if (f->flags & (FIBER_IS_READY | FIBER_IS_DEAD))
		return;

can't we hit the situation where fiber_cond_broadcast called with
dead fiber so that it won't be deleted from the list with new code?

Actually looking into fiber_loop code I see

static void
fiber_loop(MAYBE_UNUSED void *data)
{
	...
	fiber->flags |= FIBER_IS_DEAD;
	while (! rlist_empty(&fiber->wake)) {
		       struct fiber *f;
		       f = rlist_shift_entry(&fiber->wake, struct fiber,
					     state);
		       assert(f != fiber);
		       fiber_wakeup(f);
	}

so it should be safe with your patch, but just to make sure I didn't
miss something obvious.

Re: [Tarantool-patches] [PATCH 1/1] fiber_cond: remove rlist_shift usages

[Cyrill Gorcunov via Tarantool-patches]

On Tue, Mar 02, 2021 at 10:37:36AM +0300, Cyrill Gorcunov wrote:
> On Mon, Mar 01, 2021 at 11:23:13PM +0100, Vladislav Shpilevoy wrote:
> > @@ -94,7 +94,7 @@ fiber_cond_broadcast(struct fiber_cond *e)
> >  {
> >  	while (! rlist_empty(&e->waiters)) {
> >  		struct fiber *f;
> > -		f = rlist_shift_entry(&e->waiters, struct fiber, state);
> > +		f = rlist_first_entry(&e->waiters, struct fiber, state);
> >  		fiber_wakeup(f);
> >  	}
> >  }
> 
> The fiber_wakeup ignores
> 
> 	if (f->flags & (FIBER_IS_READY | FIBER_IS_DEAD))
> 		return;
> 
> can't we hit the situation where fiber_cond_broadcast called with
> dead fiber so that it won't be deleted from the list with new code?
> 
> Actually looking into fiber_loop code I see
> 
> static void
> fiber_loop(MAYBE_UNUSED void *data)
> {
> 	...
> 	fiber->flags |= FIBER_IS_DEAD;
> 	while (! rlist_empty(&fiber->wake)) {
> 		       struct fiber *f;
> 		       f = rlist_shift_entry(&fiber->wake, struct fiber,
> 					     state);
> 		       assert(f != fiber);
> 		       fiber_wakeup(f);
> 	}
> 
> so it should be safe with your patch, but just to make sure I didn't
> miss something obvious.

Still, Ack. The prev mail was just to share some thoughts.

Re: [Tarantool-patches] [PATCH 1/1] fiber_cond: remove rlist_shift usages

[Serge Petrenko via Tarantool-patches]

02.03.2021 10:37, Cyrill Gorcunov пишет:
> On Mon, Mar 01, 2021 at 11:23:13PM +0100, Vladislav Shpilevoy wrote:
>> @@ -94,7 +94,7 @@ fiber_cond_broadcast(struct fiber_cond *e)
>>   {
>>   	while (! rlist_empty(&e->waiters)) {
>>   		struct fiber *f;
>> -		f = rlist_shift_entry(&e->waiters, struct fiber, state);
>> +		f = rlist_first_entry(&e->waiters, struct fiber, state);
>>   		fiber_wakeup(f);
>>   	}
>>   }
> The fiber_wakeup ignores
>
> 	if (f->flags & (FIBER_IS_READY | FIBER_IS_DEAD))
> 		return;
>
> can't we hit the situation where fiber_cond_broadcast called with
> dead fiber so that it won't be deleted from the list with new code?

Good point, I never thought of this.

Once a fiber dies, it's removed from any list that could wake it up:

         /* reset pending wakeups */
         rlist_del(&fiber->state);

And FIBER_IS_READY is set only from fiber_wakeup() to guard from
concurrent wake-ups. So the patch must be fine indeed.

Vlad, LGTM.

>
> Actually looking into fiber_loop code I see
>
> static void
> fiber_loop(MAYBE_UNUSED void *data)
> {
> 	...
> 	fiber->flags |= FIBER_IS_DEAD;
> 	while (! rlist_empty(&fiber->wake)) {
> 		       struct fiber *f;
> 		       f = rlist_shift_entry(&fiber->wake, struct fiber,
> 					     state);
> 		       assert(f != fiber);
> 		       fiber_wakeup(f);
> 	}
>
> so it should be safe with your patch, but just to make sure I didn't
> miss something obvious.

-- 
Serge Petrenko

Re: [Tarantool-patches] [PATCH 1/1] fiber_cond: remove rlist_shift usages

[Vladislav Shpilevoy via Tarantool-patches]

Pushed to master, 2.7, 2.6, and 1.10.

Re: [Tarantool-patches] [PATCH 1/1] fiber_cond: remove rlist_shift usages

[Vladislav Shpilevoy via Tarantool-patches]

On 02.03.2021 10:04, Serge Petrenko wrote:
> 
> 
> 02.03.2021 10:37, Cyrill Gorcunov пишет:
>> On Mon, Mar 01, 2021 at 11:23:13PM +0100, Vladislav Shpilevoy wrote:
>>> @@ -94,7 +94,7 @@ fiber_cond_broadcast(struct fiber_cond *e)
>>>   {
>>>       while (! rlist_empty(&e->waiters)) {
>>>           struct fiber *f;
>>> -        f = rlist_shift_entry(&e->waiters, struct fiber, state);
>>> +        f = rlist_first_entry(&e->waiters, struct fiber, state);
>>>           fiber_wakeup(f);
>>>       }
>>>   }
>> The fiber_wakeup ignores
>>
>>     if (f->flags & (FIBER_IS_READY | FIBER_IS_DEAD))
>>         return;
>>
>> can't we hit the situation where fiber_cond_broadcast called with
>> dead fiber so that it won't be deleted from the list with new code?
> 
> Good point, I never thought of this.
> 
> Once a fiber dies, it's removed from any list that could wake it up:
> 
>         /* reset pending wakeups */
>         rlist_del(&fiber->state);
> 
> And FIBER_IS_READY is set only from fiber_wakeup() to guard from
> concurrent wake-ups. So the patch must be fine indeed.

Moreover, a fiber can't be dead and be in fiber_cond. Fiber dies only
after it returns from its main function. Until then it can be at most
CANCELLED, which does not affect the yields. You still can sleep,
wait, yield, be woken up, and so on.