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)
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.
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.
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
Pushed to master, 2.7, 2.6, and 1.10.
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.