Re: [dpdk-dev] [PATCH v7 3/3] net/failsafe: add Rx interrupts

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

On Thu, Jan 25, 2018 at 10:07:15AM +0200, Moti Haimovsky wrote:
> This patch is the last patch in the series of patches aimed
> to add support for registering and waiting for Rx interrupts
> in failsafe PMD. This allows applications to wait for Rx events
> from the PMD using the DPDK rte_epoll subsystem.
> The failsafe PMD presents to the application a facade of a single
> device to be handled by the application while internally it manages
> several devices on behalf of the application including packets
> transmission and reception.
> The Proposed failsafe Rx interrupt scheme follows this approach.
> The failsafe PMD will present the application with a single set of
> Rx interrupt vectors representing the failsafe Rx queues, while
> internally it will serve as an interrupt proxy for its subdevices.
> will allow applications to wait for Rx traffic from the failsafe
> PMD by registering and waiting for Rx events from its Rx queues.
> In order to support this the following is suggested:
>   * Every Rx queue in the failsafe (virtual) device will be assigned
>   * a Linux event file descriptor (efd) and an enable_interrupts flag.
>   * The failsafe PMD will fill in its rte_intr_handle structure with
>     the Rx efds assigned previously and register them with the EAL.
>   * The failsafe driver will create a private epoll fd (epfd) and
>   * will allocate enough space to handle all the Rx events from all its
>     subdevices.
>   * Acting as an application,
>     for each Rx queue in each active subdevice the failsafe will:
>       o Register the Rx queue with the EAL.
>       o Pass the EAL the failsafe private epoll fd as the epfd to
>         register the Rx queue event on.
>       o Pass the EAL, as a parameter, the pointer to the failsafe Rx
>         queue that handles this Rx queue.
>       o Using the DPDK service callbacks, the failsafe PMD will launch
>         an Rx proxy service that will Wait on the epoll fd for Rx
>         events from the sub-devices.
>       o For each Rx event received the proxy service will
>           - Retrieve the pointer to failsafe Rx queue that handles
>             this subdevice Rx queue from the user info returned by the
>             EAL.
>           - Trigger a failsafe Rx event on that queue by writing to
>             the event fd unless interrupts are disabled for that queue.
>   * The failsafe pmd will also implement the rx_queue_intr_enable
>   * and rx_queue_intr_disable routines that will enable and disable Rx
>     interrupts respectively on both on the failsafe and its subdevices.
> 
> Signed-off-by: Moti Haimovsky <motih@mellanox.com>
> ---
> V6:
> Separated between routines' variables definition and initialization
> according to guidelines from Gaetan Rivet.
> 
> V5:
> Modified code and split the patch into three patches in accordance to
> inputs from Gaetan Rivet in reply to
> 1516354344-13495-2-git-send-email-motih@mellanox.com
> 
> V4:
> Fixed merge conflicts found during integration with other failsafe patches
> (See cover letter).
> 
> V3:
> Fixed build failures in FreeBSD10.3_64
> 
> V2:
> Modifications according to inputs from Stephen Hemminger:
> * Removed unneeded (void *) casting.
> Fixed coding style warning.
> ---
>  drivers/net/failsafe/failsafe_intr.c    | 169 ++++++++++++++++++++++++++++++++
>  drivers/net/failsafe/failsafe_ops.c     |   6 ++
>  drivers/net/failsafe/failsafe_private.h |  17 +++-
>  3 files changed, 191 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/failsafe/failsafe_intr.c b/drivers/net/failsafe/failsafe_intr.c
> index 8f8f129..c58289b 100644
> --- a/drivers/net/failsafe/failsafe_intr.c
> +++ b/drivers/net/failsafe/failsafe_intr.c
> @@ -9,12 +9,176 @@
>  
>  #include <unistd.h>
>  
> +#include <rte_alarm.h>
> +#include <rte_config.h>
> +#include <rte_errno.h>
> +#include <rte_ethdev.h>
> +#include <rte_interrupts.h>
> +#include <rte_io.h>
> +#include <rte_service_component.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 service.
> + * The Rx event proxy is the service that listens to Rx events from the
> + * subdevices and triggers failsafe Rx events accordingly.
> + *
> + * @param priv
> + *   Pointer to failsafe private structure.
> + * @return
> + *   0 on success, negative errno value otherwise.
> + */
> +static int
> +fs_rx_event_proxy_routine(void *data)
> +{
> +	struct fs_priv *priv;
> +	struct rxq *rxq;
> +	struct rte_epoll_event *events;
> +	uint64_t u64;
> +	int i, n;
> +	int rc = 0;
> +
> +	u64 = 1;
> +	priv = data;
> +	events = priv->rxp.evec;
> +	n = rte_epoll_wait(priv->rxp.efd, events, NUM_RX_PROXIES, -1);
> +	for (i = 0; i < n; i++) {
> +		rxq = events[i].epdata.data;
> +		if (rxq->enable_events && rxq->event_fd != -1) {
> +			if (write(rxq->event_fd, &u64, sizeof(u64)) !=
> +			    sizeof(u64)) {
> +				ERROR("Failed to proxy Rx event to socket %d",
> +				       rxq->event_fd);
> +				rc = -EIO;
> +			}
> +		}
> +	}
> +	return rc;
> +}
> +
> +/**
> + * Uninstall failsafe Rx event proxy service.
> + *
> + * @param priv
> + *   Pointer to failsafe private structure.
> + */
> +static void
> +fs_rx_event_proxy_service_uninstall(struct fs_priv *priv)
> +{
> +	/* Unregister the event service. */
> +	switch (priv->rxp.sstate) {
> +	case SS_RUNNING:
> +		rte_service_map_lcore_set(priv->rxp.sid, priv->rxp.scid, 0);
> +		/* fall through */
> +	case SS_READY:
> +		rte_service_runstate_set(priv->rxp.sid, 0);
> +		rte_service_set_stats_enable(priv->rxp.sid, 0);
> +		rte_service_component_runstate_set(priv->rxp.sid, 0);
> +		/* fall through */
> +	case SS_REGISTERED:
> +		rte_service_component_unregister(priv->rxp.sid);
> +		/* fall through */
> +	default:
> +		break;
> +	}
> +}
> +
> +/**
> + * Install the failsafe Rx event proxy service.
> + *
> + * @param priv
> + *   Pointer to failsafe private structure.
> + * @return
> + *   0 on success, negative errno value otherwise.
> + */
> +static int
> +fs_rx_event_proxy_service_install(struct fs_priv *priv)
> +{
> +	struct rte_service_spec service;
> +	int32_t num_service_cores;
> +	int ret = 0;
> +
> +	num_service_cores = rte_service_lcore_count();
> +	if (num_service_cores <= 0) {
> +		ERROR("Failed to install Rx interrupts, "
> +		      "no service core found");
> +		return -ENOTSUP;

You don't seem to update rte_errno here,
while you set rc to -rte_errno when checking for errors on this function
later.

> +	}
> +	/* prepare service info */
> +	memset(&service, 0, sizeof(struct rte_service_spec));
> +	snprintf(service.name, sizeof(service.name), "%s_Rx_service",
> +		 priv->dev->data->name);
> +	service.socket_id = priv->dev->data->numa_node;
> +	service.callback = fs_rx_event_proxy_routine;
> +	service.callback_userdata = (void *)priv;

The cast is unnecessary here I think.

> +
> +	if (priv->rxp.sstate == SS_NO_SERVICE) {
> +		uint32_t service_core_list[num_service_cores];
> +
> +		/* get a service core to work with */
> +		ret = rte_service_lcore_list(service_core_list,
> +					     num_service_cores);
> +		if (ret <= 0) {
> +			ERROR("Failed to install Rx interrupts, "
> +			      "service core list empty or corrupted");
> +			return -ENOTSUP;

Same comment regarding setting rte_errno here, and afterward.

> +		}
> +		priv->rxp.scid = service_core_list[0];
> +		ret = rte_service_lcore_add(priv->rxp.scid);
> +		if (ret && ret != -EALREADY) {
> +			ERROR("Failed adding service core");
> +			return ret;
> +		}
> +		/* service core may be in "stopped" state, start it */
> +		ret = rte_service_lcore_start(priv->rxp.scid);
> +		if (ret && (ret != -EALREADY)) {
> +			ERROR("Failed to install Rx interrupts, "
> +			      "service core not started");
> +			return ret;
> +		}
> +		/* register our service */
> +		int32_t ret = rte_service_component_register(&service,
> +							     &priv->rxp.sid);
> +		if (ret) {
> +			ERROR("service register() failed");
> +			return -ENOEXEC;
> +		}
> +		priv->rxp.sstate = SS_REGISTERED;
> +		/* run the service */
> +		ret = rte_service_component_runstate_set(priv->rxp.sid, 1);
> +		if (ret < 0) {
> +			ERROR("Failed Setting component runstate\n");
> +			return ret;
> +		}
> +		ret = rte_service_set_stats_enable(priv->rxp.sid, 1);
> +		if (ret < 0) {
> +			ERROR("Failed enabling stats\n");
> +			return ret;
> +		}
> +		ret = rte_service_runstate_set(priv->rxp.sid, 1);
> +		if (ret < 0) {
> +			ERROR("Failed to run service\n");
> +			return ret;
> +		}
> +		priv->rxp.sstate = SS_READY;
> +		/* map the service with the service core */
> +		ret = rte_service_map_lcore_set(priv->rxp.sid,
> +						priv->rxp.scid, 1);
> +		if (ret) {
> +			ERROR("Failed to install Rx interrupts, "
> +			      "could not map service core");
> +			return ret;
> +		}
> +		priv->rxp.sstate = SS_RUNNING;
> +	}
> +	return 0;
> +}
> +
> +/**
>   * Install failsafe Rx event proxy subsystem.
>   * This is the way the failsafe PMD generates Rx events on behalf of its
>   * subdevices.
> @@ -47,6 +211,10 @@
>  		rc = -ENOMEM;
>  		goto error;
>  	}
> +	if (fs_rx_event_proxy_service_install(priv) < 0) {
> +		rc = -rte_errno;
> +		goto error;
> +	}
>  	return 0;
>  error:
>  	if (priv->rxp.efd >= 0) {

-- 
Gaëtan Rivet
6WIND