On Mon, 16 Dec 2019 15:14:29 -0800
Daniel Verkamp <dverkamp@xxxxxxxxxxxx> wrote:
> After 86a559787e6f ("virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_HINT"),
> the virtio-balloon device unconditionally specifies 4 virtqueues as the
> argument to find_vqs(), which means that 5 MSI-X vectors are required in
> order to assign one vector per VQ plus one for configuration changes.
>
> However, in cases where the virtio device only provides exactly as many
> MSI-X vectors as required for the features it implements (e.g. 3 for the
> basic configuration of inflate + deflate + config), this results in the
> selection of the fallback configuration where one interrupt vector is
> used for all VQs instead of having one VQ per vector.
>
> Restore the logic that chose nvqs conditionally based on enabled
> features, which was removed as part of the aforementioned commit.
> This is slightly more complex than just incrementing a counter of the
> number of VQs, since the queue for a given feature is assigned a fixed
> index.
As Wei already said, this should not be necessary, but see below.
>
> Signed-off-by: Daniel Verkamp <dverkamp@xxxxxxxxxxxx>
> ---
> drivers/virtio/virtio_balloon.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index 93f995f6cf36..67c6318d77c7 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -465,6 +465,7 @@ static int init_vqs(struct virtio_balloon *vb)
> vq_callback_t *callbacks[VIRTIO_BALLOON_VQ_MAX];
> const char *names[VIRTIO_BALLOON_VQ_MAX];
> int err;
> + unsigned nvqs;
>
> /*
> * Inflateq and deflateq are used unconditionally. The names[]
> @@ -475,20 +476,24 @@ static int init_vqs(struct virtio_balloon *vb)
> names[VIRTIO_BALLOON_VQ_INFLATE] = "inflate";
> callbacks[VIRTIO_BALLOON_VQ_DEFLATE] = balloon_ack;
> names[VIRTIO_BALLOON_VQ_DEFLATE] = "deflate";
> + nvqs = VIRTIO_BALLOON_VQ_DEFLATE + 1;
> +
> names[VIRTIO_BALLOON_VQ_STATS] = NULL;
> names[VIRTIO_BALLOON_VQ_FREE_PAGE] = NULL;
Note that we set names[q] to NULL, but not callbacks[q].
>
> if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_STATS_VQ)) {
> names[VIRTIO_BALLOON_VQ_STATS] = "stats";
> callbacks[VIRTIO_BALLOON_VQ_STATS] = stats_request;
> + nvqs = VIRTIO_BALLOON_VQ_STATS + 1;
> }
>
> if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT)) {
> names[VIRTIO_BALLOON_VQ_FREE_PAGE] = "free_page_vq";
> callbacks[VIRTIO_BALLOON_VQ_FREE_PAGE] = NULL;
> + nvqs = VIRTIO_BALLOON_VQ_FREE_PAGE + 1;
> }
>
> - err = vb->vdev->config->find_vqs(vb->vdev, VIRTIO_BALLOON_VQ_MAX,
> + err = vb->vdev->config->find_vqs(vb->vdev, nvqs,
> vqs, callbacks, names, NULL, NULL);
This will end up in vp_find_vqs_msix() eventually, which will try to
determine the number of needed vectors based upon whether callbacks[q]
is !NULL. That's probably the reason you end up trying to use more
vectors than needed. (Further down in that function, the code will skip
any queue with names[q] == NULL, but that's too late for determining
the number of vectors.)
So I think that either (a) virtio-pci should avoid trying to allocate a
vector for queues with names[q] == NULL, or (b) virtio-balloon should
clean out callbacks[q] for unused queues as well. Maybe both?
> if (err)
> return err;
_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
