From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ob0-f181.google.com (mail-ob0-f181.google.com [209.85.214.181]) by dpdk.org (Postfix) with ESMTP id 6861CADA4 for ; Tue, 24 Feb 2015 11:42:25 +0100 (CET) Received: by mail-ob0-f181.google.com with SMTP id vb8so42316493obc.12 for ; Tue, 24 Feb 2015 02:42:24 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:date :message-id:subject:from:to:cc:content-type; bh=tCU1s1LUSvsZbv6YuX92I7XyP20tVXbgSbAqcmG9j3s=; b=BdrnO/3h/UM/pJxMSPYHLzhIV8w5dLBKJ2sghVE3WdboWNzvXJTnPPDZaV8n0HJQAK fCuITJ0g0J4k056zU4TUhJQUpdSt6SylsUU7OKpPtXz01xJaEa3zmY+xvFenADFvOU6S npKHYpJMYu4AFsaw9LsH27yRLAXdTL+Sy5DaMQBmXbsUT0OFO1XrkEoe/LyqdsTpuf6+ PHtNdNAXBrGPipV6Z/CbWqoFh1pH1i1JySmB18ktHc5Jf4qBTuIe9wpN6iwlzs8yRZIb 1WsWhfFJfG0/mFKl5iq0UYbTNJB9uVQv1nNOoBTqVaKXxxyNhWRzJDLp+N1RqorYyk5O gdpA== X-Gm-Message-State: ALoCoQm1iok3iFqH79ijWde+ZMiedBXe5eOX7Q6iSMkL08rfH3nJkk6fjhkdQFdoI7YZr+Oa4qP8 MIME-Version: 1.0 X-Received: by 10.202.180.87 with SMTP id d84mr10341407oif.0.1424774544885; Tue, 24 Feb 2015 02:42:24 -0800 (PST) Received: by 10.76.133.162 with HTTP; Tue, 24 Feb 2015 02:42:24 -0800 (PST) In-Reply-To: <1424710542-14637-6-git-send-email-danny.zhou@intel.com> References: <1424710542-14637-1-git-send-email-danny.zhou@intel.com> <1424710542-14637-6-git-send-email-danny.zhou@intel.com> Date: Tue, 24 Feb 2015 11:42:24 +0100 Message-ID: From: David Marchand To: Zhou Danny Content-Type: text/plain; charset=UTF-8 X-Content-Filtered-By: Mailman/MimeDel 2.1.15 Cc: "dev@dpdk.org" Subject: Re: [dpdk-dev] [PATCH v5 5/6] 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, 24 Feb 2015 10:42:25 -0000 Hello Danny, On Mon, Feb 23, 2015 at 5:55 PM, Zhou Danny 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