Re: [PATCH 1/3] virtio: find_vqs/del_vqs virtio operations
On Wed, May 13, 2009 at 10:47:08AM +0930, Rusty Russell wrote:
On Wed, 13 May 2009 01:03:30 am Michael S. Tsirkin wrote:
On Wed, May 13, 2009 at 12:00:02AM +0930, Rusty Russell wrote
and perhaps consider
varargs for the callbacks (or would that be too horrible at the
implementation end?)
Thanks,
Rusty.
Ugh ... I think it will be. And AFAIK gcc generates a lot of code
for varargs - not something we want to do in each interrupt handler.
Err, no I mean for find_vqs: eg.
(block device)
err = vdev-config-find_vqs(vdev, 1, vblk-vq, blk_done);
(net device)
err = vdev-config-find_vqs(vdev, 3, vqs, skb_recv_done,
skb_xmit_done, NULL);
A bit neater for for the single-queue case.
Cheers,
Rusty.
Oh. I see. But it becomes messy now that we also need to pass in the
names, and we lose type safety.
Let's just add a helper function for the single vq case?
static inline struct virtqueue *virtio_find_vq(struct virtio_devide *vdev,
vq_callback_t *c, const char *n)
{
vq_callback_t *callbacks[] = { c };
const char *names[] = { n };
struct virtqueue *vq;
int err = vdev-config-find_vqs(vdev, 1, vq, callbacks, names);
if (err 0)
return ERR_PTR(err);
return vq;
}
--
MST
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] virtio: find_vqs/del_vqs virtio operations
Michael S. Tsirkin wrote:
On Wed, May 13, 2009 at 10:47:08AM +0930, Rusty Russell wrote:
On Wed, 13 May 2009 01:03:30 am Michael S. Tsirkin wrote:
On Wed, May 13, 2009 at 12:00:02AM +0930, Rusty Russell wrote
and perhaps consider
varargs for the callbacks (or would that be too horrible at the
implementation end?)
Thanks,
Rusty.
Ugh ... I think it will be. And AFAIK gcc generates a lot of code
for varargs - not something we want to do in each interrupt handler.
Err, no I mean for find_vqs: eg.
(block device)
err = vdev-config-find_vqs(vdev, 1, vblk-vq, blk_done);
(net device)
err = vdev-config-find_vqs(vdev, 3, vqs, skb_recv_done,
skb_xmit_done, NULL);
A bit neater for for the single-queue case.
Cheers,
Rusty.
Oh. I see. But it becomes messy now that we also need to pass in the
names, and we lose type safety.
Let's just add a helper function for the single vq case?
static inline struct virtqueue *virtio_find_vq(struct virtio_devide *vdev,
vq_callback_t *c, const char *n)
{
vq_callback_t *callbacks[] = { c };
const char *names[] = { n };
struct virtqueue *vq;
int err = vdev-config-find_vqs(vdev, 1, vq, callbacks, names);
if (err 0)
return ERR_PTR(err);
return vq;
}
Much saner.
--
Do not meddle in the internals of kernels, for they are subtle and quick to
panic.
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] virtio: find_vqs/del_vqs virtio operations
On Wed, 13 May 2009 04:48:34 pm Michael S. Tsirkin wrote: Let's just add a helper function for the single vq case? static inline struct virtqueue *virtio_find_vq(struct virtio_devide *vdev, vq_callback_t *c, const char *n) virtio_find_single_vq() to emphasize the singular nature, and it looks good. Thanks! Rusty. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] virtio: find_vqs/del_vqs virtio operations
Am Tuesday 12 May 2009 00:19:32 schrieb Michael S. Tsirkin:
This replaces find_vq/del_vq with find_vqs/del_vqs virtio operations,
and updates all drivers. This is needed for MSI support, because MSI
needs to know the total number of vectors upfront.
[...]
diff --git a/drivers/s390/kvm/kvm_virtio.c b/drivers/s390/kvm/kvm_virtio.c
This file contains several copy/paste breakages and needs at least the
patch below to compile.
Signed-off-by: Christian Borntraeger borntrae...@de.ibm.com
---
drivers/s390/kvm/kvm_virtio.c | 28 +++-
1 file changed, 7 insertions(+), 21 deletions(-)
Index: kvm/drivers/s390/kvm/kvm_virtio.c
===
--- kvm.orig/drivers/s390/kvm/kvm_virtio.c
+++ kvm/drivers/s390/kvm/kvm_virtio.c
@@ -237,23 +237,23 @@ static void kvm_del_vqs(struct virtio_de
int i;
if (!kdev-vqs)
return;
- for (i = 0; i ldev-nvqs; ++i)
+ for (i = 0; i kdev-nvqs; ++i)
kvm_del_vq(kdev-vqs[i]);
- kfree(ldev-vqs);
- ldev-vqs = NULL;
- ldev-nvqs = 0;
+ kfree(kdev-vqs);
+ kdev-vqs = NULL;
+ kdev-nvqs = 0;
}
static int kvm_find_vqs(struct virtio_device *vdev, unsigned nvqs,
- struct virtqueue *vqs[]
- void (*callbacks)[](struct virtqueue *))
+ struct virtqueue *vqs[],
+ virtqueue_callback *callbacks[])
{
struct kvm_device *kdev = to_kvmdev(vdev);
int i;
/* We must have this many virtqueues. */
if (nvqs kdev-desc-num_vq)
- return ERR_PTR(-ENOENT);
+ return -ENOENT;
kdev-vqs = kmalloc(GFP_KERNEL, nvqs * sizeof *kdev-vqs);
if (!kdev-vqs)
@@ -272,20 +272,6 @@ error:
return PTR_ERR(vqs[i]);
}
-static void kvm_del_vqs(struct virtio_device *vdev)
-{
- struct lguest_device *ldev = to_lgdev(vdev);
- int i;
-
- if (!ldev-vqs)
- return;
- for (i = 0; i ldev-nvqs; ++i)
- lg_del_vq(ldev-vqs[i]);
- kfree(ldev-vqs);
- ldev-vqs = NULL;
- ldev-nvqs = 0;
-}
-
/*
* The config ops structure as defined by virtio config
*/
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] virtio: find_vqs/del_vqs virtio operations
On Sun, 10 May 2009 04:55:38 pm Michael S. Tsirkin wrote: On Sun, May 10, 2009 at 01:37:06PM +0930, Rusty Russell wrote: Yes, and in fact a rough look at your patch reveals that we don't actually need del_vq: now we track them, we can just do that as part of vdev destruction, right? Let's assume that a driver encounters an error in probe after it calls find_vq. It would need a way to revert find_vq, won't it? It seems to me that bus-remove does not get called on probe failure. Isn't that right? Yep, I looked too fast. Thanks, Rusty. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] virtio: find_vqs/del_vqs virtio operations
On Tue, 12 May 2009 07:49:32 am Michael S. Tsirkin wrote: This replaces find_vq/del_vq with find_vqs/del_vqs virtio operations, and updates all drivers. This is needed for MSI support, because MSI needs to know the total number of vectors upfront. Sorry, is this not on top of my virtio_device vq linked list patch? Two other things: prefer vq_callback_t as the type name, and perhaps consider varargs for the callbacks (or would that be too horrible at the implementation end?) Thanks, Rusty. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] virtio: find_vqs/del_vqs virtio operations
On Wed, May 13, 2009 at 12:00:02AM +0930, Rusty Russell wrote: On Tue, 12 May 2009 07:49:32 am Michael S. Tsirkin wrote: This replaces find_vq/del_vq with find_vqs/del_vqs virtio operations, and updates all drivers. This is needed for MSI support, because MSI needs to know the total number of vectors upfront. Sorry, is this not on top of my virtio_device vq linked list patch? Not yet, working on that. Two other things: prefer vq_callback_t as the type name, ok and perhaps consider varargs for the callbacks (or would that be too horrible at the implementation end?) Thanks, Rusty. Ugh ... I think it will be. And AFAIK gcc generates a lot of code for varargs - not something we want to do in each interrupt handler. -- MST -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] virtio: find_vqs/del_vqs virtio operations
On Wed, 13 May 2009 01:03:30 am Michael S. Tsirkin wrote: On Wed, May 13, 2009 at 12:00:02AM +0930, Rusty Russell wrote and perhaps consider varargs for the callbacks (or would that be too horrible at the implementation end?) Thanks, Rusty. Ugh ... I think it will be. And AFAIK gcc generates a lot of code for varargs - not something we want to do in each interrupt handler. Err, no I mean for find_vqs: eg. (block device) err = vdev-config-find_vqs(vdev, 1, vblk-vq, blk_done); (net device) err = vdev-config-find_vqs(vdev, 3, vqs, skb_recv_done, skb_xmit_done, NULL); A bit neater for for the single-queue case. Cheers, Rusty. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] virtio: find_vqs/del_vqs virtio operations
On Sun, May 10, 2009 at 01:37:06PM +0930, Rusty Russell wrote: Yes, and in fact a rough look at your patch reveals that we don't actually need del_vq: now we track them, we can just do that as part of vdev destruction, right? Let's assume that a driver encounters an error in probe after it calls find_vq. It would need a way to revert find_vq, won't it? It seems to me that bus-remove does not get called on probe failure. Isn't that right? -- MST -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] virtio: find_vqs/del_vqs virtio operations
Am Sunday 10 May 2009 06:07:06 schrieb Rusty Russell: Yes, and in fact a rough look at your patch reveals that we don't actually need del_vq: now we track them, we can just do that as part of vdev destruction, right? Some of my students are working on a test module for virtio. Its a driver that matches all virtio IDs which can be used via bind/unbind. We currently use del_vq/find_vq in non device_release code. I will recheck but my feeling is that I want to keep del_vq. Christian -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] virtio: find_vqs/del_vqs virtio operations
On Fri, 8 May 2009 10:18:22 pm Michael S. Tsirkin wrote:
On Fri, May 08, 2009 at 04:37:06PM +0930, Rusty Russell wrote:
On Thu, 7 May 2009 11:40:39 pm Michael S. Tsirkin wrote:
This replaces find_vq/del_vq with find_vqs/del_vqs virtio operations,
and updates all drivers. This is needed for MSI support, because MSI
needs to know the total number of vectors upfront.
Hmm, I have a similar need for a dev to vq mapping (debugging stats).
How's this as a common basis?
This helps. Should I redo mine on top of this?
Yep, it should make your smaller as well.
void vring_del_virtqueue(struct virtqueue *vq)
{
+ list_del(vq-list);
kfree(to_vvq(vq));
}
EXPORT_SYMBOL_GPL(vring_del_virtqueue);
I note lack of locking here. This is okay in practice as
drivers don't really call find/del vq in parallel,
but making this explicit with find_vqs will be best, yes?
Yes, and in fact a rough look at your patch reveals that we don't actually
need del_vq: now we track them, we can just do that as part of vdev
destruction, right?
If you agree, please do that patch first, then do the find_vqs change on top of
that.
Thanks!
Rusty.
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] virtio: find_vqs/del_vqs virtio operations
On Thu, 7 May 2009 11:40:39 pm Michael S. Tsirkin wrote:
This replaces find_vq/del_vq with find_vqs/del_vqs virtio operations,
and updates all drivers. This is needed for MSI support, because MSI
needs to know the total number of vectors upfront.
Hmm, I have a similar need for a dev to vq mapping (debugging stats). How's
this as a common basis?
Thanks,
Rusty.
virtio: add names to virtqueue struct, mapping from devices to queues.
Add a linked list of all virtqueues for a virtio device: this helps for
debugging and is also needed for upcoming interface change.
Also, add a name field for clearer debug messages.
Signed-off-by: Rusty Russell ru...@rustcorp.com.au
---
drivers/block/virtio_blk.c |2 +-
drivers/char/hw_random/virtio-rng.c |2 +-
drivers/char/virtio_console.c |4 ++--
drivers/lguest/lguest_device.c |5 +++--
drivers/net/virtio_net.c|6 +++---
drivers/s390/kvm/kvm_virtio.c |7 ---
drivers/virtio/virtio.c |2 ++
drivers/virtio/virtio_balloon.c |4 ++--
drivers/virtio/virtio_pci.c |5 +++--
drivers/virtio/virtio_ring.c| 25 +++--
include/linux/virtio.h | 12
include/linux/virtio_config.h |6 --
include/linux/virtio_ring.h |3 ++-
net/9p/trans_virtio.c |2 +-
14 files changed, 55 insertions(+), 30 deletions(-)
diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -224,7 +224,7 @@ static int virtblk_probe(struct virtio_d
sg_init_table(vblk-sg, vblk-sg_elems);
/* We expect one virtqueue, for output. */
- vblk-vq = vdev-config-find_vq(vdev, 0, blk_done);
+ vblk-vq = vdev-config-find_vq(vdev, 0, blk_done, requests);
if (IS_ERR(vblk-vq)) {
err = PTR_ERR(vblk-vq);
goto out_free_vblk;
diff --git a/drivers/char/hw_random/virtio-rng.c
b/drivers/char/hw_random/virtio-rng.c
--- a/drivers/char/hw_random/virtio-rng.c
+++ b/drivers/char/hw_random/virtio-rng.c
@@ -94,7 +94,7 @@ static int virtrng_probe(struct virtio_d
int err;
/* We expect a single virtqueue. */
- vq = vdev-config-find_vq(vdev, 0, random_recv_done);
+ vq = vdev-config-find_vq(vdev, 0, random_recv_done, input);
if (IS_ERR(vq))
return PTR_ERR(vq);
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -202,13 +202,13 @@ static int __devinit virtcons_probe(stru
/* Find the input queue. */
/* FIXME: This is why we want to wean off hvc: we do nothing
* when input comes in. */
- in_vq = vdev-config-find_vq(vdev, 0, hvc_handle_input);
+ in_vq = vdev-config-find_vq(vdev, 0, hvc_handle_input, input);
if (IS_ERR(in_vq)) {
err = PTR_ERR(in_vq);
goto free;
}
- out_vq = vdev-config-find_vq(vdev, 1, NULL);
+ out_vq = vdev-config-find_vq(vdev, 1, NULL, output);
if (IS_ERR(out_vq)) {
err = PTR_ERR(out_vq);
goto free_in_vq;
diff --git a/drivers/lguest/lguest_device.c b/drivers/lguest/lguest_device.c
--- a/drivers/lguest/lguest_device.c
+++ b/drivers/lguest/lguest_device.c
@@ -228,7 +228,8 @@ extern void lguest_setup_irq(unsigned in
* function. */
static struct virtqueue *lg_find_vq(struct virtio_device *vdev,
unsigned index,
- void (*callback)(struct virtqueue *vq))
+ void (*callback)(struct virtqueue *vq),
+ const char *name)
{
struct lguest_device *ldev = to_lgdev(vdev);
struct lguest_vq_info *lvq;
@@ -263,7 +264,7 @@ static struct virtqueue *lg_find_vq(stru
/* OK, tell virtio_ring.c to set up a virtqueue now we know its size
* and we've got a pointer to its pages. */
vq = vring_new_virtqueue(lvq-config.num, LGUEST_VRING_ALIGN,
-vdev, lvq-pages, lg_notify, callback);
+vdev, lvq-pages, lg_notify, callback, name);
if (!vq) {
err = -ENOMEM;
goto unmap;
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -902,20 +902,20 @@ static int virtnet_probe(struct virtio_d
vi-mergeable_rx_bufs = true;
/* We expect two virtqueues, receive then send. */
- vi-rvq = vdev-config-find_vq(vdev, 0, skb_recv_done);
+ vi-rvq = vdev-config-find_vq(vdev, 0, skb_recv_done, input);
if (IS_ERR(vi-rvq)) {
err = PTR_ERR(vi-rvq);
goto free;
}
- vi-svq = vdev-config-find_vq(vdev, 1, skb_xmit_done);
Re: [PATCH 1/3] virtio: find_vqs/del_vqs virtio operations
On Fri, May 08, 2009 at 04:37:06PM +0930, Rusty Russell wrote:
On Thu, 7 May 2009 11:40:39 pm Michael S. Tsirkin wrote:
This replaces find_vq/del_vq with find_vqs/del_vqs virtio operations,
and updates all drivers. This is needed for MSI support, because MSI
needs to know the total number of vectors upfront.
Hmm, I have a similar need for a dev to vq mapping (debugging stats). How's
this as a common basis?
This helps. Should I redo mine on top of this?
Thanks,
Rusty.
virtio: add names to virtqueue struct, mapping from devices to queues.
Add a linked list of all virtqueues for a virtio device: this helps for
debugging and is also needed for upcoming interface change.
Also, add a name field for clearer debug messages.
Signed-off-by: Rusty Russell ru...@rustcorp.com.au
Yes, this will simplify supporting find_vqs.
@@ -303,10 +313,12 @@ struct virtqueue *vring_new_virtqueue(un
vq-vq.callback = callback;
vq-vq.vdev = vdev;
vq-vq.vq_ops = vring_vq_ops;
+ vq-vq.name = name;
vq-notify = notify;
vq-broken = false;
vq-last_used_idx = 0;
vq-num_added = 0;
+ list_add_tail(vq-vq.list, vdev-vqs);
#ifdef DEBUG
vq-in_use = false;
#endif
@@ -327,6 +339,7 @@ EXPORT_SYMBOL_GPL(vring_new_virtqueue);
void vring_del_virtqueue(struct virtqueue *vq)
{
+ list_del(vq-list);
kfree(to_vvq(vq));
}
EXPORT_SYMBOL_GPL(vring_del_virtqueue);
I note lack of locking here. This is okay in practice as
drivers don't really call find/del vq in parallel,
but making this explicit with find_vqs will be best, yes?
--
MST
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
