Re: [dpdk-dev] [PATCH v7 2/3] net/failsafe: slaves Rx interrupts registration

["Gaëtan Rivet" <gaetan.rivet@6wind.com>]

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 <motih@mellanox.com>
> ---
> 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 <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 =
> +				&ETH(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