From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id B7F352FA73 for ; Wed, 5 Jun 2019 08:07:50 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id Y5zczzgzwqvl for ; Wed, 5 Jun 2019 08:07:50 -0400 (EDT) Received: from smtp63.i.mail.ru (smtp63.i.mail.ru [217.69.128.43]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 6EFDD2FA0B for ; Wed, 5 Jun 2019 08:07:50 -0400 (EDT) Date: Wed, 5 Jun 2019 10:11:59 +0300 From: Konstantin Osipov Subject: [tarantool-patches] Re: [PATCH 3/5] swim: allow to hang triggers on member updates Message-ID: <20190605071159.GD28736@atlas> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-Help: List-Unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-Subscribe: List-Owner: List-post: List-Archive: To: Vladislav Shpilevoy Cc: tarantool-patches@freelists.org * Vladislav Shpilevoy [19/06/03 14:33]: I like the design: elegant and lean. Some comments below. > This commit allows to hang triggers on member updates. "hang triggers" sounds ominous. I think you simply meant "set triggers". See also patch subject, please change as well. > struct rlist in_dissemination_queue; > + /** > + * Each time a member is updated, or created, or dropped, > + * it is added to an update queue. Members from this queue > + * are dispatched into user defined triggers. > + */ > + struct stailq_entry in_update_queue; I think the name is a bit confusing. After all, the dissemination queue is also an update queue. in_local_queue? in_trigger_queue? in_event_queue? in_pending_member_update_queue? in_member_update? in_member_update_queue? Please also make sure that all API names continue to agree with each other and internal member names after the rename. > + /** > + * Mask of events happened with this member since a > + * previous trigger invocation. Once the events are > + * delivered into a trigger, the mask is nullified and > + * starts collecting new events. > + */ > + enum swim_ev_mask events; Since you use 'events' for an associated mask, perhaps in_event_queue or in_local_event_queue is good enough. > + /** > + * Worker fiber to serve member update triggers. This task > + * is being done in a separate fiber, because user > + * triggers can yield and libev callbacks, processing > + * member updates, are not allowed to yield. > + */ > + struct fiber *worker; I think as long as the worker is only used for triggers, the name should be more specific. when the worker is used for other events or actions, it could be renamed. > @@ -734,7 +801,6 @@ swim_delete_member(struct swim *swim, struct swim_member *member) > mh_int_t rc = mh_swim_table_find(swim->members, key, NULL); > assert(rc != mh_end(swim->members)); > mh_swim_table_del(swim->members, rc, NULL); > - swim_cached_round_msg_invalidate(swim); > rlist_del_entry(member, in_round_queue); Stray change? At least I don't understand it. > + struct swim *s = va_arg(va, struct swim *); > + struct swim_on_member_update_ctx ctx; > + while (! fiber_is_cancelled()) { > + while (stailq_empty(&s->update_queue)) { > + fiber_yield(); > + if (fiber_is_cancelled()) > + goto end; > + } Since you use goto anyway, why a double loop? while (! fiber_is_cancelled()) { if (stailq_empty(&s->update_queue) { fiber_yield(); continue; } ... } > + } > +end: > + swim_do_delete(s); I don't mind swim_do_delete(), but I think the worker should not be destorying the instance. Looks a breach of encapsulation, this assumes the workers knows about all other uses of struct swim. Why not cancel and join the worker from the destructor and then safely do the delete? > + swim->worker = fiber_new("SWIM worker", swim_worker_f); > + if (swim->worker == NULL) { > + free(swim); > + return NULL; > + } > swim->members = mh_swim_table_new(); > if (swim->members == NULL) { > + fiber_cancel(swim->worker); Better to start the worker last, then you will have easy time destroying it on failure. This is a minor nit, ignore if you wish. > free(swim); > diag_set(OutOfMemory, sizeof(*swim->members), > "mh_swim_table_new", "members"); > @@ -1700,7 +1832,10 @@ swim_new(void) > @@ -1951,17 +2088,19 @@ swim_size(const struct swim *swim) > return mh_size(swim->members); > } > > > +void > +swim_delete(struct swim *swim) > +{ > + fiber_wakeup(swim->worker); > + fiber_cancel(swim->worker); > + fiber_sleep(0); This looks like a hack, please cancel and join the worker instead. > @@ -1991,7 +2139,8 @@ swim_quit_step_complete(struct swim_task *task, > (void) rc; > struct swim *swim = swim_by_scheduler(scheduler); > if (rlist_empty(&swim->round_queue)) { > - swim_delete(swim); > + fiber_wakeup(swim->worker); > + fiber_cancel(swim->worker); > return; And now this code implicitly destroys the swim instance, which is hard to follow when reading. -- Konstantin Osipov, Moscow, Russia