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@xxxxxxxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
