From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr0-f196.google.com (mail-wr0-f196.google.com [209.85.128.196]) by dpdk.org (Postfix) with ESMTP id 28B00FFA for ; Thu, 25 Jan 2018 12:49:57 +0100 (CET) Received: by mail-wr0-f196.google.com with SMTP id f11so7352109wre.4 for ; Thu, 25 Jan 2018 03:49:57 -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=qD6QobLLNmLizOJMsdymTMCU4ZOaiQZjsDeZyj0SUYg=; b=FodTIBFL63u1jed7nX4gDm+p4TD0L2gc/lFNAjF8EGK9Hhl3b6COaWVyEntTqqbPin sVw/N6ABQwhTRg7w5KzHp4S/NBV/E7U95xBR4rOvYABXk3D+K9LLGy1XZhDYJDQsZkEU bgdXq6HL67fcpj3bhRERRFjR1CK4VO3hvn1E6FdhH/7M5m1u+GzbKHBFXFxNcYgsTvhL 8kdjmSizBuN8ph+qdubgm48Q3AJ2C1F63uDhTLLXfCzQhe76KbruxbVQzmHvcUJU+J3M lbvWzUUEKO3thNyervlcvfk7+1AEMWfZ1GU33VYQOj1GyFl0QcxcXMUB95okL4rkmRhU Zq4Q== 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=qD6QobLLNmLizOJMsdymTMCU4ZOaiQZjsDeZyj0SUYg=; b=HO4NmR9g0nitC3y37sE470BWBCnxYmLfD+EFLRsLEmit+HLfX+OELy6fvJ74f+IB60 5ct2INC/1ILgBb/kpZL1JLFFLvCCIFUMV7yuvCPQhD2e/A0RKch0+etlvWknGTwm310k 5JrzJ6ZHzY9hgT86NXMYXT6r6gfOEXQVsNxFAIsaixHGpeAcL2a34yvwH/SxRvnnJBQB unCK8cCxTQY7PKlYXZZUhw+i4D49QxPpvX3C8tBtRN5kn5Vw/LSsSWk9y1uSb1BsFrCv w2oddxxRF3m2S+NFP6AlJvp/pq9A8wsgPgS4r9ChiS8TJyFg5VJlpZshHh/MKkji8tc8 urkg== X-Gm-Message-State: AKwxytdEDNWuwijbh1P1MDKcpJwgIPSu+bta47p41dVvfV9HT1vx/Kvg BkoSRdH0KWac3AtTzvoEljzfRA== X-Google-Smtp-Source: AH8x224TVwbGoZdL2hYan7LJTrtrxD8oHtUBRwiHrRZe8mib/dsb7yEHTWAcWX1S5ZK4CqcFNfeKvA== X-Received: by 10.223.155.196 with SMTP id e4mr9249565wrc.63.1516880996523; Thu, 25 Jan 2018 03:49:56 -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 f22sm1156003wmi.24.2018.01.25.03.49.55 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Thu, 25 Jan 2018 03:49:55 -0800 (PST) Date: Thu, 25 Jan 2018 12:49:43 +0100 From: =?iso-8859-1?Q?Ga=EBtan?= Rivet To: Moti Haimovsky Cc: ferruh.yigit@intel.com, dev@dpdk.org Message-ID: <20180125114943.2rdjt4boyugql4lm@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-3-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-3-git-send-email-motih@mellanox.com> User-Agent: NeoMutt/20170113 (1.7.2) Subject: Re: [dpdk-dev] [PATCH v7 2/3] net/failsafe: slaves Rx interrupts registration 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:49:57 -0000 On Thu, Jan 25, 2018 at 10:07:14AM +0200, Moti Haimovsky wrote: > This commit adds the following functionality to failsafe PMD: > * Register and unregister slaves Rx interrupts. > * Enable and Disable slaves Rx interrupts. > The interrupts events generated by the slaves are not handled in this > commit. > > Signed-off-by: Moti Haimovsky > --- > V7: > Fixed compilation errors in FreeBSD. > See 1516810328-39383-3-git-send-email-motih@mellanox.com > > V6: > Added a wrapper around epoll_create1 since it is not supported in FreeBSD. > See: 1516193643-130838-1-git-send-email-motih@mellanox.com > > 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 > --- > drivers/net/failsafe/Makefile | 5 + > drivers/net/failsafe/failsafe_epoll.h | 10 ++ > drivers/net/failsafe/failsafe_epoll_bsdapp.c | 19 +++ > drivers/net/failsafe/failsafe_epoll_linuxapp.c | 18 +++ > drivers/net/failsafe/failsafe_ether.c | 1 + > drivers/net/failsafe/failsafe_intr.c | 198 +++++++++++++++++++++++++ > drivers/net/failsafe/failsafe_ops.c | 36 ++++- > drivers/net/failsafe/failsafe_private.h | 16 ++ > 8 files changed, 301 insertions(+), 2 deletions(-) > create mode 100644 drivers/net/failsafe/failsafe_epoll.h > create mode 100644 drivers/net/failsafe/failsafe_epoll_bsdapp.c > create mode 100644 drivers/net/failsafe/failsafe_epoll_linuxapp.c > > diff --git a/drivers/net/failsafe/Makefile b/drivers/net/failsafe/Makefile > index 91a734b..4e6a983 100644 > --- a/drivers/net/failsafe/Makefile > +++ b/drivers/net/failsafe/Makefile > @@ -47,6 +47,11 @@ 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 > +ifeq ($(CONFIG_RTE_EXEC_ENV_LINUXAPP),y) > +SRCS-$(CONFIG_RTE_LIBRTE_PMD_FAILSAFE) += failsafe_epoll_linuxapp.c > +else > +SRCS-$(CONFIG_RTE_LIBRTE_PMD_FAILSAFE) += failsafe_epoll_bsdapp.c > +endif I'm not a fan of whole additional source files for only one function. Why not something akin to: ifeq ($(CONFIG_RTE_EXEC_ENV_LINUXAPP),y) CFLAGS += -DLINUX elif ($(CONFIG_RTE_EXEC_ENV_BSDAPP),y) CFLAGS += -DBSD endif Then, within failsafe_intr.c: static int fs_epoll_create1(int flags) { #if define(LINUX) return epoll_create1(flags); #elif defined(BSD) RTE_SET_USED(flags); return -ENOTSUP; #endif } > > # No exported include files > > diff --git a/drivers/net/failsafe/failsafe_epoll.h b/drivers/net/failsafe/failsafe_epoll.h > new file mode 100644 > index 0000000..8e6a1ec > --- /dev/null > +++ b/drivers/net/failsafe/failsafe_epoll.h > @@ -0,0 +1,10 @@ > +/* SPDX-License-Identifier: BSD-3-Clause > + * Copyright 2018 Mellanox Technologies, Ltd. > + */ > + > +#ifndef _RTE_ETH_FAILSAFE_EPOLL_H_ > +#define _RTE_ETH_FAILSAFE_EPOLL_H_ > + > +int failsafe_epoll_create1(int flags); > + > +#endif /* _RTE_ETH_FAILSAFE_EPOLL_H_*/ > diff --git a/drivers/net/failsafe/failsafe_epoll_bsdapp.c b/drivers/net/failsafe/failsafe_epoll_bsdapp.c > new file mode 100644 > index 0000000..46c839b > --- /dev/null > +++ b/drivers/net/failsafe/failsafe_epoll_bsdapp.c > @@ -0,0 +1,19 @@ > +/* SPDX-License-Identifier: BSD-3-Clause > + * Copyright 2018 Mellanox Technologies, Ltd. > + */ > + > +/** > + * @file > + * epoll wrapper for failsafe driver. > + */ > + > +#include > + > +#include "failsafe_epoll.h" > + > +int > +failsafe_epoll_create1(int flags) > +{ > + RTE_SET_USED(flags); > + return -ENOTSUP; > +} > diff --git a/drivers/net/failsafe/failsafe_epoll_linuxapp.c b/drivers/net/failsafe/failsafe_epoll_linuxapp.c > new file mode 100644 > index 0000000..d82ee0a > --- /dev/null > +++ b/drivers/net/failsafe/failsafe_epoll_linuxapp.c > @@ -0,0 +1,18 @@ > +/* SPDX-License-Identifier: BSD-3-Clause > + * Copyright 2018 Mellanox Technologies, Ltd. > + */ > + > +/** > + * @file > + * epoll wrapper for failsafe driver. > + */ > + > +#include > + > +#include "failsafe_epoll.h" > + > +int > +failsafe_epoll_create1(int flags) > +{ > + return epoll_create1(flags); > +} > diff --git a/drivers/net/failsafe/failsafe_ether.c b/drivers/net/failsafe/failsafe_ether.c > index 8a4cacf..0f1630e 100644 > --- a/drivers/net/failsafe/failsafe_ether.c > +++ b/drivers/net/failsafe/failsafe_ether.c > @@ -283,6 +283,7 @@ > return; > switch (sdev->state) { > case DEV_STARTED: > + failsafe_rx_intr_uninstall_subdevice(sdev); > rte_eth_dev_stop(PORT_ID(sdev)); > sdev->state = DEV_ACTIVE; > /* fallthrough */ > diff --git a/drivers/net/failsafe/failsafe_intr.c b/drivers/net/failsafe/failsafe_intr.c > index 54ef2f4..8f8f129 100644 > --- a/drivers/net/failsafe/failsafe_intr.c > +++ b/drivers/net/failsafe/failsafe_intr.c > @@ -9,8 +9,198 @@ > > #include > > +#include "failsafe_epoll.h" > #include "failsafe_private.h" > > +#define NUM_RX_PROXIES (FAILSAFE_MAX_ETHPORTS * RTE_MAX_RXTX_INTR_VEC_ID) > + > +/** > + * Install failsafe Rx event proxy subsystem. > + * This is the way the failsafe PMD generates Rx events on behalf of its > + * subdevices. > + * > + * @param priv > + * Pointer to failsafe private structure. > + * @return > + * 0 on success, negative errno value otherwise and rte_errno is set. > + */ > +static int > +fs_rx_event_proxy_install(struct fs_priv *priv) > +{ > + int rc = 0; > + > + /* > + * Create the epoll fd and event vector for the proxy service to > + * wait on for Rx events generated by the subdevices. > + */ > + priv->rxp.efd = failsafe_epoll_create1(0); > + if (priv->rxp.efd < 0) { > + rte_errno = errno; > + ERROR("Failed to create epoll," > + " Rx interrupts will not be supported"); > + return -rte_errno; > + } > + priv->rxp.evec = calloc(NUM_RX_PROXIES, sizeof(*priv->rxp.evec)); > + if (priv->rxp.evec == NULL) { > + ERROR("Failed to allocate memory for event vectors," > + " Rx interrupts will not be supported"); > + rc = -ENOMEM; > + goto error; > + } > + return 0; > +error: > + if (priv->rxp.efd >= 0) { > + close(priv->rxp.efd); > + priv->rxp.efd = -1; > + } > + if (priv->rxp.evec != NULL) { > + free(priv->rxp.evec); > + priv->rxp.evec = NULL; > + } > + rte_errno = -rc; > + return rc; > +} > + > +/** > + * RX Interrupt control per subdevice. > + * > + * @param sdev > + * Pointer to sub-device structure. > + * @param op > + * The operation be performed for the vector. > + * Operation type of {RTE_INTR_EVENT_ADD, RTE_INTR_EVENT_DEL}. > + * @return > + * - On success, zero. > + * - On failure, a negative value. > + */ > +static int > +failsafe_eth_rx_intr_ctl_subdevice(struct sub_device *sdev, int op) > +{ > + struct rte_eth_dev *dev; > + struct rte_eth_dev *fsdev; > + int epfd; > + uint16_t pid; > + uint16_t qid; > + struct rxq *fsrxq; > + int rc; > + int ret = 0; > + > + if (sdev == NULL || (ETH(sdev) == NULL) || > + sdev->fs_dev == NULL || (PRIV(sdev->fs_dev) == NULL)) { > + ERROR("Called with invalid arguments"); > + return -EINVAL; > + } > + dev = ETH(sdev); > + fsdev = sdev->fs_dev; > + epfd = PRIV(sdev->fs_dev)->rxp.efd; > + pid = PORT_ID(sdev); > + > + if (epfd <= 0) { > + if (op == RTE_INTR_EVENT_ADD) { > + ERROR("Proxy events are not initialized"); > + return -EBADF; > + } else { > + return 0; > + } > + } > + if (dev->data->nb_rx_queues > fsdev->data->nb_rx_queues) { > + ERROR("subdevice has too many queues," > + " Interrupts will not be enabled"); > + return -E2BIG; > + } > + for (qid = 0; qid < dev->data->nb_rx_queues; qid++) { > + fsrxq = fsdev->data->rx_queues[qid]; > + rc = rte_eth_dev_rx_intr_ctl_q(pid, qid, epfd, > + op, (void *)fsrxq); > + if (rc) { > + ERROR("rte_eth_dev_rx_intr_ctl_q failed for " > + "port %d queue %d, epfd %d, error %d", > + pid, qid, epfd, rc); > + ret = rc; > + } > + } > + return ret; > +} > + > +/** > + * Install Rx interrupts subsystem for a subdevice. > + * This is a support for dynamically adding subdevices. > + * > + * @param sdev > + * Pointer to subdevice structure. > + * > + * @return > + * 0 on success, negative errno value otherwise and rte_errno is set. > + */ > +int failsafe_rx_intr_install_subdevice(struct sub_device *sdev) > +{ > + int rc; > + int qid; > + struct rte_eth_dev *fsdev; > + struct rxq **rxq; > + const struct rte_intr_conf *const intr_conf = > + Ð(sdev)->data->dev_conf.intr_conf; > + > + fsdev = sdev->fs_dev; > + rxq = (struct rxq **)fsdev->data->rx_queues; > + if (intr_conf->rxq == 0) > + return 0; > + rc = failsafe_eth_rx_intr_ctl_subdevice(sdev, RTE_INTR_EVENT_ADD); > + if (rc) > + return rc; > + /* enable interrupts on already-enabled queues */ > + for (qid = 0; qid < ETH(sdev)->data->nb_rx_queues; qid++) { > + if (rxq[qid]->enable_events) { > + int ret = rte_eth_dev_rx_intr_enable(PORT_ID(sdev), > + qid); > + if (ret && (ret != -ENOTSUP)) { > + ERROR("Failed to enable interrupts on " > + "port %d queue %d", PORT_ID(sdev), qid); > + rc = ret; > + } > + } > + } > + return rc; > +} > + > +/** > + * Uninstall Rx interrupts subsystem for a subdevice. > + * This is a support for dynamically removing subdevices. > + * > + * @param sdev > + * Pointer to subdevice structure. > + * > + * @return > + * 0 on success, negative errno value otherwise and rte_errno is set. > + */ > +void failsafe_rx_intr_uninstall_subdevice(struct sub_device *sdev) > +{ > + int qid; > + > + for (qid = 0; qid < ETH(sdev)->data->nb_rx_queues; qid++) > + rte_eth_dev_rx_intr_disable(PORT_ID(sdev), qid); I think here you assume the underlying PMD has been properly implemented, and that calling rte_eth_dev_rx_intr_disable has no effect. I think that it would be better to write defensively in general regarding other PMDs, and assume a bare minimal respect of the API. As such, here you might want to check that the intr_conf asked for interrupt enabling before attempting to disable it. > + failsafe_eth_rx_intr_ctl_subdevice(sdev, RTE_INTR_EVENT_DEL); > +} > + > +/** > + * Uninstall failsafe Rx event proxy. > + * > + * @param priv > + * Pointer to failsafe private structure. > + */ > +static void > +fs_rx_event_proxy_uninstall(struct fs_priv *priv) > +{ > + if (priv->rxp.evec != NULL) { > + free(priv->rxp.evec); > + priv->rxp.evec = NULL; > + } > + if (priv->rxp.efd > 0) { > + close(priv->rxp.efd); > + priv->rxp.efd = -1; > + } > +} > + > /** > * Uninstall failsafe interrupt vector. > * > @@ -107,8 +297,12 @@ > failsafe_rx_intr_uninstall(struct rte_eth_dev *dev) > { > struct fs_priv *priv; > + struct rte_intr_handle *intr_handle; > > priv = PRIV(dev); > + intr_handle = &priv->intr_handle; > + rte_intr_free_epoll_fd(intr_handle); > + fs_rx_event_proxy_uninstall(priv); > fs_rx_intr_vec_uninstall(priv); > dev->intr_handle = NULL; > } > @@ -133,6 +327,10 @@ > return 0; > if (fs_rx_intr_vec_install(priv) < 0) > return -rte_errno; > + if (fs_rx_event_proxy_install(priv) < 0) { > + fs_rx_intr_vec_uninstall(priv); > + 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 d6a82b3..2ea9cdd 100644 > --- a/drivers/net/failsafe/failsafe_ops.c > +++ b/drivers/net/failsafe/failsafe_ops.c > @@ -214,6 +214,13 @@ > continue; > return ret; > } > + ret = failsafe_rx_intr_install_subdevice(sdev); > + if (ret) { > + if (!fs_err(sdev, ret)) > + continue; > + rte_eth_dev_stop(PORT_ID(sdev)); > + return ret; > + } > sdev->state = DEV_STARTED; > } > if (PRIV(dev)->state < DEV_STARTED) > @@ -231,6 +238,7 @@ > PRIV(dev)->state = DEV_STARTED - 1; > FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_STARTED) { > rte_eth_dev_stop(PORT_ID(sdev)); > + failsafe_rx_intr_uninstall_subdevice(sdev); > sdev->state = DEV_STARTED - 1; > } > failsafe_rx_intr_uninstall(dev); > @@ -413,6 +421,10 @@ > fs_rx_intr_enable(struct rte_eth_dev *dev, uint16_t idx) > { > struct rxq *rxq; > + struct sub_device *sdev; > + uint8_t i; > + int ret; > + int rc = 0; > > if (idx >= dev->data->nb_rx_queues) { > rte_errno = EINVAL; > @@ -424,14 +436,26 @@ > return -rte_errno; > } > rxq->enable_events = 1; > - return 0; > + FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) { > + ret = rte_eth_dev_rx_intr_enable(PORT_ID(sdev), idx); > + ret = fs_err(sdev, ret); > + if (ret) > + rc = ret; why not rc = fs_err(sdev, ret); if (rc) break; instead? I'm not sure ret is even really useful to have here, but I don't see the rest of the function. Regards, -- Gaëtan Rivet 6WIND