Tarantool development patches archive
 help / color / mirror / Atom feed
* [Tarantool-patches] [PATCH 1/1] fiber_cond: remove rlist_shift usages
@ 2021-03-01 22:23 Vladislav Shpilevoy via Tarantool-patches
  2021-03-02  7:37 ` Cyrill Gorcunov via Tarantool-patches
  2021-03-03 21:37 ` Vladislav Shpilevoy via Tarantool-patches
  0 siblings, 2 replies; 6+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-03-01 22:23 UTC (permalink / raw)
  To: tarantool-patches, sergepetrenko, gorcunov

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)


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Tarantool-patches] [PATCH 1/1] fiber_cond: remove rlist_shift usages
  2021-03-01 22:23 [Tarantool-patches] [PATCH 1/1] fiber_cond: remove rlist_shift usages Vladislav Shpilevoy via Tarantool-patches
@ 2021-03-02  7:37 ` Cyrill Gorcunov via Tarantool-patches
  2021-03-02  7:54   ` Cyrill Gorcunov via Tarantool-patches
  2021-03-02  9:04   ` Serge Petrenko via Tarantool-patches
  2021-03-03 21:37 ` Vladislav Shpilevoy via Tarantool-patches
  1 sibling, 2 replies; 6+ messages in thread
From: Cyrill Gorcunov via Tarantool-patches @ 2021-03-02  7:37 UTC (permalink / raw)
  To: Vladislav Shpilevoy; +Cc: 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.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Tarantool-patches] [PATCH 1/1] fiber_cond: remove rlist_shift usages
  2021-03-02  7:37 ` Cyrill Gorcunov via Tarantool-patches
@ 2021-03-02  7:54   ` Cyrill Gorcunov via Tarantool-patches
  2021-03-02  9:04   ` Serge Petrenko via Tarantool-patches
  1 sibling, 0 replies; 6+ messages in thread
From: Cyrill Gorcunov via Tarantool-patches @ 2021-03-02  7:54 UTC (permalink / raw)
  To: Vladislav Shpilevoy; +Cc: 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.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Tarantool-patches] [PATCH 1/1] fiber_cond: remove rlist_shift usages
  2021-03-02  7:37 ` Cyrill Gorcunov via Tarantool-patches
  2021-03-02  7:54   ` Cyrill Gorcunov via Tarantool-patches
@ 2021-03-02  9:04   ` Serge Petrenko via Tarantool-patches
  2021-03-03 21:43     ` Vladislav Shpilevoy via Tarantool-patches
  1 sibling, 1 reply; 6+ messages in thread
From: Serge Petrenko via Tarantool-patches @ 2021-03-02  9:04 UTC (permalink / raw)
  To: Cyrill Gorcunov, Vladislav Shpilevoy; +Cc: 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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Tarantool-patches] [PATCH 1/1] fiber_cond: remove rlist_shift usages
  2021-03-01 22:23 [Tarantool-patches] [PATCH 1/1] fiber_cond: remove rlist_shift usages Vladislav Shpilevoy via Tarantool-patches
  2021-03-02  7:37 ` Cyrill Gorcunov via Tarantool-patches
@ 2021-03-03 21:37 ` Vladislav Shpilevoy via Tarantool-patches
  1 sibling, 0 replies; 6+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-03-03 21:37 UTC (permalink / raw)
  To: tarantool-patches, sergepetrenko, gorcunov

Pushed to master, 2.7, 2.6, and 1.10.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Tarantool-patches] [PATCH 1/1] fiber_cond: remove rlist_shift usages
  2021-03-02  9:04   ` Serge Petrenko via Tarantool-patches
@ 2021-03-03 21:43     ` Vladislav Shpilevoy via Tarantool-patches
  0 siblings, 0 replies; 6+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-03-03 21:43 UTC (permalink / raw)
  To: Serge Petrenko, Cyrill Gorcunov; +Cc: 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.

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2021-03-03 21:43 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-01 22:23 [Tarantool-patches] [PATCH 1/1] fiber_cond: remove rlist_shift usages Vladislav Shpilevoy via Tarantool-patches
2021-03-02  7:37 ` Cyrill Gorcunov via Tarantool-patches
2021-03-02  7:54   ` Cyrill Gorcunov via Tarantool-patches
2021-03-02  9:04   ` Serge Petrenko via Tarantool-patches
2021-03-03 21:43     ` Vladislav Shpilevoy via Tarantool-patches
2021-03-03 21:37 ` Vladislav Shpilevoy via Tarantool-patches

Tarantool development patches archive

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://lists.tarantool.org/tarantool-patches/0 tarantool-patches/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 tarantool-patches tarantool-patches/ https://lists.tarantool.org/tarantool-patches \
		tarantool-patches@dev.tarantool.org.
	public-inbox-index tarantool-patches

Example config snippet for mirrors.


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git