From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=58312 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PPUz1-0002Zc-Is for qemu-devel@nongnu.org; Mon, 06 Dec 2010 01:55:36 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1PPUz0-0005N4-Du for qemu-devel@nongnu.org; Mon, 06 Dec 2010 01:55:35 -0500 Received: from mx1.redhat.com ([209.132.183.28]:63236) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1PPUz0-0005N0-6s for qemu-devel@nongnu.org; Mon, 06 Dec 2010 01:55:34 -0500 Date: Mon, 6 Dec 2010 12:25:06 +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: <20101206065506.GD11457@amit-x200.redhat.com> References: <201012011308.26315.paul@codesourcery.com> <20101202092100.GD3313@amit-x200.redhat.com> <201012021731.36291.paul@codesourcery.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <201012021731.36291.paul@codesourcery.com> List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paul Brook Cc: Gerd Hoffmann , qemu-devel@nongnu.org, Juan Quintela On (Thu) Dec 02 2010 [17:31:36], Paul Brook wrote: > > > > 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 don't buy this argument. The current implementation of qemu_chr_write never > generates transient failures, so they don't need to. And applying this patch won't change the situation. What I proposed in the earlier mail was to buffer only the data that has to be re-submitted in case the caller is capable of stopping further output till the char layer indicates it's free to start again. > Once data has been transmitted, we have three options: > > a) Block until the write completes. This makes the whole patch fairly > pointless as host and guest block boundaries are unlikely to align. This is what currently happens and will remain so for callers of qemu_chr_write() which don't have a .write_unblocked() pointer assigned in the char dev struct. > b) Store the data on the side somewhere. Tell the device all data has been > sent, and arrange for this data to be flushed before accepting any more data. > This is bad because it allows the guest to allocate arbitrarily large[1] > buffers on the host. i.e. a fairly easily exploitable DoS attack. With virtio-serial, this is what's in use. The buffer is limited to the length of the vq (which is a compile-time constant) and there also is the virtio_serial_throttle_port() call that tells the guest to not send any more data to the host till the char layer indicates it's OK to send more data. > c) Return a partial write to the guest. The guest already has to handle > retries due to EAGAIN, and DMA capable devices already have to handle partial > mappings, so this doesn't seem too onerous a requirement. This is not a new > concept, it's the same as the unix write(2)/send(2) functions. This isn't possible with the current vq design. Amit