From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by dpdk.org (Postfix) with ESMTP id 86A7EB6BD for ; Tue, 17 Feb 2015 09:14:53 +0100 (CET) Received: from orsmga003.jf.intel.com ([10.7.209.27]) by orsmga103.jf.intel.com with ESMTP; 17 Feb 2015 00:09:33 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.09,593,1418112000"; d="scan'208";a="528516878" Received: from kmsmsx151.gar.corp.intel.com ([172.21.73.86]) by orsmga003.jf.intel.com with ESMTP; 17 Feb 2015 00:06:18 -0800 Received: from shsmsx103.ccr.corp.intel.com (10.239.4.69) by KMSMSX151.gar.corp.intel.com (172.21.73.86) with Microsoft SMTP Server (TLS) id 14.3.195.1; Tue, 17 Feb 2015 16:14:49 +0800 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.161]) by SHSMSX103.ccr.corp.intel.com ([169.254.4.197]) with mapi id 14.03.0195.001; Tue, 17 Feb 2015 16:14:42 +0800 From: "Zhou, Danny" To: "Liang, Cunming" , "dev@dpdk.org" Thread-Topic: [dpdk-dev] [PATCH v2 4/5] eal: add per rx queue interrupt handling based on VFIO Thread-Index: AQHQP4on4OYLD4FNq0S6djWShyuDhJzt8jBQgAahR3A= Date: Tue, 17 Feb 2015 08:14:42 +0000 Message-ID: References: <1422951511-28143-1-git-send-email-danny.zhou@intel.com> <1422951511-28143-5-git-send-email-danny.zhou@intel.com> In-Reply-To: Accept-Language: zh-CN, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH v2 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: Tue, 17 Feb 2015 08:14:54 -0000 > -----Original Message----- > From: Liang, Cunming > Sent: Friday, February 13, 2015 11:48 AM > To: Zhou, Danny; dev@dpdk.org > Subject: RE: [dpdk-dev] [PATCH v2 4/5] eal: add per rx queue interrupt ha= ndling based on VFIO >=20 > Hi, >=20 > > -----Original Message----- > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Zhou Danny > > Sent: Tuesday, February 03, 2015 4:19 PM > > To: dev@dpdk.org > > Subject: [dpdk-dev] [PATCH v2 4/5] eal: add per rx queue interrupt hand= ling > > based on VFIO > > > [...] > > #include "eal_private.h" > > #include "eal_vfio.h" > > @@ -127,6 +128,7 @@ static pthread_t intr_thread; > > #ifdef VFIO_PRESENT > > > > #define IRQ_SET_BUF_LEN (sizeof(struct vfio_irq_set) + sizeof(int)) > > +#define MSIX_IRQ_SET_BUF_LEN (sizeof(struct vfio_irq_set) + sizeof(int= ) * > > (VFIO_MAX_QUEUE_ID + 1)) > [LCM] Does it better to add comment for '+1' which is the max other inter= rupts besides rxtx. Will add. > > > > /* enable legacy (INTx) interrupts */ > > static int > > @@ -221,7 +223,7 @@ vfio_disable_intx(struct rte_intr_handle *intr_hand= le) { > > /* enable MSI-X interrupts */ > [LCM] typo on comment. 'enable MSI interrupts' ? Bug in legacy code and will fix it in V3. >=20 > > > > @@ -292,8 +284,8 @@ vfio_disable_msi(struct rte_intr_handle *intr_handl= e) { > > /* 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 +293,19 @@ vfio_enable_msix(struct rte_intr_handle *intr_han= dle) > > { > > > > irq_set =3D (struct vfio_irq_set *) irq_set_buf; > > irq_set->argsz =3D len; > > - irq_set->count =3D 1; > > + if ((!intr_handle->max_intr) || > > + (intr_handle->max_intr > VFIO_MAX_QUEUE_ID)) > > + max_intr =3D VFIO_MAX_QUEUE_ID + 1; > > + else > > + max_intr =3D intr_handle->max_intr; > > + > > + irq_set->count =3D max_intr; > > irq_set->flags =3D VFIO_IRQ_SET_DATA_EVENTFD | > > VFIO_IRQ_SET_ACTION_TRIGGER; > > irq_set->index =3D VFIO_PCI_MSIX_IRQ_INDEX; > > irq_set->start =3D 0; > > fd_ptr =3D (int *) &irq_set->data; > > - *fd_ptr =3D intr_handle->fd; > > + memcpy(fd_ptr, intr_handle->queue_fd, sizeof(intr_handle- > > >queue_fd)); > > + fd_ptr[max_intr - 1] =3D intr_handle->fd; > > > > ret =3D ioctl(intr_handle->vfio_dev_fd, VFIO_DEVICE_SET_IRQS, irq_set= ); > > > > @@ -316,22 +315,6 @@ vfio_enable_msix(struct rte_intr_handle *intr_hand= le) { > > return -1; > > } > > > > - /* manually trigger interrupt to enable it */ > > - memset(irq_set, 0, len); > > - len =3D sizeof(struct vfio_irq_set); > > - irq_set->argsz =3D len; > > - irq_set->count =3D 1; > > - irq_set->flags =3D VFIO_IRQ_SET_DATA_NONE | > > VFIO_IRQ_SET_ACTION_TRIGGER; > > - irq_set->index =3D VFIO_PCI_MSIX_IRQ_INDEX; > > - irq_set->start =3D 0; > > - > > - ret =3D 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 +322,7 @@ vfio_enable_msix(struct rte_intr_handle *intr_handl= e) { > > 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 =3D sizeof(struct vfio_irq_set); > > @@ -824,3 +807,119 @@ rte_eal_intr_init(void) > > return -ret; > > } > > > > +static void > > +eal_intr_process_rx_interrupts(uint8_t port_id, struct epoll_event *ev= ents, int > > nfds) > > +{ > > + int n, bytes_read; > > + union rte_intr_read_buffer buf; > > + struct rte_intr_handle intr_handle =3D rte_eth_devices[port_id].pci_d= ev- > > >intr_handle; > [LCM] column number large than 80. Will fix this in V3 patechset > > + > > + for (n =3D 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 =3D sizeof(buf.uio_intr_count); > > + break; > > + case RTE_INTR_HANDLE_ALARM: > > + bytes_read =3D 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 =3D sizeof(buf.vfio_intr_count); > > + break; > > +#endif > > + default: > > + bytes_read =3D 1; > > + break; > > + } > > + > > + /** > > + * read out to clear the ready-to-be-read flag > > + * for epoll_wait. > > + */ > > + bytes_read =3D 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 =3D=3D 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 total= fds) > > +{ > > + struct epoll_event events[totalfds]; > > + int nfds =3D 0; > > + > > + do { > > + nfds =3D 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 =3D=3D 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 =3D rte_eth_devices[port_id].pci_d= ev- > > >intr_handle; > [LCM] exceed margin. >=20 Will fix it in V3 patchset. > > + struct epoll_event ev; > > + unsigned numfds =3D 0; > > + > > + /* create epoll fd */ > > + int pfd =3D epoll_create(1); > > + if (pfd < 0) { > > + RTE_LOG(ERR, EAL, "Cannot create epoll instance\n"); > > + return -1; > > + } > > + > > + rte_spinlock_lock(&intr_lock); > > + > > + ev.events =3D EPOLLIN | EPOLLPRI; > > + switch (intr_handle.type) { > > + case RTE_INTR_HANDLE_UIO: > > + ev.data.fd =3D 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 =3D 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..63d0ae8 100644 > > --- a/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c > > +++ b/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c > > @@ -283,7 +283,6 @@ pci_vfio_setup_interrupts(struct rte_pci_device *de= v, int > > vfio_dev_fd) > > > > dev->intr_handle.fd =3D fd; > > dev->intr_handle.vfio_dev_fd =3D vfio_dev_fd; > > - > > switch (i) { > > case VFIO_PCI_MSIX_IRQ_INDEX: > > internal_config.vfio_intr_mode =3D RTE_INTR_MODE_MSIX; > > @@ -302,6 +301,16 @@ pci_vfio_setup_interrupts(struct rte_pci_device *d= ev, > > int vfio_dev_fd) > > return -1; > > } > > > > + for (i =3D 0; i < VFIO_MAX_QUEUE_ID; i++) { > [LCM] I think before assigning queue_fd, it shall check VFIO_PCI_MSIX_IRQ= _INDEX first. > Otherwise, no matter MSI or INTX, either one will set queue_fd as well. Will move this to "case" block. >=20 > > + fd =3D eventfd(0, 0); > > + if (fd < 0) { > > + RTE_LOG(ERR, EAL, " cannot set up eventfd, " > > + "error %i (%s)\n", errno, > > strerror(errno)); > [LCM] Cleanup the eventfd before return ? It is not allocated successful if it returns error, so no need to clean it = up.=20 >=20 >=20 > -Steve