From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from [140.186.70.92] (port=57955 helo=eggs.gnu.org)
	by lists.gnu.org with esmtp (Exim 4.43) id 1PNlLZ-00078d-KH
	for qemu-devel@nongnu.org; Wed, 01 Dec 2010 06:59:42 -0500
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <paul@codesourcery.com>) id 1PNlLX-0005MW-WD
	for qemu-devel@nongnu.org; Wed, 01 Dec 2010 06:59:41 -0500
Received: from mail.codesourcery.com ([38.113.113.100]:34971)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <paul@codesourcery.com>) id 1PNlLX-0005M2-MD
	for qemu-devel@nongnu.org; Wed, 01 Dec 2010 06:59:39 -0500
From: Paul Brook <paul@codesourcery.com>
Subject: Re: [Qemu-devel] [PATCH v8 7/7] virtio-console: Enable port
	throttling when chardev is slow to consume data
Date: Wed, 1 Dec 2010 11:59:35 +0000
References: <cover.1291196789.git.amit.shah@redhat.com>
	<201012011130.58630.paul@codesourcery.com>
	<20101201114837.GB2962@amit-x200.redhat.com>
In-Reply-To: <20101201114837.GB2962@amit-x200.redhat.com>
MIME-Version: 1.0
Content-Type: Text/Plain;
  charset="iso-8859-1"
Content-Transfer-Encoding: 7bit
Message-Id: <201012011159.35947.paul@codesourcery.com>
List-Id: qemu-devel.nongnu.org
List-Unsubscribe: <http://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <http://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Amit Shah <amit.shah@redhat.com>
Cc: Juan Quintela <quintela@redhat.com>, qemu-devel@nongnu.org, Gerd Hoffmann <kraxel@redhat.com>

> > > -    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?

Allowing EAGAIN without allowing short writes means that the writes will still 
block for arbitrarily long periods of time.  You're relying on the kernel 
blocking at the same point the guest happens to split a DMA block. If you're 
transferring any significant quantities of data the chances of that happening 
seem pretty slim.

Paul