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 (Mon) Dec 06 2010 [13:23:50], Paul Brook wrote:
> > Sure.  My proposal is for qemu_chr_write() to succeed all the time.  If
> > the backend can block, and the caller can handle it, it can get a
> > -EAGAIN (or WSAEWOULDBLOCK) return.  When the backend becomes writable,
> > the chardev can call the ->writes_unblocked() callback for that caller.
> > Individual callers need not bother about re-submitting partial writes,
> > since the buffering can be done in common code in one place
> > (qemu-char.c).
> 
> That's only OK if you assume it's OK to buffer all but one byte of the 
> transmit request.

Is that a fair assumption to make?

> > > I'm extremely reluctant to add a new layer of buffering that is not
> > > visible to ether the kernel or the device.  In practice we still need to
> > > be able to split oversize requests, so handling small split requests
> > > should be pretty much free.
> > 
> > So do you propose to propagate this -EAGAIN error all the way to the
> > guest?  That won't work for older virtio guests (for virtio, host and
> > guest changes will be needed).
> 
> Huh? That doesn't make any sense. The guest is already using an asyncronous 
> submission mechanism.  
> With a virtio device the status of a buffer becomes indeterminate once it has 
> been placed into the queue. Only when it is removed from the queue do we know 
> that it has completed.  The device may transfer all or part of that buffer at 
> any time in between.

Yes, just making sure this isn't what you're talking about.

> > > > > 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.
> > > 
> > > No.
> > > 
> > > Firstly you're assuming all users are virtio based. That may be all you
> > > care about, but is not acceptable if you want to get this code merged.
> > 
> > OK, but it's assumed that once a -EAGAIN is returned, the caller will
> > take appropriate actions to restrict the data sent.  Especially,
> > send_all has:
> > 
> >     if (chr->write_blocked) {
> >         /*
> >          * We don't handle this situation: the caller should not send
> >          * us data while we're blocked.
> >          *
> >          * We could buffer this data here but that'll only encourage
> >          * bad behaviour on part of the callers.
> 
> >          */
> >         return -1;
> >     }
> 
> If you're being draconian about this then do it properly and make this an 
> abort. Otherwise return -EAGAIN. Returning a random error seems like the worst 
> of both worlds.  Your code results in spurious guest errors (or lost data) 
> with real indication that this is actually a qemu device emulation bug.

Yeah; abort() is a good idea.

(BTW the previous send_all() routine just returned -1 for anything other
than -EINTR and -EAGAIN, so I went for consistency.)

> > > Secondly, the virtqueue only restricts the number of direct ring buffer
> > > entries. It does not restrict the quantity of data each ring entry points
> > > to.
> > 
> > But that's entirely in guest memory, so it's limited to the amount of
> > RAM that has been allocated to the guest.
> 
> Exactly. The guest can cause ram_size * nr_ports of additional host memory to 
> be allocated.  Not acceptable. 

OK -- so this is how it adds up:

- guest vq
- virtio-serial-bus converts iov to buf
- qemu-char stores the buf in case it wasn't able to send

but then, since it's all async, we have:

- virtio-serial-bus frees the buf
- guest deletes the buf and removes it from the vq

So what's left is only the data in qemu-char's buf.  Now this can be
(buf_size - 1) * nr_ports in the worst case.

The alternative would be to keep using the guest buffer till all's done,
but then that depends on qemu getting async support - separating out the
qemu_chr_write() into a separate thread and allowing vcpu and chr io
operations to be run simultaneously.

		Amit