From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f46.google.com (mail-wm0-f46.google.com [74.125.82.46]) by dpdk.org (Postfix) with ESMTP id E809FA49A for ; Thu, 25 Jan 2018 12:36:25 +0100 (CET) Received: by mail-wm0-f46.google.com with SMTP id v123so14308326wmd.5 for ; Thu, 25 Jan 2018 03:36:25 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:content-transfer-encoding:in-reply-to :user-agent; bh=a6Dq56U/dX/LB6ywxh5NisjQOIhDdf1XujHuS4JGuWQ=; b=oVbS4OI7XylCOBGtnLlxPolK3gR7rA85iJvlEDo8VqGcAnSmBhFTiDszobRbJD3Su4 SQCEmg+bMcgrTUsE2WOE78SNsExV5v2J86JkadHvlV1YHchzHXZtSEK24rdP9w40DhlD AC7LFck3WjPeRKWEB8oHzvIojTvTXkukMSLWrP3c/e/5z/wsF2CbKJsfvM2HUOgpHjzo XbUGANwIx4jgFG/oBKtMPC5Mn+Oa0cHdcSoMUXzcxprmZfPpGDVIpwxTA0TaOlU7Skhq q17yEBV1dIuryJ8x9BWTj8yfoUneWGcrZxLGMQgIf17Cbnm3IiccioM/Z3fDroCmZ71R XZWQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to:user-agent; bh=a6Dq56U/dX/LB6ywxh5NisjQOIhDdf1XujHuS4JGuWQ=; b=ZobZyfKoeZ1gmgKzJUTUt4I2xiI5iJ6S8q09Esv/TyDUWtMQhrCnbux8ZEv+8wWv1e yjNy9zf/be3e/mljmrAzqkbV2oPiALirhEBGsu+p/I4SNft9F6i12ZsWbBS8NHGAjJOM gSBnhLK7d40K3m4PGxULTKaFhgRpvn8UNb4gtxHzmY58wx2MZt02dtUWzAc3f4lT07sZ Se5P1GLOoUGI8P56y2rYYd/Lb62u3M3/j1pFJtI3UwC9QVROsEBbYMGgSRXZt0olOYnL LO4icL7deeesAJn3aHeXm4fnjXCon7po8qkNYvsjwjtITN8s8SAyvf6fNyXFHHmSvzn6 oV4w== X-Gm-Message-State: AKwxyteK5Cv2KwBKxr9lvNEDpOpxeXdhabZVoz6owUrOy8CmdJWELIcX N3/sN3NwG26wTYa4LwORslymOJRe X-Google-Smtp-Source: AH8x227LUiDmUIsZjHPrIscoLl1AEAWACjWEjI/v6TDiV2kOqKQ0vikDTTADKZtUfV6Za4ES1x+hiQ== X-Received: by 10.28.10.6 with SMTP id 6mr7019690wmk.1.1516880185223; Thu, 25 Jan 2018 03:36:25 -0800 (PST) Received: from bidouze.vm.6wind.com (host.78.145.23.62.rev.coltfrance.com. [62.23.145.78]) by smtp.gmail.com with ESMTPSA id 51sm5868026wrw.15.2018.01.25.03.36.24 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Thu, 25 Jan 2018 03:36:24 -0800 (PST) Date: Thu, 25 Jan 2018 12:36:11 +0100 From: =?iso-8859-1?Q?Ga=EBtan?= Rivet To: Moti Haimovsky Cc: ferruh.yigit@intel.com, dev@dpdk.org Message-ID: <20180125113611.wu6dor3xjghkhfez@bidouze.vm.6wind.com> References: <1516810328-39383-3-git-send-email-motih@mellanox.com> <1516867635-67104-1-git-send-email-motih@mellanox.com> <1516867635-67104-2-git-send-email-motih@mellanox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1516867635-67104-2-git-send-email-motih@mellanox.com> User-Agent: NeoMutt/20170113 (1.7.2) Subject: Re: [dpdk-dev] [PATCH v7 1/3] net/failsafe: register as an Rx interrupt mode PMD 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: , X-List-Received-Date: Thu, 25 Jan 2018 11:36:26 -0000 Hi Moti, Thanks for splitting the patches, A few comments. On Thu, Jan 25, 2018 at 10:07:13AM +0200, Moti Haimovsky wrote: > This patch adds registering the Rx queues of the failsafe PMD with EAL > Rx interrupts subsystem. > Each failsafe RX queue is assigned with a unique eventfd and an enable > interrupts flag. > The PMD creates an interrupt vector containing the above eventfds and > Registers it with EAL. The PMD also implements the Rx interrupts enable > and disable interface routines. > This patch does not implement the generation of Rx interrupts, so an > application can now wait for failsafe Rx interrupts but it will not > receive one. > > Signed-off-by: Moti Haimovsky > --- > V6: > Fixed typo in commit subject. > > V5: > Initial version of this patch in accordance to inputs from Gaetan Rivet > in reply to > 1516354344-13495-2-git-send-email-motih@mellanox.com > --- > doc/guides/nics/features/failsafe.ini | 1 + > drivers/net/failsafe/Makefile | 1 + > drivers/net/failsafe/failsafe.c | 4 + > drivers/net/failsafe/failsafe_intr.c | 138 ++++++++++++++++++++++++++++++++ > drivers/net/failsafe/failsafe_ops.c | 64 +++++++++++++++ > drivers/net/failsafe/failsafe_private.h | 9 +++ > 6 files changed, 217 insertions(+) > create mode 100644 drivers/net/failsafe/failsafe_intr.c > > diff --git a/doc/guides/nics/features/failsafe.ini b/doc/guides/nics/features/failsafe.ini > index a42e344..39ee579 100644 > --- a/doc/guides/nics/features/failsafe.ini > +++ b/doc/guides/nics/features/failsafe.ini > @@ -6,6 +6,7 @@ > [Features] > Link status = Y > Link status event = Y > +Rx interrupt = Y It would have been more logical to update this once everything was properly implemented, but this is inconsequential. > MTU update = Y > Jumbo frame = Y > Promiscuous mode = Y > diff --git a/drivers/net/failsafe/Makefile b/drivers/net/failsafe/Makefile > index ea2a8fe..91a734b 100644 > --- a/drivers/net/failsafe/Makefile > +++ b/drivers/net/failsafe/Makefile > @@ -46,6 +46,7 @@ SRCS-$(CONFIG_RTE_LIBRTE_PMD_FAILSAFE) += failsafe_ops.c > SRCS-$(CONFIG_RTE_LIBRTE_PMD_FAILSAFE) += failsafe_rxtx.c > SRCS-$(CONFIG_RTE_LIBRTE_PMD_FAILSAFE) += failsafe_ether.c > SRCS-$(CONFIG_RTE_LIBRTE_PMD_FAILSAFE) += failsafe_flow.c > +SRCS-$(CONFIG_RTE_LIBRTE_PMD_FAILSAFE) += failsafe_intr.c > > # No exported include files > > diff --git a/drivers/net/failsafe/failsafe.c b/drivers/net/failsafe/failsafe.c > index cb274eb..921e656 100644 > --- a/drivers/net/failsafe/failsafe.c > +++ b/drivers/net/failsafe/failsafe.c > @@ -244,6 +244,10 @@ Please, can you update your git (I see 1.8, debian stable uses 2.11). The context without the function name is really hard to follow, especially in the devops section, where the context can be very sparse. I need each time to follow with the file opened on the side and verify the line numbers. > mac->addr_bytes[2], mac->addr_bytes[3], > mac->addr_bytes[4], mac->addr_bytes[5]); > dev->data->dev_flags |= RTE_ETH_DEV_INTR_LSC; > + PRIV(dev)->intr_handle = (struct rte_intr_handle){ > + .fd = -1, > + .type = RTE_INTR_HANDLE_EXT, > + }; > return 0; > free_args: > failsafe_args_free(dev); > diff --git a/drivers/net/failsafe/failsafe_intr.c b/drivers/net/failsafe/failsafe_intr.c > new file mode 100644 > index 0000000..54ef2f4 > --- /dev/null > +++ b/drivers/net/failsafe/failsafe_intr.c > @@ -0,0 +1,138 @@ > +/* SPDX-License-Identifier: BSD-3-Clause > + * Copyright 2018 Mellanox Technologies, Ltd. > + */ > + > +/** > + * @file > + * Interrupts handling for failsafe driver. > + */ > + > +#include > + > +#include "failsafe_private.h" > + > +/** > + * Uninstall failsafe interrupt vector. > + * > + * @param priv > + * Pointer to failsafe private structure. > + */ > +static void > +fs_rx_intr_vec_uninstall(struct fs_priv *priv) > +{ > + struct rte_intr_handle *intr_handle; > + > + intr_handle = &priv->intr_handle; > + if (intr_handle->intr_vec != NULL) { > + free(intr_handle->intr_vec); > + intr_handle->intr_vec = NULL; > + } > + intr_handle->nb_efd = 0; > +} > + > +/** > + * Installs failsafe interrupt vector to be registered with EAL later on. > + * > + * @param priv > + * Pointer to failsafe private structure. > + * > + * @return > + * 0 on success, negative errno value otherwise and rte_errno is set. > + */ > +static int > +fs_rx_intr_vec_install(struct fs_priv *priv) > +{ > + unsigned int i; > + unsigned int rxqs_n; > + unsigned int n; > + unsigned int count; > + struct rte_intr_handle *intr_handle; > + > + rxqs_n = priv->dev->data->nb_rx_queues; > + n = RTE_MIN(rxqs_n, (uint32_t)RTE_MAX_RXTX_INTR_VEC_ID); > + count = 0; > + intr_handle = &priv->intr_handle; > + /* Allocate the interrupt vector of the failsafe Rx proxy interrupts */ > + intr_handle->intr_vec = malloc(n * sizeof(intr_handle->intr_vec[0])); > + if (intr_handle->intr_vec == NULL) { > + fs_rx_intr_vec_uninstall(priv); > + rte_errno = ENOMEM; > + ERROR("Failed to allocate memory for interrupt vector," > + " Rx interrupts will not be supported"); > + return -rte_errno; > + } > + for (i = 0; i < n; i++) { > + struct rxq *rxq = priv->dev->data->rx_queues[i]; > + > + /* Skip queues that cannot request interrupts. */ > + if (rxq == NULL || rxq->event_fd < 0) { > + /* Use invalid intr_vec[] index to disable entry. */ > + intr_handle->intr_vec[i] = > + RTE_INTR_VEC_RXTX_OFFSET + > + RTE_MAX_RXTX_INTR_VEC_ID; > + continue; > + } > + if (count >= RTE_MAX_RXTX_INTR_VEC_ID) { > + rte_errno = E2BIG; > + ERROR("Too many Rx queues for interrupt vector size" > + " (%d), Rx interrupts cannot be enabled", > + RTE_MAX_RXTX_INTR_VEC_ID); > + fs_rx_intr_vec_uninstall(priv); > + return -rte_errno; > + } > + intr_handle->intr_vec[i] = RTE_INTR_VEC_RXTX_OFFSET + count; > + intr_handle->efds[count] = rxq->event_fd; > + count++; > + } > + if (count == 0) { > + fs_rx_intr_vec_uninstall(priv); > + } else { > + intr_handle->nb_efd = count; > + intr_handle->efd_counter_size = sizeof(uint64_t); > + } > + return 0; > +} > + > + > +/** > + * Uninstall failsafe Rx interrupts subsystem. > + * > + * @param priv > + * Pointer to private structure. > + * > + * @return > + * 0 on success, negative errno value otherwise and rte_errno is set. > + */ > +void > +failsafe_rx_intr_uninstall(struct rte_eth_dev *dev) > +{ > + struct fs_priv *priv; > + > + priv = PRIV(dev); > + fs_rx_intr_vec_uninstall(priv); Why not > + fs_rx_intr_vec_uninstall(PRIV(dev)); Here? Is it ok to call this without having enabled the interrupts before? I see the check on intr_conf done within intr_install, but then none is made here. It is probably fine right now, but what will happen once someone updates fs_rx_intr_vec_uninstall, with potentially dangerous side-effects? It would be safer to have a symmetric code flow on both init and uninit. > + dev->intr_handle = NULL; > +} > + > +/** > + * Install failsafe Rx interrupts subsystem. > + * > + * @param priv > + * Pointer to private structure. > + * > + * @return > + * 0 on success, negative errno value otherwise and rte_errno is set. > + */ > +int > +failsafe_rx_intr_install(struct rte_eth_dev *dev) > +{ > + struct fs_priv *priv = PRIV(dev); > + const struct rte_intr_conf *const intr_conf = > + &priv->dev->data->dev_conf.intr_conf; > + > + if (intr_conf->rxq == 0 || priv->intr_handle.intr_vec != NULL) Do you plan on calling this function with priv->intr_handle.intr_vec != NULL? Is it a condition that should ever happen? I think if this function was called and the vectors were already allocated, it would be a programming mistake. Putting an RTE_ASSERT() on the condition might be better for catching refactoring mistakes or reorders in configuration that might happen afterward. > + return 0; > + if (fs_rx_intr_vec_install(priv) < 0) > + return -rte_errno; > + dev->intr_handle = &priv->intr_handle; > + return 0; > +} > diff --git a/drivers/net/failsafe/failsafe_ops.c b/drivers/net/failsafe/failsafe_ops.c > index 946ac98..d6a82b3 100644 > --- a/drivers/net/failsafe/failsafe_ops.c > +++ b/drivers/net/failsafe/failsafe_ops.c > @@ -33,6 +33,7 @@ > > #include > #include > +#include > > #include > #include > @@ -199,6 +200,10 @@ > uint8_t i; > int ret; > > + ret = failsafe_rx_intr_install(dev); > + if (ret) > + return ret; > + Supperfluous line here. > FOREACH_SUBDEV(sdev, i, dev) { > if (sdev->state != DEV_ACTIVE) > continue; > @@ -228,6 +233,7 @@ > rte_eth_dev_stop(PORT_ID(sdev)); > sdev->state = DEV_STARTED - 1; > } > + failsafe_rx_intr_uninstall(dev); > } > > static int > @@ -317,6 +323,8 @@ > if (queue == NULL) > return; > rxq = queue; > + if (rxq->event_fd > 0) > + close(rxq->event_fd); > dev = rxq->priv->dev; > FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) > SUBOPS(sdev, rx_queue_release) > @@ -333,6 +341,16 @@ > const struct rte_eth_rxconf *rx_conf, > struct rte_mempool *mb_pool) > { > + /* > + * FIXME: Add a proper interface in rte_eal_interrupts for > + * allocating eventfd as an interrupt vector. > + * For the time being, fake as if we are using MSIX interrupts, > + * this will cause rte_intr_efd_enable to allocate an eventfd for us. > + */ > + struct rte_intr_handle intr_handle = { > + .type = RTE_INTR_HANDLE_VFIO_MSIX, > + .efds = {-1, }, Missing space: > + .efds = { -1, }, Regards, -- Gaëtan Rivet 6WIND