Paolo Bonzini
2012-Apr-18 13:33 UTC
[RFC PATCH] virtio_console: link vq to port with a private pointer in struct virtqueue
For virtio-scsi multiqueue support I would like to have an easy and fast way to go from a virtqueue to the internal struct for that queue. It turns out that virtio-serial has the same need, but it gets by with a simple list walk. This patch adds a pointer to struct virtqueue that is reserved for the virtio device, and uses it in virtio-serial. Cc: Amit Shah <amit.shah at redhat.com> Cc: Rusty Russell <rusty at rustcorp.com.au> Cc: "Michael S. Tsirkin" <mst at redhat.com> Signed-off-by: Paolo Bonzini <pbonzini at redhat.com> --- Untested; what do you think? Would this patch be acceptable as is, or only with a more pressing need in virtio-scsi, or never? drivers/char/virtio_console.c | 16 +++++----------- include/linux/virtio.h | 2 ++ 2 files changed, 7 insertions(+), 11 deletions(-) diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c index 1c74734..cfc7a63 100644 --- a/drivers/char/virtio_console.c +++ b/drivers/char/virtio_console.c @@ -297,17 +297,7 @@ out: static struct port *find_port_by_vq(struct ports_device *portdev, struct virtqueue *vq) { - struct port *port; - unsigned long flags; - - spin_lock_irqsave(&portdev->ports_lock, flags); - list_for_each_entry(port, &portdev->ports, list) - if (port->in_vq == vq || port->out_vq == vq) - goto out; - port = NULL; -out: - spin_unlock_irqrestore(&portdev->ports_lock, flags); - return port; + return vq->vdev_priv; } static bool is_console_port(struct port *port) @@ -1159,6 +1149,8 @@ static int add_port(struct ports_device *portdev, u32 id) port->in_vq = portdev->in_vqs[port->id]; port->out_vq = portdev->out_vqs[port->id]; + port->in_vq->vdev_priv = port; + port->out_vq->vdev_priv = port; port->cdev = cdev_alloc(); if (!port->cdev) { @@ -1872,6 +1864,8 @@ static int virtcons_restore(struct virtio_device *vdev) list_for_each_entry(port, &portdev->ports, list) { port->in_vq = portdev->in_vqs[port->id]; port->out_vq = portdev->out_vqs[port->id]; + port->in_vq->vdev_priv = port; + port->out_vq->vdev_priv = port; fill_queue(port->in_vq, &port->inbuf_lock); diff --git a/include/linux/virtio.h b/include/linux/virtio.h index c193ccf..6b39c1a 100644 --- a/include/linux/virtio.h +++ b/include/linux/virtio.h @@ -14,6 +14,7 @@ * @callback: the function to call when buffers are consumed (can be NULL). * @name: the name of this virtqueue (mainly for debugging) * @vdev: the virtio device this queue was created for. + * @vdev_priv: a pointer for the virtio device to use. * @priv: a pointer for the virtqueue implementation to use. */ struct virtqueue { @@ -21,6 +22,7 @@ struct virtqueue { void (*callback)(struct virtqueue *vq); const char *name; struct virtio_device *vdev; + void *vdev_priv; void *priv; }; -- 1.7.9.3
Michael S. Tsirkin
2012-Apr-18 14:21 UTC
[RFC PATCH] virtio_console: link vq to port with a private pointer in struct virtqueue
On Wed, Apr 18, 2012 at 03:33:33PM +0200, Paolo Bonzini wrote:> For virtio-scsi multiqueue support I would like to have an easy and > fast way to go from a virtqueue to the internal struct for that > queue. > > It turns out that virtio-serial has the same need, but it gets > by with a simple list walk. > > This patch adds a pointer to struct virtqueue that is reserved for > the virtio device, and uses it in virtio-serial. > > Cc: Amit Shah <amit.shah at redhat.com> > Cc: Rusty Russell <rusty at rustcorp.com.au> > Cc: "Michael S. Tsirkin" <mst at redhat.com> > Signed-off-by: Paolo Bonzini <pbonzini at redhat.com> > --- > Untested; what do you think? Would this patch be acceptable > as is, or only with a more pressing need in virtio-scsi, or never? > > drivers/char/virtio_console.c | 16 +++++----------- > include/linux/virtio.h | 2 ++ > 2 files changed, 7 insertions(+), 11 deletions(-) > > diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c > index 1c74734..cfc7a63 100644 > --- a/drivers/char/virtio_console.c > +++ b/drivers/char/virtio_console.c > @@ -297,17 +297,7 @@ out: > static struct port *find_port_by_vq(struct ports_device *portdev, > struct virtqueue *vq) > { > - struct port *port; > - unsigned long flags; > - > - spin_lock_irqsave(&portdev->ports_lock, flags); > - list_for_each_entry(port, &portdev->ports, list) > - if (port->in_vq == vq || port->out_vq == vq) > - goto out; > - port = NULL; > -out: > - spin_unlock_irqrestore(&portdev->ports_lock, flags); > - return port; > + return vq->vdev_priv; > } > > static bool is_console_port(struct port *port) > @@ -1159,6 +1149,8 @@ static int add_port(struct ports_device *portdev, u32 id) > > port->in_vq = portdev->in_vqs[port->id]; > port->out_vq = portdev->out_vqs[port->id]; > + port->in_vq->vdev_priv = port; > + port->out_vq->vdev_priv = port; > > port->cdev = cdev_alloc(); > if (!port->cdev) { > @@ -1872,6 +1864,8 @@ static int virtcons_restore(struct virtio_device *vdev) > list_for_each_entry(port, &portdev->ports, list) { > port->in_vq = portdev->in_vqs[port->id]; > port->out_vq = portdev->out_vqs[port->id]; > + port->in_vq->vdev_priv = port; > + port->out_vq->vdev_priv = port; > > fill_queue(port->in_vq, &port->inbuf_lock); >Let's add an API to set this pointer. Document that you must not set it after probe/restore returned. With an API we can actually have a BUG_ON that checks it's not modified after probe.> diff --git a/include/linux/virtio.h b/include/linux/virtio.h > index c193ccf..6b39c1a 100644 > --- a/include/linux/virtio.h > +++ b/include/linux/virtio.h > @@ -14,6 +14,7 @@ > * @callback: the function to call when buffers are consumed (can be NULL). > * @name: the name of this virtqueue (mainly for debugging) > * @vdev: the virtio device this queue was created for. > + * @vdev_priv: a pointer for the virtio device to use.It's for the driver actually.> * @priv: a pointer for the virtqueue implementation to use. > */ > struct virtqueue { > @@ -21,6 +22,7 @@ struct virtqueue { > void (*callback)(struct virtqueue *vq); > const char *name; > struct virtio_device *vdev; > + void *vdev_priv; > void *priv;The name is confusing: it seems to imply it's a device pointer. Maybe we should rename priv to something like __priv and make priv useful for devices?> }; > > -- > 1.7.9.3
Amit Shah
2012-Apr-19 06:21 UTC
[RFC PATCH] virtio_console: link vq to port with a private pointer in struct virtqueue
On (Wed) 18 Apr 2012 [15:33:33], Paolo Bonzini wrote:> For virtio-scsi multiqueue support I would like to have an easy and > fast way to go from a virtqueue to the internal struct for that > queue. > > It turns out that virtio-serial has the same need, but it gets > by with a simple list walk. > > This patch adds a pointer to struct virtqueue that is reserved for > the virtio device, and uses it in virtio-serial. > > Cc: Amit Shah <amit.shah at redhat.com> > Cc: Rusty Russell <rusty at rustcorp.com.au> > Cc: "Michael S. Tsirkin" <mst at redhat.com> > Signed-off-by: Paolo Bonzini <pbonzini at redhat.com> > --- > Untested; what do you think? Would this patch be acceptable > as is, or only with a more pressing need in virtio-scsi, or never?Yes, this is useful. Saves taking a spin lock and walking the list each time any data is received from the host. Acked-by: Amit Shah <amit.shah at redhat.com> Thanks, Amit
Rusty Russell
2012-May-08 02:11 UTC
[RFC PATCH] virtio_console: link vq to port with a private pointer in struct virtqueue
On Wed, 18 Apr 2012 15:33:33 +0200, Paolo Bonzini <pbonzini at redhat.com> wrote:> For virtio-scsi multiqueue support I would like to have an easy and > fast way to go from a virtqueue to the internal struct for that > queue. > > It turns out that virtio-serial has the same need, but it gets > by with a simple list walk. > > This patch adds a pointer to struct virtqueue that is reserved for > the virtio device, and uses it in virtio-serial.I ike the concept, but share Michael's concern with naming confusion. How bad would be it to get rid of the current ->priv and use container_of() instead? ie. have virtio_pci, virtio_mmio, lguest_bus and s390's kvm_virtio embed the struct virtqueue? Thanks, Rusty.
Apparently Analagous Threads
- [RFC PATCH] virtio_console: link vq to port with a private pointer in struct virtqueue
- virtio: console: Return -EFAULT on copy_xx_user errors, allow larger writes
- virtio: console: Return -EFAULT on copy_xx_user errors, allow larger writes
- [PATCH 2/2] virtio-serial: setup_port_vq when adding port
- [PATCH 2/2] virtio-serial: setup_port_vq when adding port