Re: [PATCHv4 2/2] virtio: refactor find_vqs

2009-08-27 Thread Rusty Russell 
On Tue, 25 Aug 2009 09:34:34 pm Michael S. Tsirkin wrote:
  That's because we didn't do the request_irq's for the per_vector case, 
  because
  we don't have the names.  This is what prevented me from doing a nice
  encapsulation.
 
 Yes. But let's split free_vectors out into free_msix_vectors and
 free_intx as well?

Perhaps.  Patch welcome :)

 Yes, I agree, this is good cleanup, structure field names should be
 desriptive.  Are you sure we want to make all local variables named
 vector renamed to msix_vector though? CodingStyle says local var names
 should be short ...  A couple of ideas below.

msix_vec would work.  But vector for something which isn't always the vector
is misleading, IMHO.

  -   if (vector != VIRTIO_MSI_NO_VECTOR) {
  -   iowrite16(vector, vp_dev-ioaddr + VIRTIO_MSI_QUEUE_VECTOR);
  -   vector = ioread16(vp_dev-ioaddr + VIRTIO_MSI_QUEUE_VECTOR);
  -   if (vector == VIRTIO_MSI_NO_VECTOR) {
  +   if (msix_vector != VIRTIO_MSI_NO_VECTOR) {
  +   iowrite16(msix_vector, vp_dev-ioaddr+VIRTIO_MSI_QUEUE_VECTOR);
  +   msix_vector = ioread16(vp_dev-ioaddr+VIRTIO_MSI_QUEUE_VECTOR);
 
 checkpatch will complain that space is lacking around +.
 We won't have a problem if we keep it named vector.

Yeah, but OTOH ignoring checkpatch warnings is good for the soul.

  if (!use_msix) {
  /* Old style: one normal interrupt for change and all vqs. */
  vp_dev-msix_vectors = 0;
  -   vp_dev-per_vq_vectors = false;
 
 I know it's enough to look at msix_vectors, but isn't
 it cleaner to have per_vq_vectors consistent as well?
 E.g. del_vqs seems to only look at per_vq_vectors.

This should in fact be:

if (!use_msix) {
/* Old style: one normal interrupt for change and all vqs. */
-   vp_dev-msix_vectors = 0;
-   vp_dev-per_vq_vectors = false;
err = request_irq(vp_dev-pci_dev-irq, vp_interrupt,
  IRQF_SHARED, dev_name(vdev-dev), vp_dev);
-   if (!err)
-   vp_dev-intx_enabled = 1;
-   return err;
+   if (err)
+   return err;
+   vp_dev-intx_enabled = 1;


The msix fields should all be ignored if msix_enabled is false.  Think about
running valgrind (or equiv) over the code: if you initialize something which
shouldn't be accessed, valgrind won't spot it.

For similar reasons, I dislike memseting structs to 0, or kzallocing them.

  err = request_irq(vp_dev-pci_dev-irq, vp_interrupt,
IRQF_SHARED, dev_name(vdev-dev), vp_dev);
  -   if (!err)
  -   vp_dev-intx_enabled = 1;
  -   return err;
  +   if (err)
  +   return err;
 
 Maybe move this part out into a separate function?  This way
 vp_try_to_find_vqs does high-level processing.

Nice.  Moved this to vp_request_intx().

  +   } else {
  +   if (per_vq_vectors) {
  +   /* Best option: one for change interrupt, one per vq. */
  +   nvectors = 1;
  +   for (i = 0; i  nvqs; ++i)
  +   if (callbacks[i])
  +   ++nvectors;
  +   } else {
  +   /* Second best: one for change, shared for all vqs. */
  +   nvectors = 2;
 
 Out of curiosity: why do you put {} here? They aren't
 strictly necessary ...

Comment made it multiline, looked a bit neater?

Here's the diff result:

diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
--- a/drivers/virtio/virtio_pci.c
+++ b/drivers/virtio/virtio_pci.c
@@ -346,10 +346,22 @@ error:
return err;
 }
 
+static int vp_request_intx(struct virtio_device *vdev)
+{
+   int err;
+   struct virtio_pci_device *vp_dev = to_vp_device(vdev);
+
+   err = request_irq(vp_dev-pci_dev-irq, vp_interrupt,
+ IRQF_SHARED, dev_name(vdev-dev), vp_dev);
+   if (!err)
+   vp_dev-intx_enabled = 1;
+   return err;
+}
+
 static struct virtqueue *setup_vq(struct virtio_device *vdev, unsigned index,
  void (*callback)(struct virtqueue *vq),
  const char *name,
- u16 msix_vector)
+ u16 msix_vec)
 {
struct virtio_pci_device *vp_dev = to_vp_device(vdev);
struct virtio_pci_vq_info *info;
@@ -374,7 +386,7 @@ static struct virtqueue *setup_vq(struct
 
info-queue_index = index;
info-num = num;
-   info-msix_vector = msix_vector;
+   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);
@@ -398,10 +410,10 @@ static struct virtqueue *setup_vq(struct
vq-priv = info;
info-vq = vq;
 
-   if

Re: [PATCHv4 2/2] virtio: refactor find_vqs

2009-08-27 Thread Michael S. Tsirkin 
On Thu, Aug 27, 2009 at 07:00:34PM +0930, Rusty Russell wrote:
 On Tue, 25 Aug 2009 09:34:34 pm Michael S. Tsirkin wrote:
   That's because we didn't do the request_irq's for the per_vector case, 
   because
   we don't have the names.  This is what prevented me from doing a nice
   encapsulation.
  
  Yes. But let's split free_vectors out into free_msix_vectors and
  free_intx as well?
 
 Perhaps.  Patch welcome :)

Could you put the end result somewhere so I can work on top of it?

  Yes, I agree, this is good cleanup, structure field names should be
  desriptive.  Are you sure we want to make all local variables named
  vector renamed to msix_vector though? CodingStyle says local var names
  should be short ...  A couple of ideas below.
 
 msix_vec would work.

Yes, ok.

 But vector for something which isn't always the vector
 is misleading, IMHO.

I think you mean it's isn't always used? It's always a vector ...

   - if (vector != VIRTIO_MSI_NO_VECTOR) {
   - iowrite16(vector, vp_dev-ioaddr + VIRTIO_MSI_QUEUE_VECTOR);
   - vector = ioread16(vp_dev-ioaddr + VIRTIO_MSI_QUEUE_VECTOR);
   - if (vector == VIRTIO_MSI_NO_VECTOR) {
   + if (msix_vector != VIRTIO_MSI_NO_VECTOR) {
   + iowrite16(msix_vector, vp_dev-ioaddr+VIRTIO_MSI_QUEUE_VECTOR);
   + msix_vector = ioread16(vp_dev-ioaddr+VIRTIO_MSI_QUEUE_VECTOR);
  
  checkpatch will complain that space is lacking around +.
  We won't have a problem if we keep it named vector.
 
 Yeah, but OTOH ignoring checkpatch warnings is good for the soul.
 
 if (!use_msix) {
 /* Old style: one normal interrupt for change and all vqs. */
 vp_dev-msix_vectors = 0;
   - vp_dev-per_vq_vectors = false;
  
  I know it's enough to look at msix_vectors, but isn't
  it cleaner to have per_vq_vectors consistent as well?
  E.g. del_vqs seems to only look at per_vq_vectors.
 
 This should in fact be:
 
   if (!use_msix) {
   /* Old style: one normal interrupt for change and all vqs. */
 - vp_dev-msix_vectors = 0;
 - vp_dev-per_vq_vectors = false;
   err = request_irq(vp_dev-pci_dev-irq, vp_interrupt,
 IRQF_SHARED, dev_name(vdev-dev), vp_dev);
 - if (!err)
 - vp_dev-intx_enabled = 1;
 - return err;
 + if (err)
 + return err;
 + vp_dev-intx_enabled = 1;
 
 
 The msix fields should all be ignored if msix_enabled is false.

Need to audit code to make sure it's so then.  I actually think
per_vq_vectors is kind of not directly tied to msix.
Don't you think it's nicer like del_vqs does:
if (vp_dev-per_vq_vectors) {
}

Than

if (vp_dev-msix_enabled  vp_dev-per_vq_vectors) {
}

BTW, let's get rid of msix_enabled completely?
We can always use msix_vectors ...

 Think about
 running valgrind (or equiv) over the code: if you initialize something which
 shouldn't be accessed, valgrind won't spot it.
 
 For similar reasons, I dislike memseting structs to 0, or kzallocing them.

Yes, I agree generally.

 err = request_irq(vp_dev-pci_dev-irq, vp_interrupt,
   IRQF_SHARED, dev_name(vdev-dev), vp_dev);
   - if (!err)
   - vp_dev-intx_enabled = 1;
   - return err;
   + if (err)
   + return err;
  
  Maybe move this part out into a separate function?  This way
  vp_try_to_find_vqs does high-level processing.
 
 Nice.  Moved this to vp_request_intx().
 
   + } else {
   + if (per_vq_vectors) {
   + /* Best option: one for change interrupt, one per vq. */
   + nvectors = 1;
   + for (i = 0; i  nvqs; ++i)
   + if (callbacks[i])
   + ++nvectors;
   + } else {
   + /* Second best: one for change, shared for all vqs. */
   + nvectors = 2;
  
  Out of curiosity: why do you put {} here? They aren't
  strictly necessary ...
 
 Comment made it multiline, looked a bit neater?
 
 Here's the diff result:

Looks good to me. When we are done with cleanups, need to remember to
test this with all the possible combinations: no msix, 2 vectors, 1
vector.

 diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
 --- a/drivers/virtio/virtio_pci.c
 +++ b/drivers/virtio/virtio_pci.c
 @@ -346,10 +346,22 @@ error:
   return err;
  }
  
 +static int vp_request_intx(struct virtio_device *vdev)
 +{
 + int err;
 + struct virtio_pci_device *vp_dev = to_vp_device(vdev);
 +
 + err = request_irq(vp_dev-pci_dev-irq, vp_interrupt,
 +   IRQF_SHARED, dev_name(vdev-dev), vp_dev);
 + if (!err)
 + vp_dev-intx_enabled = 1;
 + return err;
 +}
 +
  static struct virtqueue *setup_vq(struct virtio_device *vdev, unsigned index,
 void (*callback)(struct

Re: [PATCHv4 2/2] virtio: refactor find_vqs

2009-08-27 Thread Rusty Russell 
On Thu, 27 Aug 2009 07:19:26 pm Michael S. Tsirkin wrote:
 On Thu, Aug 27, 2009 at 07:00:34PM +0930, Rusty Russell wrote:
  On Tue, 25 Aug 2009 09:34:34 pm Michael S. Tsirkin wrote:
That's because we didn't do the request_irq's for the per_vector case, 
because
we don't have the names.  This is what prevented me from doing a nice
encapsulation.
   
   Yes. But let's split free_vectors out into free_msix_vectors and
   free_intx as well?
  
  Perhaps.  Patch welcome :)
 
 Could you put the end result somewhere so I can work on top of it?

Sure, it'll hit linux-next tomorrow, otherwise you can steal from
http://ozlabs.org/~rusty/kernel/rr-latest (virtio:pci-minor-cleanups.patch
and virtio:pci-minor-cleanups-fix.patch).

  But vector for something which isn't always the vector
  is misleading, IMHO.
 
 I think you mean it's isn't always used? It's always a vector ...

The non-MSI case, it's set to VIRTIO_MSI_NO_VECTOR, and we use a normal
interrupt vector.

 BTW, let's get rid of msix_enabled completely?
 We can always use msix_vectors ...

That would be nice.  But yes, requiring more audit.

Ideally, if msix_vectors == 0, implies intx_enabled.

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: [PATCHv4 2/2] virtio: refactor find_vqs

2009-08-27 Thread Michael S. Tsirkin 
On Thu, Aug 27, 2009 at 08:32:24PM +0930, Rusty Russell wrote:
 On Thu, 27 Aug 2009 07:19:26 pm Michael S. Tsirkin wrote:
  On Thu, Aug 27, 2009 at 07:00:34PM +0930, Rusty Russell wrote:
   On Tue, 25 Aug 2009 09:34:34 pm Michael S. Tsirkin wrote:
 That's because we didn't do the request_irq's for the per_vector 
 case, because
 we don't have the names.  This is what prevented me from doing a nice
 encapsulation.

Yes. But let's split free_vectors out into free_msix_vectors and
free_intx as well?
   
   Perhaps.  Patch welcome :)
  
  Could you put the end result somewhere so I can work on top of it?
 
 Sure, it'll hit linux-next tomorrow, otherwise you can steal from
 http://ozlabs.org/~rusty/kernel/rr-latest (virtio:pci-minor-cleanups.patch
 and virtio:pci-minor-cleanups-fix.patch).
 
   But vector for something which isn't always the vector
   is misleading, IMHO.
  
  I think you mean it's isn't always used? It's always a vector ...
 
 The non-MSI case, it's set to VIRTIO_MSI_NO_VECTOR, and we use a normal
 interrupt vector.
 
  BTW, let's get rid of msix_enabled completely?
  We can always use msix_vectors ...
 
 That would be nice.  But yes, requiring more audit.
 
 Ideally, if msix_vectors == 0, implies intx_enabled.

It seems that since we *can* request both an intx
and msix vectors, it's better to have them independent even if we
currently don't do that. No?

 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: [PATCHv4 2/2] virtio: refactor find_vqs

2009-08-25 Thread Michael S. Tsirkin 
Sorry it took a bit to respond to this.  I kept hoping I'll have time to
test it but looks like I won't before I'm on vacation.  I agree it's
good cleanup, and maybe we can go even further, see below.

On Mon, Aug 10, 2009 at 09:07:52AM +0930, Rusty Russell wrote:
 On Tue, 28 Jul 2009 06:03:08 pm Michael S. Tsirkin wrote:
  On Tue, Jul 28, 2009 at 12:44:31PM +0930, Rusty Russell wrote:
   On Mon, 27 Jul 2009 01:17:09 am Michael S. Tsirkin wrote:
This refactors find_vqs, making it more readable and robust, and fixing
two regressions from 2.6.30:
- double free_irq causing BUG_ON on device removal
- probe failure when vq can't be assigned to msi-x vector
  (reported on old host kernels)

An older version of this patch was tested by Amit Shah.
   
   OK, I've applied both of these; I'd like to see a new test by Amit to
   make sure tho.
   
   I really like this cleanup!  I looked harder at this code, and my best
   attempts to untangle it further came to very little.  This is what I
   ended up with, but it's all cosmetic and can wait until next merge window.
   See what you think.
   
   Thanks!
   Rusty.
   
   virtio_pci: minor MSI-X cleanups
   
   1) Rename vp_request_vectors to vp_request_msix_vectors, and take
  non-MSI-X case out to caller.
  
  I'm not sure this change was for the best: we still have a separate code
  path under if !use_msix, only in another place now.  See below.
  And this seems to break the symmetry between request_ and free_vectors.
 
 Yes, but unfortunately request_vectors was never really symmetrical :(
 
 That's because we didn't do the request_irq's for the per_vector case, because
 we don't have the names.  This is what prevented me from doing a nice
 encapsulation.

Yes. But let's split free_vectors out into free_msix_vectors and
free_intx as well?

   - err = vp_request_vectors(vdev, nvectors, per_vq_vectors);
   + if (!use_msix) {
   + /* Old style: one normal interrupt for change and all vqs. */
   + vp_dev-msix_vectors = 0;
   + vp_dev-per_vq_vectors = false;
   + err = request_irq(vp_dev-pci_dev-irq, vp_interrupt,
   +   IRQF_SHARED, dev_name(vdev-dev), vp_dev);
   + if (!err)
   + vp_dev-intx_enabled = 1;
  
  shorter as vp_dev-intx_enabled = !err
  
   + return err;
  
  Is that all? Don't we need to create the vqs?
 
 Oh, yeah :)
 
 This patch applies on top.  Basically reverts that part, and
 renames vector to msix_vector, which I think makes the code more
 readable.

Yes, I agree, this is good cleanup, structure field names should be
desriptive.  Are you sure we want to make all local variables named
vector renamed to msix_vector though? CodingStyle says local var names
should be short ...  A couple of ideas below.

 virtio: more PCI minor cleanups
 
 Signed-off-by: Rusty Russell ru...@rustcorp.com.au
 ---
  drivers/virtio/virtio_pci.c |   73 
 +---
  1 file changed, 36 insertions(+), 37 deletions(-)
 
 diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
 --- a/drivers/virtio/virtio_pci.c
 +++ b/drivers/virtio/virtio_pci.c
 @@ -84,7 +84,7 @@ struct virtio_pci_vq_info
   struct list_head node;
  
   /* MSI-X vector (or none) */
 - unsigned vector;
 + unsigned msix_vector;

Good. And now we don't need the comment.

  };
  
  /* Qumranet donated their vendor ID for devices 0x1000 thru 0x10FF. */
 @@ -349,7 +349,7 @@ error:
  static struct virtqueue *setup_vq(struct virtio_device *vdev, unsigned index,
 void (*callback)(struct virtqueue *vq),
 const char *name,
 -   u16 vector)
 +   u16 msix_vector)
  {
   struct virtio_pci_device *vp_dev = to_vp_device(vdev);
   struct virtio_pci_vq_info *info;
 @@ -374,7 +374,7 @@ static struct virtqueue *setup_vq(struct
  
   info-queue_index = index;
   info-num = num;
 - info-vector = vector;
 + info-msix_vector = msix_vector;
  
   size = PAGE_ALIGN(vring_size(num, VIRTIO_PCI_VRING_ALIGN));
   info-queue = alloc_pages_exact(size, GFP_KERNEL|__GFP_ZERO);
 @@ -398,10 +398,10 @@ static struct virtqueue *setup_vq(struct
   vq-priv = info;
   info-vq = vq;
  
 - if (vector != VIRTIO_MSI_NO_VECTOR) {
 - iowrite16(vector, vp_dev-ioaddr + VIRTIO_MSI_QUEUE_VECTOR);
 - vector = ioread16(vp_dev-ioaddr + VIRTIO_MSI_QUEUE_VECTOR);
 - if (vector == VIRTIO_MSI_NO_VECTOR) {
 + if (msix_vector != VIRTIO_MSI_NO_VECTOR) {
 + iowrite16(msix_vector, vp_dev-ioaddr+VIRTIO_MSI_QUEUE_VECTOR);
 + msix_vector = ioread16(vp_dev-ioaddr+VIRTIO_MSI_QUEUE_VECTOR);

checkpatch will complain that space is lacking around +.
We won't have a problem if we keep it named vector.

 + if (msix_vector == VIRTIO_MSI_NO_VECTOR) {

Re: [PATCHv4 2/2] virtio: refactor find_vqs

2009-08-09 Thread Rusty Russell 
On Tue, 28 Jul 2009 06:03:08 pm Michael S. Tsirkin wrote:
 On Tue, Jul 28, 2009 at 12:44:31PM +0930, Rusty Russell wrote:
  On Mon, 27 Jul 2009 01:17:09 am Michael S. Tsirkin wrote:
   This refactors find_vqs, making it more readable and robust, and fixing
   two regressions from 2.6.30:
   - double free_irq causing BUG_ON on device removal
   - probe failure when vq can't be assigned to msi-x vector
 (reported on old host kernels)
   
   An older version of this patch was tested by Amit Shah.
  
  OK, I've applied both of these; I'd like to see a new test by Amit to
  make sure tho.
  
  I really like this cleanup!  I looked harder at this code, and my best
  attempts to untangle it further came to very little.  This is what I
  ended up with, but it's all cosmetic and can wait until next merge window.
  See what you think.
  
  Thanks!
  Rusty.
  
  virtio_pci: minor MSI-X cleanups
  
  1) Rename vp_request_vectors to vp_request_msix_vectors, and take
 non-MSI-X case out to caller.
 
 I'm not sure this change was for the best: we still have a separate code
 path under if !use_msix, only in another place now.  See below.
 And this seems to break the symmetry between request_ and free_vectors.

Yes, but unfortunately request_vectors was never really symmetrical :(

That's because we didn't do the request_irq's for the per_vector case, because
we don't have the names.  This is what prevented me from doing a nice
encapsulation.

  -   err = vp_request_vectors(vdev, nvectors, per_vq_vectors);
  +   if (!use_msix) {
  +   /* Old style: one normal interrupt for change and all vqs. */
  +   vp_dev-msix_vectors = 0;
  +   vp_dev-per_vq_vectors = false;
  +   err = request_irq(vp_dev-pci_dev-irq, vp_interrupt,
  + IRQF_SHARED, dev_name(vdev-dev), vp_dev);
  +   if (!err)
  +   vp_dev-intx_enabled = 1;
 
 shorter as vp_dev-intx_enabled = !err
 
  +   return err;
 
 Is that all? Don't we need to create the vqs?

Oh, yeah :)

This patch applies on top.  Basically reverts that part, and
renames vector to msix_vector, which I think makes the code more
readable.

virtio: more PCI minor cleanups

Signed-off-by: Rusty Russell ru...@rustcorp.com.au
---
 drivers/virtio/virtio_pci.c |   73 +---
 1 file changed, 36 insertions(+), 37 deletions(-)

diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
--- a/drivers/virtio/virtio_pci.c
+++ b/drivers/virtio/virtio_pci.c
@@ -84,7 +84,7 @@ struct virtio_pci_vq_info
struct list_head node;
 
/* MSI-X vector (or none) */
-   unsigned vector;
+   unsigned msix_vector;
 };
 
 /* Qumranet donated their vendor ID for devices 0x1000 thru 0x10FF. */
@@ -349,7 +349,7 @@ error:
 static struct virtqueue *setup_vq(struct virtio_device *vdev, unsigned index,
  void (*callback)(struct virtqueue *vq),
  const char *name,
- u16 vector)
+ u16 msix_vector)
 {
struct virtio_pci_device *vp_dev = to_vp_device(vdev);
struct virtio_pci_vq_info *info;
@@ -374,7 +374,7 @@ static struct virtqueue *setup_vq(struct
 
info-queue_index = index;
info-num = num;
-   info-vector = vector;
+   info-msix_vector = msix_vector;
 
size = PAGE_ALIGN(vring_size(num, VIRTIO_PCI_VRING_ALIGN));
info-queue = alloc_pages_exact(size, GFP_KERNEL|__GFP_ZERO);
@@ -398,10 +398,10 @@ static struct virtqueue *setup_vq(struct
vq-priv = info;
info-vq = vq;
 
-   if (vector != VIRTIO_MSI_NO_VECTOR) {
-   iowrite16(vector, vp_dev-ioaddr + VIRTIO_MSI_QUEUE_VECTOR);
-   vector = ioread16(vp_dev-ioaddr + VIRTIO_MSI_QUEUE_VECTOR);
-   if (vector == VIRTIO_MSI_NO_VECTOR) {
+   if (msix_vector != VIRTIO_MSI_NO_VECTOR) {
+   iowrite16(msix_vector, vp_dev-ioaddr+VIRTIO_MSI_QUEUE_VECTOR);
+   msix_vector = ioread16(vp_dev-ioaddr+VIRTIO_MSI_QUEUE_VECTOR);
+   if (msix_vector == VIRTIO_MSI_NO_VECTOR) {
err = -EBUSY;
goto out_assign;
}
@@ -462,7 +462,8 @@ static void vp_del_vqs(struct virtio_dev
list_for_each_entry_safe(vq, n, vdev-vqs, list) {
info = vq-priv;
if (vp_dev-per_vq_vectors)
-   free_irq(vp_dev-msix_entries[info-vector].vector, vq);
+   free_irq(vp_dev-msix_entries[info-msix_vector].vector,
+vq);
vp_del_vq(vq);
}
vp_dev-per_vq_vectors = false;
@@ -478,58 +479,56 @@ static int vp_try_to_find_vqs(struct vir
  bool per_vq_vectors)
 {
struct virtio_pci_device *vp_dev = to_vp_device(vdev);
-   u16 vector;
+   u16 msix_vector;
int i,

Re: [PATCHv4 2/2] virtio: refactor find_vqs

2009-07-28 Thread Michael S. Tsirkin 
On Tue, Jul 28, 2009 at 12:44:31PM +0930, Rusty Russell wrote:
 On Mon, 27 Jul 2009 01:17:09 am Michael S. Tsirkin wrote:
  This refactors find_vqs, making it more readable and robust, and fixing
  two regressions from 2.6.30:
  - double free_irq causing BUG_ON on device removal
  - probe failure when vq can't be assigned to msi-x vector
(reported on old host kernels)
  
  An older version of this patch was tested by Amit Shah.
 
 OK, I've applied both of these; I'd like to see a new test by Amit to
 make sure tho.
 
 I really like this cleanup!  I looked harder at this code, and my best
 attempts to untangle it further came to very little.  This is what I
 ended up with, but it's all cosmetic and can wait until next merge window.
 See what you think.
 
 Thanks!
 Rusty.
 
 virtio_pci: minor MSI-X cleanups
 
 1) Rename vp_request_vectors to vp_request_msix_vectors, and take
non-MSI-X case out to caller.

I'm not sure this change was for the best: we still have a separate code
path under if !use_msix, only in another place now.  See below.
And this seems to break the symmetry between request_ and free_vectors.

 2) Comment weird pci_enable_msix API
 3) Rename vp_find_vq to setup_vq.
 4) Fix spaces to tabs
 5) Make nvectors calc internal to vp_try_to_find_vqs()

The other changes look good to me.

 
 Signed-off-by: Rusty Russell ru...@rustcorp.com.au
 ---
  drivers/virtio/virtio_pci.c |   84 
 +++-
  1 file changed, 45 insertions(+), 39 deletions(-)
 
 diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
 --- a/drivers/virtio/virtio_pci.c
 +++ b/drivers/virtio/virtio_pci.c
 @@ -280,25 +280,14 @@ static void vp_free_vectors(struct virti
   vp_dev-msix_entries = NULL;
  }
  
 -static int vp_request_vectors(struct virtio_device *vdev, int nvectors,
 -   bool per_vq_vectors)
 +static int vp_request_msix_vectors(struct virtio_device *vdev, int nvectors,
 +bool per_vq_vectors)
  {
   struct virtio_pci_device *vp_dev = to_vp_device(vdev);
   const char *name = dev_name(vp_dev-vdev.dev);
   unsigned i, v;
   int err = -ENOMEM;
  
 - if (!nvectors) {
 - /* Can't allocate MSI-X vectors, use regular interrupt */
 - vp_dev-msix_vectors = 0;
 - err = request_irq(vp_dev-pci_dev-irq, vp_interrupt,
 -   IRQF_SHARED, name, vp_dev);
 - if (err)
 - return err;
 - vp_dev-intx_enabled = 1;
 - return 0;
 - }
 -
   vp_dev-msix_entries = kmalloc(nvectors * sizeof *vp_dev-msix_entries,
  GFP_KERNEL);
   if (!vp_dev-msix_entries)
 @@ -311,6 +300,7 @@ static int vp_request_vectors(struct vir
   for (i = 0; i  nvectors; ++i)
   vp_dev-msix_entries[i].entry = i;
  
 + /* pci_enable_msix returns positive if we can't get this many. */
   err = pci_enable_msix(vp_dev-pci_dev, vp_dev-msix_entries, nvectors);
   if (err  0)
   err = -ENOSPC;
 @@ -356,10 +346,10 @@ error:
   return err;
  }
  
 -static struct virtqueue *vp_find_vq(struct virtio_device *vdev, unsigned 
 index,
 - void (*callback)(struct virtqueue *vq),
 - const char *name,
 - u16 vector)
 +static struct virtqueue *setup_vq(struct virtio_device *vdev, unsigned index,
 +   void (*callback)(struct virtqueue *vq),
 +   const char *name,
 +   u16 vector)
  {
   struct virtio_pci_device *vp_dev = to_vp_device(vdev);
   struct virtio_pci_vq_info *info;
 @@ -408,7 +398,7 @@ static struct virtqueue *vp_find_vq(stru
   vq-priv = info;
   info-vq = vq;
  
 -  if (vector != VIRTIO_MSI_NO_VECTOR) {
 + if (vector != VIRTIO_MSI_NO_VECTOR) {
   iowrite16(vector, vp_dev-ioaddr + VIRTIO_MSI_QUEUE_VECTOR);
   vector = ioread16(vp_dev-ioaddr + VIRTIO_MSI_QUEUE_VECTOR);
   if (vector == VIRTIO_MSI_NO_VECTOR) {
 @@ -484,14 +474,36 @@ static int vp_try_to_find_vqs(struct vir
 struct virtqueue *vqs[],
 vq_callback_t *callbacks[],
 const char *names[],
 -   int nvectors,
 +   bool use_msix,
 bool per_vq_vectors)
  {
   struct virtio_pci_device *vp_dev = to_vp_device(vdev);
   u16 vector;
 - int i, err, allocated_vectors;
 + int i, err, nvectors, allocated_vectors;
  
 - err = vp_request_vectors(vdev, nvectors, per_vq_vectors);
 + if (!use_msix) {
 + /* Old style: one normal interrupt for change and all vqs. */
 + vp_dev-msix_vectors = 0;
 + vp_dev-per_vq_vectors = false;
 + err =

Re: [PATCHv4 2/2] virtio: refactor find_vqs

2009-07-28 Thread Amit Shah 
On (Tue) Jul 28 2009 [12:44:31], Rusty Russell wrote:
 On Mon, 27 Jul 2009 01:17:09 am Michael S. Tsirkin wrote:
  This refactors find_vqs, making it more readable and robust, and fixing
  two regressions from 2.6.30:
  - double free_irq causing BUG_ON on device removal
  - probe failure when vq can't be assigned to msi-x vector
(reported on old host kernels)
  
  An older version of this patch was tested by Amit Shah.
 
 OK, I've applied both of these; I'd like to see a new test by Amit to
 make sure tho.

Tested these patches as well. They work fine.

Tested-by: Amit Shah amit.s...@redhat.com
--
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: [PATCHv4 2/2] virtio: refactor find_vqs

2009-07-28 Thread Michael S. Tsirkin 
On Tue, Jul 28, 2009 at 08:00:52PM +0530, Amit Shah wrote:
 On (Tue) Jul 28 2009 [12:44:31], Rusty Russell wrote:
  On Mon, 27 Jul 2009 01:17:09 am Michael S. Tsirkin wrote:
   This refactors find_vqs, making it more readable and robust, and fixing
   two regressions from 2.6.30:
   - double free_irq causing BUG_ON on device removal
   - probe failure when vq can't be assigned to msi-x vector
 (reported on old host kernels)
   
   An older version of this patch was tested by Amit Shah.
  
  OK, I've applied both of these; I'd like to see a new test by Amit to
  make sure tho.
 
 Tested these patches as well. They work fine.
 
 Tested-by: Amit Shah amit.s...@redhat.com

Amit, thanks very much for the testing.

-- 
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: [PATCHv4 2/2] virtio: refactor find_vqs

2009-07-27 Thread Rusty Russell 
On Mon, 27 Jul 2009 01:17:09 am Michael S. Tsirkin wrote:
 This refactors find_vqs, making it more readable and robust, and fixing
 two regressions from 2.6.30:
 - double free_irq causing BUG_ON on device removal
 - probe failure when vq can't be assigned to msi-x vector
   (reported on old host kernels)
 
 An older version of this patch was tested by Amit Shah.

OK, I've applied both of these; I'd like to see a new test by Amit to
make sure tho.

I really like this cleanup!  I looked harder at this code, and my best
attempts to untangle it further came to very little.  This is what I
ended up with, but it's all cosmetic and can wait until next merge window.
See what you think.

Thanks!
Rusty.

virtio_pci: minor MSI-X cleanups

1) Rename vp_request_vectors to vp_request_msix_vectors, and take
   non-MSI-X case out to caller.
2) Comment weird pci_enable_msix API
3) Rename vp_find_vq to setup_vq.
4) Fix spaces to tabs
5) Make nvectors calc internal to vp_try_to_find_vqs()

Signed-off-by: Rusty Russell ru...@rustcorp.com.au
---
 drivers/virtio/virtio_pci.c |   84 +++-
 1 file changed, 45 insertions(+), 39 deletions(-)

diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
--- a/drivers/virtio/virtio_pci.c
+++ b/drivers/virtio/virtio_pci.c
@@ -280,25 +280,14 @@ static void vp_free_vectors(struct virti
vp_dev-msix_entries = NULL;
 }
 
-static int vp_request_vectors(struct virtio_device *vdev, int nvectors,
- bool per_vq_vectors)
+static int vp_request_msix_vectors(struct virtio_device *vdev, int nvectors,
+  bool per_vq_vectors)
 {
struct virtio_pci_device *vp_dev = to_vp_device(vdev);
const char *name = dev_name(vp_dev-vdev.dev);
unsigned i, v;
int err = -ENOMEM;
 
-   if (!nvectors) {
-   /* Can't allocate MSI-X vectors, use regular interrupt */
-   vp_dev-msix_vectors = 0;
-   err = request_irq(vp_dev-pci_dev-irq, vp_interrupt,
- IRQF_SHARED, name, vp_dev);
-   if (err)
-   return err;
-   vp_dev-intx_enabled = 1;
-   return 0;
-   }
-
vp_dev-msix_entries = kmalloc(nvectors * sizeof *vp_dev-msix_entries,
   GFP_KERNEL);
if (!vp_dev-msix_entries)
@@ -311,6 +300,7 @@ static int vp_request_vectors(struct vir
for (i = 0; i  nvectors; ++i)
vp_dev-msix_entries[i].entry = i;
 
+   /* pci_enable_msix returns positive if we can't get this many. */
err = pci_enable_msix(vp_dev-pci_dev, vp_dev-msix_entries, nvectors);
if (err  0)
err = -ENOSPC;
@@ -356,10 +346,10 @@ error:
return err;
 }
 
-static struct virtqueue *vp_find_vq(struct virtio_device *vdev, unsigned index,
-   void (*callback)(struct virtqueue *vq),
-   const char *name,
-   u16 vector)
+static struct virtqueue *setup_vq(struct virtio_device *vdev, unsigned index,
+ void (*callback)(struct virtqueue *vq),
+ const char *name,
+ u16 vector)
 {
struct virtio_pci_device *vp_dev = to_vp_device(vdev);
struct virtio_pci_vq_info *info;
@@ -408,7 +398,7 @@ static struct virtqueue *vp_find_vq(stru
vq-priv = info;
info-vq = vq;
 
-if (vector != VIRTIO_MSI_NO_VECTOR) {
+   if (vector != VIRTIO_MSI_NO_VECTOR) {
iowrite16(vector, vp_dev-ioaddr + VIRTIO_MSI_QUEUE_VECTOR);
vector = ioread16(vp_dev-ioaddr + VIRTIO_MSI_QUEUE_VECTOR);
if (vector == VIRTIO_MSI_NO_VECTOR) {
@@ -484,14 +474,36 @@ static int vp_try_to_find_vqs(struct vir
  struct virtqueue *vqs[],
  vq_callback_t *callbacks[],
  const char *names[],
- int nvectors,
+ bool use_msix,
  bool per_vq_vectors)
 {
struct virtio_pci_device *vp_dev = to_vp_device(vdev);
u16 vector;
-   int i, err, allocated_vectors;
+   int i, err, nvectors, allocated_vectors;
 
-   err = vp_request_vectors(vdev, nvectors, per_vq_vectors);
+   if (!use_msix) {
+   /* Old style: one normal interrupt for change and all vqs. */
+   vp_dev-msix_vectors = 0;
+   vp_dev-per_vq_vectors = false;
+   err = request_irq(vp_dev-pci_dev-irq, vp_interrupt,
+ IRQF_SHARED, dev_name(vdev-dev), vp_dev);
+   if (!err)
+   vp_dev-intx_enabled = 1;
+   return err;
+   }
+
+   if (per_vq_vectors) {
+   /* Best option: one for change

[PATCHv4 2/2] virtio: refactor find_vqs

2009-07-26 Thread Michael S. Tsirkin 
This refactors find_vqs, making it more readable and robust, and fixing
two regressions from 2.6.30:
- double free_irq causing BUG_ON on device removal
- probe failure when vq can't be assigned to msi-x vector
  (reported on old host kernels)

An older version of this patch was tested by Amit Shah.

Reported-by: Amit Shah amit.s...@redhat.com
Signed-off-by: Michael S. Tsirkin m...@redhat.com
---
 drivers/virtio/virtio_pci.c |  212 ---
 1 files changed, 119 insertions(+), 93 deletions(-)

diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
index 4c74c72..c17b830 100644
--- a/drivers/virtio/virtio_pci.c
+++ b/drivers/virtio/virtio_pci.c
@@ -52,8 +52,10 @@ struct virtio_pci_device
char (*msix_names)[256];
/* Number of available vectors */
unsigned msix_vectors;
-   /* Vectors allocated */
+   /* Vectors allocated, excluding per-vq vectors if any */
unsigned msix_used_vectors;
+   /* Whether we have vector per vq */
+   bool per_vq_vectors;
 };
 
 /* Constants for MSI-X */
@@ -278,27 +280,24 @@ static void vp_free_vectors(struct virtio_device *vdev)
vp_dev-msix_entries = NULL;
 }
 
-static int vp_enable_msix(struct pci_dev *dev, struct msix_entry *entries,
- int *options, int noptions)
-{
-   int i;
-   for (i = 0; i  noptions; ++i)
-   if (!pci_enable_msix(dev, entries, options[i]))
-   return options[i];
-   return -EBUSY;
-}
-
-static int vp_request_vectors(struct virtio_device *vdev, unsigned max_vqs)
+static int vp_request_vectors(struct virtio_device *vdev, int nvectors,
+ bool per_vq_vectors)
 {
struct virtio_pci_device *vp_dev = to_vp_device(vdev);
const char *name = dev_name(vp_dev-vdev.dev);
unsigned i, v;
int err = -ENOMEM;
-   /* We want at most one vector per queue and one for config changes.
-* Fallback to separate vectors for config and a shared for queues.
-* Finally fall back to regular interrupts. */
-   int options[] = { max_vqs + 1, 2 };
-   int nvectors = max(options[0], options[1]);
+
+   if (!nvectors) {
+   /* Can't allocate MSI-X vectors, use regular interrupt */
+   vp_dev-msix_vectors = 0;
+   err = request_irq(vp_dev-pci_dev-irq, vp_interrupt,
+ IRQF_SHARED, name, vp_dev);
+   if (err)
+   return err;
+   vp_dev-intx_enabled = 1;
+   return 0;
+   }
 
vp_dev-msix_entries = kmalloc(nvectors * sizeof *vp_dev-msix_entries,
   GFP_KERNEL);
@@ -312,41 +311,34 @@ static int vp_request_vectors(struct virtio_device *vdev, 
unsigned max_vqs)
for (i = 0; i  nvectors; ++i)
vp_dev-msix_entries[i].entry = i;
 
-   err = vp_enable_msix(vp_dev-pci_dev, vp_dev-msix_entries,
-options, ARRAY_SIZE(options));
-   if (err  0) {
-   /* Can't allocate enough MSI-X vectors, use regular interrupt */
-   vp_dev-msix_vectors = 0;
-   err = request_irq(vp_dev-pci_dev-irq, vp_interrupt,
- IRQF_SHARED, name, vp_dev);
-   if (err)
-   goto error;
-   vp_dev-intx_enabled = 1;
-   } else {
-   vp_dev-msix_vectors = err;
-   vp_dev-msix_enabled = 1;
-
-   /* Set the vector used for configuration */
-   v = vp_dev-msix_used_vectors;
-   snprintf(vp_dev-msix_names[v], sizeof *vp_dev-msix_names,
-%s-config, name);
-   err = request_irq(vp_dev-msix_entries[v].vector,
- vp_config_changed, 0, vp_dev-msix_names[v],
- vp_dev);
-   if (err)
-   goto error;
-   ++vp_dev-msix_used_vectors;
+   err = pci_enable_msix(vp_dev-pci_dev, vp_dev-msix_entries, nvectors);
+   if (err  0)
+   err = -ENOSPC;
+   if (err)
+   goto error;
+   vp_dev-msix_vectors = nvectors;
+   vp_dev-msix_enabled = 1;
+
+   /* Set the vector used for configuration */
+   v = vp_dev-msix_used_vectors;
+   snprintf(vp_dev-msix_names[v], sizeof *vp_dev-msix_names,
+%s-config, name);
+   err = request_irq(vp_dev-msix_entries[v].vector,
+ vp_config_changed, 0, vp_dev-msix_names[v],
+ vp_dev);
+   if (err)
+   goto error;
+   ++vp_dev-msix_used_vectors;
 
-   iowrite16(v, vp_dev-ioaddr + VIRTIO_MSI_CONFIG_VECTOR);
-   /* Verify we had enough resources to assign the vector */
-   v = ioread16(vp_dev-ioaddr + VIRTIO_MSI_CONFIG_VECTOR);
-   if (v ==