From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp.tuxdriver.com (charlotte.tuxdriver.com [70.61.120.58]) by dpdk.org (Postfix) with ESMTP id 16D57B5C4 for ; Thu, 19 Feb 2015 14:04:14 +0100 (CET) Received: from hmsreliant.think-freely.org ([2001:470:8:a08:7aac:c0ff:fec2:933b] helo=localhost) by smtp.tuxdriver.com with esmtpsa (TLSv1:AES128-SHA:128) (Exim 4.63) (envelope-from ) id 1YOQlz-0003LE-PJ; Thu, 19 Feb 2015 08:04:11 -0500 Date: Thu, 19 Feb 2015 08:04:07 -0500 From: Neil Horman To: "Zhou, Danny" Message-ID: <20150219130407.GC24069@hmsreliant.think-freely.org> References: <1424180839-24483-1-git-send-email-danny.zhou@intel.com> <1424180839-24483-5-git-send-email-danny.zhou@intel.com> <20150217155852.GD6309@neilslaptop.think-freely.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) X-Spam-Score: -2.9 (--) X-Spam-Status: No Cc: "dev@dpdk.org" Subject: Re: [dpdk-dev] [PATCH v3 4/5] eal: add per rx queue interrupt handling based on VFIO X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 19 Feb 2015 13:04:14 -0000 On Thu, Feb 19, 2015 at 08:10:47AM +0000, Zhou, Danny wrote: > > > > -----Original Message----- > > From: Neil Horman [mailto:nhorman@tuxdriver.com] > > Sent: Tuesday, February 17, 2015 11:59 PM > > To: Zhou, Danny > > Cc: dev@dpdk.org > > Subject: Re: [dpdk-dev] [PATCH v3 4/5] eal: add per rx queue interrupt handling based on VFIO > > > > On Tue, Feb 17, 2015 at 09:47:18PM +0800, Zhou Danny wrote: > > > v3 changes: > > > - Fix review comments > > > > > > v2 changes: > > > - Fix compilation issue for a missed header file > > > - Bug fix: free unreleased resources on the exception path before return > > > - Consolidate coding style related review comments > > > > > > This patch does below: > > > - Create multiple VFIO eventfd for rx queues. > > > - Handle per rx queue interrupt. > > > - Eliminate unnecessary suspended DPDK polling thread wakeup mechanism > > > for rx interrupt by allowing polling thread epoll_wait rx queue > > > interrupt notification. > > > > > > Signed-off-by: Danny Zhou > > > Tested-by: Yong Liu > > > --- > > > lib/librte_eal/common/include/rte_eal.h | 12 ++ > > > lib/librte_eal/linuxapp/eal/Makefile | 1 + > > > lib/librte_eal/linuxapp/eal/eal_interrupts.c | 190 ++++++++++++++++----- > > > lib/librte_eal/linuxapp/eal/eal_pci_vfio.c | 12 +- > > > .../linuxapp/eal/include/exec-env/rte_interrupts.h | 4 + > > > 5 files changed, 175 insertions(+), 44 deletions(-) > > > > > > diff --git a/lib/librte_eal/common/include/rte_eal.h b/lib/librte_eal/common/include/rte_eal.h > > > index f4ecd2e..d81331f 100644 > > > --- a/lib/librte_eal/common/include/rte_eal.h > > > +++ b/lib/librte_eal/common/include/rte_eal.h > > > @@ -150,6 +150,18 @@ int rte_eal_iopl_init(void); > > > * - On failure, a negative error value. > > > */ > > > int rte_eal_init(int argc, char **argv); > > > + > > > +/** > > > + * @param port_id > > > + * the port id > > > + * @param queue_id > > > + * the queue id > > > + * @return > > > + * - On success, return 0 > > > + * - On failure, returns -1. > > > + */ > > > +int rte_eal_wait_rx_intr(uint8_t port_id, uint8_t queue_id); > > > + > > > /** > > > * Usage function typedef used by the application usage function. > > > * > > > diff --git a/lib/librte_eal/linuxapp/eal/Makefile b/lib/librte_eal/linuxapp/eal/Makefile > > > index e117cec..c593dfa 100644 > > > --- a/lib/librte_eal/linuxapp/eal/Makefile > > > +++ b/lib/librte_eal/linuxapp/eal/Makefile > > > @@ -43,6 +43,7 @@ CFLAGS += -I$(SRCDIR)/include > > > CFLAGS += -I$(RTE_SDK)/lib/librte_eal/common > > > CFLAGS += -I$(RTE_SDK)/lib/librte_eal/common/include > > > CFLAGS += -I$(RTE_SDK)/lib/librte_ring > > > +CFLAGS += -I$(RTE_SDK)/lib/librte_mbuf > > > CFLAGS += -I$(RTE_SDK)/lib/librte_mempool > > > CFLAGS += -I$(RTE_SDK)/lib/librte_malloc > > > CFLAGS += -I$(RTE_SDK)/lib/librte_ether > > > diff --git a/lib/librte_eal/linuxapp/eal/eal_interrupts.c b/lib/librte_eal/linuxapp/eal/eal_interrupts.c > > > index dc2668a..97215ad 100644 > > > --- a/lib/librte_eal/linuxapp/eal/eal_interrupts.c > > > +++ b/lib/librte_eal/linuxapp/eal/eal_interrupts.c > > > @@ -64,6 +64,7 @@ > > > #include > > > #include > > > #include > > > +#include > > > > > > #include "eal_private.h" > > > #include "eal_vfio.h" > > > @@ -127,6 +128,9 @@ static pthread_t intr_thread; > > > #ifdef VFIO_PRESENT > > > > > > #define IRQ_SET_BUF_LEN (sizeof(struct vfio_irq_set) + sizeof(int)) > > > +/* irq set buffer length for queue interrupts and LSC interrupt */ > > > +#define MSIX_IRQ_SET_BUF_LEN (sizeof(struct vfio_irq_set) + \ > > > + sizeof(int) * (VFIO_MAX_QUEUE_ID + 1)) > > > > > > /* enable legacy (INTx) interrupts */ > > > static int > > > @@ -218,10 +222,10 @@ vfio_disable_intx(struct rte_intr_handle *intr_handle) { > > > return 0; > > > } > > > > > > -/* enable MSI-X interrupts */ > > > +/* enable MSI interrupts */ > > > static int > > > vfio_enable_msi(struct rte_intr_handle *intr_handle) { > > > - int len, ret; > > > + int len, ret, max_intr; > > > char irq_set_buf[IRQ_SET_BUF_LEN]; > > > struct vfio_irq_set *irq_set; > > > int *fd_ptr; > > > @@ -230,12 +234,19 @@ vfio_enable_msi(struct rte_intr_handle *intr_handle) { > > > > > > irq_set = (struct vfio_irq_set *) irq_set_buf; > > > irq_set->argsz = len; > > > - irq_set->count = 1; > > > + if ((!intr_handle->max_intr) || > > > + (intr_handle->max_intr > VFIO_MAX_QUEUE_ID)) > > > + max_intr = VFIO_MAX_QUEUE_ID + 1; > > > + else > > > + max_intr = intr_handle->max_intr; > > > + > > > + irq_set->count = max_intr; > > > irq_set->flags = VFIO_IRQ_SET_DATA_EVENTFD | VFIO_IRQ_SET_ACTION_TRIGGER; > > > irq_set->index = VFIO_PCI_MSI_IRQ_INDEX; > > > irq_set->start = 0; > > > fd_ptr = (int *) &irq_set->data; > > > - *fd_ptr = intr_handle->fd; > > > + memcpy(fd_ptr, intr_handle->queue_fd, sizeof(intr_handle->queue_fd)); > > > + fd_ptr[max_intr - 1] = intr_handle->fd; > > > > > > ret = ioctl(intr_handle->vfio_dev_fd, VFIO_DEVICE_SET_IRQS, irq_set); > > > > > > @@ -244,27 +255,10 @@ vfio_enable_msi(struct rte_intr_handle *intr_handle) { > > > intr_handle->fd); > > > return -1; > > > } > > > - > > > - /* manually trigger interrupt to enable it */ > > > - memset(irq_set, 0, len); > > > - len = sizeof(struct vfio_irq_set); > > > - irq_set->argsz = len; > > > - irq_set->count = 1; > > > - irq_set->flags = VFIO_IRQ_SET_DATA_NONE | VFIO_IRQ_SET_ACTION_TRIGGER; > > > - irq_set->index = VFIO_PCI_MSI_IRQ_INDEX; > > > - irq_set->start = 0; > > > - > > > - ret = ioctl(intr_handle->vfio_dev_fd, VFIO_DEVICE_SET_IRQS, irq_set); > > > - > > > - if (ret) { > > > - RTE_LOG(ERR, EAL, "Error triggering MSI interrupts for fd %d\n", > > > - intr_handle->fd); > > > - return -1; > > > - } > > > return 0; > > > } > > > > > > -/* disable MSI-X interrupts */ > > > +/* disable MSI interrupts */ > > > static int > > > vfio_disable_msi(struct rte_intr_handle *intr_handle) { > > > struct vfio_irq_set *irq_set; > > > @@ -292,8 +286,8 @@ vfio_disable_msi(struct rte_intr_handle *intr_handle) { > > > /* enable MSI-X interrupts */ > > > static int > > > vfio_enable_msix(struct rte_intr_handle *intr_handle) { > > > - int len, ret; > > > - char irq_set_buf[IRQ_SET_BUF_LEN]; > > > + int len, ret, max_intr; > > > + char irq_set_buf[MSIX_IRQ_SET_BUF_LEN]; > > > struct vfio_irq_set *irq_set; > > > int *fd_ptr; > > > > > > @@ -301,12 +295,19 @@ vfio_enable_msix(struct rte_intr_handle *intr_handle) { > > > > > > irq_set = (struct vfio_irq_set *) irq_set_buf; > > > irq_set->argsz = len; > > > - irq_set->count = 1; > > > + if ((!intr_handle->max_intr) || > > > + (intr_handle->max_intr > VFIO_MAX_QUEUE_ID)) > > > + max_intr = VFIO_MAX_QUEUE_ID + 1; > > > + else > > > + max_intr = intr_handle->max_intr; > > > + > > > + irq_set->count = max_intr; > > > irq_set->flags = VFIO_IRQ_SET_DATA_EVENTFD | VFIO_IRQ_SET_ACTION_TRIGGER; > > > irq_set->index = VFIO_PCI_MSIX_IRQ_INDEX; > > > irq_set->start = 0; > > > fd_ptr = (int *) &irq_set->data; > > > - *fd_ptr = intr_handle->fd; > > > + memcpy(fd_ptr, intr_handle->queue_fd, sizeof(intr_handle->queue_fd)); > > > + fd_ptr[max_intr - 1] = intr_handle->fd; > > > > > > ret = ioctl(intr_handle->vfio_dev_fd, VFIO_DEVICE_SET_IRQS, irq_set); > > > > > > @@ -316,22 +317,6 @@ vfio_enable_msix(struct rte_intr_handle *intr_handle) { > > > return -1; > > > } > > > > > > - /* manually trigger interrupt to enable it */ > > > - memset(irq_set, 0, len); > > > - len = sizeof(struct vfio_irq_set); > > > - irq_set->argsz = len; > > > - irq_set->count = 1; > > > - irq_set->flags = VFIO_IRQ_SET_DATA_NONE | VFIO_IRQ_SET_ACTION_TRIGGER; > > > - irq_set->index = VFIO_PCI_MSIX_IRQ_INDEX; > > > - irq_set->start = 0; > > > - > > > - ret = ioctl(intr_handle->vfio_dev_fd, VFIO_DEVICE_SET_IRQS, irq_set); > > > - > > > - if (ret) { > > > - RTE_LOG(ERR, EAL, "Error triggering MSI-X interrupts for fd %d\n", > > > - intr_handle->fd); > > > - return -1; > > > - } > > > return 0; > > > } > > > > > > @@ -339,7 +324,7 @@ vfio_enable_msix(struct rte_intr_handle *intr_handle) { > > > static int > > > vfio_disable_msix(struct rte_intr_handle *intr_handle) { > > > struct vfio_irq_set *irq_set; > > > - char irq_set_buf[IRQ_SET_BUF_LEN]; > > > + char irq_set_buf[MSIX_IRQ_SET_BUF_LEN]; > > > int len, ret; > > > > > > len = sizeof(struct vfio_irq_set); > > > @@ -824,3 +809,122 @@ rte_eal_intr_init(void) > > > return -ret; > > > } > > > > > > +static void > > > +eal_intr_process_rx_interrupts(uint8_t port_id, > > > + struct epoll_event *events, int nfds) > > > +{ > > > + int n, bytes_read; > > > + union rte_intr_read_buffer buf; > > > + struct rte_intr_handle intr_handle = > > > + rte_eth_devices[port_id].pci_dev->intr_handle; > > > + > > > + for (n = 0; n < nfds; n++) { > > > + /* set the length to be read for different handle type */ > > > + switch (intr_handle.type) { > > > + case RTE_INTR_HANDLE_UIO: > > > + bytes_read = sizeof(buf.uio_intr_count); > > > + break; > > > + case RTE_INTR_HANDLE_ALARM: > > > + bytes_read = sizeof(buf.timerfd_num); > > > + break; > > > +#ifdef VFIO_PRESENT > > > + case RTE_INTR_HANDLE_VFIO_MSIX: > > > + case RTE_INTR_HANDLE_VFIO_MSI: > > > + case RTE_INTR_HANDLE_VFIO_LEGACY: > > > + bytes_read = sizeof(buf.vfio_intr_count); > > > + break; > > > +#endif > > > + default: > > > + bytes_read = 1; > > > + break; > > > + } > > > + > > > + /** > > > + * read out to clear the ready-to-be-read flag > > > + * for epoll_wait. > > > + */ > > > + bytes_read = read(events[n].data.fd, &buf, bytes_read); > > > + if (bytes_read < 0) > > > + RTE_LOG(ERR, EAL, "Error reading from file " > > > + "descriptor %d: %s\n", events[n].data.fd, > > > + strerror(errno)); > > > + else if (bytes_read == 0) > > > + RTE_LOG(ERR, EAL, "Read nothing from file " > > > + "descriptor %d\n", events[n].data.fd); > > > + } > > > +} > > > + > > > +static void > > > +eal_intr_handle_rx_interrupts(uint8_t port_id, int pfd, unsigned totalfds) > > > +{ > > > + struct epoll_event events[totalfds]; > > > + int nfds = 0; > > > + > > > + do { > > > + nfds = epoll_wait(pfd, events, totalfds, > > > + EAL_INTR_EPOLL_WAIT_FOREVER); > > > + /* epoll_wait fail */ > > > + if (nfds < 0) { > > > + RTE_LOG(ERR, EAL, > > > + "epoll_wait returns with fail\n"); > > > + return; > > > + } > > > + } while (nfds == 0); > > > + > > > + /* epoll_wait has at least one fd ready to read */ > > > + eal_intr_process_rx_interrupts(port_id, events, nfds); > > > +} > > > + > > > +int > > > +rte_eal_wait_rx_intr(uint8_t port_id, uint8_t queue_id) > > > +{ > > > + struct rte_intr_handle intr_handle = > > > + rte_eth_devices[port_id].pci_dev->intr_handle; > > > + struct epoll_event ev; > > > + unsigned numfds = 0; > > > + > > > + /* create epoll fd */ > > > + int pfd = epoll_create(1); > > > + if (pfd < 0) { > > > + RTE_LOG(ERR, EAL, "Cannot create epoll instance\n"); > > > + return -1; > > > + } > > > + > > > + 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(port_id, pfd, numfds); > > > + > > > + /** > > > + * when we return, we need to rebuild the > > > + * list of fds to monitor. > > > + */ > > > + close(pfd); > > > + > > > + return 0; > > > +} > > > diff --git a/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c b/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c > > > index 20e0977..0e5fa76 100644 > > > --- a/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c > > > +++ b/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c > > > @@ -283,11 +283,21 @@ pci_vfio_setup_interrupts(struct rte_pci_device *dev, int vfio_dev_fd) > > > > > > dev->intr_handle.fd = fd; > > > dev->intr_handle.vfio_dev_fd = vfio_dev_fd; > > > - > > > switch (i) { > > > case VFIO_PCI_MSIX_IRQ_INDEX: > > > internal_config.vfio_intr_mode = RTE_INTR_MODE_MSIX; > > > dev->intr_handle.type = RTE_INTR_HANDLE_VFIO_MSIX; > > > + for (i = 0; i < VFIO_MAX_QUEUE_ID; i++) { > > > + fd = eventfd(0, 0); > > > + if (fd < 0) { > > > + RTE_LOG(ERR, EAL, > > > + "cannot setup eventfd," > > > + "error %i (%s)\n", > > > + errno, strerror(errno)); > > > + return -1; > > > + } > > > + dev->intr_handle.queue_fd[i] = fd; > > > + } > > > break; > > > case VFIO_PCI_MSI_IRQ_INDEX: > > > internal_config.vfio_intr_mode = RTE_INTR_MODE_MSI; > > > diff --git a/lib/librte_eal/linuxapp/eal/include/exec-env/rte_interrupts.h > > b/lib/librte_eal/linuxapp/eal/include/exec-env/rte_interrupts.h > > > index 23eafd9..c6982cf 100644 > > > --- a/lib/librte_eal/linuxapp/eal/include/exec-env/rte_interrupts.h > > > +++ b/lib/librte_eal/linuxapp/eal/include/exec-env/rte_interrupts.h > > > @@ -38,6 +38,8 @@ > > > #ifndef _RTE_LINUXAPP_INTERRUPTS_H_ > > > #define _RTE_LINUXAPP_INTERRUPTS_H_ > > > > > > +#define VFIO_MAX_QUEUE_ID 32 > > > + > > > enum rte_intr_handle_type { > > > RTE_INTR_HANDLE_UNKNOWN = 0, > > > RTE_INTR_HANDLE_UIO, /**< uio device handle */ > > > @@ -52,6 +54,8 @@ enum rte_intr_handle_type { > > > struct rte_intr_handle { > > > int vfio_dev_fd; /**< VFIO device file descriptor */ > > > int fd; /**< file descriptor */ > > > + int max_intr; /**< max interrupt requested */ > > > + int queue_fd[VFIO_MAX_QUEUE_ID]; /**< rx and tx queue interrupt file descriptor */ > > This is used outside of this library, you need to move these new fields to the > > end of the structure. > > > > neil > > Alright, I will move them to the end in V4 patch. > > Neil, do you have any simple writeup on guideline about how to add APIs and new fields to existing > structure in order to make sure new stuff does not break ABI? It might help all the developers to avoid > making similar mistakes in the future. > Not as such, but the ABI document in the release notes gives some examples. We can certainly start one if you like, though many of them are situationally specific. > > > > > enum rte_intr_handle_type type; /**< handle type */ > > > }; > > > > > > -- > > > 1.8.1.4 > > > > > > >