[RFC PATCH] virtio_console: link vq to port with a private pointer in struct virtqueue

[RFC PATCH] virtio_console: link vq to port with a private pointer in struct virtqueue

[Paolo Bonzini]

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@redhat.com>
Cc: Rusty Russell <rusty@rustcorp.com.au>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@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

[RFC PATCH] virtio_console: link vq to port with a private pointer in struct virtqueue

[Paolo Bonzini]

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@redhat.com>
Cc: Rusty Russell <rusty@rustcorp.com.au>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@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

Re: [RFC PATCH] virtio_console: link vq to port with a private pointer in struct virtqueue

[Michael S. Tsirkin]

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@redhat.com>
> Cc: Rusty Russell <rusty@rustcorp.com.au>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@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

Re: [RFC PATCH] virtio_console: link vq to port with a private pointer in struct virtqueue

[Michael S. Tsirkin]

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@redhat.com>
> Cc: Rusty Russell <rusty@rustcorp.com.au>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@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

Re: [RFC PATCH] virtio_console: link vq to port with a private pointer in struct virtqueue

[Paolo Bonzini]

Il 18/04/2012 16:21, Michael S. Tsirkin ha scritto:
>> @@ -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.

Why?

> 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.

Right.  However...

>>   * @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.

... it's private to the driver that owns vdev, hence the name.

> Maybe we should rename priv to something like __priv and make
> priv useful for devices?

I wanted to go for the smallest possible changes.  Right now we have 1
user for each member (virtio-ring vs. virtio-console) so neither member
is really dominating.

Paolo

Re: [RFC PATCH] virtio_console: link vq to port with a private pointer in struct virtqueue

[Paolo Bonzini]

Il 18/04/2012 16:21, Michael S. Tsirkin ha scritto:
>> @@ -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.

Why?

> 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.

Right.  However...

>>   * @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.

... it's private to the driver that owns vdev, hence the name.

> Maybe we should rename priv to something like __priv and make
> priv useful for devices?

I wanted to go for the smallest possible changes.  Right now we have 1
user for each member (virtio-ring vs. virtio-console) so neither member
is really dominating.

Paolo

Re: [RFC PATCH] virtio_console: link vq to port with a private pointer in struct virtqueue

[Michael S. Tsirkin]

On Wed, Apr 18, 2012 at 04:34:12PM +0200, Paolo Bonzini wrote:
> Il 18/04/2012 16:21, Michael S. Tsirkin ha scritto:
> >> @@ -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.
> 
> Why?

How would you prevent races if you do?

> > 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.
> 
> Right.  However...

pointer for a device can also be misunerstood as
'pointer to a device'. Note priv below actually
gets the correct meaning however you interpret 'for'.

Better 'pointer for the virtqueue driver 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;
> > 
> > The name is confusing: it seems to imply it's a device pointer.
> 
> ... it's private to the driver that owns vdev, hence the name.

I own a car but I'm not called Michael Car :)
driver_priv might be ok too. unfortunately virtio-pci
is also a driver so it can be misunderstood.

> > Maybe we should rename priv to something like __priv and make
> > priv useful for devices?
> 
> I wanted to go for the smallest possible changes.  Right now we have 1
> user for each member (virtio-ring vs. virtio-console) so neither member
> is really dominating.
> 
> Paolo

devices should dominate. ring is an implementation detail.

-- 
MST

Re: [RFC PATCH] virtio_console: link vq to port with a private pointer in struct virtqueue

[Michael S. Tsirkin]

On Wed, Apr 18, 2012 at 04:34:12PM +0200, Paolo Bonzini wrote:
> Il 18/04/2012 16:21, Michael S. Tsirkin ha scritto:
> >> @@ -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.
> 
> Why?

How would you prevent races if you do?

> > 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.
> 
> Right.  However...

pointer for a device can also be misunerstood as
'pointer to a device'. Note priv below actually
gets the correct meaning however you interpret 'for'.

Better 'pointer for the virtqueue driver 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;
> > 
> > The name is confusing: it seems to imply it's a device pointer.
> 
> ... it's private to the driver that owns vdev, hence the name.

I own a car but I'm not called Michael Car :)
driver_priv might be ok too. unfortunately virtio-pci
is also a driver so it can be misunderstood.

> > Maybe we should rename priv to something like __priv and make
> > priv useful for devices?
> 
> I wanted to go for the smallest possible changes.  Right now we have 1
> user for each member (virtio-ring vs. virtio-console) so neither member
> is really dominating.
> 
> Paolo

devices should dominate. ring is an implementation detail.

-- 
MST

Re: [RFC PATCH] virtio_console: link vq to port with a private pointer in struct virtqueue

[Paolo Bonzini]

Il 18/04/2012 18:10, Michael S. Tsirkin ha scritto:
> On Wed, Apr 18, 2012 at 04:34:12PM +0200, Paolo Bonzini wrote:
>> Il 18/04/2012 16:21, Michael S. Tsirkin ha scritto:
>>>> @@ -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.
>>
>> Why?
> 
> How would you prevent races if you do?

With some lock in the driver.  It's private to the driver, so the driver
decides how to synchronize access.

>>>>   * @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.
>>
>> ... it's private to the driver that owns vdev, hence the name.
> 
> I own a car but I'm not called Michael Car :)
> driver_priv might be ok too. unfortunately virtio-pci
> is also a driver so it can be misunderstood.

Yes.  Is fixing the comment and keeping the vdev_priv name ok with you?

> devices should dominate. ring is an implementation detail.

Ring came first, ring gets the nice name. :)

Paolo

Re: [RFC PATCH] virtio_console: link vq to port with a private pointer in struct virtqueue

[Paolo Bonzini]

Il 18/04/2012 18:10, Michael S. Tsirkin ha scritto:
> On Wed, Apr 18, 2012 at 04:34:12PM +0200, Paolo Bonzini wrote:
>> Il 18/04/2012 16:21, Michael S. Tsirkin ha scritto:
>>>> @@ -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.
>>
>> Why?
> 
> How would you prevent races if you do?

With some lock in the driver.  It's private to the driver, so the driver
decides how to synchronize access.

>>>>   * @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.
>>
>> ... it's private to the driver that owns vdev, hence the name.
> 
> I own a car but I'm not called Michael Car :)
> driver_priv might be ok too. unfortunately virtio-pci
> is also a driver so it can be misunderstood.

Yes.  Is fixing the comment and keeping the vdev_priv name ok with you?

> devices should dominate. ring is an implementation detail.

Ring came first, ring gets the nice name. :)

Paolo

Re: [RFC PATCH] virtio_console: link vq to port with a private pointer in struct virtqueue

[Michael S. Tsirkin]

On Wed, Apr 18, 2012 at 08:38:00PM +0200, Paolo Bonzini wrote:
> >>>>   * @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.
> >>
> >> ... it's private to the driver that owns vdev, hence the name.
> > 
> > I own a car but I'm not called Michael Car :)
> > driver_priv might be ok too. unfortunately virtio-pci
> > is also a driver so it can be misunderstood.
> 
> Yes.  Is fixing the comment and keeping the vdev_priv name ok with you?

This puts it lower on the scale of bad interfaces but
I think we still need a better name.

> > devices should dominate. ring is an implementation detail.
> 
> Ring came first, ring gets the nice name. :)
> 
> Paolo

Re: [RFC PATCH] virtio_console: link vq to port with a private pointer in struct virtqueue

[Michael S. Tsirkin]

On Wed, Apr 18, 2012 at 08:38:00PM +0200, Paolo Bonzini wrote:
> >>>>   * @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.
> >>
> >> ... it's private to the driver that owns vdev, hence the name.
> > 
> > I own a car but I'm not called Michael Car :)
> > driver_priv might be ok too. unfortunately virtio-pci
> > is also a driver so it can be misunderstood.
> 
> Yes.  Is fixing the comment and keeping the vdev_priv name ok with you?

This puts it lower on the scale of bad interfaces but
I think we still need a better name.

> > devices should dominate. ring is an implementation detail.
> 
> Ring came first, ring gets the nice name. :)
> 
> Paolo

Re: [RFC PATCH] virtio_console: link vq to port with a private pointer in struct virtqueue

[Amit Shah]

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@redhat.com>
> Cc: Rusty Russell <rusty@rustcorp.com.au>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@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@redhat.com>

Thanks,

		Amit

Re: [RFC PATCH] virtio_console: link vq to port with a private pointer in struct virtqueue

[Amit Shah]

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@redhat.com>
> Cc: Rusty Russell <rusty@rustcorp.com.au>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@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@redhat.com>

Thanks,

		Amit

Re: [RFC PATCH] virtio_console: link vq to port with a private pointer in struct virtqueue

[Rusty Russell]

On Wed, 18 Apr 2012 15:33:33 +0200, Paolo Bonzini <pbonzini@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.

Re: [RFC PATCH] virtio_console: link vq to port with a private pointer in struct virtqueue

[Rusty Russell]

On Wed, 18 Apr 2012 15:33:33 +0200, Paolo Bonzini <pbonzini@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.

Re: [RFC PATCH] virtio_console: link vq to port with a private pointer in struct virtqueue

[Paolo Bonzini]

Il 08/05/2012 04:11, Rusty Russell ha scritto:
>> > 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?

How bad is not the question, it would actually be pretty good... the
question is more how hard! :)

lguest and s390 are a bit different from the others because ->priv
points into a memory-mapped descriptor provided by the host; PCI and
MMIO have lots of similarities.

Looks doable...

Paolo

Re: [RFC PATCH] virtio_console: link vq to port with a private pointer in struct virtqueue

[Paolo Bonzini]

Il 08/05/2012 04:11, Rusty Russell ha scritto:
>> > 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?

How bad is not the question, it would actually be pretty good... the
question is more how hard! :)

lguest and s390 are a bit different from the others because ->priv
points into a memory-mapped descriptor provided by the host; PCI and
MMIO have lots of similarities.

Looks doable...

Paolo

Re: [RFC PATCH] virtio_console: link vq to port with a private pointer in struct virtqueue

[Paolo Bonzini]

> 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?

Something like the following, compile-tested only...

The layout of vring_virtqueue gets a bit complex, with private vring
data _before_ the struct virtqueue and private bus data after it.

The alternative is to make vring_virtqueue public.  It has some
appeal (for example many backends duplicate the num and pages members
in their private data) but overall I think I prefer this.

Anyhow, it is doable, the patches aren't too small but they are easily
split and quite boring.  Shall I pursue this instead?

Paolo

diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
index 2e03d41..047ce01 100644
--- a/drivers/virtio/virtio_pci.c
+++ b/drivers/virtio/virtio_pci.c
@@ -71,10 +71,10 @@ enum {
 	VP_MSIX_VQ_VECTOR = 1,
 };
 
-struct virtio_pci_vq_info
+struct virtio_pci_vq
 {
 	/* the actual virtqueue */
-	struct virtqueue *vq;
+	struct virtqueue vq;
 
 	/* the number of entries in the queue */
 	int num;
@@ -106,6 +106,12 @@ static struct virtio_pci_device *to_vp_device(struct virtio_device *vdev)
 	return container_of(vdev, struct virtio_pci_device, vdev);
 }
 
+/* Convert a generic virtio virtqueue to our structure */
+static struct virtio_pci_vq *to_vp_queue(struct virtqueue *vq)
+{
+	return container_of(vq, struct virtio_pci_vq, vq);
+}
+
 /* virtio config->get_features() implementation */
 static u32 vp_get_features(struct virtio_device *vdev)
 {
@@ -202,11 +208,11 @@ static void vp_reset(struct virtio_device *vdev)
 static void vp_notify(struct virtqueue *vq)
 {
 	struct virtio_pci_device *vp_dev = to_vp_device(vq->vdev);
-	struct virtio_pci_vq_info *info = vq->priv;
+	struct virtio_pci_vq *vp_queue = to_vp_queue(vq);
 
 	/* we write the queue's selector into the notification register to
 	 * signal the other end */
-	iowrite16(info->queue_index, vp_dev->ioaddr + VIRTIO_PCI_QUEUE_NOTIFY);
+	iowrite16(vp_queue->queue_index, vp_dev->ioaddr + VIRTIO_PCI_QUEUE_NOTIFY);
 }
 
 /* Handle a configuration change: Tell driver if it wants to know. */
@@ -226,13 +232,13 @@ static irqreturn_t vp_config_changed(int irq, void *opaque)
 static irqreturn_t vp_vring_interrupt(int irq, void *opaque)
 {
 	struct virtio_pci_device *vp_dev = opaque;
-	struct virtio_pci_vq_info *info;
+	struct virtio_pci_vq *vp_queue;
 	irqreturn_t ret = IRQ_NONE;
 	unsigned long flags;
 
 	spin_lock_irqsave(&vp_dev->lock, flags);
-	list_for_each_entry(info, &vp_dev->virtqueues, node) {
-		if (vring_interrupt(irq, info->vq) == IRQ_HANDLED)
+	list_for_each_entry(vp_queue, &vp_dev->virtqueues, node) {
+		if (vring_interrupt(irq, &vp_queue->vq) == IRQ_HANDLED)
 			ret = IRQ_HANDLED;
 	}
 	spin_unlock_irqrestore(&vp_dev->lock, flags);
@@ -382,9 +388,10 @@ static struct virtqueue *setup_vq(struct virtio_device *vdev, unsigned index,
 				  u16 msix_vec)
 {
 	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
-	struct virtio_pci_vq_info *info;
+	struct virtio_pci_vq *vp_queue;
 	struct virtqueue *vq;
 	unsigned long flags, size;
+	void *queue;
 	u16 num;
 	int err;
 
@@ -398,35 +405,32 @@ static struct virtqueue *setup_vq(struct virtio_device *vdev, unsigned index,
 
 	/* allocate and fill out our structure the represents an active
 	 * queue */
-	info = kmalloc(sizeof(struct virtio_pci_vq_info), GFP_KERNEL);
-	if (!info)
-		return ERR_PTR(-ENOMEM);
-
-	info->queue_index = index;
-	info->num = num;
-	info->msix_vector = msix_vec;
-
 	size = PAGE_ALIGN(vring_size(num, VIRTIO_PCI_VRING_ALIGN));
-	info->queue = alloc_pages_exact(size, GFP_KERNEL|__GFP_ZERO);
-	if (info->queue == NULL) {
+	queue = alloc_pages_exact(size, GFP_KERNEL|__GFP_ZERO);
+	if (queue == NULL) {
 		err = -ENOMEM;
-		goto out_info;
+		goto out;
 	}
 
 	/* activate the queue */
-	iowrite32(virt_to_phys(info->queue) >> VIRTIO_PCI_QUEUE_ADDR_SHIFT,
+	iowrite32(virt_to_phys(queue) >> VIRTIO_PCI_QUEUE_ADDR_SHIFT,
 		  vp_dev->ioaddr + VIRTIO_PCI_QUEUE_PFN);
 
 	/* create the vring */
-	vq = vring_new_virtqueue(info->num, VIRTIO_PCI_VRING_ALIGN, vdev,
-				 true, info->queue, vp_notify, callback, name);
+	vq = vring_new_virtqueue(sizeof(struct virtio_pci_vq),
+				 num, VIRTIO_PCI_VRING_ALIGN, vdev,
+				 true, queue, vp_notify, callback, name);
 	if (!vq) {
 		err = -ENOMEM;
 		goto out_activate_queue;
 	}
 
-	vq->priv = info;
-	info->vq = vq;
+	/* fill out our structure */
+	vp_queue = to_vp_queue(vq);
+	vp_queue->queue_index = index;
+	vp_queue->num = num;
+	vp_queue->msix_vector = msix_vec;
+	vp_queue->queue = queue;
 
 	if (msix_vec != VIRTIO_MSI_NO_VECTOR) {
 		iowrite16(msix_vec, vp_dev->ioaddr + VIRTIO_MSI_QUEUE_VECTOR);
@@ -439,10 +443,10 @@ static struct virtqueue *setup_vq(struct virtio_device *vdev, unsigned index,
 
 	if (callback) {
 		spin_lock_irqsave(&vp_dev->lock, flags);
-		list_add(&info->node, &vp_dev->virtqueues);
+		list_add(&vp_queue->node, &vp_dev->virtqueues);
 		spin_unlock_irqrestore(&vp_dev->lock, flags);
 	} else {
-		INIT_LIST_HEAD(&info->node);
+		INIT_LIST_HEAD(&vp_queue->node);
 	}
 
 	return vq;
@@ -451,23 +455,22 @@ out_assign:
 	vring_del_virtqueue(vq);
 out_activate_queue:
 	iowrite32(0, vp_dev->ioaddr + VIRTIO_PCI_QUEUE_PFN);
-	free_pages_exact(info->queue, size);
-out_info:
-	kfree(info);
+	free_pages_exact(queue, size);
+out:
 	return ERR_PTR(err);
 }
 
 static void vp_del_vq(struct virtqueue *vq)
 {
 	struct virtio_pci_device *vp_dev = to_vp_device(vq->vdev);
-	struct virtio_pci_vq_info *info = vq->priv;
+	struct virtio_pci_vq *vp_queue = to_vp_queue(vq);
 	unsigned long flags, size;
 
 	spin_lock_irqsave(&vp_dev->lock, flags);
-	list_del(&info->node);
+	list_del(&vp_queue->node);
 	spin_unlock_irqrestore(&vp_dev->lock, flags);
 
-	iowrite16(info->queue_index, vp_dev->ioaddr + VIRTIO_PCI_QUEUE_SEL);
+	iowrite16(vp_queue->queue_index, vp_dev->ioaddr + VIRTIO_PCI_QUEUE_SEL);
 
 	if (vp_dev->msix_enabled) {
 		iowrite16(VIRTIO_MSI_NO_VECTOR,
@@ -481,9 +484,8 @@ static void vp_del_vq(struct virtqueue *vq)
 	/* Select and deactivate the queue */
 	iowrite32(0, vp_dev->ioaddr + VIRTIO_PCI_QUEUE_PFN);
 
-	size = PAGE_ALIGN(vring_size(info->num, VIRTIO_PCI_VRING_ALIGN));
-	free_pages_exact(info->queue, size);
-	kfree(info);
+	size = PAGE_ALIGN(vring_size(vp_queue->num, VIRTIO_PCI_VRING_ALIGN));
+	free_pages_exact(vp_queue->queue, size);
 }
 
 /* the config->del_vqs() implementation */
@@ -491,13 +493,13 @@ static void vp_del_vqs(struct virtio_device *vdev)
 {
 	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
 	struct virtqueue *vq, *n;
-	struct virtio_pci_vq_info *info;
+	struct virtio_pci_vq *vp_queue;
 
 	list_for_each_entry_safe(vq, n, &vdev->vqs, list) {
-		info = vq->priv;
+		vp_queue = to_vp_queue(vq);
 		if (vp_dev->per_vq_vectors &&
-			info->msix_vector != VIRTIO_MSI_NO_VECTOR)
-			free_irq(vp_dev->msix_entries[info->msix_vector].vector,
+			vp_queue->msix_vector != VIRTIO_MSI_NO_VECTOR)
+			free_irq(vp_dev->msix_entries[vp_queue->msix_vector].vector,
 				 vq);
 		vp_del_vq(vq);
 	}
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 5aa43c3..8feee6b 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -76,8 +76,6 @@
 
 struct vring_virtqueue
 {
-	struct virtqueue vq;
-
 	/* Actual memory layout for this queue */
 	struct vring vring;
 
@@ -106,6 +104,9 @@ struct vring_virtqueue
 	/* How to notify other side. FIXME: commonalize hcalls! */
 	void (*notify)(struct virtqueue *vq);
 
+	/* Tokens for callbacks. */
+	void **data;
+
 #ifdef DEBUG
 	/* They're supposed to lock for us. */
 	unsigned int in_use;
@@ -115,8 +116,9 @@ struct vring_virtqueue
 	ktime_t last_add_time;
 #endif
 
-	/* Tokens for callbacks. */
-	void *data[];
+	struct virtqueue vq;
+
+	/* Bus-specific virtqueue data goes here.  */
 };
 
 #define to_vvq(_vq) container_of(_vq, struct vring_virtqueue, vq)
@@ -616,7 +618,8 @@ irqreturn_t vring_interrupt(int irq, void *_vq)
 }
 EXPORT_SYMBOL_GPL(vring_interrupt);
 
-struct virtqueue *vring_new_virtqueue(unsigned int num,
+struct virtqueue *vring_new_virtqueue(size_t sz,
+				      unsigned int num,
 				      unsigned int vring_align,
 				      struct virtio_device *vdev,
 				      bool weak_barriers,
@@ -634,7 +637,23 @@ struct virtqueue *vring_new_virtqueue(unsigned int num,
 		return NULL;
 	}
 
-	vq = kmalloc(sizeof(*vq) + sizeof(void *)*num, GFP_KERNEL);
+	BUG_ON(sz < sizeof(vq->vq));
+
+	/* Add room for our own bits, which go before what we return.  */
+	sz += offsetof(struct vring_virtqueue, vq);
+
+	/* Make sure vq->data is properly aligned, since we carve it from
+	 * the same memory block.
+	 */
+	sz = round_up(sz, __alignof__(void *));
+
+	/* Now allocate the whole memory block:
+	 * 1) first goes the vring-specific parts;
+	 * 2) then the generic virtqueue data;
+	 * 3) then the bus-specific parts;
+	 * 4) then the callback tokens, pointed to by vq->data.
+	 */
+	vq = kmalloc(sz + sizeof(void *)*num, GFP_KERNEL);
 	if (!vq)
 		return NULL;
 
@@ -642,6 +661,7 @@ struct virtqueue *vring_new_virtqueue(unsigned int num,
 	vq->vq.callback = callback;
 	vq->vq.vdev = vdev;
 	vq->vq.name = name;
+	vq->data = (void **) ((char *)vq + sz);
 	vq->notify = notify;
 	vq->weak_barriers = weak_barriers;
 	vq->broken = false;
diff --git a/include/linux/virtio.h b/include/linux/virtio.h
index 8efd28a..7e61e2e 100644
--- a/include/linux/virtio.h
+++ b/include/linux/virtio.h
@@ -15,7 +15,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.
- * @priv: a pointer for the virtqueue implementation to use.
+ * @priv: a pointer for the virtio device driver to use.
  */
 struct virtqueue {
 	struct list_head list;

Re: [RFC PATCH] virtio_console: link vq to port with a private pointer in struct virtqueue

[Paolo Bonzini]

> 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?

Something like the following, compile-tested only...

The layout of vring_virtqueue gets a bit complex, with private vring
data _before_ the struct virtqueue and private bus data after it.

The alternative is to make vring_virtqueue public.  It has some
appeal (for example many backends duplicate the num and pages members
in their private data) but overall I think I prefer this.

Anyhow, it is doable, the patches aren't too small but they are easily
split and quite boring.  Shall I pursue this instead?

Paolo

diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
index 2e03d41..047ce01 100644
--- a/drivers/virtio/virtio_pci.c
+++ b/drivers/virtio/virtio_pci.c
@@ -71,10 +71,10 @@ enum {
 	VP_MSIX_VQ_VECTOR = 1,
 };
 
-struct virtio_pci_vq_info
+struct virtio_pci_vq
 {
 	/* the actual virtqueue */
-	struct virtqueue *vq;
+	struct virtqueue vq;
 
 	/* the number of entries in the queue */
 	int num;
@@ -106,6 +106,12 @@ static struct virtio_pci_device *to_vp_device(struct virtio_device *vdev)
 	return container_of(vdev, struct virtio_pci_device, vdev);
 }
 
+/* Convert a generic virtio virtqueue to our structure */
+static struct virtio_pci_vq *to_vp_queue(struct virtqueue *vq)
+{
+	return container_of(vq, struct virtio_pci_vq, vq);
+}
+
 /* virtio config->get_features() implementation */
 static u32 vp_get_features(struct virtio_device *vdev)
 {
@@ -202,11 +208,11 @@ static void vp_reset(struct virtio_device *vdev)
 static void vp_notify(struct virtqueue *vq)
 {
 	struct virtio_pci_device *vp_dev = to_vp_device(vq->vdev);
-	struct virtio_pci_vq_info *info = vq->priv;
+	struct virtio_pci_vq *vp_queue = to_vp_queue(vq);
 
 	/* we write the queue's selector into the notification register to
 	 * signal the other end */
-	iowrite16(info->queue_index, vp_dev->ioaddr + VIRTIO_PCI_QUEUE_NOTIFY);
+	iowrite16(vp_queue->queue_index, vp_dev->ioaddr + VIRTIO_PCI_QUEUE_NOTIFY);
 }
 
 /* Handle a configuration change: Tell driver if it wants to know. */
@@ -226,13 +232,13 @@ static irqreturn_t vp_config_changed(int irq, void *opaque)
 static irqreturn_t vp_vring_interrupt(int irq, void *opaque)
 {
 	struct virtio_pci_device *vp_dev = opaque;
-	struct virtio_pci_vq_info *info;
+	struct virtio_pci_vq *vp_queue;
 	irqreturn_t ret = IRQ_NONE;
 	unsigned long flags;
 
 	spin_lock_irqsave(&vp_dev->lock, flags);
-	list_for_each_entry(info, &vp_dev->virtqueues, node) {
-		if (vring_interrupt(irq, info->vq) == IRQ_HANDLED)
+	list_for_each_entry(vp_queue, &vp_dev->virtqueues, node) {
+		if (vring_interrupt(irq, &vp_queue->vq) == IRQ_HANDLED)
 			ret = IRQ_HANDLED;
 	}
 	spin_unlock_irqrestore(&vp_dev->lock, flags);
@@ -382,9 +388,10 @@ static struct virtqueue *setup_vq(struct virtio_device *vdev, unsigned index,
 				  u16 msix_vec)
 {
 	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
-	struct virtio_pci_vq_info *info;
+	struct virtio_pci_vq *vp_queue;
 	struct virtqueue *vq;
 	unsigned long flags, size;
+	void *queue;
 	u16 num;
 	int err;
 
@@ -398,35 +405,32 @@ static struct virtqueue *setup_vq(struct virtio_device *vdev, unsigned index,
 
 	/* allocate and fill out our structure the represents an active
 	 * queue */
-	info = kmalloc(sizeof(struct virtio_pci_vq_info), GFP_KERNEL);
-	if (!info)
-		return ERR_PTR(-ENOMEM);
-
-	info->queue_index = index;
-	info->num = num;
-	info->msix_vector = msix_vec;
-
 	size = PAGE_ALIGN(vring_size(num, VIRTIO_PCI_VRING_ALIGN));
-	info->queue = alloc_pages_exact(size, GFP_KERNEL|__GFP_ZERO);
-	if (info->queue == NULL) {
+	queue = alloc_pages_exact(size, GFP_KERNEL|__GFP_ZERO);
+	if (queue == NULL) {
 		err = -ENOMEM;
-		goto out_info;
+		goto out;
 	}
 
 	/* activate the queue */
-	iowrite32(virt_to_phys(info->queue) >> VIRTIO_PCI_QUEUE_ADDR_SHIFT,
+	iowrite32(virt_to_phys(queue) >> VIRTIO_PCI_QUEUE_ADDR_SHIFT,
 		  vp_dev->ioaddr + VIRTIO_PCI_QUEUE_PFN);
 
 	/* create the vring */
-	vq = vring_new_virtqueue(info->num, VIRTIO_PCI_VRING_ALIGN, vdev,
-				 true, info->queue, vp_notify, callback, name);
+	vq = vring_new_virtqueue(sizeof(struct virtio_pci_vq),
+				 num, VIRTIO_PCI_VRING_ALIGN, vdev,
+				 true, queue, vp_notify, callback, name);
 	if (!vq) {
 		err = -ENOMEM;
 		goto out_activate_queue;
 	}
 
-	vq->priv = info;
-	info->vq = vq;
+	/* fill out our structure */
+	vp_queue = to_vp_queue(vq);
+	vp_queue->queue_index = index;
+	vp_queue->num = num;
+	vp_queue->msix_vector = msix_vec;
+	vp_queue->queue = queue;
 
 	if (msix_vec != VIRTIO_MSI_NO_VECTOR) {
 		iowrite16(msix_vec, vp_dev->ioaddr + VIRTIO_MSI_QUEUE_VECTOR);
@@ -439,10 +443,10 @@ static struct virtqueue *setup_vq(struct virtio_device *vdev, unsigned index,
 
 	if (callback) {
 		spin_lock_irqsave(&vp_dev->lock, flags);
-		list_add(&info->node, &vp_dev->virtqueues);
+		list_add(&vp_queue->node, &vp_dev->virtqueues);
 		spin_unlock_irqrestore(&vp_dev->lock, flags);
 	} else {
-		INIT_LIST_HEAD(&info->node);
+		INIT_LIST_HEAD(&vp_queue->node);
 	}
 
 	return vq;
@@ -451,23 +455,22 @@ out_assign:
 	vring_del_virtqueue(vq);
 out_activate_queue:
 	iowrite32(0, vp_dev->ioaddr + VIRTIO_PCI_QUEUE_PFN);
-	free_pages_exact(info->queue, size);
-out_info:
-	kfree(info);
+	free_pages_exact(queue, size);
+out:
 	return ERR_PTR(err);
 }
 
 static void vp_del_vq(struct virtqueue *vq)
 {
 	struct virtio_pci_device *vp_dev = to_vp_device(vq->vdev);
-	struct virtio_pci_vq_info *info = vq->priv;
+	struct virtio_pci_vq *vp_queue = to_vp_queue(vq);
 	unsigned long flags, size;
 
 	spin_lock_irqsave(&vp_dev->lock, flags);
-	list_del(&info->node);
+	list_del(&vp_queue->node);
 	spin_unlock_irqrestore(&vp_dev->lock, flags);
 
-	iowrite16(info->queue_index, vp_dev->ioaddr + VIRTIO_PCI_QUEUE_SEL);
+	iowrite16(vp_queue->queue_index, vp_dev->ioaddr + VIRTIO_PCI_QUEUE_SEL);
 
 	if (vp_dev->msix_enabled) {
 		iowrite16(VIRTIO_MSI_NO_VECTOR,
@@ -481,9 +484,8 @@ static void vp_del_vq(struct virtqueue *vq)
 	/* Select and deactivate the queue */
 	iowrite32(0, vp_dev->ioaddr + VIRTIO_PCI_QUEUE_PFN);
 
-	size = PAGE_ALIGN(vring_size(info->num, VIRTIO_PCI_VRING_ALIGN));
-	free_pages_exact(info->queue, size);
-	kfree(info);
+	size = PAGE_ALIGN(vring_size(vp_queue->num, VIRTIO_PCI_VRING_ALIGN));
+	free_pages_exact(vp_queue->queue, size);
 }
 
 /* the config->del_vqs() implementation */
@@ -491,13 +493,13 @@ static void vp_del_vqs(struct virtio_device *vdev)
 {
 	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
 	struct virtqueue *vq, *n;
-	struct virtio_pci_vq_info *info;
+	struct virtio_pci_vq *vp_queue;
 
 	list_for_each_entry_safe(vq, n, &vdev->vqs, list) {
-		info = vq->priv;
+		vp_queue = to_vp_queue(vq);
 		if (vp_dev->per_vq_vectors &&
-			info->msix_vector != VIRTIO_MSI_NO_VECTOR)
-			free_irq(vp_dev->msix_entries[info->msix_vector].vector,
+			vp_queue->msix_vector != VIRTIO_MSI_NO_VECTOR)
+			free_irq(vp_dev->msix_entries[vp_queue->msix_vector].vector,
 				 vq);
 		vp_del_vq(vq);
 	}
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 5aa43c3..8feee6b 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -76,8 +76,6 @@
 
 struct vring_virtqueue
 {
-	struct virtqueue vq;
-
 	/* Actual memory layout for this queue */
 	struct vring vring;
 
@@ -106,6 +104,9 @@ struct vring_virtqueue
 	/* How to notify other side. FIXME: commonalize hcalls! */
 	void (*notify)(struct virtqueue *vq);
 
+	/* Tokens for callbacks. */
+	void **data;
+
 #ifdef DEBUG
 	/* They're supposed to lock for us. */
 	unsigned int in_use;
@@ -115,8 +116,9 @@ struct vring_virtqueue
 	ktime_t last_add_time;
 #endif
 
-	/* Tokens for callbacks. */
-	void *data[];
+	struct virtqueue vq;
+
+	/* Bus-specific virtqueue data goes here.  */
 };
 
 #define to_vvq(_vq) container_of(_vq, struct vring_virtqueue, vq)
@@ -616,7 +618,8 @@ irqreturn_t vring_interrupt(int irq, void *_vq)
 }
 EXPORT_SYMBOL_GPL(vring_interrupt);
 
-struct virtqueue *vring_new_virtqueue(unsigned int num,
+struct virtqueue *vring_new_virtqueue(size_t sz,
+				      unsigned int num,
 				      unsigned int vring_align,
 				      struct virtio_device *vdev,
 				      bool weak_barriers,
@@ -634,7 +637,23 @@ struct virtqueue *vring_new_virtqueue(unsigned int num,
 		return NULL;
 	}
 
-	vq = kmalloc(sizeof(*vq) + sizeof(void *)*num, GFP_KERNEL);
+	BUG_ON(sz < sizeof(vq->vq));
+
+	/* Add room for our own bits, which go before what we return.  */
+	sz += offsetof(struct vring_virtqueue, vq);
+
+	/* Make sure vq->data is properly aligned, since we carve it from
+	 * the same memory block.
+	 */
+	sz = round_up(sz, __alignof__(void *));
+
+	/* Now allocate the whole memory block:
+	 * 1) first goes the vring-specific parts;
+	 * 2) then the generic virtqueue data;
+	 * 3) then the bus-specific parts;
+	 * 4) then the callback tokens, pointed to by vq->data.
+	 */
+	vq = kmalloc(sz + sizeof(void *)*num, GFP_KERNEL);
 	if (!vq)
 		return NULL;
 
@@ -642,6 +661,7 @@ struct virtqueue *vring_new_virtqueue(unsigned int num,
 	vq->vq.callback = callback;
 	vq->vq.vdev = vdev;
 	vq->vq.name = name;
+	vq->data = (void **) ((char *)vq + sz);
 	vq->notify = notify;
 	vq->weak_barriers = weak_barriers;
 	vq->broken = false;
diff --git a/include/linux/virtio.h b/include/linux/virtio.h
index 8efd28a..7e61e2e 100644
--- a/include/linux/virtio.h
+++ b/include/linux/virtio.h
@@ -15,7 +15,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.
- * @priv: a pointer for the virtqueue implementation to use.
+ * @priv: a pointer for the virtio device driver to use.
  */
 struct virtqueue {
 	struct list_head list;

Re: [RFC PATCH] virtio_console: link vq to port with a private pointer in struct virtqueue

[Michael S. Tsirkin]

On Tue, May 08, 2012 at 11:45:55AM +0200, Paolo Bonzini wrote:
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -76,8 +76,6 @@
>  
>  struct vring_virtqueue
>  {
> -	struct virtqueue vq;
> -
>  	/* Actual memory layout for this queue */
>  	struct vring vring;
>  
> @@ -106,6 +104,9 @@ struct vring_virtqueue
>  	/* How to notify other side. FIXME: commonalize hcalls! */
>  	void (*notify)(struct virtqueue *vq);
>  
> +	/* Tokens for callbacks. */
> +	void **data;
> +
>  #ifdef DEBUG
>  	/* They're supposed to lock for us. */
>  	unsigned int in_use;
> @@ -115,8 +116,9 @@ struct vring_virtqueue
>  	ktime_t last_add_time;
>  #endif
>  
> -	/* Tokens for callbacks. */
> -	void *data[];
> +	struct virtqueue vq;
> +
> +	/* Bus-specific virtqueue data goes here.  */
>  };
>  
>  #define to_vvq(_vq) container_of(_vq, struct vring_virtqueue, vq)


This moves the data out of line and the bus specific stuff inline.
But bus accesses only happen on io and interrupt which are already
slow, while data accesses happen on fast path.

>From that POV this looks like a wrong thing to do.

-- 
MST

Re: [RFC PATCH] virtio_console: link vq to port with a private pointer in struct virtqueue

[Michael S. Tsirkin]

On Tue, May 08, 2012 at 11:45:55AM +0200, Paolo Bonzini wrote:
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -76,8 +76,6 @@
>  
>  struct vring_virtqueue
>  {
> -	struct virtqueue vq;
> -
>  	/* Actual memory layout for this queue */
>  	struct vring vring;
>  
> @@ -106,6 +104,9 @@ struct vring_virtqueue
>  	/* How to notify other side. FIXME: commonalize hcalls! */
>  	void (*notify)(struct virtqueue *vq);
>  
> +	/* Tokens for callbacks. */
> +	void **data;
> +
>  #ifdef DEBUG
>  	/* They're supposed to lock for us. */
>  	unsigned int in_use;
> @@ -115,8 +116,9 @@ struct vring_virtqueue
>  	ktime_t last_add_time;
>  #endif
>  
> -	/* Tokens for callbacks. */
> -	void *data[];
> +	struct virtqueue vq;
> +
> +	/* Bus-specific virtqueue data goes here.  */
>  };
>  
>  #define to_vvq(_vq) container_of(_vq, struct vring_virtqueue, vq)


This moves the data out of line and the bus specific stuff inline.
But bus accesses only happen on io and interrupt which are already
slow, while data accesses happen on fast path.

From that POV this looks like a wrong thing to do.

-- 
MST

[PATCH untested] virtio: allocate extra memory before the ring ( was Re: [RFC PATCH] virtio_console: link vq to port with a private) pointer in struct virtqueue

[Michael S. Tsirkin]

On Tue, May 08, 2012 at 11:45:55AM +0200, Paolo Bonzini wrote:
> > 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?
> 
> Something like the following, compile-tested only...
> 
> The layout of vring_virtqueue gets a bit complex, with private vring
> data _before_ the struct virtqueue and private bus data after it.

If we want to do this, the right thing IMO is keeping the data inline
and at fixed offset and that means at the end.

And it's not a problem if virtqueue is exactly at start of
vring_virtqueue: we just need to allocate a bit more at start, and
offset when we free.  Here's how I would do this: first apply patch
below that adds the offset parameter, then update all transports, one
patch at a time to not use priv pointer, finally you can repurpose priv
pointer to let devices use it.
As a bonus we get small incremental patches.

Patch below is completely untested, just to give you the idea.
By the way, we need to document vring_new_virtqueue/vring_del_virtqueue.

----------------------------------------------->

virtio: allocate extra memory before the ring

This let transports put their data inline instead
of indirect acces through priv pointer.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

---

diff --git a/drivers/lguest/lguest_device.c b/drivers/lguest/lguest_device.c
index 9e8388e..555b5d5 100644
--- a/drivers/lguest/lguest_device.c
+++ b/drivers/lguest/lguest_device.c
@@ -296,7 +296,7 @@ static struct virtqueue *lg_find_vq(struct virtio_device *vdev,
 	 * to 'true': the host just a(nother) SMP CPU, so we only need inter-cpu
 	 * barriers.
 	 */
-	vq = vring_new_virtqueue(lvq->config.num, LGUEST_VRING_ALIGN, vdev,
+	vq = vring_new_virtqueue(0, lvq->config.num, LGUEST_VRING_ALIGN, vdev,
 				 true, lvq->pages, lg_notify, callback, name);
 	if (!vq) {
 		err = -ENOMEM;
@@ -331,7 +331,7 @@ static struct virtqueue *lg_find_vq(struct virtio_device *vdev,
 free_desc:
 	irq_free_desc(lvq->config.irq);
 destroy_vring:
-	vring_del_virtqueue(vq);
+	vring_del_virtqueue(vq, 0);
 unmap:
 	lguest_unmap(lvq->pages);
 free_lvq:
@@ -348,7 +348,7 @@ static void lg_del_vq(struct virtqueue *vq)
 	/* Release the interrupt */
 	free_irq(lvq->config.irq, vq);
 	/* Tell virtio_ring.c to free the virtqueue. */
-	vring_del_virtqueue(vq);
+	vring_del_virtqueue(vq, 0);
 	/* Unmap the pages containing the ring. */
 	lguest_unmap(lvq->pages);
 	/* Free our own queue information. */
diff --git a/drivers/remoteproc/remoteproc_virtio.c b/drivers/remoteproc/remoteproc_virtio.c
index ecf6121..cc714af 100644
--- a/drivers/remoteproc/remoteproc_virtio.c
+++ b/drivers/remoteproc/remoteproc_virtio.c
@@ -99,7 +99,7 @@ static struct virtqueue *rp_find_vq(struct virtio_device *vdev,
 	 * Create the new vq, and tell virtio we're not interested in
 	 * the 'weak' smp barriers, since we're talking with a real device.
 	 */
-	vq = vring_new_virtqueue(len, rvring->align, vdev, false, addr,
+	vq = vring_new_virtqueue(0, len, rvring->align, vdev, false, addr,
 					rproc_virtio_notify, callback, name);
 	if (!vq) {
 		dev_err(rproc->dev, "vring_new_virtqueue %s failed\n", name);
@@ -124,7 +124,7 @@ static void rproc_virtio_del_vqs(struct virtio_device *vdev)
 	list_for_each_entry_safe(vq, n, &vdev->vqs, list) {
 		rvring = vq->priv;
 		rvring->vq = NULL;
-		vring_del_virtqueue(vq);
+		vring_del_virtqueue(vq, 0);
 	}
 }
 
diff --git a/drivers/s390/kvm/kvm_virtio.c b/drivers/s390/kvm/kvm_virtio.c
index d74e9ae..e51fcdf 100644
--- a/drivers/s390/kvm/kvm_virtio.c
+++ b/drivers/s390/kvm/kvm_virtio.c
@@ -197,7 +197,7 @@ static struct virtqueue *kvm_find_vq(struct virtio_device *vdev,
 	if (err)
 		goto out;
 
-	vq = vring_new_virtqueue(config->num, KVM_S390_VIRTIO_RING_ALIGN,
+	vq = vring_new_virtqueue(0, config->num, KVM_S390_VIRTIO_RING_ALIGN,
 				 vdev, true, (void *) config->address,
 				 kvm_notify, callback, name);
 	if (!vq) {
@@ -225,7 +225,7 @@ static void kvm_del_vq(struct virtqueue *vq)
 {
 	struct kvm_vqconfig *config = vq->priv;
 
-	vring_del_virtqueue(vq);
+	vring_del_virtqueue(vq, 0);
 	vmem_remove_mapping(config->address,
 			    vring_size(config->num,
 				       KVM_S390_VIRTIO_RING_ALIGN));
diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
index 01d6dc2..98e56dd 100644
--- a/drivers/virtio/virtio_mmio.c
+++ b/drivers/virtio/virtio_mmio.c
@@ -229,7 +229,7 @@ static void vm_del_vq(struct virtqueue *vq)
 	list_del(&info->node);
 	spin_unlock_irqrestore(&vm_dev->lock, flags);
 
-	vring_del_virtqueue(vq);
+	vring_del_virtqueue(vq, 0);
 
 	/* Select and deactivate the queue */
 	writel(info->queue_index, vm_dev->base + VIRTIO_MMIO_QUEUE_SEL);
@@ -310,7 +310,7 @@ static struct virtqueue *vm_setup_vq(struct virtio_device *vdev, unsigned index,
 			vm_dev->base + VIRTIO_MMIO_QUEUE_PFN);
 
 	/* Create the vring */
-	vq = vring_new_virtqueue(info->num, VIRTIO_MMIO_VRING_ALIGN, vdev,
+	vq = vring_new_virtqueue(0, info->num, VIRTIO_MMIO_VRING_ALIGN, vdev,
 				 true, info->queue, vm_notify, callback, name);
 	if (!vq) {
 		err = -ENOMEM;
diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
index 2e03d41..3c3eea9 100644
--- a/drivers/virtio/virtio_pci.c
+++ b/drivers/virtio/virtio_pci.c
@@ -418,7 +418,7 @@ static struct virtqueue *setup_vq(struct virtio_device *vdev, unsigned index,
 		  vp_dev->ioaddr + VIRTIO_PCI_QUEUE_PFN);
 
 	/* create the vring */
-	vq = vring_new_virtqueue(info->num, VIRTIO_PCI_VRING_ALIGN, vdev,
+	vq = vring_new_virtqueue(0, info->num, VIRTIO_PCI_VRING_ALIGN, vdev,
 				 true, info->queue, vp_notify, callback, name);
 	if (!vq) {
 		err = -ENOMEM;
@@ -448,7 +448,7 @@ static struct virtqueue *setup_vq(struct virtio_device *vdev, unsigned index,
 	return vq;
 
 out_assign:
-	vring_del_virtqueue(vq);
+	vring_del_virtqueue(vq, 0);
 out_activate_queue:
 	iowrite32(0, vp_dev->ioaddr + VIRTIO_PCI_QUEUE_PFN);
 	free_pages_exact(info->queue, size);
@@ -476,7 +476,7 @@ static void vp_del_vq(struct virtqueue *vq)
 		ioread8(vp_dev->ioaddr + VIRTIO_PCI_ISR);
 	}
 
-	vring_del_virtqueue(vq);
+	vring_del_virtqueue(vq, 0);
 
 	/* Select and deactivate the queue */
 	iowrite32(0, vp_dev->ioaddr + VIRTIO_PCI_QUEUE_PFN);
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 5aa43c3..688859b 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -616,7 +616,8 @@ irqreturn_t vring_interrupt(int irq, void *_vq)
 }
 EXPORT_SYMBOL_GPL(vring_interrupt);
 
-struct virtqueue *vring_new_virtqueue(unsigned int num,
+struct virtqueue *vring_new_virtqueue(unsigned int offset,
+				      unsigned int num,
 				      unsigned int vring_align,
 				      struct virtio_device *vdev,
 				      bool weak_barriers,
@@ -627,6 +628,7 @@ struct virtqueue *vring_new_virtqueue(unsigned int num,
 {
 	struct vring_virtqueue *vq;
 	unsigned int i;
+	void *buf;
 
 	/* We assume num is a power of 2. */
 	if (num & (num - 1)) {
@@ -634,9 +636,11 @@ struct virtqueue *vring_new_virtqueue(unsigned int num,
 		return NULL;
 	}
 
-	vq = kmalloc(sizeof(*vq) + sizeof(void *)*num, GFP_KERNEL);
-	if (!vq)
+	buf = kmalloc(offset + sizeof(*vq) + sizeof(void *)*num, GFP_KERNEL);
+	if (!buf)
 		return NULL;
+	BUG_ON(offset % __alignof__(*vq));
+	vq = buf + offset;
 
 	vring_init(&vq->vring, num, pages, vring_align);
 	vq->vq.callback = callback;
@@ -669,14 +673,17 @@ struct virtqueue *vring_new_virtqueue(unsigned int num,
 	}
 	vq->data[i] = NULL;
 
+	/* Callers can overwrite off bytes before the returned pointer. */
+	BUILD_BUG_ON(offset_of(typeof(*vq), vq));
 	return &vq->vq;
 }
 EXPORT_SYMBOL_GPL(vring_new_virtqueue);
 
-void vring_del_virtqueue(struct virtqueue *vq)
+void vring_del_virtqueue(struct virtqueue *vq, unsigned int offset)
 {
+	void *buf = to_vvq(vq);
 	list_del(&vq->list);
-	kfree(to_vvq(vq));
+	kfree(buf - offset);
 }
 EXPORT_SYMBOL_GPL(vring_del_virtqueue);
 
diff --git a/include/linux/virtio_ring.h b/include/linux/virtio_ring.h
index e338730..b91b8c1 100644
--- a/include/linux/virtio_ring.h
+++ b/include/linux/virtio_ring.h
@@ -165,7 +165,8 @@ static inline int vring_need_event(__u16 event_idx, __u16 new_idx, __u16 old)
 struct virtio_device;
 struct virtqueue;
 
-struct virtqueue *vring_new_virtqueue(unsigned int num,
+struct virtqueue *vring_new_virtqueue(unsigned int offset,
+				      unsigned int num,
 				      unsigned int vring_align,
 				      struct virtio_device *vdev,
 				      bool weak_barriers,
diff --git a/tools/virtio/virtio_test.c b/tools/virtio/virtio_test.c
index 6bf95f9..1a24799 100644
--- a/tools/virtio/virtio_test.c
+++ b/tools/virtio/virtio_test.c
@@ -92,7 +92,7 @@ static void vq_info_add(struct vdev_info *dev, int num)
 	assert(r >= 0);
 	memset(info->ring, 0, vring_size(num, 4096));
 	vring_init(&info->vring, num, info->ring, 4096);
-	info->vq = vring_new_virtqueue(info->vring.num, 4096, &dev->vdev,
+	info->vq = vring_new_virtqueue(0, info->vring.num, 4096, &dev->vdev,
 				       true, info->ring,
 				       vq_notify, vq_callback, "test");
 	assert(info->vq);

[PATCH untested] virtio: allocate extra memory before the ring ( was Re: [RFC PATCH] virtio_console: link vq to port with a private) pointer in struct virtqueue

[Michael S. Tsirkin]

On Tue, May 08, 2012 at 11:45:55AM +0200, Paolo Bonzini wrote:
> > 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?
> 
> Something like the following, compile-tested only...
> 
> The layout of vring_virtqueue gets a bit complex, with private vring
> data _before_ the struct virtqueue and private bus data after it.

If we want to do this, the right thing IMO is keeping the data inline
and at fixed offset and that means at the end.

And it's not a problem if virtqueue is exactly at start of
vring_virtqueue: we just need to allocate a bit more at start, and
offset when we free.  Here's how I would do this: first apply patch
below that adds the offset parameter, then update all transports, one
patch at a time to not use priv pointer, finally you can repurpose priv
pointer to let devices use it.
As a bonus we get small incremental patches.

Patch below is completely untested, just to give you the idea.
By the way, we need to document vring_new_virtqueue/vring_del_virtqueue.

----------------------------------------------->

virtio: allocate extra memory before the ring

This let transports put their data inline instead
of indirect acces through priv pointer.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

---

diff --git a/drivers/lguest/lguest_device.c b/drivers/lguest/lguest_device.c
index 9e8388e..555b5d5 100644
--- a/drivers/lguest/lguest_device.c
+++ b/drivers/lguest/lguest_device.c
@@ -296,7 +296,7 @@ static struct virtqueue *lg_find_vq(struct virtio_device *vdev,
 	 * to 'true': the host just a(nother) SMP CPU, so we only need inter-cpu
 	 * barriers.
 	 */
-	vq = vring_new_virtqueue(lvq->config.num, LGUEST_VRING_ALIGN, vdev,
+	vq = vring_new_virtqueue(0, lvq->config.num, LGUEST_VRING_ALIGN, vdev,
 				 true, lvq->pages, lg_notify, callback, name);
 	if (!vq) {
 		err = -ENOMEM;
@@ -331,7 +331,7 @@ static struct virtqueue *lg_find_vq(struct virtio_device *vdev,
 free_desc:
 	irq_free_desc(lvq->config.irq);
 destroy_vring:
-	vring_del_virtqueue(vq);
+	vring_del_virtqueue(vq, 0);
 unmap:
 	lguest_unmap(lvq->pages);
 free_lvq:
@@ -348,7 +348,7 @@ static void lg_del_vq(struct virtqueue *vq)
 	/* Release the interrupt */
 	free_irq(lvq->config.irq, vq);
 	/* Tell virtio_ring.c to free the virtqueue. */
-	vring_del_virtqueue(vq);
+	vring_del_virtqueue(vq, 0);
 	/* Unmap the pages containing the ring. */
 	lguest_unmap(lvq->pages);
 	/* Free our own queue information. */
diff --git a/drivers/remoteproc/remoteproc_virtio.c b/drivers/remoteproc/remoteproc_virtio.c
index ecf6121..cc714af 100644
--- a/drivers/remoteproc/remoteproc_virtio.c
+++ b/drivers/remoteproc/remoteproc_virtio.c
@@ -99,7 +99,7 @@ static struct virtqueue *rp_find_vq(struct virtio_device *vdev,
 	 * Create the new vq, and tell virtio we're not interested in
 	 * the 'weak' smp barriers, since we're talking with a real device.
 	 */
-	vq = vring_new_virtqueue(len, rvring->align, vdev, false, addr,
+	vq = vring_new_virtqueue(0, len, rvring->align, vdev, false, addr,
 					rproc_virtio_notify, callback, name);
 	if (!vq) {
 		dev_err(rproc->dev, "vring_new_virtqueue %s failed\n", name);
@@ -124,7 +124,7 @@ static void rproc_virtio_del_vqs(struct virtio_device *vdev)
 	list_for_each_entry_safe(vq, n, &vdev->vqs, list) {
 		rvring = vq->priv;
 		rvring->vq = NULL;
-		vring_del_virtqueue(vq);
+		vring_del_virtqueue(vq, 0);
 	}
 }
 
diff --git a/drivers/s390/kvm/kvm_virtio.c b/drivers/s390/kvm/kvm_virtio.c
index d74e9ae..e51fcdf 100644
--- a/drivers/s390/kvm/kvm_virtio.c
+++ b/drivers/s390/kvm/kvm_virtio.c
@@ -197,7 +197,7 @@ static struct virtqueue *kvm_find_vq(struct virtio_device *vdev,
 	if (err)
 		goto out;
 
-	vq = vring_new_virtqueue(config->num, KVM_S390_VIRTIO_RING_ALIGN,
+	vq = vring_new_virtqueue(0, config->num, KVM_S390_VIRTIO_RING_ALIGN,
 				 vdev, true, (void *) config->address,
 				 kvm_notify, callback, name);
 	if (!vq) {
@@ -225,7 +225,7 @@ static void kvm_del_vq(struct virtqueue *vq)
 {
 	struct kvm_vqconfig *config = vq->priv;
 
-	vring_del_virtqueue(vq);
+	vring_del_virtqueue(vq, 0);
 	vmem_remove_mapping(config->address,
 			    vring_size(config->num,
 				       KVM_S390_VIRTIO_RING_ALIGN));
diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
index 01d6dc2..98e56dd 100644
--- a/drivers/virtio/virtio_mmio.c
+++ b/drivers/virtio/virtio_mmio.c
@@ -229,7 +229,7 @@ static void vm_del_vq(struct virtqueue *vq)
 	list_del(&info->node);
 	spin_unlock_irqrestore(&vm_dev->lock, flags);
 
-	vring_del_virtqueue(vq);
+	vring_del_virtqueue(vq, 0);
 
 	/* Select and deactivate the queue */
 	writel(info->queue_index, vm_dev->base + VIRTIO_MMIO_QUEUE_SEL);
@@ -310,7 +310,7 @@ static struct virtqueue *vm_setup_vq(struct virtio_device *vdev, unsigned index,
 			vm_dev->base + VIRTIO_MMIO_QUEUE_PFN);
 
 	/* Create the vring */
-	vq = vring_new_virtqueue(info->num, VIRTIO_MMIO_VRING_ALIGN, vdev,
+	vq = vring_new_virtqueue(0, info->num, VIRTIO_MMIO_VRING_ALIGN, vdev,
 				 true, info->queue, vm_notify, callback, name);
 	if (!vq) {
 		err = -ENOMEM;
diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
index 2e03d41..3c3eea9 100644
--- a/drivers/virtio/virtio_pci.c
+++ b/drivers/virtio/virtio_pci.c
@@ -418,7 +418,7 @@ static struct virtqueue *setup_vq(struct virtio_device *vdev, unsigned index,
 		  vp_dev->ioaddr + VIRTIO_PCI_QUEUE_PFN);
 
 	/* create the vring */
-	vq = vring_new_virtqueue(info->num, VIRTIO_PCI_VRING_ALIGN, vdev,
+	vq = vring_new_virtqueue(0, info->num, VIRTIO_PCI_VRING_ALIGN, vdev,
 				 true, info->queue, vp_notify, callback, name);
 	if (!vq) {
 		err = -ENOMEM;
@@ -448,7 +448,7 @@ static struct virtqueue *setup_vq(struct virtio_device *vdev, unsigned index,
 	return vq;
 
 out_assign:
-	vring_del_virtqueue(vq);
+	vring_del_virtqueue(vq, 0);
 out_activate_queue:
 	iowrite32(0, vp_dev->ioaddr + VIRTIO_PCI_QUEUE_PFN);
 	free_pages_exact(info->queue, size);
@@ -476,7 +476,7 @@ static void vp_del_vq(struct virtqueue *vq)
 		ioread8(vp_dev->ioaddr + VIRTIO_PCI_ISR);
 	}
 
-	vring_del_virtqueue(vq);
+	vring_del_virtqueue(vq, 0);
 
 	/* Select and deactivate the queue */
 	iowrite32(0, vp_dev->ioaddr + VIRTIO_PCI_QUEUE_PFN);
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 5aa43c3..688859b 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -616,7 +616,8 @@ irqreturn_t vring_interrupt(int irq, void *_vq)
 }
 EXPORT_SYMBOL_GPL(vring_interrupt);
 
-struct virtqueue *vring_new_virtqueue(unsigned int num,
+struct virtqueue *vring_new_virtqueue(unsigned int offset,
+				      unsigned int num,
 				      unsigned int vring_align,
 				      struct virtio_device *vdev,
 				      bool weak_barriers,
@@ -627,6 +628,7 @@ struct virtqueue *vring_new_virtqueue(unsigned int num,
 {
 	struct vring_virtqueue *vq;
 	unsigned int i;
+	void *buf;
 
 	/* We assume num is a power of 2. */
 	if (num & (num - 1)) {
@@ -634,9 +636,11 @@ struct virtqueue *vring_new_virtqueue(unsigned int num,
 		return NULL;
 	}
 
-	vq = kmalloc(sizeof(*vq) + sizeof(void *)*num, GFP_KERNEL);
-	if (!vq)
+	buf = kmalloc(offset + sizeof(*vq) + sizeof(void *)*num, GFP_KERNEL);
+	if (!buf)
 		return NULL;
+	BUG_ON(offset % __alignof__(*vq));
+	vq = buf + offset;
 
 	vring_init(&vq->vring, num, pages, vring_align);
 	vq->vq.callback = callback;
@@ -669,14 +673,17 @@ struct virtqueue *vring_new_virtqueue(unsigned int num,
 	}
 	vq->data[i] = NULL;
 
+	/* Callers can overwrite off bytes before the returned pointer. */
+	BUILD_BUG_ON(offset_of(typeof(*vq), vq));
 	return &vq->vq;
 }
 EXPORT_SYMBOL_GPL(vring_new_virtqueue);
 
-void vring_del_virtqueue(struct virtqueue *vq)
+void vring_del_virtqueue(struct virtqueue *vq, unsigned int offset)
 {
+	void *buf = to_vvq(vq);
 	list_del(&vq->list);
-	kfree(to_vvq(vq));
+	kfree(buf - offset);
 }
 EXPORT_SYMBOL_GPL(vring_del_virtqueue);
 
diff --git a/include/linux/virtio_ring.h b/include/linux/virtio_ring.h
index e338730..b91b8c1 100644
--- a/include/linux/virtio_ring.h
+++ b/include/linux/virtio_ring.h
@@ -165,7 +165,8 @@ static inline int vring_need_event(__u16 event_idx, __u16 new_idx, __u16 old)
 struct virtio_device;
 struct virtqueue;
 
-struct virtqueue *vring_new_virtqueue(unsigned int num,
+struct virtqueue *vring_new_virtqueue(unsigned int offset,
+				      unsigned int num,
 				      unsigned int vring_align,
 				      struct virtio_device *vdev,
 				      bool weak_barriers,
diff --git a/tools/virtio/virtio_test.c b/tools/virtio/virtio_test.c
index 6bf95f9..1a24799 100644
--- a/tools/virtio/virtio_test.c
+++ b/tools/virtio/virtio_test.c
@@ -92,7 +92,7 @@ static void vq_info_add(struct vdev_info *dev, int num)
 	assert(r >= 0);
 	memset(info->ring, 0, vring_size(num, 4096));
 	vring_init(&info->vring, num, info->ring, 4096);
-	info->vq = vring_new_virtqueue(info->vring.num, 4096, &dev->vdev,
+	info->vq = vring_new_virtqueue(0, info->vring.num, 4096, &dev->vdev,
 				       true, info->ring,
 				       vq_notify, vq_callback, "test");
 	assert(info->vq);

Re: [PATCH untested] virtio: allocate extra memory before the ring ( was Re: [RFC PATCH] virtio_console: link vq to port with a private) pointer in struct virtqueue

[Paolo Bonzini]

> And it's not a problem if virtqueue is exactly at start of
> vring_virtqueue: we just need to allocate a bit more at start, and
> offset when we free.  Here's how I would do this: first apply patch
> below that adds the offset parameter, then update all transports, one
> patch at a time to not use priv pointer, finally you can repurpose
> priv pointer to let devices use it.

I like it, though I prefer to have the new arguments as sizeof()
rather than offsets.

I'll pick up this and test it.

> As a bonus we get small incremental patches.

FWIW, that's possible with the patch I posted too.

Paolo

Re: [PATCH untested] virtio: allocate extra memory before the ring ( was Re: [RFC PATCH] virtio_console: link vq to port with a private) pointer in struct virtqueue

[Paolo Bonzini]

> And it's not a problem if virtqueue is exactly at start of
> vring_virtqueue: we just need to allocate a bit more at start, and
> offset when we free.  Here's how I would do this: first apply patch
> below that adds the offset parameter, then update all transports, one
> patch at a time to not use priv pointer, finally you can repurpose
> priv pointer to let devices use it.

I like it, though I prefer to have the new arguments as sizeof()
rather than offsets.

I'll pick up this and test it.

> As a bonus we get small incremental patches.

FWIW, that's possible with the patch I posted too.

Paolo

Re: [PATCH untested] virtio: allocate extra memory before the ring ( was Re: [RFC PATCH] virtio_console: link vq to port with a private) pointer in struct virtqueue

[Michael S. Tsirkin]

> virtio: allocate extra memory before the ring
> 
> This let transports put their data inline instead
> of indirect acces through priv pointer.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

And transports would do something like the below to avoid
using priv.  Warning: completely untested.

------------------------------>
    virtio_pci: put vq info structure inline
    
    vq->priv is now unused by virtio_pci.
    
    Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

---

diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
index 3c3eea9..4092006 100644
--- a/drivers/virtio/virtio_pci.c
+++ b/drivers/virtio/virtio_pci.c
@@ -73,9 +73,6 @@ enum {
 
 struct virtio_pci_vq_info
 {
-	/* the actual virtqueue */
-	struct virtqueue *vq;
-
 	/* the number of entries in the queue */
 	int num;
 
@@ -90,8 +87,15 @@ struct virtio_pci_vq_info
 
 	/* MSI-X vector (or none) */
 	unsigned msix_vector;
+
+	/* the actual virtqueue. Note: must be the last field */
+	struct virtqueue vq;
 };
 
+#define to_vpinfo(vq) container_of(info, struct virtio_pci_vq_info, vq)
+/* How much space we need to reserve before struct virtqueue */
+#define VPINFO_PRIV offsetof(struct virtio_pci_vq_info, vq)
+
 /* Qumranet donated their vendor ID for devices 0x1000 thru 0x10FF. */
 static struct pci_device_id virtio_pci_id_table[] = {
 	{ 0x1af4, PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0 },
@@ -202,7 +206,7 @@ static void vp_reset(struct virtio_device *vdev)
 static void vp_notify(struct virtqueue *vq)
 {
 	struct virtio_pci_device *vp_dev = to_vp_device(vq->vdev);
-	struct virtio_pci_vq_info *info = vq->priv;
+	struct virtio_pci_vq_info *info = to_vpinfo(vq);
 
 	/* we write the queue's selector into the notification register to
 	 * signal the other end */
@@ -232,7 +236,7 @@ static irqreturn_t vp_vring_interrupt(int irq, void *opaque)
 
 	spin_lock_irqsave(&vp_dev->lock, flags);
 	list_for_each_entry(info, &vp_dev->virtqueues, node) {
-		if (vring_interrupt(irq, info->vq) == IRQ_HANDLED)
+		if (vring_interrupt(irq, &info->vq) == IRQ_HANDLED)
 			ret = IRQ_HANDLED;
 	}
 	spin_unlock_irqrestore(&vp_dev->lock, flags);
@@ -385,6 +389,7 @@ static struct virtqueue *setup_vq(struct virtio_device *vdev, unsigned index,
 	struct virtio_pci_vq_info *info;
 	struct virtqueue *vq;
 	unsigned long flags, size;
+	void *queue;
 	u16 num;
 	int err;
 
@@ -398,35 +403,34 @@ static struct virtqueue *setup_vq(struct virtio_device *vdev, unsigned index,
 
 	/* allocate and fill out our structure the represents an active
 	 * queue */
-	info = kmalloc(sizeof(struct virtio_pci_vq_info), GFP_KERNEL);
-	if (!info)
-		return ERR_PTR(-ENOMEM);
-
-	info->queue_index = index;
-	info->num = num;
-	info->msix_vector = msix_vec;
 
 	size = PAGE_ALIGN(vring_size(num, VIRTIO_PCI_VRING_ALIGN));
-	info->queue = alloc_pages_exact(size, GFP_KERNEL|__GFP_ZERO);
-	if (info->queue == NULL) {
+	queue = alloc_pages_exact(size, GFP_KERNEL|__GFP_ZERO);
+	if (queue == NULL) {
 		err = -ENOMEM;
 		goto out_info;
 	}
 
 	/* activate the queue */
-	iowrite32(virt_to_phys(info->queue) >> VIRTIO_PCI_QUEUE_ADDR_SHIFT,
+	iowrite32(virt_to_phys(queue) >> VIRTIO_PCI_QUEUE_ADDR_SHIFT,
 		  vp_dev->ioaddr + VIRTIO_PCI_QUEUE_PFN);
 
-	/* create the vring */
-	vq = vring_new_virtqueue(0, info->num, VIRTIO_PCI_VRING_ALIGN, vdev,
-				 true, info->queue, vp_notify, callback, name);
+	/* We can store transport specific data before the vq field.
+	 * Make sure we don't have any fields after it. */
+	BUILD_BUG_ON(VPINFO_PRIV + sizeof info->vq != sizeof *info);
+
+	vq = vring_new_virtqueue(VPINFO_PRIV, num, VIRTIO_PCI_VRING_ALIGN, vdev,
+				 true, queue, vp_notify, callback, name);
 	if (!vq) {
 		err = -ENOMEM;
 		goto out_activate_queue;
 	}
 
-	vq->priv = info;
-	info->vq = vq;
+	info = to_vpinfo(vq);
+
+	info->queue_index = index;
+	info->num = num;
+	info->msix_vector = msix_vec;
 
 	if (msix_vec != VIRTIO_MSI_NO_VECTOR) {
 		iowrite16(msix_vec, vp_dev->ioaddr + VIRTIO_MSI_QUEUE_VECTOR);
@@ -448,20 +452,21 @@ static struct virtqueue *setup_vq(struct virtio_device *vdev, unsigned index,
 	return vq;
 
 out_assign:
-	vring_del_virtqueue(vq, 0);
+	vring_del_virtqueue(vq, VPINFO_PRIV);
 out_activate_queue:
 	iowrite32(0, vp_dev->ioaddr + VIRTIO_PCI_QUEUE_PFN);
 	free_pages_exact(info->queue, size);
 out_info:
-	kfree(info);
 	return ERR_PTR(err);
 }
 
 static void vp_del_vq(struct virtqueue *vq)
 {
 	struct virtio_pci_device *vp_dev = to_vp_device(vq->vdev);
-	struct virtio_pci_vq_info *info = vq->priv;
+	struct virtio_pci_vq_info *info = to_vpinfo(vq);
 	unsigned long flags, size;
+	void *queue = info->queue;
+	u16 num = info->num;
 
 	spin_lock_irqsave(&vp_dev->lock, flags);
 	list_del(&info->node);
@@ -476,14 +481,13 @@ static void vp_del_vq(struct virtqueue *vq)
 		ioread8(vp_dev->ioaddr + VIRTIO_PCI_ISR);
 	}
 
-	vring_del_virtqueue(vq, 0);
+	vring_del_virtqueue(vq, VPINFO_PRIV);
 
 	/* Select and deactivate the queue */
 	iowrite32(0, vp_dev->ioaddr + VIRTIO_PCI_QUEUE_PFN);
 
 	size = PAGE_ALIGN(vring_size(info->num, VIRTIO_PCI_VRING_ALIGN));
-	free_pages_exact(info->queue, size);
-	kfree(info);
+	free_pages_exact(queue, size);
 }
 
 /* the config->del_vqs() implementation */
@@ -494,7 +498,7 @@ static void vp_del_vqs(struct virtio_device *vdev)
 	struct virtio_pci_vq_info *info;
 
 	list_for_each_entry_safe(vq, n, &vdev->vqs, list) {
-		info = vq->priv;
+		info = to_vpinfo(vq);
 		if (vp_dev->per_vq_vectors &&
 			info->msix_vector != VIRTIO_MSI_NO_VECTOR)
 			free_irq(vp_dev->msix_entries[info->msix_vector].vector,

Re: [PATCH untested] virtio: allocate extra memory before the ring ( was Re: [RFC PATCH] virtio_console: link vq to port with a private) pointer in struct virtqueue

[Michael S. Tsirkin]

> virtio: allocate extra memory before the ring
> 
> This let transports put their data inline instead
> of indirect acces through priv pointer.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

And transports would do something like the below to avoid
using priv.  Warning: completely untested.

------------------------------>
    virtio_pci: put vq info structure inline
    
    vq->priv is now unused by virtio_pci.
    
    Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

---

diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
index 3c3eea9..4092006 100644
--- a/drivers/virtio/virtio_pci.c
+++ b/drivers/virtio/virtio_pci.c
@@ -73,9 +73,6 @@ enum {
 
 struct virtio_pci_vq_info
 {
-	/* the actual virtqueue */
-	struct virtqueue *vq;
-
 	/* the number of entries in the queue */
 	int num;
 
@@ -90,8 +87,15 @@ struct virtio_pci_vq_info
 
 	/* MSI-X vector (or none) */
 	unsigned msix_vector;
+
+	/* the actual virtqueue. Note: must be the last field */
+	struct virtqueue vq;
 };
 
+#define to_vpinfo(vq) container_of(info, struct virtio_pci_vq_info, vq)
+/* How much space we need to reserve before struct virtqueue */
+#define VPINFO_PRIV offsetof(struct virtio_pci_vq_info, vq)
+
 /* Qumranet donated their vendor ID for devices 0x1000 thru 0x10FF. */
 static struct pci_device_id virtio_pci_id_table[] = {
 	{ 0x1af4, PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0 },
@@ -202,7 +206,7 @@ static void vp_reset(struct virtio_device *vdev)
 static void vp_notify(struct virtqueue *vq)
 {
 	struct virtio_pci_device *vp_dev = to_vp_device(vq->vdev);
-	struct virtio_pci_vq_info *info = vq->priv;
+	struct virtio_pci_vq_info *info = to_vpinfo(vq);
 
 	/* we write the queue's selector into the notification register to
 	 * signal the other end */
@@ -232,7 +236,7 @@ static irqreturn_t vp_vring_interrupt(int irq, void *opaque)
 
 	spin_lock_irqsave(&vp_dev->lock, flags);
 	list_for_each_entry(info, &vp_dev->virtqueues, node) {
-		if (vring_interrupt(irq, info->vq) == IRQ_HANDLED)
+		if (vring_interrupt(irq, &info->vq) == IRQ_HANDLED)
 			ret = IRQ_HANDLED;
 	}
 	spin_unlock_irqrestore(&vp_dev->lock, flags);
@@ -385,6 +389,7 @@ static struct virtqueue *setup_vq(struct virtio_device *vdev, unsigned index,
 	struct virtio_pci_vq_info *info;
 	struct virtqueue *vq;
 	unsigned long flags, size;
+	void *queue;
 	u16 num;
 	int err;
 
@@ -398,35 +403,34 @@ static struct virtqueue *setup_vq(struct virtio_device *vdev, unsigned index,
 
 	/* allocate and fill out our structure the represents an active
 	 * queue */
-	info = kmalloc(sizeof(struct virtio_pci_vq_info), GFP_KERNEL);
-	if (!info)
-		return ERR_PTR(-ENOMEM);
-
-	info->queue_index = index;
-	info->num = num;
-	info->msix_vector = msix_vec;
 
 	size = PAGE_ALIGN(vring_size(num, VIRTIO_PCI_VRING_ALIGN));
-	info->queue = alloc_pages_exact(size, GFP_KERNEL|__GFP_ZERO);
-	if (info->queue == NULL) {
+	queue = alloc_pages_exact(size, GFP_KERNEL|__GFP_ZERO);
+	if (queue == NULL) {
 		err = -ENOMEM;
 		goto out_info;
 	}
 
 	/* activate the queue */
-	iowrite32(virt_to_phys(info->queue) >> VIRTIO_PCI_QUEUE_ADDR_SHIFT,
+	iowrite32(virt_to_phys(queue) >> VIRTIO_PCI_QUEUE_ADDR_SHIFT,
 		  vp_dev->ioaddr + VIRTIO_PCI_QUEUE_PFN);
 
-	/* create the vring */
-	vq = vring_new_virtqueue(0, info->num, VIRTIO_PCI_VRING_ALIGN, vdev,
-				 true, info->queue, vp_notify, callback, name);
+	/* We can store transport specific data before the vq field.
+	 * Make sure we don't have any fields after it. */
+	BUILD_BUG_ON(VPINFO_PRIV + sizeof info->vq != sizeof *info);
+
+	vq = vring_new_virtqueue(VPINFO_PRIV, num, VIRTIO_PCI_VRING_ALIGN, vdev,
+				 true, queue, vp_notify, callback, name);
 	if (!vq) {
 		err = -ENOMEM;
 		goto out_activate_queue;
 	}
 
-	vq->priv = info;
-	info->vq = vq;
+	info = to_vpinfo(vq);
+
+	info->queue_index = index;
+	info->num = num;
+	info->msix_vector = msix_vec;
 
 	if (msix_vec != VIRTIO_MSI_NO_VECTOR) {
 		iowrite16(msix_vec, vp_dev->ioaddr + VIRTIO_MSI_QUEUE_VECTOR);
@@ -448,20 +452,21 @@ static struct virtqueue *setup_vq(struct virtio_device *vdev, unsigned index,
 	return vq;
 
 out_assign:
-	vring_del_virtqueue(vq, 0);
+	vring_del_virtqueue(vq, VPINFO_PRIV);
 out_activate_queue:
 	iowrite32(0, vp_dev->ioaddr + VIRTIO_PCI_QUEUE_PFN);
 	free_pages_exact(info->queue, size);
 out_info:
-	kfree(info);
 	return ERR_PTR(err);
 }
 
 static void vp_del_vq(struct virtqueue *vq)
 {
 	struct virtio_pci_device *vp_dev = to_vp_device(vq->vdev);
-	struct virtio_pci_vq_info *info = vq->priv;
+	struct virtio_pci_vq_info *info = to_vpinfo(vq);
 	unsigned long flags, size;
+	void *queue = info->queue;
+	u16 num = info->num;
 
 	spin_lock_irqsave(&vp_dev->lock, flags);
 	list_del(&info->node);
@@ -476,14 +481,13 @@ static void vp_del_vq(struct virtqueue *vq)
 		ioread8(vp_dev->ioaddr + VIRTIO_PCI_ISR);
 	}
 
-	vring_del_virtqueue(vq, 0);
+	vring_del_virtqueue(vq, VPINFO_PRIV);
 
 	/* Select and deactivate the queue */
 	iowrite32(0, vp_dev->ioaddr + VIRTIO_PCI_QUEUE_PFN);
 
 	size = PAGE_ALIGN(vring_size(info->num, VIRTIO_PCI_VRING_ALIGN));
-	free_pages_exact(info->queue, size);
-	kfree(info);
+	free_pages_exact(queue, size);
 }
 
 /* the config->del_vqs() implementation */
@@ -494,7 +498,7 @@ static void vp_del_vqs(struct virtio_device *vdev)
 	struct virtio_pci_vq_info *info;
 
 	list_for_each_entry_safe(vq, n, &vdev->vqs, list) {
-		info = vq->priv;
+		info = to_vpinfo(vq);
 		if (vp_dev->per_vq_vectors &&
 			info->msix_vector != VIRTIO_MSI_NO_VECTOR)
 			free_irq(vp_dev->msix_entries[info->msix_vector].vector,

Re: [PATCH untested] virtio: allocate extra memory before the ring ( was Re: [RFC PATCH] virtio_console: link vq to port with a private) pointer in struct virtqueue

[Michael S. Tsirkin]

On Tue, May 08, 2012 at 07:06:04AM -0400, Paolo Bonzini wrote:
> > And it's not a problem if virtqueue is exactly at start of
> > vring_virtqueue: we just need to allocate a bit more at start, and
> > offset when we free.  Here's how I would do this: first apply patch
> > below that adds the offset parameter, then update all transports, one
> > patch at a time to not use priv pointer, finally you can repurpose
> > priv pointer to let devices use it.
> 
> I like it, though I prefer to have the new arguments as sizeof()
> rather than offsets.

Well it tells us both how much extra memory
to allocate and where in the allocated buffer to put
struct virtqueue.

So size is kind of vague, offset is a more specific name.


> I'll pick up this and test it.

BTW still somewhat unhappy that the specific layout requirements make it a
bit fragile, even though I added BUG_ONs to at least fail in an obvious
way if we break the rules. But I wonder whether Rusty has a better idea.


> > As a bonus we get small incremental patches.
> 
> FWIW, that's possible with the patch I posted too.
> 
> Paolo

Re: [PATCH untested] virtio: allocate extra memory before the ring ( was Re: [RFC PATCH] virtio_console: link vq to port with a private) pointer in struct virtqueue

[Michael S. Tsirkin]

On Tue, May 08, 2012 at 07:06:04AM -0400, Paolo Bonzini wrote:
> > And it's not a problem if virtqueue is exactly at start of
> > vring_virtqueue: we just need to allocate a bit more at start, and
> > offset when we free.  Here's how I would do this: first apply patch
> > below that adds the offset parameter, then update all transports, one
> > patch at a time to not use priv pointer, finally you can repurpose
> > priv pointer to let devices use it.
> 
> I like it, though I prefer to have the new arguments as sizeof()
> rather than offsets.

Well it tells us both how much extra memory
to allocate and where in the allocated buffer to put
struct virtqueue.

So size is kind of vague, offset is a more specific name.


> I'll pick up this and test it.

BTW still somewhat unhappy that the specific layout requirements make it a
bit fragile, even though I added BUG_ONs to at least fail in an obvious
way if we break the rules. But I wonder whether Rusty has a better idea.


> > As a bonus we get small incremental patches.
> 
> FWIW, that's possible with the patch I posted too.
> 
> Paolo

Re: [RFC PATCH] virtio_console: link vq to port with a private pointer in struct virtqueue

[Rusty Russell]

On Tue, 8 May 2012 13:18:39 +0300, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Tue, May 08, 2012 at 11:45:55AM +0200, Paolo Bonzini wrote:
> > --- a/drivers/virtio/virtio_ring.c
> > +++ b/drivers/virtio/virtio_ring.c
> > @@ -76,8 +76,6 @@
> >  
> >  struct vring_virtqueue
> >  {
> > -	struct virtqueue vq;
> > -
> >  	/* Actual memory layout for this queue */
> >  	struct vring vring;
> >  
> > @@ -106,6 +104,9 @@ struct vring_virtqueue
> >  	/* How to notify other side. FIXME: commonalize hcalls! */
> >  	void (*notify)(struct virtqueue *vq);
> >  
> > +	/* Tokens for callbacks. */
> > +	void **data;
> > +
> >  #ifdef DEBUG
> >  	/* They're supposed to lock for us. */
> >  	unsigned int in_use;
> > @@ -115,8 +116,9 @@ struct vring_virtqueue
> >  	ktime_t last_add_time;
> >  #endif
> >  
> > -	/* Tokens for callbacks. */
> > -	void *data[];
> > +	struct virtqueue vq;
> > +
> > +	/* Bus-specific virtqueue data goes here.  */
> >  };
> >  
> >  #define to_vvq(_vq) container_of(_vq, struct vring_virtqueue, vq)
> 
> 
> This moves the data out of line and the bus specific stuff inline.
> But bus accesses only happen on io and interrupt which are already
> slow, while data accesses happen on fast path.
> 
> >From that POV this looks like a wrong thing to do.

(Resend to all)

Most of it was on a different cacheline anyway though, so it's probably
not measurable.  We avoid the pci_vq->vq indirection, but add the
vq->data indirection.

I share your discomfort with the offset arg method, too.

So unless benchmarks show otherwise, let's do it the vanilla way?

I think that making vring_virtqueue public looks like the way to go,
too.

Of course, a nice series would be great as well :)

Thanks!
Rusty.

Re: [RFC PATCH] virtio_console: link vq to port with a private pointer in struct virtqueue

[Rusty Russell]

On Tue, 8 May 2012 13:18:39 +0300, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Tue, May 08, 2012 at 11:45:55AM +0200, Paolo Bonzini wrote:
> > --- a/drivers/virtio/virtio_ring.c
> > +++ b/drivers/virtio/virtio_ring.c
> > @@ -76,8 +76,6 @@
> >  
> >  struct vring_virtqueue
> >  {
> > -	struct virtqueue vq;
> > -
> >  	/* Actual memory layout for this queue */
> >  	struct vring vring;
> >  
> > @@ -106,6 +104,9 @@ struct vring_virtqueue
> >  	/* How to notify other side. FIXME: commonalize hcalls! */
> >  	void (*notify)(struct virtqueue *vq);
> >  
> > +	/* Tokens for callbacks. */
> > +	void **data;
> > +
> >  #ifdef DEBUG
> >  	/* They're supposed to lock for us. */
> >  	unsigned int in_use;
> > @@ -115,8 +116,9 @@ struct vring_virtqueue
> >  	ktime_t last_add_time;
> >  #endif
> >  
> > -	/* Tokens for callbacks. */
> > -	void *data[];
> > +	struct virtqueue vq;
> > +
> > +	/* Bus-specific virtqueue data goes here.  */
> >  };
> >  
> >  #define to_vvq(_vq) container_of(_vq, struct vring_virtqueue, vq)
> 
> 
> This moves the data out of line and the bus specific stuff inline.
> But bus accesses only happen on io and interrupt which are already
> slow, while data accesses happen on fast path.
> 
> >From that POV this looks like a wrong thing to do.

(Resend to all)

Most of it was on a different cacheline anyway though, so it's probably
not measurable.  We avoid the pci_vq->vq indirection, but add the
vq->data indirection.

I share your discomfort with the offset arg method, too.

So unless benchmarks show otherwise, let's do it the vanilla way?

I think that making vring_virtqueue public looks like the way to go,
too.

Of course, a nice series would be great as well :)

Thanks!
Rusty.

Re: [RFC PATCH] virtio_console: link vq to port with a private pointer in struct virtqueue

[Michael S. Tsirkin]

On Thu, May 10, 2012 at 10:56:09AM +0930, Rusty Russell wrote:
> I think that making vring_virtqueue public looks like the way to go,
> too.

Then it's easy: transports can include vring_virtqueue
at end of transport structure.

Re: [RFC PATCH] virtio_console: link vq to port with a private pointer in struct virtqueue

[Michael S. Tsirkin]

On Thu, May 10, 2012 at 10:56:09AM +0930, Rusty Russell wrote:
> I think that making vring_virtqueue public looks like the way to go,
> too.

Then it's easy: transports can include vring_virtqueue
at end of transport structure.