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 <mo...@mellanox.com>
> ---
> V7:
> Fixed compilation errors in FreeBSD.
> See 1516810328-39383-3-git-send-email-mo...@mellanox.com
>
> V6:
> Added a wrapper around epoll_create1 since it is not supported in FreeBSD.
> See: 1516193643-130838-1-git-send-email-mo...@mellanox.com
>
> V5:
> Initial version of this patch in accordance to inputs from Gaetan Rivet
> in reply to
> 1516354344-13495-2-git-send-email-mo...@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 <rte_common.h>
> +
> +#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 <sys/epoll.h>
> +
> +#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 <unistd.h>
>
> +#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
