Re: [PATCH] virtio-balloon: request nvqs based on features

["Michael S. Tsirkin" <mst@redhat.com>]

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@chromium.org> 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@chromium.org>
> > ---
> >  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