| Message ID | 200908100907.53059.rusty@rustcorp.com.au (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <rusty@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) { > 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, err, nvectors, allocated_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; 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. > 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. > + } 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 ... > + } > + > + err = vp_request_msix_vectors(vdev, nvectors, per_vq_vectors); > + if (err) > + goto error_request; > } > > - 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 one for all vqs. */ > - nvectors = 2; > - } > - > - err = vp_request_msix_vectors(vdev, nvectors, per_vq_vectors); > - if (err) > - goto error_request; > - > vp_dev->per_vq_vectors = per_vq_vectors; > allocated_vectors = vp_dev->msix_used_vectors; > for (i = 0; i < nvqs; ++i) { > if (!callbacks[i] || !vp_dev->msix_enabled) > - vector = VIRTIO_MSI_NO_VECTOR; > + msix_vector = VIRTIO_MSI_NO_VECTOR; > else if (vp_dev->per_vq_vectors) > - vector = allocated_vectors++; > + msix_vector = allocated_vectors++; > else > - vector = VP_MSIX_VQ_VECTOR; > - vqs[i] = setup_vq(vdev, i, callbacks[i], names[i], vector); > + msix_vector = VP_MSIX_VQ_VECTOR; > + vqs[i] = setup_vq(vdev, i, callbacks[i], names[i], msix_vector); > if (IS_ERR(vqs[i])) { > err = PTR_ERR(vqs[i]); > goto error_find; > } > /* allocate per-vq irq if available and necessary */ > - if (vp_dev->per_vq_vectors && vector != VIRTIO_MSI_NO_VECTOR) { > - snprintf(vp_dev->msix_names[vector], > + if (vp_dev->per_vq_vectors) { > + snprintf(vp_dev->msix_names[msix_vector], > sizeof *vp_dev->msix_names, > "%s-%s", > dev_name(&vp_dev->vdev.dev), names[i]); > - err = request_irq(vp_dev->msix_entries[vector].vector, > - vring_interrupt, 0, > - vp_dev->msix_names[vector], vqs[i]); > + err = request_irq(msix_vector, vring_interrupt, 0, > + vp_dev->msix_names[msix_vector], > + vqs[i]); > if (err) { > vp_del_vq(vqs[i]); > goto error_find; -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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, err, nvectors, allocated_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 (err) + return err; + } 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; + } + + err = vp_request_msix_vectors(vdev, nvectors, per_vq_vectors); + if (err) + goto error_request; } - 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 one for all vqs. */ - nvectors = 2; - } - - err = vp_request_msix_vectors(vdev, nvectors, per_vq_vectors); - if (err) - goto error_request; - vp_dev->per_vq_vectors = per_vq_vectors; allocated_vectors = vp_dev->msix_used_vectors; for (i = 0; i < nvqs; ++i) { if (!callbacks[i] || !vp_dev->msix_enabled) - vector = VIRTIO_MSI_NO_VECTOR; + msix_vector = VIRTIO_MSI_NO_VECTOR; else if (vp_dev->per_vq_vectors) - vector = allocated_vectors++; + msix_vector = allocated_vectors++; else - vector = VP_MSIX_VQ_VECTOR; - vqs[i] = setup_vq(vdev, i, callbacks[i], names[i], vector); + msix_vector = VP_MSIX_VQ_VECTOR; + vqs[i] = setup_vq(vdev, i, callbacks[i], names[i], msix_vector); if (IS_ERR(vqs[i])) { err = PTR_ERR(vqs[i]); goto error_find; } /* allocate per-vq irq if available and necessary */ - if (vp_dev->per_vq_vectors && vector != VIRTIO_MSI_NO_VECTOR) { - snprintf(vp_dev->msix_names[vector], + if (vp_dev->per_vq_vectors) { + snprintf(vp_dev->msix_names[msix_vector], sizeof *vp_dev->msix_names, "%s-%s", dev_name(&vp_dev->vdev.dev), names[i]); - err = request_irq(vp_dev->msix_entries[vector].vector, - vring_interrupt, 0, - vp_dev->msix_names[vector], vqs[i]); + err = request_irq(msix_vector, vring_interrupt, 0, + vp_dev->msix_names[msix_vector], + vqs[i]); if (err) { vp_del_vq(vqs[i]); goto error_find;