[PATCH 8/8] virtio: console: struct ports for multiple ports per device.

[PATCH 8/8] virtio: console: struct ports for multiple ports per device.

[Rusty Russell]

Rather than assume a single port, add a 'struct ports' with an array
of ports.  Currently, there's always only one, but that will change.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
 drivers/char/virtio_console.c |   77 +++++++++++++++++++++++++++++-------------
 1 file changed, 54 insertions(+), 23 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -39,6 +39,13 @@ struct port {
 	u32 vtermno;
 };
 
+/* This is vdev->priv. */
+struct ports {
+	/* We use a simple array. */
+	unsigned int num_ports;
+	struct port port[0];
+};
+
 /* The list of ports. */
 static LIST_HEAD(ports_by_vtermno);
 
@@ -122,8 +129,11 @@ static int get_chars(u32 vtermno, char *
 
 	/* No more in buffer?  See if they've (re)used it. */
 	if (port->offset == port->used_len) {
-		if (!port->in_vq->vq_ops->get_buf(port->in_vq, &port->used_len))
+		unsigned int len;
+
+		if (!port->in_vq->vq_ops->get_buf(port->in_vq, &len))
 			return 0;
+		port->used_len = len;
 		port->offset = 0;
 	}
 
@@ -148,7 +158,7 @@ static int get_chars(u32 vtermno, char *
  */
 static void virtcons_apply_config(struct virtio_device *dev)
 {
-	struct port *port = dev->priv;
+	struct ports *ports = dev->priv;
 	struct winsize ws;
 
 	if (virtio_has_feature(dev, VIRTIO_CONSOLE_F_SIZE)) {
@@ -158,7 +168,9 @@ static void virtcons_apply_config(struct
 		dev->config->get(dev,
 				 offsetof(struct virtio_console_config, rows),
 				 &ws.ws_row, sizeof(u16));
-		hvc_resize(port->hvc, ws);
+		/* This is the pre-multiport style: we use control messages
+		 * these days which specify the port.  So this means port 0. */
+		hvc_resize(ports->port[0].hvc, ws);
 	}
 }
 
@@ -181,9 +193,14 @@ static void notifier_del_vio(struct hvc_
 
 static void hvc_handle_input(struct virtqueue *vq)
 {
-	struct port *port = vq->vdev->priv;
+	struct ports *ports = vq->vdev->priv;
+	unsigned int i;
+	bool activity = false;
 
-	if (hvc_poll(port->hvc))
+	for (i = 0; i < ports->num_ports; i++)
+		activity |= hvc_poll(ports->port[i].hvc);
+
+	if (activity)
 		hvc_kick();
 }
 
@@ -208,28 +225,37 @@ int __init virtio_cons_early_init(int (*
 	return hvc_instantiate(0, 0, &hv_ops);
 }
 
-static struct port *__devinit alloc_port(u32 vtermno)
+static struct ports *__devinit alloc_ports(unsigned int num)
 {
-	struct port *port = kmalloc(sizeof *port, GFP_KERNEL);
+	struct ports *ports;
+	int i;
 
-	if (!port)
+	ports = kmalloc(sizeof *ports + sizeof(ports->port[0]) * num,
+			GFP_KERNEL);
+	if (!ports)
 		return NULL;
 
-	port->used_len = port->offset = 0;
-	port->inbuf = kmalloc(PAGE_SIZE, GFP_KERNEL);
-	if (!port->inbuf) {
-		kfree(port);
-		return NULL;
+	ports->num_ports = num;
+	for (i = 0; i < ports->num_ports; i++) {
+		ports->port[i].used_len = ports->port[i].offset = 0;
+		ports->port[i].inbuf = kmalloc(PAGE_SIZE, GFP_KERNEL);
+		if (unlikely(!ports->port[i].inbuf)) {
+			while (--i >= 0)
+				kfree(ports->port[i].inbuf);
+			kfree(ports);
+			return NULL;
+		}
 	}
-	port->hvc = NULL;
-	port->vtermno = vtermno;
-	return port;
+	return ports;
 }
 
-static void free_port(struct port *port)
+static void free_ports(struct ports *ports)
 {
-	kfree(port->inbuf);
-	kfree(port);
+	unsigned int i;
+
+	for (i = 0; i < ports->num_ports; i++)
+		kfree(ports->port[i].inbuf);
+	kfree(ports);
 }
 
 /* Once we're further in boot, we get probed like any other virtio device.
@@ -245,17 +271,21 @@ static int __devinit virtcons_probe(stru
 	const char *names[] = { "input", "output" };
 	struct virtqueue *vqs[2];
 	int err;
+	struct ports *ports;
 	struct port *port;
 
-	port = alloc_port(0);
-	if (!port) {
+	ports = alloc_ports(1);
+	if (!ports) {
 		err = -ENOMEM;
 		goto fail;
 	}
 
+	/* Convenience variable since we only have one port. */
+	port = &ports->port[0];
+
 	/* Attach this port to this virtio_device, and vice-versa. */
 	port->vdev = vdev;
-	vdev->priv = port;
+	vdev->priv = ports;
 
 	/* Find the queues. */
 	err = vdev->config->find_vqs(vdev, 2, vqs, callbacks, names);
@@ -275,6 +305,7 @@ static int __devinit virtcons_probe(stru
 	 * The final argument is the output buffer size: we can do any size,
 	 * so we put PAGE_SIZE here.
 	 */
+	port->vtermno = 0;
 	port->hvc = hvc_alloc(port->vtermno, 0, &hv_ops, PAGE_SIZE);
 	if (IS_ERR(port->hvc)) {
 		err = PTR_ERR(port->hvc);
@@ -296,7 +327,7 @@ static int __devinit virtcons_probe(stru
 free_vqs:
 	vdev->config->del_vqs(vdev);
 free:
-	free_port(port);
+	free_ports(ports);
 fail:
 	return err;
 }

Re: [PATCH 8/8] virtio: console: struct ports for multiple ports per device.

[Rusty Russell]

On Tue, 10 Nov 2009 04:57:30 pm Rusty Russell wrote:
> 
> Rather than assume a single port, add a 'struct ports' with an array
> of ports.  Currently, there's always only one, but that will change.

Now, from here we need several more patches.  At least:

1) Generalize the output path so we can sleep, rather than spinning.
   This means a non-NULL callback, a 'done' flag and a struct completion.

2) Do we really need more than input buffer at a time?  If not, it's easy to
   generalize the input callback.  This will be slow, but shouldn't be a
   problem.

3) Add a header.  I suggest we change your proposed format a little, rather
   than an explicit length use a "continues" bit:

	struct virtio_console_header {
		/* Port number */
		__u32 id;
		/* VIRTIO_CONSOLE_HDR_CONTROLMSG, VIRTIO_CONSOLE_HDR_CONTINUES. */
		__u32 flags;
	} __attribute__((packed));

   This should be really easy to construct, and for input in the !multiport
   path we can fake one up.  We ignore CONTINUES on input since we don't have
   a userspace API which understands framing (we'd need recvmsg).

4) Hook it all together with the new feature bit.

5) Add the debugfs and sysfs stuff in stages.

6) See if we really need throttling now we're only allowing 1 buffer at a
   time.

Cheers,
Rusty.

Re: [PATCH 8/8] virtio: console: struct ports for multiple ports per device.

[Amit Shah]

On (Tue) Nov 10 2009 [17:27:03], Rusty Russell wrote:
> On Tue, 10 Nov 2009 04:57:30 pm Rusty Russell wrote:
> > 
> > Rather than assume a single port, add a 'struct ports' with an array
> > of ports.  Currently, there's always only one, but that will change.
> 
> Now, from here we need several more patches.  At least:

Hey Rusty,

Thanks. I'll look through these.

I've split up my code as well, have put it up at

http://git.kernel.org/?p=linux/kernel/git/amit/virtio-console.git


> 1) Generalize the output path so we can sleep, rather than spinning.
>    This means a non-NULL callback, a 'done' flag and a struct completion.

Instead of the buffer pool? I think we can go with the buffer pool of
fixed-sized buffers, but a lot lesser than the 1024 I had.

(Also meshes with the vnc comment below)

On the other hand, performance isn't critical, so we could do away with
the pool.

> 2) Do we really need more than input buffer at a time?  If not, it's easy to
>    generalize the input callback.  This will be slow, but shouldn't be a
>    problem.

In my testing of a vnc clipboard copy/paste, the vnc client only sent the
clipboard data once and didn't bother about retransmitting if write()
returned < len. It's a problem with the vnc client I used (tigervnc, which is
based off tightvnc) though but adding that support would hurt?

> 3) Add a header.  I suggest we change your proposed format a little, rather
>    than an explicit length use a "continues" bit:
> 
> 	struct virtio_console_header {
> 		/* Port number */
> 		__u32 id;
> 		/* VIRTIO_CONSOLE_HDR_CONTROLMSG, VIRTIO_CONSOLE_HDR_CONTINUES. */
> 		__u32 flags;
> 	} __attribute__((packed));

Good idea.
 
>    This should be really easy to construct, and for input in the !multiport
>    path we can fake one up.  We ignore CONTINUES on input since we don't have
>    a userspace API which understands framing (we'd need recvmsg).

The header should only be sent (and expected) in the multiport case, so
this won't matter when the .._F_MULTIPORT feature is not found.

> 4) Hook it all together with the new feature bit.

I've actually split it into 4 feature bits:
MULTIPORT means multiple ports and a header
THROTTLE to save host and guest from OOM
CACHING to allow ports to buffer data even after the char device is
  closed
UNPLUG to allow port unplug

(I added these as part of the splitting effort because they're now in
individual patches)

> 5) Add the debugfs and sysfs stuff in stages.

Agreed (and have done in my scratch series).

> 6) See if we really need throttling now we're only allowing 1 buffer at a
>    time.

That also depends if we cache data per port -- the throttling I had was
for the per-port unconsumed buffers.

Thanks again,

		Amit

Re: [PATCH 3/8] hvc_console: make the ops pointer const.

[Christian Borntraeger]

Am Dienstag 10 November 2009 07:27:30 schrieb Rusty Russell:
> This is nicer for modern R/O protection.  And noone needs it non-const, so
> constify the callers as well.
> 
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> To: Christian Borntraeger <borntraeger@de.ibm.com>
> Cc: linuxppc-dev@ozlabs.org
> ---
>  drivers/char/hvc_beat.c       |    2 +-
>  drivers/char/hvc_console.c    |    7 ++++---
>  drivers/char/hvc_console.h    |    7 ++++---
>  drivers/char/hvc_iseries.c    |    2 +-
>  drivers/char/hvc_iucv.c       |    2 +-
>  drivers/char/hvc_rtas.c       |    2 +-
>  drivers/char/hvc_udbg.c       |    2 +-
>  drivers/char/hvc_vio.c        |    2 +-
>  drivers/char/hvc_xen.c        |    2 +-
>  drivers/char/virtio_console.c |    2 +-
>  10 files changed, 16 insertions(+), 14 deletions(-)

Ok, I started with patches 1-3. I like the result.

So for the first 3 patches you can add
Tested-by: Christian Borntraeger <borntraeger@de.ibm.com> (on s390)
Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>

I have not yet looked at the other ones, but I will try to find some time to 
look at them.

Christian

Re: [PATCH 8/8] virtio: console: struct ports for multiple ports per device.

[Amit Shah]

On (Tue) Nov 10 2009 [16:57:30], Rusty Russell wrote:
> 
> Rather than assume a single port, add a 'struct ports' with an array
> of ports.  Currently, there's always only one, but that will change.

Hey Rusty,

>  static void virtcons_apply_config(struct virtio_device *dev)
>  {
> -	struct port *port = dev->priv;
> +	struct ports *ports = dev->priv;
>  	struct winsize ws;
>  
>  	if (virtio_has_feature(dev, VIRTIO_CONSOLE_F_SIZE)) {
> @@ -158,7 +168,9 @@ static void virtcons_apply_config(struct
>  		dev->config->get(dev,
>  				 offsetof(struct virtio_console_config, rows),
>  				 &ws.ws_row, sizeof(u16));
> -		hvc_resize(port->hvc, ws);
> +		/* This is the pre-multiport style: we use control messages
> +		 * these days which specify the port.  So this means port 0. */
> +		hvc_resize(ports->port[0].hvc, ws);

I've fixed a bug in the latest patches that I have (just on git so far;
not sent to lists) -- this function is also called from hvc's
notifier_add, so instead of passing vdev here, we can just pass the port
and look up the port in the notifier_add_vio() function via
get_port_from_vtermno().

> -static struct port *__devinit alloc_port(u32 vtermno)
> +static struct ports *__devinit alloc_ports(unsigned int num)

This will have to be changed when we add support for hotplug. So instead
of doing this, just have a linked list from the start?

>  {
> -	struct port *port = kmalloc(sizeof *port, GFP_KERNEL);
> +	struct ports *ports;
> +	int i;
>  
> -	if (!port)
> +	ports = kmalloc(sizeof *ports + sizeof(ports->port[0]) * num,
> +			GFP_KERNEL);
> +	if (!ports)
>  		return NULL;


Other than this, the series is good; I can base my patches on top of
these.

I guess we can also assign a number to each vdev that gets probed so
that sysfs and debugfs entries for ports can be put in their
vdev-specific directories, like

/sys/class/virtio-console0/vcon0/name

etc.

Also, if you think the send/receive workqueues are fine and we move to
those, they will have to be introduced slightly earlier in this patch
series.

(I'll send out my patches to the list in a while; some polishing is
required still but they're all style issues rather than functionality
ones.)

		Amit

Re: [PATCH 8/8] virtio: console: struct ports for multiple ports per device.

[Rusty Russell]

On Tue, 10 Nov 2009 08:03:28 pm Amit Shah wrote:
> On (Tue) Nov 10 2009 [16:57:30], Rusty Russell wrote:
> > 
> > Rather than assume a single port, add a 'struct ports' with an array
> > of ports.  Currently, there's always only one, but that will change.
> 
> Hey Rusty,

Hi Amit,

> > -static struct port *__devinit alloc_port(u32 vtermno)
> > +static struct ports *__devinit alloc_ports(unsigned int num)
> 
> This will have to be changed when we add support for hotplug. So instead
> of doing this, just have a linked list from the start?

No, for hotplug I think we just shift from a dangling array to a pointer
to an array.  That changes the alloc and free functions, but *not* change
to any users.

> Other than this, the series is good; I can base my patches on top of
> these.

Excellent!

> I guess we can also assign a number to each vdev that gets probed so
> that sysfs and debugfs entries for ports can be put in their
> vdev-specific directories, like
> 
> /sys/class/virtio-console0/vcon0/name

That makes sense; we do the same with virtio_blk IIRC.

> Also, if you think the send/receive workqueues are fine and we move to
> those, they will have to be introduced slightly earlier in this patch
> series.

So far I haven't seen a need for them.  This is simple and works.  But if
a later patch needs it, we do it and then maybe shuffle the patch backwards
in the sequence.

(As you can tell, I don't use git for development :)

Thanks,
Rusty.

Re: [PATCH 8/8] virtio: console: struct ports for multiple ports per device.

[Rusty Russell]

On Tue, 10 Nov 2009 05:54:14 pm Amit Shah wrote:
> > 2) Do we really need more than input buffer at a time?  If not, it's easy to
> >    generalize the input callback.  This will be slow, but shouldn't be a
> >    problem.
> 
> In my testing of a vnc clipboard copy/paste, the vnc client only sent the
> clipboard data once and didn't bother about retransmitting if write()
> returned < len. It's a problem with the vnc client I used (tigervnc, which is
> based off tightvnc) though but adding that support would hurt?

Well, input buffer == read, output buffer == write.  But same deal.

And sure, simplest implementation would just return a short read/write.
But we can certainly loop inside our ->write and wait until all the data is
written, too (document why, maybe with O_NONBLOCK not looping).

> >    This should be really easy to construct, and for input in the !multiport
> >    path we can fake one up.  We ignore CONTINUES on input since we don't have
> >    a userspace API which understands framing (we'd need recvmsg).
> 
> The header should only be sent (and expected) in the multiport case, so
> this won't matter when the .._F_MULTIPORT feature is not found.

Yeah, but our code might be neater if we "always" have a header internally;
only one place would need to branch.  Obviously we have to see, but I was
thinking ahead.

> > 4) Hook it all together with the new feature bit.
> 
> I've actually split it into 4 feature bits:
> MULTIPORT means multiple ports and a header
> THROTTLE to save host and guest from OOM
> CACHING to allow ports to buffer data even after the char device is
>   closed
> UNPLUG to allow port unplug
> 
> (I added these as part of the splitting effort because they're now in
> individual patches)

OK, though I'm not adverse to partial feature implementations during a
consecutive patch series.
Technically, it's EXPERIMENTAL, so we can do stuff like that :)

Cheers,
Rusty.