| Message ID | 20090721155925.GC3306@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 22 Jul 2009 01:29:25 am Michael S. Tsirkin wrote: > - if (info->vector != VIRTIO_MSI_NO_VECTOR) > + if (info->vector != VIRTIO_MSI_NO_VECTOR) { > free_irq(vp_dev->msix_entries[info->vector].vector, vq); > + --vp_dev->msix_used_vectors; > + } > This only works because the only current caller of vp_del_vq is vp_del_vqs, so msix_used_vectors will be 0 after all the queues have been freed. Make up your mind. Either find_vq allocates and del_vq frees, or it's find_vqs and del_vqs. I suggest the former, and setting the value VIRTIO_MSI_NO_VECTOR to indicate it's already freed. I think with some cleanups, that loop in vp_free_vectors might go away, too. Rusty. -- 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
On Thu, Jul 23, 2009 at 02:10:31PM +0930, Rusty Russell wrote: > On Wed, 22 Jul 2009 01:29:25 am Michael S. Tsirkin wrote: > > - if (info->vector != VIRTIO_MSI_NO_VECTOR) > > + if (info->vector != VIRTIO_MSI_NO_VECTOR) { > > free_irq(vp_dev->msix_entries[info->vector].vector, vq); > > + --vp_dev->msix_used_vectors; > > + } > > > > This only works because the only current caller of vp_del_vq is vp_del_vqs, Right > so msix_used_vectors will be 0 after all the queues have been freed. Not 0, actually, we have vectors for control and possibly a shared vector for all vqs. > Make up your mind. Either find_vq allocates and del_vq frees, or it's find_vqs > and del_vqs. I suggest the former, and setting the value VIRTIO_MSI_NO_VECTOR > to indicate it's already freed. I think with some cleanups, that loop in > vp_free_vectors might go away, too. > > Rusty. IOW, msix_used_vectors counter will be for control and shared vector for all vqs, excluding per-vq vectors. Makes sense. I'll put out a patch.
On Thu, Jul 23, 2009 at 02:10:31PM +0930, Rusty Russell wrote: > On Wed, 22 Jul 2009 01:29:25 am Michael S. Tsirkin wrote: > > - if (info->vector != VIRTIO_MSI_NO_VECTOR) > > + if (info->vector != VIRTIO_MSI_NO_VECTOR) { > > free_irq(vp_dev->msix_entries[info->vector].vector, vq); > > + --vp_dev->msix_used_vectors; > > + } > > > > This only works because the only current caller of vp_del_vq is vp_del_vqs, so > msix_used_vectors will be 0 after all the queues have been freed. > > Make up your mind. Either find_vq allocates and del_vq frees, or it's find_vqs > and del_vqs. I suggest the former, and setting the value VIRTIO_MSI_NO_VECTOR > to indicate it's already freed. Hmm, there's nowhere to set this value: del_vq does kfree on info. But I think I see a solution. Cleaned up patch RSN. > I think with some cleanups, that loop in vp_free_vectors might go away, too. Hmm, I don't see how, yet. It's there to free the common vectors: config and shared vq vector. > Rusty. > > > -- > 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 -- 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 index dab3c86..9dcc368 100644 --- a/drivers/virtio/virtio_pci.c +++ b/drivers/virtio/virtio_pci.c @@ -466,8 +466,10 @@ static void vp_del_vq(struct virtqueue *vq) iowrite16(info->queue_index, vp_dev->ioaddr + VIRTIO_PCI_QUEUE_SEL); - if (info->vector != VIRTIO_MSI_NO_VECTOR) + if (info->vector != VIRTIO_MSI_NO_VECTOR) { free_irq(vp_dev->msix_entries[info->vector].vector, vq); + --vp_dev->msix_used_vectors; + } if (vp_dev->msix_enabled) { iowrite16(VIRTIO_MSI_NO_VECTOR,
Decrement used vectors counter when removing the vq so that vp_free_vectors does not try to free the vector again. Signed-off-by: Michael S. Tsirkin <mst@redhat.com> --- drivers/virtio/virtio_pci.c | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-)