Re: [Qemu-devel] [PATCH 00/12][RFC] char: add flow control and fix guest_[open|close]

[Anthony Liguori]

On 08/04/2011 01:45 AM, Amit Shah wrote:
> On (Mon) 01 Aug 2011 [09:22:58], Anthony Liguori wrote:
> > The char layer has been growing some nasty warts for some time now as we ask it
> > to do things it was never intended on doing.  It's been long over due for an
> > overhaul and its become evident to me that we need to address this first before
> > adding any more features to the char layer.
> >
> > This series is the start at sanitizing the char layer.  It effectively turns
> > the char layer into an internal pipe.  It supports flow control using an
> > intermediate ring queue for each direction.
>
> Let's break down what we have now:
>
> * chardev ->  guest (backend writes):
>     we have connect/disconnect notifications, we have an ugly
>     can_read() implementation that is executed each time iohandlers are
>     run.  However, it gives us reasonable flow control.

It has one major flaw.  It assumes that the backends can implement 
polling to determine when the front end can read.

This makes it impossible to implement a backend that isn't associated 
with a file descriptor (like a QMP server as required by the guest agent).

> * guest ->  chardev (frontend writes):
>     we don't get chardev connect/disconnect events, neither do we get
>     to know if the chardev is overwhelmed with data and to stop sending
>     any more till it has some room in its queue.

We do have connect/disconnect event--that's open/close.


>     This is because we
>     need poll semantics instead of select to get POLLIN and POLLOUT
>     events, using which we can ascertain what state the chardev is in.
>     There's no call corresponding to the existing qemu_chr_can_read(),
>     which essentially confirms if a chardev is able to handle data for
>     output.  This series only adds a qemu_char_fe_can_write(), doesn't
>     add callbacks for connect/disconnect events since that depends on
>     polling.

There are already events for connect/disconnect so I'm not sure what 
you're talking about.

> The problem I think with adding a buffer is it just solves the flow
> control problem without solving the connect/disconnect events issue by
> just switching to poll,

I don't understand at all what you're saying here :-)

Hopefully you'd agree, that from a design perspective, the closer a 
chrdrv looks to a normal unix socket, the more correct it is.

After this series, we have:

1) read/write methods that behave like unix read/write (except zero 
indicates EAGAIN, not EOF).

2) OPENED/CLOSE events that map to accept/EOF

3) edge events for readability that semantically map to epoll()

I think that's pretty complete.  I don't see anything that's missing.

> and solving all the problems at once.  Is
> there something that we solve by having a buffer in addition to
> poll()?
>
> Also, introducing a buffer doesn't really solve all the problems; just
> shifts them into the char layer instead of letting the OS handle the
> queues naturally.

What does "naturally" mean?

Regards,

Anthony Liguori

>                 Amit