From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 88214A046B for ; Mon, 22 Jul 2019 21:28:46 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 5D0111BEE6; Mon, 22 Jul 2019 21:28:46 +0200 (CEST) Received: from mail-vs1-f65.google.com (mail-vs1-f65.google.com [209.85.217.65]) by dpdk.org (Postfix) with ESMTP id 0F5811BEE6 for ; Mon, 22 Jul 2019 21:28:45 +0200 (CEST) Received: by mail-vs1-f65.google.com with SMTP id u3so27056221vsh.6 for ; Mon, 22 Jul 2019 12:28:44 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=wCEA6NBzOmPpR5e8ciGxPVNXC0g37+JkoAFuwgffjsE=; b=Bc4tdj79bkKV5fW8W225AT1X/rU9xgCqX9xBUrU1PZGThLhBCwKH+VI6svj92jULII 5upCujWF/3Lp/k+k5RUEAiKXTir4/2WTfkExhhf35iwZJnkiXrQK8IKmL2u2ScPy10vy ZfiDGr73ZJel33eipNjpf/lZcCtw9KDvm08ISd3xMVN9bDYxnk0aspWZGz5UTkeBm7Cu FWdkNedKZzi4Um4M7V6m41D/8Jo2tPWoNLDPjQp9DgAep22iVAuStySa1qpcffeg0OLG 8e0ArCfYLZQgDAQ2rLO4a7+TuJu/6hUyK8KPKp9v2i+7uOdwdKiUxefrfdZUhY4f5Wa5 8lQw== X-Gm-Message-State: APjAAAX+0AaZ3qNdEfY5q35N/6ZabWWlTuIZmQPx0PdAZdSvRhPSONqH /9hNf+aa0h2mdU8hO9KOOSNT6bcWKpW7ohUw7tJImA== X-Google-Smtp-Source: APXvYqxFdZz9FD0iDKDewFQrozr2Nibe9Di7V9A3EXqEP2kaQzer5rggfNsB0qkOQdzSr3dmT4daaxESSEi2YwV8EZU= X-Received: by 2002:a67:e9ca:: with SMTP id q10mr12285980vso.105.1563823724462; Mon, 22 Jul 2019 12:28:44 -0700 (PDT) MIME-Version: 1.0 References: <20190717115852.171416-1-ndabilpuram@marvell.com> <20190718143628.169623-1-ndabilpuram@marvell.com> <20190718143628.169623-3-ndabilpuram@marvell.com> In-Reply-To: <20190718143628.169623-3-ndabilpuram@marvell.com> From: David Marchand Date: Mon, 22 Jul 2019 21:28:33 +0200 Message-ID: To: Nithin Dabilpuram Cc: Hyong Youb Kim , Thomas Monjalon , Ferruh Yigit , Bruce Richardson , Jerin Jacob Kollanukkaran , John Daley , Shahed Shaikh , dev Content-Type: text/plain; charset="UTF-8" Subject: Re: [dpdk-dev] [PATCH v4 2/3] eal: add ack interrupt API X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Thu, Jul 18, 2019 at 4:36 PM Nithin Dabilpuram wrote: > > Add new ack interrupt API to avoid using > VFIO_IRQ_SET_ACTION_TRIGGER(rte_intr_enable()) for > acking interrupt purpose for VFIO based interrupt handlers. > This implementation is specific to Linux. > > Using rte_intr_enable() for acking interrupt has below issues > > * Time consuming to do for every interrupt received as it will > free_irq() followed by request_irq() and all other initializations > * A race condition because of a window between free_irq() and > request_irq() with packet reception still on and device still > enabled and would throw warning messages like below. > [158764.159833] do_IRQ: 9.34 No irq handler for vector > > In this patch, rte_intr_ack() is a no-op for VFIO_MSIX/VFIO_MSI interrupts > as they are edge triggered and kernel would not mask the interrupt before > delivering the event to userspace and we don't need to ack. > > Signed-off-by: Nithin Dabilpuram > Signed-off-by: Jerin Jacob > Tested-by: Shahed Shaikh > --- > v4: > * Move note to implementation and change > the expectation to must call for new api. > v3: > * Update note on new api > v2: > * No change > > lib/librte_eal/common/include/rte_interrupts.h | 18 ++++++ > lib/librte_eal/freebsd/eal/eal_interrupts.c | 9 +++ > lib/librte_eal/linux/eal/eal_interrupts.c | 90 ++++++++++++++++++++++++++ > lib/librte_eal/rte_eal_version.map | 1 + > 4 files changed, 118 insertions(+) > > diff --git a/lib/librte_eal/common/include/rte_interrupts.h b/lib/librte_eal/common/include/rte_interrupts.h > index c1e912c..c463265 100644 > --- a/lib/librte_eal/common/include/rte_interrupts.h > +++ b/lib/librte_eal/common/include/rte_interrupts.h > @@ -118,6 +118,24 @@ int rte_intr_enable(const struct rte_intr_handle *intr_handle); > */ > int rte_intr_disable(const struct rte_intr_handle *intr_handle); > > +/** Missing a banner. * @warning * @b EXPERIMENTAL: this API may change without prior notice > + * It acks an interrupt raised for the specified handle. acknowledges > + * > + * This function should be called at the end of each interrupt > + * handler either from application or driver, so that > + * currently raised interrupt is acked and further > + * new interrupts are raised. > + * > + * @param intr_handle > + * pointer to the interrupt handle. > + * > + * @return > + * - On success, zero. > + * - On failure, a negative value. > + */ > +__rte_experimental > +int rte_intr_ack(const struct rte_intr_handle *intr_handle); > + > #ifdef __cplusplus > } > #endif > diff --git a/lib/librte_eal/freebsd/eal/eal_interrupts.c b/lib/librte_eal/freebsd/eal/eal_interrupts.c > index 10375bd..f6831b7 100644 > --- a/lib/librte_eal/freebsd/eal/eal_interrupts.c > +++ b/lib/librte_eal/freebsd/eal/eal_interrupts.c > @@ -387,6 +387,15 @@ rte_intr_disable(const struct rte_intr_handle *intr_handle) > return 0; > } > > +int > +rte_intr_ack(const struct rte_intr_handle *intr_handle) > +{ > + if (intr_handle && intr_handle->type == RTE_INTR_HANDLE_VDEV) > + return 0; > + > + return -1; > +} > + > static void > eal_intr_process_interrupts(struct kevent *events, int nfds) > { > diff --git a/lib/librte_eal/linux/eal/eal_interrupts.c b/lib/librte_eal/linux/eal/eal_interrupts.c > index 79ad5e8..794374e 100644 > --- a/lib/librte_eal/linux/eal/eal_interrupts.c > +++ b/lib/librte_eal/linux/eal/eal_interrupts.c > @@ -197,6 +197,35 @@ vfio_disable_intx(const struct rte_intr_handle *intr_handle) { > return 0; > } > > +/* unmask/ack legacy (INTx) interrupts */ > +static int > +vfio_ack_intx(const struct rte_intr_handle *intr_handle) > +{ > + struct vfio_irq_set *irq_set; > + char irq_set_buf[IRQ_SET_BUF_LEN]; > + int len, ret; > + > + len = sizeof(struct vfio_irq_set); > + > + /* unmask INTx */ > + irq_set = (struct vfio_irq_set *) irq_set_buf; > + memset(irq_set, 0, len); > + irq_set->argsz = len; > + irq_set->count = 1; > + irq_set->flags = VFIO_IRQ_SET_DATA_NONE | VFIO_IRQ_SET_ACTION_UNMASK; > + irq_set->index = VFIO_PCI_INTX_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 unmasking INTx interrupts for fd %d\n", > + intr_handle->fd); > + return -1; > + } > + return 0; > +} > + We are not setting any fd. No need to have the irq_set_buf[] array, you can directly declare irq_set on the stack and remove the len and ret variables. Something like: /* unmask/ack legacy (INTx) interrupts */ static int vfio_ack_intx(const struct rte_intr_handle *intr_handle) { struct vfio_irq_set irq_set; /* unmask INTx */ memset(&irq_set, 0, sizeof(irq_set)); irq_set.argsz = sizeof(irq_set); irq_set.count = 1; irq_set.flags = VFIO_IRQ_SET_DATA_NONE | VFIO_IRQ_SET_ACTION_UNMASK; irq_set.index = VFIO_PCI_INTX_IRQ_INDEX; irq_set.start = 0; if (ioctl(intr_handle->vfio_dev_fd, VFIO_DEVICE_SET_IRQS, &irq_set)) { RTE_LOG(ERR, EAL, "Error unmasking INTx interrupts for fd %d\n", intr_handle->fd); return -1; } return 0; } > /* enable MSI interrupts */ > static int > vfio_enable_msi(const struct rte_intr_handle *intr_handle) { > @@ -693,6 +722,67 @@ rte_intr_enable(const struct rte_intr_handle *intr_handle) > return 0; > } > > + Double empty line. > +/** > + * PMD generally calls this function at the end of its IRQ callback. > + * Internally, it unmasks the interrupt if possible. For INTx, unmasking > + * is required as the interrupt is auto-masked prior to invoking > + * callback. For MSI/MSI-X, unmasking is typically not needed as the > + * interrupt is not auto-masked. In fact, for interrupt handle types > + * VFIO_MSIX and VFIO_MSI, this function is no-op. No double space at the beginning of the lines. How about separating the cases, like: /** * PMD generally calls this function at the end of its IRQ callback. * Internally, it unmasks the interrupt if possible. * * For INTx, unmasking is required as the interrupt is auto-masked prior to * invoking callback. * * For MSI/MSI-X, unmasking is typically not needed as the interrupt is not * auto-masked. In fact, for interrupt handle types VFIO_MSIX and VFIO_MSI, * this function is no-op. */ > + */ > +int > +rte_intr_ack(const struct rte_intr_handle *intr_handle) > +{ > + if (intr_handle && intr_handle->type == RTE_INTR_HANDLE_VDEV) > + return 0; > + > + if (!intr_handle || intr_handle->fd < 0 || intr_handle->uio_cfg_fd < 0) > + return -1; > + > + switch (intr_handle->type) { > + /* Both acking and disabling are same for UIO */ enabling? > + case RTE_INTR_HANDLE_UIO: > + if (uio_intr_enable(intr_handle)) > + return -1; > + break; > + case RTE_INTR_HANDLE_UIO_INTX: > + if (uio_intx_intr_enable(intr_handle)) > + return -1; > + break; > + /* not used at this moment */ > + case RTE_INTR_HANDLE_ALARM: > + return -1; > +#ifdef VFIO_PRESENT > + /* Since VFIO_MSIX is implicitly acked > + * unlike INTx, we report success > + */ Indent is weird. How about: /* VFIO MSI* is implicitly acked unlike INTx, nothing to do */ > + case RTE_INTR_HANDLE_VFIO_MSIX: > + case RTE_INTR_HANDLE_VFIO_MSI: > + return 0; > + case RTE_INTR_HANDLE_VFIO_LEGACY: > + if (vfio_ack_intx(intr_handle)) > + return -1; > + break; > +#ifdef HAVE_VFIO_DEV_REQ_INTERFACE > + case RTE_INTR_HANDLE_VFIO_REQ: > + return -1; > +#endif > +#endif > + /* not used at this moment */ > + case RTE_INTR_HANDLE_DEV_EVENT: > + return -1; > + /* unknown handle type */ > + default: > + RTE_LOG(ERR, EAL, > + "Unknown handle type of fd %d\n", > + intr_handle->fd); > + return -1; > + } > + > + return 0; > +} > + > int > rte_intr_disable(const struct rte_intr_handle *intr_handle) > { > diff --git a/lib/librte_eal/rte_eal_version.map b/lib/librte_eal/rte_eal_version.map > index 1892d9e..0537a6d 100644 > --- a/lib/librte_eal/rte_eal_version.map > +++ b/lib/librte_eal/rte_eal_version.map > @@ -407,4 +407,5 @@ EXPERIMENTAL { > rte_lcore_to_cpu_id; > rte_mcfg_timer_lock; > rte_mcfg_timer_unlock; > + rte_intr_ack; This should be alphabetically sorted. > }; > -- > 2.8.4 > -- David Marchand