Re: [PATCH v2 5/9] net/virtio: setup rxq interrupts

[Yuanhan Liu <yuanhan.liu@linux.intel.com>]

On Wed, Jan 04, 2017 at 02:56:50PM +0800, Tan, Jianfeng wrote:
> 
> 
> On 12/30/2016 2:27 PM, Yuanhan Liu wrote:
> >On Thu, Dec 29, 2016 at 07:30:39AM +0000, Jianfeng Tan wrote:
> >>This patch mainly allocates structure to store queue/irq mapping,
> >>and configure queue/irq mapping down through PCI ops. It also creates
> >>eventfds for each Rx queue and tell the kernel about the eventfd/intr
> >>binding.
> >>
> >>Mostly importantly, different from previous NICs (usually implements
> >>these logic in dev_start()), virtio's interrupt settings should be
> >>configured down to QEMU before sending DRIVER_OK notification.
> >Isn't it obvious we have to have all driver stuff (including interrupt
> >settings) configured properly before setting DRIVER_OK? :) That said,
> >it's meanless to state the fact that virtio acts differently than other
> >nics here on dev_start/stop.
> >
> >>Note: We only support 1:1 queue/irq mapping so far, which means, each
> >>rx queue has one exclusive interrupt (corresponding to irqfd in the
> >>qemu/kvm) to get notified when packets are available on that queue.
> >That means you have to setup the "vectors=N" option has to set correctly
> >in QEMU, otherwise it won't work?
> 
> Yes, actually, the correct value should be "vectors>=N+1", with N standing

Yeah, and it's a typo.

> for the number of queue pairs. It's due to the hard coded mapping logic:
> 0 -> config irq
> 1 -> rxq0
> 2 -> rxq1
> ...
> 
> >  If so, you also have to doc it somewhere.
> 
> Agreed.
> 
> [...]
> >>+
> >>+	if (virtio_queues_bind_intr(dev) < 0) {
> >>+		PMD_INIT_LOG(ERR, "Failed to bind queue/interrupt");
> >>+		return -1;
> >You have to free intr_handle->intr_vec, otherwise, memory leak occurs.
> 
> It's freed at dev_close(). Do you mean freeing and reallocating here? As

The typical way is free the resources have been allocated when errors
happens.

> nr_rx_queues is not a changeable value, I don't see the necessity here. I
> miss something?

No. nb_rx_queues does change, when people reconfigure the queue number.
However, the MAX queues the virito supports does not change. You could
use that number for allocation.

	--yliu