On Thu, 23 Jul 2009 09:27:31 pm Michael S. Tsirkin wrote:
> msix_user_vectors counted both per-vq and shared/config vectors.
> This causes BUG_ON when device is removed, as
> free_vectors tries to free per-vq vectors.
OK, I looked at this patch, then looked at this code (after it was applied).
I'm still very confused.
Looking at the call site for vp_find_vq:
for (i = 0; i < nvqs; ++i) {
if (!callbacks[i])
vector = per_vq_vector = VIRTIO_MSI_NO_VECTOR;
else if (vp_dev->msix_used_vectors < vp_dev->msix_vectors)
per_vq_vector = vector = allocated_vectors++;
else {
vector = VP_MSIX_VQ_VECTOR;
per_vq_vector = VIRTIO_MSI_NO_VECTOR;
}
Now, I can't find where msix_used_vectors is set, only once where it's
incremented. It seems completely redundant and confusing now? And
this "< vp_dev->msix_vectors" test is wrong?
AFAICT there are three cases:
1) We don't have MSI, so we use a normal interrupt for all vqs (old style).
This request_irq is done in vp_request_vectors.
2) We get some, but not enough for one per queue. We then use 2: one for
changes, and one for all vqs. Requested in vp_request_vectors.
3) We get enough. Use one for changes, one per vq. Each vq
requests in vp_find_vq.
I suggest you be explicit, and don't do any request in vp_find_vq(). Do the
per-vq request (case 3) in the find_vqs() loop, so vp_find_vq doesn't need
to know anything except to do the iowrite16 if vector != VIRTIO_MSI_NO_VECTOR.
Maybe an explicit "bool per_vq_vectors" would make it clearer, too.
Note: this is partially my fault for not reviewing this code when it went in.
I know Anthony is disclaiming virtio_pci :)
Thanks,
Rusty.
--
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
