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 D02F730B91 for ; Thu, 20 Jun 2019 03:53:26 -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 J5X3FTt7NsX2 for ; Thu, 20 Jun 2019 03:53:26 -0400 (EDT) Received: from smtp18.mail.ru (smtp18.mail.ru [94.100.176.155]) (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 85DDC30893 for ; Thu, 20 Jun 2019 03:53:26 -0400 (EDT) Date: Thu, 20 Jun 2019 10:53:22 +0300 From: Konstantin Osipov Subject: [tarantool-patches] Re: [PATCH v4 5/9] wal: a dedicated wal scheduling fiber Message-ID: <20190620075322.GE15155@atlas> References: <635cf91a85c898d73b176a7c264ca4f63d1b40ff.1560978655.git.georgy@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <635cf91a85c898d73b176a7c264ca4f63d1b40ff.1560978655.git.georgy@tarantool.org> 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: tarantool-patches@freelists.org Cc: Georgy Kirichenko * Georgy Kirichenko [19/06/20 09:54]: First of all I think this patch is small enough to be collapsed with the subsequent patch which uses the machinery. Vova will remove yield from vinyl pretty soon, so we might not need this patch after all. > + /** A queue to schedule journal entry completions. */ > + struct stailq schedule_queue; > + /** True if writer is in rollback state. */ > + bool is_in_rollback; Why do you need a separate boolean variable? There is plenty of state which could be used to check that we're in rollback already. > + /** A condition to signal about new schedule queue entries. */ > + struct fiber_cond schedule_cond; > /* ----------------- wal ------------------- */ > /** A setting from instance configuration - rows_per_wal */ > int64_t wal_max_rows; > @@ -245,23 +251,35 @@ xlog_write_entry(struct xlog *l, struct journal_entry *entry) > return xlog_tx_commit(l); > } > > +/* > + * Tx schedule fiber function. > + */ > +static int > +tx_schedule_f(va_list ap) > +{ > + struct wal_writer *writer = va_arg(ap, struct wal_writer *); > + while (!fiber_is_cancelled()) { > + while (!stailq_empty(&writer->schedule_queue)) { > + struct journal_entry *req = > + stailq_shift_entry(&writer->schedule_queue, > + struct journal_entry, fifo); > + fiber_wakeup(req->fiber); > + } > + writer->is_in_rollback = false; > + fiber_cond_wait(&writer->schedule_cond); > + } > + return 0; > +} > + > /** > - * Invoke fibers waiting for their journal_entry's to be > - * completed. The fibers are invoked in strict fifo order: > - * this ensures that, in case of rollback, requests are > - * rolled back in strict reverse order, producing > - * a consistent database state. > + * Attach requests to a scheduling queue. > */ > static void > tx_schedule_queue(struct stailq *queue) > { > - /* > - * fiber_wakeup() is faster than fiber_call() when there > - * are many ready fibers. > - */ > - struct journal_entry *req; > - stailq_foreach_entry(req, queue, fifo) > - fiber_wakeup(req->fiber); > + struct wal_writer *writer = &wal_writer_singleton; > + stailq_concat(&writer->schedule_queue, queue); > + fiber_cond_signal(&writer->schedule_cond); > } Sorry for a nit, but you use fiber_wakeup() for tx fibers but cond_signal, which incurs an extra stailq manipulation, with this fiber. Why do you need another queue? Shouldn't you replace existing queues with a single one instead? Having commit, rollback and schedule queues is confusing. I like it that you effectively finalize all tx in one roll, and then yield to the user fibers. Effectively this is a double sweep over the list of tx, but it is less context switches overall, since we don't switch in and out of the scheduler fiber in this case, but switch from one ready fiber to another. Could you please confirm? -- Konstantin Osipov, Moscow, Russia