Re: [PATCHv4 2/2] virtio: refactor find_vqs

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

On Thu, Aug 27, 2009 at 07:00:34PM +0930, Rusty Russell wrote:
> On Tue, 25 Aug 2009 09:34:34 pm Michael S. Tsirkin wrote:
> > > That's because we didn't do the request_irq's for the per_vector case, because
> > > we don't have the names.  This is what prevented me from doing a nice
> > > encapsulation.
> > 
> > Yes. But let's split free_vectors out into free_msix_vectors and
> > free_intx as well?
> 
> Perhaps.  Patch welcome :)

Could you put the end result somewhere so I can work on top of it?

> > Yes, I agree, this is good cleanup, structure field names should be
> > desriptive.  Are you sure we want to make all local variables named
> > vector renamed to msix_vector though? CodingStyle says local var names
> > should be short ...  A couple of ideas below.
> 
> msix_vec would work.

Yes, ok.

> But vector for something which isn't always the vector
> is misleading, IMHO.

I think you mean it's isn't always used? It's always a vector ...

> > > -	if (vector != VIRTIO_MSI_NO_VECTOR) {
> > > -		iowrite16(vector, vp_dev->ioaddr + VIRTIO_MSI_QUEUE_VECTOR);
> > > -		vector = ioread16(vp_dev->ioaddr + VIRTIO_MSI_QUEUE_VECTOR);
> > > -		if (vector == VIRTIO_MSI_NO_VECTOR) {
> > > +	if (msix_vector != VIRTIO_MSI_NO_VECTOR) {
> > > +		iowrite16(msix_vector, vp_dev->ioaddr+VIRTIO_MSI_QUEUE_VECTOR);
> > > +		msix_vector = ioread16(vp_dev->ioaddr+VIRTIO_MSI_QUEUE_VECTOR);
> > 
> > checkpatch will complain that space is lacking around "+".
> > We won't have a problem if we keep it named vector.
> 
> Yeah, but OTOH ignoring checkpatch warnings is good for the soul.
> 
> > >  	if (!use_msix) {
> > >  		/* Old style: one normal interrupt for change and all vqs. */
> > >  		vp_dev->msix_vectors = 0;
> > > -		vp_dev->per_vq_vectors = false;
> > 
> > I know it's enough to look at msix_vectors, but isn't
> > it cleaner to have per_vq_vectors consistent as well?
> > E.g. del_vqs seems to only look at per_vq_vectors.
> 
> This should in fact be:
> 
>  	if (!use_msix) {
>  		/* Old style: one normal interrupt for change and all vqs. */
> -		vp_dev->msix_vectors = 0;
> -		vp_dev->per_vq_vectors = false;
>  		err = request_irq(vp_dev->pci_dev->irq, vp_interrupt,
>  				  IRQF_SHARED, dev_name(&vdev->dev), vp_dev);
> -		if (!err)
> -			vp_dev->intx_enabled = 1;
> -		return err;
> +		if (err)
> +			return err;
> +		vp_dev->intx_enabled = 1;
> 
> 
> The msix fields should all be ignored if msix_enabled is false.

Need to audit code to make sure it's so then.  I actually think
per_vq_vectors is kind of not directly tied to msix.
Don't you think it's nicer like del_vqs does:
if (vp_dev->per_vq_vectors) {
}

Than

if (vp_dev->msix_enabled && vp_dev->per_vq_vectors) {
}

BTW, let's get rid of msix_enabled completely?
We can always use msix_vectors ...

> Think about
> running valgrind (or equiv) over the code: if you initialize something which
> shouldn't be accessed, valgrind won't spot it.
> 
> For similar reasons, I dislike memseting structs to 0, or kzallocing them.

Yes, I agree generally.

> > >  		err = request_irq(vp_dev->pci_dev->irq, vp_interrupt,
> > >  				  IRQF_SHARED, dev_name(&vdev->dev), vp_dev);
> > > -		if (!err)
> > > -			vp_dev->intx_enabled = 1;
> > > -		return err;
> > > +		if (err)
> > > +			return err;
> > 
> > Maybe move this part out into a separate function?  This way
> > vp_try_to_find_vqs does high-level processing.
> 
> Nice.  Moved this to vp_request_intx().
> 
> > > +	} else {
> > > +		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 = 2;
> > 
> > Out of curiosity: why do you put {} here? They aren't
> > strictly necessary ...
> 
> Comment made it multiline, looked a bit neater?
> 
> Here's the diff result:

Looks good to me. When we are done with cleanups, need to remember to
test this with all the possible combinations: no msix, 2 vectors, 1
vector.

> diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
> --- a/drivers/virtio/virtio_pci.c
> +++ b/drivers/virtio/virtio_pci.c
> @@ -346,10 +346,22 @@ error:
>  	return err;
>  }
>  
> +static int vp_request_intx(struct virtio_device *vdev)
> +{
> +	int err;
> +	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
> +
> +	err = request_irq(vp_dev->pci_dev->irq, vp_interrupt,
> +			  IRQF_SHARED, dev_name(&vdev->dev), vp_dev);
> +	if (!err)
> +		vp_dev->intx_enabled = 1;
> +	return err;
> +}
> +
>  static struct virtqueue *setup_vq(struct virtio_device *vdev, unsigned index,
>  				  void (*callback)(struct virtqueue *vq),
>  				  const char *name,
> -				  u16 msix_vector)
> +				  u16 msix_vec)
>  {
>  	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
>  	struct virtio_pci_vq_info *info;
> @@ -374,7 +386,7 @@ static struct virtqueue *setup_vq(struct
>  
>  	info->queue_index = index;
>  	info->num = num;
> -	info->msix_vector = msix_vector;
> +	info->msix_vector = msix_vec;
>  
>  	size = PAGE_ALIGN(vring_size(num, VIRTIO_PCI_VRING_ALIGN));
>  	info->queue = alloc_pages_exact(size, GFP_KERNEL|__GFP_ZERO);
> @@ -398,10 +410,10 @@ static struct virtqueue *setup_vq(struct
>  	vq->priv = info;
>  	info->vq = vq;
>  
> -	if (msix_vector != VIRTIO_MSI_NO_VECTOR) {
> -		iowrite16(msix_vector, vp_dev->ioaddr+VIRTIO_MSI_QUEUE_VECTOR);
> -		msix_vector = ioread16(vp_dev->ioaddr+VIRTIO_MSI_QUEUE_VECTOR);
> -		if (msix_vector == VIRTIO_MSI_NO_VECTOR) {
> +	if (msix_vec != VIRTIO_MSI_NO_VECTOR) {
> +		iowrite16(msix_vec, vp_dev->ioaddr + VIRTIO_MSI_QUEUE_VECTOR);
> +		msix_vec = ioread16(vp_dev->ioaddr + VIRTIO_MSI_QUEUE_VECTOR);
> +		if (msix_vec == VIRTIO_MSI_NO_VECTOR) {
>  			err = -EBUSY;
>  			goto out_assign;
>  		}
> @@ -479,16 +491,14 @@ static int vp_try_to_find_vqs(struct vir
>  			      bool per_vq_vectors)
>  {
>  	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
> -	u16 msix_vector;
> +	u16 msix_vec;
>  	int i, err, nvectors, allocated_vectors;
>  
>  	if (!use_msix) {
>  		/* Old style: one normal interrupt for change and all vqs. */
> -		err = request_irq(vp_dev->pci_dev->irq, vp_interrupt,
> -				  IRQF_SHARED, dev_name(&vdev->dev), vp_dev);
> +		err = vp_request_intx(vdev);
>  		if (err)
> -			return err;
> -		vp_dev->intx_enabled = 1;
> +			goto error_request;
>  	} else {
>  		if (per_vq_vectors) {
>  			/* Best option: one for change interrupt, one per vq. */
> @@ -510,24 +520,24 @@ static int vp_try_to_find_vqs(struct vir
>  	allocated_vectors = vp_dev->msix_used_vectors;
>  	for (i = 0; i < nvqs; ++i) {
>  		if (!callbacks[i] || !vp_dev->msix_enabled)
> -			msix_vector = VIRTIO_MSI_NO_VECTOR;
> +			msix_vec = VIRTIO_MSI_NO_VECTOR;
>  		else if (vp_dev->per_vq_vectors)
> -			msix_vector = allocated_vectors++;
> +			msix_vec = allocated_vectors++;
>  		else
> -			msix_vector = VP_MSIX_VQ_VECTOR;
> -		vqs[i] = setup_vq(vdev, i, callbacks[i], names[i], msix_vector);
> +			msix_vec = VP_MSIX_VQ_VECTOR;
> +		vqs[i] = setup_vq(vdev, i, callbacks[i], names[i], msix_vec);
>  		if (IS_ERR(vqs[i])) {
>  			err = PTR_ERR(vqs[i]);
>  			goto error_find;
>  		}
>  		/* allocate per-vq irq if available and necessary */
>  		if (vp_dev->per_vq_vectors) {
> -			snprintf(vp_dev->msix_names[msix_vector],
> +			snprintf(vp_dev->msix_names[msix_vec],
>  				 sizeof *vp_dev->msix_names,
>  				 "%s-%s",
>  				 dev_name(&vp_dev->vdev.dev), names[i]);
> -			err = request_irq(msix_vector, vring_interrupt, 0,
> -					  vp_dev->msix_names[msix_vector],
> +			err = request_irq(msix_vec, vring_interrupt, 0,
> +					  vp_dev->msix_names[msix_vec],
>  					  vqs[i]);
>  			if (err) {
>  				vp_del_vq(vqs[i]);