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

["Zhou, Danny" <danny.zhou@intel.com>]

Thanks for comments and please see my answers inline.

From: David Marchand [mailto:david.marchand@6wind.com]
Sent: Tuesday, February 24, 2015 6:42 PM
To: Zhou, Danny
Cc: dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH v5 5/6] eal: add per rx queue interrupt handling based on VFIO

Hello Danny,

On Mon, Feb 23, 2015 at 5:55 PM, Zhou Danny <danny.zhou@intel.com<mailto: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.

DZ: See comments below.

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 ?

DZ: To avoid recreating the epoll instance for each queue, the struct rte_intr_handle(or a new structure added to ethdev)
should be extended by adding fields storing per-queue pfd. This way, it could reduce user/kernel context  switch overhead
when calling epoll_create() each time.

Sounds good?

+
+       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.

Agreed, it is not needed.

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.

DZ: The queueid is just an index to the queue related eventfd array stored in EAL. If this array is still in the EAL and ethdev can apply for it and setup mapping for certain queue, there
might be issue for multiple-process use case where the fd resources allocated for secondary process are not freed if the secondary process exits unexpectedly.

Probably we can setup the eventfd array inside ethdev,  and we just need EAL API to wait for ethdev’fd. So application invokes ethdev API with portid and queueid, and ethdev calls eal
API to wait on a ethdev fd which correlates with the specified portid and queueid.

Sounds ok to you?

I am going to travel tomorrow and Steve Liang might follow up on V6 patch submission when I am absent. Thanks Steve!

--
David Marchand