On Tue, Dec 17, 2019 at 10:11:08AM +0100, Cornelia Huck wrote:
> 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;
I'm inclined to either do a or both.
--
MST
_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
