From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=58414 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1POAa3-0003sD-Pz for qemu-devel@nongnu.org; Thu, 02 Dec 2010 09:56:39 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1PO5Lm-0007eU-PT for qemu-devel@nongnu.org; Thu, 02 Dec 2010 04:21:16 -0500 Received: from mx1.redhat.com ([209.132.183.28]:9594) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1PO5Lm-0007eG-Ia for qemu-devel@nongnu.org; Thu, 02 Dec 2010 04:21:14 -0500 Date: Thu, 2 Dec 2010 14:51:00 +0530 From: Amit Shah Subject: Re: [Qemu-devel] [PATCH v8 7/7] virtio-console: Enable port throttling when chardev is slow to consume data Message-ID: <20101202092100.GD3313@amit-x200.redhat.com> References: <201012011159.35947.paul@codesourcery.com> <20101201121256.GD2962@amit-x200.redhat.com> <201012011308.26315.paul@codesourcery.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <201012011308.26315.paul@codesourcery.com> List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paul Brook Cc: Juan Quintela , qemu-devel@nongnu.org, Gerd Hoffmann On (Wed) Dec 01 2010 [13:08:26], Paul Brook wrote: > > On (Wed) Dec 01 2010 [11:59:35], Paul Brook wrote: > > > > > > - qemu_chr_write(vcon->chr, buf, len); > > > > > > + ret = qemu_chr_write(vcon->chr, buf, len); > > > > > > + if (ret == -EAGAIN) { > > > > > > + virtio_serial_throttle_port(port, true); > > > > > > + } > > > > > > > > > > > > } > > > > > > > > > > This looks wrong. It will loose data in the case of a partial write > > > > > (i.e. ret < len) > > > > > > > > That doesn't happen currently (qemu_chr_write doesn't return a value > > > > > 0 but < len). > > > > > > > > I had code in there to handle it, but that would change behaviour for > > > > current users of qemu_chr_write(), which is a risk. > > > > > > Doesn't that make the code almost completely pointless? > > > > Not really -- I did have code for partial writes, but removed it before > > this submission (had it in previous versions). > > > > The (new) do_send loop: > > > > len = len1; > > while (len > 0) { > > ret = write(fd, buf, len); > > if (ret < 0) { > > if (errno == EAGAIN && nonblock) { > > return -EAGAIN; > > } > > if (errno != EINTR && errno != EAGAIN) { > > return -1; > > } > > } else if (ret == 0) { > > break; > > } else { > > buf += ret; > > len -= ret; > > } > > } > > > > when there's a partial write, it tries to do a write again, which will > > fail with -EAGAIN. > > Doesn't that cause the first partial chunk to be incorrectly transmitted > twice? You may only return EAGAIN if no data was transmitted. Except for the fact that no caller of qemu_chr_write() resubmits (or even checks) partial writes. I think the only way to solve this in a caller-neutral way while keeping the current semantics is to buffer all the data that comes in for qemu_chr_write() and re-submit partial writes in the unblock routine (ie, what I did in virtio-console.c in v7 is to be done in qemu-char.c now). That OK? Amit