Re: [dpdk-dev] [PATCH 12/14] net/mlx5: update install/uninstall int handler routines

[Shahaf Shuler <shahafs@mellanox.com>]

Thursday, March 21, 2019 10:11 AM, Viacheslav Ovsiienko:
> Subject: [PATCH 12/14] net/mlx5: update install/uninstall int handler routines
> 
> We are implementing the support for multport Infiniband device withj
> representors attached to these multiple ports. Asynchronous device event
> notifications (link status change, removal event, etc.) should be shared
> between ports. We are going to implement shared event handler and this
> patch introduces appropriate device structure changes and updated event
> handler install and uninstall routines.
> 
> Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
> ---
>  drivers/net/mlx5/mlx5.c        |  14 ++++-
>  drivers/net/mlx5/mlx5.h        |   3 +-
>  drivers/net/mlx5/mlx5_ethdev.c | 118
> ++++++++++++++++++++++++++++++++---------
>  3 files changed, 107 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c index
> 312c42b..44b7a87 100644
> --- a/drivers/net/mlx5/mlx5.c
> +++ b/drivers/net/mlx5/mlx5.c
> @@ -165,6 +165,7 @@ struct mlx5_dev_spawn_data {  {
>  	struct mlx5_ibv_shared *sh;
>  	int err = 0;
> +	uint32_t i;
> 
>  	assert(spawn);
>  	/* Search for IB context by device name. */ @@ -212,6 +213,9 @@
> struct mlx5_dev_spawn_data {
>  		sizeof(sh->ibdev_name));
>  	strncpy(sh->ibdev_path, sh->ctx->device->ibdev_path,
>  		sizeof(sh->ibdev_path));
> +	pthread_mutex_init(&sh->intr_mutex, NULL);
> +	for (i = 0; i < sh->max_port; i++)
> +		sh->port[i].port_id = RTE_MAX_ETHPORTS;

Why you need struct here? You port array is not just of uint32_t type?

>  	if (rte_eal_process_type() == RTE_PROC_SECONDARY) {
>  		/*
>  		 * For secondary process we just open the IB device @@ -
> 276,6 +280,15 @@ struct mlx5_dev_spawn_data {
>  		assert(!sh->pd);
>  	}
>  	LIST_REMOVE(sh, next);
> +	/*
> +	 *  Ensure there is no async event handler installed.
> +	 *  Only primary process handles async device events.
> +	 **/
> +	assert(!sh->intr_cnt);
> +	if (sh->intr_cnt)
> +		rte_intr_callback_unregister
> +			(&sh->intr_handle, mlx5_dev_interrupt_handler,
> sh);
> +	pthread_mutex_destroy(&sh->intr_mutex);
>  	if (sh->pd)
>  		claim_zero(mlx5_glue->dealloc_pd(sh->pd));
>  	if (sh->ctx)
> @@ -283,7 +296,6 @@ struct mlx5_dev_spawn_data {
>  	rte_free(sh);
>  }
> 
> -
>  /**
>   * Prepare shared data between primary and secondary process.
>   */
> diff --git a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h index
> d816d24..f23298e 100644
> --- a/drivers/net/mlx5/mlx5.h
> +++ b/drivers/net/mlx5/mlx5.h
> @@ -216,6 +216,8 @@ struct mlx5_ibv_shared {
>  	char ibdev_name[IBV_SYSFS_NAME_MAX]; /* IB device name. */
>  	char ibdev_path[IBV_SYSFS_PATH_MAX]; /* IB device path for
> secondary */
>  	struct ibv_device_attr_ex device_attr; /* Device properties. */
> +	pthread_mutex_t intr_mutex; /* Interrupt config mutex. */
> +	uint32_t intr_cnt; /* Interrupt handler reference counter. */
>  	struct rte_intr_handle intr_handle; /* Interrupt handler for device.
> */
>  	struct mlx5_ibv_shared_port port[]; /* per device port data array. */
> }; @@ -245,7 +247,6 @@ struct mlx5_priv {
>  	struct mlx5_txq_data *(*txqs)[]; /* TX queues. */
>  	struct rte_mempool *mprq_mp; /* Mempool for Multi-Packet RQ.
> */
>  	struct rte_eth_rss_conf rss_conf; /* RSS configuration. */
> -	struct rte_intr_handle intr_handle; /* Interrupt handler. */
>  	unsigned int (*reta_idx)[]; /* RETA index table. */
>  	unsigned int reta_idx_n; /* RETA index size. */
>  	struct mlx5_drop drop_queue; /* Flow drop queues. */ diff --git
> a/drivers/net/mlx5/mlx5_ethdev.c b/drivers/net/mlx5/mlx5_ethdev.c index
> 1b2173b..8358cd2 100644
> --- a/drivers/net/mlx5/mlx5_ethdev.c
> +++ b/drivers/net/mlx5/mlx5_ethdev.c
> @@ -1109,6 +1109,96 @@ int mlx5_fw_version_get(struct rte_eth_dev
> *dev, char *fw_ver, size_t fw_size)  }
> 
>  /**
> + * Uninstall shared asynchronous device events handler.
> + * This function is implemeted to support event sharing
> + * between multiple ports of single IB device.
> + *
> + * @param dev
> + *   Pointer to Ethernet device.
> + */
> +static void
> +mlx5_dev_shared_handler_uninstall(struct rte_eth_dev *dev) {
> +	struct mlx5_priv *priv = dev->data->dev_private;
> +	struct mlx5_ibv_shared *sh = priv->sh;
> +
> +	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
> +		return;
> +	pthread_mutex_lock(&sh->intr_mutex);
> +	assert(priv->ibv_port);
> +	assert(priv->ibv_port <= sh->max_port);
> +	assert(dev->data->port_id < RTE_MAX_ETHPORTS);
> +	if (sh->port[priv->ibv_port - 1].port_id >= RTE_MAX_ETHPORTS)
> +		goto exit;
> +	assert(sh->port[priv->ibv_port - 1].port_id ==
> +					(uint32_t)dev->data->port_id);
> +	assert(sh->intr_cnt);
> +	sh->port[priv->ibv_port - 1].port_id = RTE_MAX_ETHPORTS;
> +	if (!sh->intr_cnt || --sh->intr_cnt)
> +		goto exit;
> +	rte_intr_callback_unregister(&sh->intr_handle,
> +				     mlx5_dev_interrupt_handler, sh);
> +	sh->intr_handle.fd = 0;
> +	sh->intr_handle.type = RTE_INTR_HANDLE_UNKNOWN;
> +exit:
> +	pthread_mutex_unlock(&sh->intr_mutex);
> +}
> +
> +/**
> + * Install shared asyncronous device events handler.
> + * This function is implemeted to support event sharing
> + * between multiple ports of single IB device.
> + *
> + * @param dev
> + *   Pointer to Ethernet device.
> + */
> +static void
> +mlx5_dev_shared_handler_install(struct rte_eth_dev *dev) {
> +	struct mlx5_priv *priv = dev->data->dev_private;
> +	struct mlx5_ibv_shared *sh = priv->sh;
> +	int ret;
> +	int flags;
> +
> +	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
> +		return;
> +	pthread_mutex_lock(&sh->intr_mutex);
> +	assert(priv->ibv_port);
> +	assert(priv->ibv_port <= sh->max_port);
> +	assert(dev->data->port_id < RTE_MAX_ETHPORTS);
> +	if (sh->port[priv->ibv_port - 1].port_id < RTE_MAX_ETHPORTS) {

I don't understand why need an array to understand handler is already exists. 
Why not the refcnt?

> +		/* The handler is already installed for this port. */
> +		assert(sh->intr_cnt++);

Asserts are compiled only in debug mode. You should not put any logic (++) into them. 

> +		goto exit;
> +	}
> +	sh->port[priv->ibv_port - 1].port_id = (uint32_t)dev->data->port_id;
> +	if (sh->intr_cnt) {
> +		sh->intr_cnt++;
> +		goto exit;
> +	}
> +	/* No shared handler installed. */
> +	assert(sh->ctx->async_fd > 0);
> +	flags = fcntl(sh->ctx->async_fd, F_GETFL);
> +	ret = fcntl(sh->ctx->async_fd, F_SETFL, flags | O_NONBLOCK);
> +	if (ret) {
> +		DRV_LOG(INFO, "failed to change file descriptor"
> +			      " async event queue");
> +		/* Indicate there will be no interrupts. */
> +		dev->data->dev_conf.intr_conf.lsc = 0;
> +		dev->data->dev_conf.intr_conf.rmv = 0;
> +		sh->port[priv->ibv_port - 1].port_id = RTE_MAX_ETHPORTS;
> +		goto exit;
> +	}
> +	sh->intr_handle.fd = sh->ctx->async_fd;
> +	sh->intr_handle.type = RTE_INTR_HANDLE_EXT;
> +	rte_intr_callback_register(&sh->intr_handle,
> +				   mlx5_dev_interrupt_handler, sh);
> +	sh->intr_cnt++;
> +exit:
> +	pthread_mutex_unlock(&sh->intr_mutex);
> +}
> +
> +/**
>   * Uninstall interrupt handler.
>   *
>   * @param dev
> @@ -1119,15 +1209,10 @@ int mlx5_fw_version_get(struct rte_eth_dev
> *dev, char *fw_ver, size_t fw_size)  {
>  	struct mlx5_priv *priv = dev->data->dev_private;
> 
> -	if (dev->data->dev_conf.intr_conf.lsc ||
> -	    dev->data->dev_conf.intr_conf.rmv)
> -		rte_intr_callback_unregister(&priv->intr_handle,
> -					     mlx5_dev_interrupt_handler,
> dev);
> +	mlx5_dev_shared_handler_uninstall(dev);
>  	if (priv->primary_socket)
>  		rte_intr_callback_unregister(&priv->intr_handle_socket,
>  					     mlx5_dev_handler_socket, dev);
> -	priv->intr_handle.fd = 0;
> -	priv->intr_handle.type = RTE_INTR_HANDLE_UNKNOWN;
>  	priv->intr_handle_socket.fd = 0;
>  	priv->intr_handle_socket.type = RTE_INTR_HANDLE_UNKNOWN;  }
> @@ -1142,28 +1227,9 @@ int mlx5_fw_version_get(struct rte_eth_dev
> *dev, char *fw_ver, size_t fw_size)
> mlx5_dev_interrupt_handler_install(struct rte_eth_dev *dev)  {
>  	struct mlx5_priv *priv = dev->data->dev_private;
> -	struct ibv_context *ctx = priv->sh->ctx;
>  	int ret;
> -	int flags;
> 
> -	assert(ctx->async_fd > 0);
> -	flags = fcntl(ctx->async_fd, F_GETFL);
> -	ret = fcntl(ctx->async_fd, F_SETFL, flags | O_NONBLOCK);
> -	if (ret) {
> -		DRV_LOG(INFO,
> -			"port %u failed to change file descriptor async event"
> -			" queue",
> -			dev->data->port_id);
> -		dev->data->dev_conf.intr_conf.lsc = 0;
> -		dev->data->dev_conf.intr_conf.rmv = 0;
> -	}
> -	if (dev->data->dev_conf.intr_conf.lsc ||
> -	    dev->data->dev_conf.intr_conf.rmv) {
> -		priv->intr_handle.fd = ctx->async_fd;
> -		priv->intr_handle.type = RTE_INTR_HANDLE_EXT;
> -		rte_intr_callback_register(&priv->intr_handle,
> -					   mlx5_dev_interrupt_handler, dev);
> -	}
> +	mlx5_dev_shared_handler_install(dev);
>  	ret = mlx5_socket_init(dev);
>  	if (ret)
>  		DRV_LOG(ERR, "port %u cannot initialise socket: %s",
> --
> 1.8.3.1