Re: [PATCH 4/9] virtio_pci: simplify MSI-X setup

[Jason Wang <jasowang@xxxxxxxxxx>]

On 2017年01月27日 16:16, Christoph Hellwig wrote:
> Try to grab the MSI-X vectors early and fall back to the shared one
> before doing lots of allocations.
>
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> ---

Reviewed-by: Jason Wang <jasowang@xxxxxxxxxx>

>   drivers/virtio/virtio_pci_common.c | 58 +++++++++++++++++++-------------------
>   1 file changed, 29 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
> index 9c4ad7d3f..74ff74c0 100644
> --- a/drivers/virtio/virtio_pci_common.c
> +++ b/drivers/virtio/virtio_pci_common.c
> @@ -142,32 +142,39 @@ void vp_del_vqs(struct virtio_device *vdev)
>   }
>   
>   static int vp_find_vqs_msix(struct virtio_device *vdev, unsigned nvqs,
> -			      struct virtqueue *vqs[],
> -			      vq_callback_t *callbacks[],
> -			      const char * const names[],
> -			      bool per_vq_vectors)
> +		struct virtqueue *vqs[], vq_callback_t *callbacks[],
> +		const char * const names[])
>   {
>   	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
>   	const char *name = dev_name(&vp_dev->vdev.dev);
>   	int i, err = -ENOMEM, allocated_vectors, nvectors;
> +	bool shared = false;
>   	u16 msix_vec;
>   
> -	if (per_vq_vectors) {
> -		/* Best option: one for change interrupt, one per vq. */
> -		nvectors = 1;
> -		for (i = 0; i < nvqs; ++i)
> -			if (callbacks[i])
> -				++nvectors;
> -	} else {
> -		/* Second best: one for change, shared for all vqs. */
> +	nvectors = 1;
> +	for (i = 0; i < nvqs; i++) {
> +		if (callbacks[i])
> +			nvectors++;
> +	}
> +
> +	/* Try one vector per queue first. */
> +	err = pci_alloc_irq_vectors(vp_dev->pci_dev, nvectors, nvectors,
> +			PCI_IRQ_MSIX);
> +	if (err < 0) {
> +		/* Fallback to one vector for config, one shared for queues. */
> +		shared = true;
>   		nvectors = 2;
> +		err = pci_alloc_irq_vectors(vp_dev->pci_dev, nvectors, nvectors,
> +				PCI_IRQ_MSIX);
> +		if (err < 0)
> +			return err;
>   	}
>   
>   	vp_dev->msix_vectors = nvectors;
>   	vp_dev->msix_names = kmalloc_array(nvectors,
>   			sizeof(*vp_dev->msix_names), GFP_KERNEL);
>   	if (!vp_dev->msix_names)
> -		return err;
> +		goto out_free_irq_vectors;
>   
>   	vp_dev->msix_affinity_masks = kcalloc(nvectors,
>   			sizeof(*vp_dev->msix_affinity_masks), GFP_KERNEL);
> @@ -180,18 +187,13 @@ static int vp_find_vqs_msix(struct virtio_device *vdev, unsigned nvqs,
>   			goto out_free_msix_affinity_masks;
>   	}
>   
> -	err = pci_alloc_irq_vectors(vp_dev->pci_dev, nvectors, nvectors,
> -			PCI_IRQ_MSIX);
> -	if (err < 0)
> -		goto out_free_msix_affinity_masks;
> -
>   	/* Set the vector used for configuration */
>   	snprintf(vp_dev->msix_names[0], sizeof(*vp_dev->msix_names),
>   		 "%s-config", name);
>   	err = request_irq(pci_irq_vector(vp_dev->pci_dev, 0), vp_config_changed,
>   			0, vp_dev->msix_names[0], vp_dev);
>   	if (err)
> -		goto out_free_irq_vectors;
> +		goto out_free_msix_affinity_masks;
>   
>   	/* Verify we had enough resources to assign the vector */
>   	if (vp_dev->config_vector(vp_dev, 0) == VIRTIO_MSI_NO_VECTOR) {
> @@ -241,7 +243,11 @@ static int vp_find_vqs_msix(struct virtio_device *vdev, unsigned nvqs,
>   		}
>   		vp_dev->msix_vector_map[i] = msix_vec;
>   
> -		if (per_vq_vectors)
> +		/*
> +		 * Use a different vector for each queue if they are available,
> +		 * else share the same vector for all VQs.
> +		 */
> +		if (!shared)
>   			allocated_vectors++;
>   	}
>   
> @@ -254,8 +260,6 @@ static int vp_find_vqs_msix(struct virtio_device *vdev, unsigned nvqs,
>   	vp_dev->config_vector(vp_dev, VIRTIO_MSI_NO_VECTOR);
>   out_free_config_irq:
>   	free_irq(pci_irq_vector(vp_dev->pci_dev, 0), vp_dev);
> -out_free_irq_vectors:
> -	pci_free_irq_vectors(vp_dev->pci_dev);
>   out_free_msix_affinity_masks:
>   	for (i = 0; i < nvectors; i++) {
>   		if (vp_dev->msix_affinity_masks[i])
> @@ -264,6 +268,8 @@ static int vp_find_vqs_msix(struct virtio_device *vdev, unsigned nvqs,
>   	kfree(vp_dev->msix_affinity_masks);
>   out_free_msix_names:
>   	kfree(vp_dev->msix_names);
> +out_free_irq_vectors:
> +	pci_free_irq_vectors(vp_dev->pci_dev);
>   	return err;
>   }
>   
> @@ -308,15 +314,9 @@ int vp_find_vqs(struct virtio_device *vdev, unsigned nvqs,
>   {
>   	int err;
>   
> -	/* Try MSI-X with one vector per queue. */
> -	err = vp_find_vqs_msix(vdev, nvqs, vqs, callbacks, names, true);
> -	if (!err)
> -		return 0;
> -	/* Fallback: MSI-X with one vector for config, one shared for queues. */
> -	err = vp_find_vqs_msix(vdev, nvqs, vqs, callbacks, names, false);
> +	err = vp_find_vqs_msix(vdev, nvqs, vqs, callbacks, names);
>   	if (!err)
>   		return 0;
> -	/* Finally fall back to regular interrupts. */
>   	return vp_find_vqs_intx(vdev, nvqs, vqs, callbacks, names);
>   }
>   

--
To unsubscribe from this list: send the line "unsubscribe linux-block" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html