Re: [dpdk-dev] [PATCH v5 5/6] eal: add per rx queue interrupt handling based on VFIO

[David Marchand <david.marchand@6wind.com>]

Hello Danny,

On Mon, Feb 23, 2015 at 5:55 PM, Zhou Danny <danny.zhou@intel.com> wrote:

[snip]

+/**
> + * @param intr_handle
> + *   pointer to the interrupt handle.
> + * @param queue_id
> + *   the queue id
> + * @return
> + *   - On success, return 0
> + *   - On failure, returns -1.
> + */
> +int rte_intr_wait_rx_pkt(struct rte_intr_handle *intr_handle,
> +                       uint8_t queue_id);
> +
>

>From my point of view, the queue_id (just like port_id) is something that
should be handled by ethdev, not eal.

diff --git a/lib/librte_eal/linuxapp/eal/eal_interrupts.c
> b/lib/librte_eal/linuxapp/eal/eal_interrupts.c
> index 8c5b834..ee0f019 100644
> --- a/lib/librte_eal/linuxapp/eal/eal_interrupts.c
> +++ b/lib/librte_eal/linuxapp/eal/eal_interrupts.c
>

 [snip]

+int
> +rte_intr_wait_rx_pkt(struct rte_intr_handle *intr_handle, uint8_t
> queue_id)
> +{
> +       struct epoll_event ev;
> +       unsigned numfds = 0;
> +
> +       if (!intr_handle || intr_handle->fd < 0 || intr_handle->uio_cfg_fd
> < 0)
> +               return -1;
> +       if (queue_id >= VFIO_MAX_QUEUE_ID)
> +               return -1;
> +
> +       /* create epoll fd */
> +       int pfd = epoll_create(1);
> +       if (pfd < 0) {
> +               RTE_LOG(ERR, EAL, "Cannot create epoll instance\n");
> +               return -1;
> +       }
>

Why recreate the epoll instance at each call to this function ?

+
> +       rte_spinlock_lock(&intr_lock);
> +
> +       ev.events = EPOLLIN | EPOLLPRI;
> +       switch (intr_handle->type) {
> +       case RTE_INTR_HANDLE_UIO:
> +               ev.data.fd = intr_handle->fd;
> +               break;
> +#ifdef VFIO_PRESENT
> +       case RTE_INTR_HANDLE_VFIO_MSIX:
> +       case RTE_INTR_HANDLE_VFIO_MSI:
> +       case RTE_INTR_HANDLE_VFIO_LEGACY:
> +               ev.data.fd = intr_handle->queue_fd[queue_id];
> +               break;
> +#endif
> +       default:
> +               rte_spinlock_unlock(&intr_lock);
> +               close(pfd);
> +               return -1;
> +       }
> +
> +       if (epoll_ctl(pfd, EPOLL_CTL_ADD, ev.data.fd, &ev) < 0) {
> +               RTE_LOG(ERR, EAL, "Error adding fd %d epoll_ctl, %s\n",
> +                       intr_handle->queue_fd[queue_id], strerror(errno));
> +       } else
> +               numfds++;
> +
> +       rte_spinlock_unlock(&intr_lock);
> +       /* serve the interrupt */
> +       eal_intr_handle_rx_interrupts(intr_handle, pfd, numfds);
> +
> +       /**
> +       * when we return, we need to rebuild the
> +       * list of fds to monitor.
> +       */
> +       close(pfd);
>

Why do we need to rebuild this "list of fds" ?
Afaics, the fds we want to observe are not supposed to change in the
meantime.
epoll maintains this list, you don't have to care about this.


Looking at this patchset, I think there is a design issue.
eal does not need to know about portid neither queueid.

eal can provide an api to retrieve the interrupt fds, configure an epoll
instance, wait on an epoll instance etc...
ethdev is then responsible to setup the mapping between port id / queue id
and interrupt fds by asking the eal about those fds.

This would result in an eal api even simpler and we could add other fds in
a single epoll fd for other uses.


-- 
David Marchand