virtio: console: struct ports for multiple ports per device.
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 3 lines · 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.
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.
Quoted 6 lines · Rusty Russell
Quoted 3 lines · 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.
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
Quoted 2 lines
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.
Quoted 3 lines
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?
Quoted 9 lines
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.
Quoted 3 lines
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.
Quoted 1 line
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)
Quoted 1 line
5) Add the debugfs and sysfs stuff in stages.
Agreed (and have done in my scratch series).
Quoted 2 lines
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
Quoted 8 lines · Amit Shah
Quoted 3 lines
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).
Quoted 6 lines
Quoted 3 lines
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.
Quoted 11 lines
Quoted 1 line
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.
Re: [PATCH 3/8] hvc_console: make the ops pointer const.
In reply to a message not in the archiveAm Dienstag 10 November 2009 07:27:30 schrieb Rusty Russell:
Quoted 6 lines
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 <[email protected]>To: Christian Borntraeger <[email protected]>
Cc: [email protected]
Quoted 12 lines
--- 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
I have not yet looked at the other ones, but I will try to find some time to look at them.
Christian
Re: [PATCH 3/8] hvc_console: make the ops pointer const.
In reply to a message not in the archiveAm Dienstag 10 November 2009 07:27:30 schrieb Rusty Russell:
Quoted 6 lines
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 <[email protected]>To: Christian Borntraeger <[email protected]>
Cc: [email protected]
Quoted 12 lines
--- 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
I have not yet looked at the other ones, but I will try to find some time to look at them.
Christian
Quoted 3 lines · 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.
Hey Rusty,
Quoted 8 lines
static void
virtcons_apply_config(struct virtio_device *dev){(quoted diff) +1
-1- struct port *port = dev->priv; + struct ports *ports = dev->priv; struct winsize ws; if (virtio_has_feature(dev, VIRTIO_CONSOLE_F_SIZE)) {
Review static void virtcons_apply_config(struct · line 173
Quoted 7 lines
(quoted diff) +3
-1@@ -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().
Review static void virtcons_apply_config(struct · line 174
Quoted 2 lines
(quoted diff) +1
-1-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?
Review static void virtcons_apply_config(struct
Quoted 10 lines
(quoted diff) +5
-2{ - 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
Quoted 6 lines · Amit Shah
Quoted 3 lines · 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.
Hey Rusty,
Hi Amit,
Quoted 5 lines
Review
Quoted 2 lines
(quoted diff) +1
-1-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.
Quoted 2 lines
Other than this, the series is good; I can base my patches on top of these.
Excellent!
Quoted 5 lines
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.
Quoted 3 lines
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.