linux-kernel - Re: [RFC PATCH 2/2] Assign a new irq handler while irqfd enabled

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20141026115646.GB5497@redhat.com>
Date:	Sun, 26 Oct 2014 13:56:46 +0200
From:	"Michael S. Tsirkin" <mst@...hat.com>
To:	"john.liuli" <john.liuli@...wei.com>
Cc:	linux-kernel@...r.kernel.org, qemu-devel@...gnu.org,
	peter.huangpeng@...wei.com, rusty@...tcorp.com.au,
	virtualization@...ts.linux-foundation.org,
	n.nikolaev@...tualopensystems.com, yingshiuan.pan@...il.com,
	remy.gauguey@....fr, joel.schopp@....com
Subject: Re: [RFC PATCH 2/2] Assign a new irq handler while irqfd enabled

On Sat, Oct 25, 2014 at 04:24:54PM +0800, john.liuli wrote:
> From: Li Liu <john.liuli@...wei.com>
> 
> This irq handler will get the interrupt reason from a
> shared memory. And will be assigned only while irqfd
> enabled.
> 
> Signed-off-by: Li Liu <john.liuli@...wei.com>
> ---
>  drivers/virtio/virtio_mmio.c |   34 ++++++++++++++++++++++++++++++++--
>  1 file changed, 32 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
> index 28ddb55..7229605 100644
> --- a/drivers/virtio/virtio_mmio.c
> +++ b/drivers/virtio/virtio_mmio.c
> @@ -259,7 +259,31 @@ static irqreturn_t vm_interrupt(int irq, void *opaque)
>  	return ret;
>  }
>  
> +/* Notify all virtqueues on an interrupt. */
> +static irqreturn_t vm_interrupt_irqfd(int irq, void *opaque)
> +{
> +	struct virtio_mmio_device *vm_dev = opaque;
> +	struct virtio_mmio_vq_info *info;
> +	unsigned long status;
> +	unsigned long flags;
> +	irqreturn_t ret = IRQ_NONE;
>  
> +	/* Read the interrupt reason and reset it */
> +	status = *vm_dev->isr_mem;
> +	*vm_dev->isr_mem = 0x0;

you are reading and modifying shared memory
without atomics and any memory barriers.
Why is this safe?

> +
> +	if (unlikely(status & VIRTIO_MMIO_INT_CONFIG)) {
> +		virtio_config_changed(&vm_dev->vdev);
> +		ret = IRQ_HANDLED;
> +	}
> +
> +	spin_lock_irqsave(&vm_dev->lock, flags);
> +	list_for_each_entry(info, &vm_dev->virtqueues, node)
> +		ret |= vring_interrupt(irq, info->vq);
> +	spin_unlock_irqrestore(&vm_dev->lock, flags);
> +
> +	return ret;
> +}
>  
>  static void vm_del_vq(struct virtqueue *vq)
>  {

So you invoke callbacks for all VQs.
This won't scale well as the number of VQs grows, will it?

> @@ -391,6 +415,7 @@ error_available:
>  	return ERR_PTR(err);
>  }
>  
> +#define VIRTIO_MMIO_F_IRQFD        (1 << 7)
>  static int vm_find_vqs(struct virtio_device *vdev, unsigned nvqs,
>  		       struct virtqueue *vqs[],
>  		       vq_callback_t *callbacks[],
> @@ -400,8 +425,13 @@ static int vm_find_vqs(struct virtio_device *vdev, unsigned nvqs,
>  	unsigned int irq = platform_get_irq(vm_dev->pdev, 0);
>  	int i, err;
>  
> -	err = request_irq(irq, vm_interrupt, IRQF_SHARED,
> -			dev_name(&vdev->dev), vm_dev);
> +	if (*vm_dev->isr_mem & VIRTIO_MMIO_F_IRQFD) {
> +		err = request_irq(irq, vm_interrupt_irqfd, IRQF_SHARED,
> +				  dev_name(&vdev->dev), vm_dev);
> +	} else {
> +		err = request_irq(irq, vm_interrupt, IRQF_SHARED,
> +				  dev_name(&vdev->dev), vm_dev);
> +	}
>  	if (err)
>  		return err;


So still a single interrupt for all VQs.
Again this doesn't scale: a single CPU has to handle
interrupts for all of them.
I think you need to find a way to get per-VQ interrupts.

> -- 
> 1.7.9.5
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/