Linux Virtualization - Apr 2012 - [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

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

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

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.