From patchwork Fri Jul 11 09:41:08 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Kirill Batuzov X-Patchwork-Id: 369095 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id A1EE81400DD for ; Fri, 11 Jul 2014 19:42:00 +1000 (EST) Received: from localhost ([::1]:43525 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1X5XL3-0003MU-P2 for incoming@patchwork.ozlabs.org; Fri, 11 Jul 2014 05:41:57 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42878) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1X5XKg-00035F-6x for qemu-devel@nongnu.org; Fri, 11 Jul 2014 05:41:40 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1X5XKa-0003NC-43 for qemu-devel@nongnu.org; Fri, 11 Jul 2014 05:41:34 -0400 Received: from smtp.ispras.ru ([83.149.199.79]:55530) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1X5XKZ-0003L5-ND for qemu-devel@nongnu.org; Fri, 11 Jul 2014 05:41:28 -0400 Received: from bulbul.intra.ispras.ru (unknown [83.149.199.91]) by smtp.ispras.ru (Postfix) with ESMTP id 6133E224C2; Fri, 11 Jul 2014 13:41:24 +0400 (MSK) From: Kirill Batuzov To: qemu-devel@nongnu.org Date: Fri, 11 Jul 2014 13:41:08 +0400 Message-Id: <1405071668-30158-1-git-send-email-batuzovk@ispras.ru> X-Mailer: git-send-email 1.7.10.4 X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x X-Received-From: 83.149.199.79 Cc: Peter Crosthwaite , "Michael S . Tsirkin" , Kirill Batuzov Subject: [Qemu-devel] [PATCH for-2.1] serial: change retry logic to avoid concurrency X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Whenever serial_xmit fails to transmit a byte it adds a watch that would call it again when the "line" becomes ready. This results in a retry chain: serial_xmit -> add_watch -> serial_xmit Each chain is able to transmit one character, and for every character passed to serial by the guest driver a new chain is spawned. The problem lays with the fact that a new chain is spawned even when there is one already waiting on the watch. So there can be several retry chains waiting concurrently on one "line". Every chain tries to transmit current character, so character order is not messed up. But also every chain increases retry counter (tsr_retry). If there are enough concurrent chains this counter will hit MAX_XMIT_RETRY value and the character will be dropped. To reproduce this bug you need to feed serial output to some program consuming it slowly enough. A python script from bug #1335444 description is an example of such program. This commit changes retry logic in the following way to avoid concurrency: instead of spawning a new chain for each character being transmitted spawn only one and make it transmit characters until FIFO is empty. The change consists of two parts: - add a do {} while () loop in serial_xmit (diff is a bit erratic for this part, diff -w will show actual change), - do not call serial_xmit from serial_ioport_write if there is one waiting on the watch already. This should fix another issue causing bug #1335444. Signed-off-by: Kirill Batuzov --- hw/char/serial.c | 59 ++++++++++++++++++++++++++++++------------------------ 1 file changed, 33 insertions(+), 26 deletions(-) diff --git a/hw/char/serial.c b/hw/char/serial.c index 54180a9..764e184 100644 --- a/hw/char/serial.c +++ b/hw/char/serial.c @@ -223,37 +223,42 @@ static gboolean serial_xmit(GIOChannel *chan, GIOCondition cond, void *opaque) { SerialState *s = opaque; - if (s->tsr_retry <= 0) { - if (s->fcr & UART_FCR_FE) { - if (fifo8_is_empty(&s->xmit_fifo)) { + do { + if (s->tsr_retry <= 0) { + if (s->fcr & UART_FCR_FE) { + if (fifo8_is_empty(&s->xmit_fifo)) { + return FALSE; + } + s->tsr = fifo8_pop(&s->xmit_fifo); + if (!s->xmit_fifo.num) { + s->lsr |= UART_LSR_THRE; + } + } else if ((s->lsr & UART_LSR_THRE)) { return FALSE; - } - s->tsr = fifo8_pop(&s->xmit_fifo); - if (!s->xmit_fifo.num) { + } else { + s->tsr = s->thr; s->lsr |= UART_LSR_THRE; + s->lsr &= ~UART_LSR_TEMT; } - } else if ((s->lsr & UART_LSR_THRE)) { - return FALSE; - } else { - s->tsr = s->thr; - s->lsr |= UART_LSR_THRE; - s->lsr &= ~UART_LSR_TEMT; } - } - if (s->mcr & UART_MCR_LOOP) { - /* in loopback mode, say that we just received a char */ - serial_receive1(s, &s->tsr, 1); - } else if (qemu_chr_fe_write(s->chr, &s->tsr, 1) != 1) { - if (s->tsr_retry >= 0 && s->tsr_retry < MAX_XMIT_RETRY && - qemu_chr_fe_add_watch(s->chr, G_IO_OUT|G_IO_HUP, serial_xmit, s) > 0) { - s->tsr_retry++; - return FALSE; + if (s->mcr & UART_MCR_LOOP) { + /* in loopback mode, say that we just received a char */ + serial_receive1(s, &s->tsr, 1); + } else if (qemu_chr_fe_write(s->chr, &s->tsr, 1) != 1) { + if (s->tsr_retry >= 0 && s->tsr_retry < MAX_XMIT_RETRY && + qemu_chr_fe_add_watch(s->chr, G_IO_OUT|G_IO_HUP, + serial_xmit, s) > 0) { + s->tsr_retry++; + return FALSE; + } + s->tsr_retry = 0; + } else { + s->tsr_retry = 0; } - s->tsr_retry = 0; - } else { - s->tsr_retry = 0; - } + /* Transmit another byte if it is already available. It is only + possible when FIFO is enabled and not empty. */ + } while ((s->fcr & UART_FCR_FE) && !fifo8_is_empty(&s->xmit_fifo)); s->last_xmit_ts = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); @@ -293,7 +298,9 @@ static void serial_ioport_write(void *opaque, hwaddr addr, uint64_t val, s->thr_ipending = 0; s->lsr &= ~UART_LSR_THRE; serial_update_irq(s); - serial_xmit(NULL, G_IO_OUT, s); + if (s->tsr_retry <= 0) { + serial_xmit(NULL, G_IO_OUT, s); + } } break; case 1: