Re: [Qemu-devel] [PATCH v8 7/7] virtio-console: Enable port throttling when chardev is slow to consume data

[Amit Shah <amit.shah@redhat.com>]

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