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

[Slava Ovsiienko <viacheslavo@mellanox.com>]

> -----Original Message-----
> From: Shahaf Shuler
> Sent: Thursday, March 21, 2019 14:15
> To: Slava Ovsiienko <viacheslavo@mellanox.com>; dev@dpdk.org
> Subject: RE: [PATCH 12/14] net/mlx5: update install/uninstall int handler
> routines
> 
> 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?

For the case if we would like to add some other per-port data
accessible only from shared context. For example - in interrupt
handler we have only one parameter - the shared context, and we
should deduce eth_dev for the some device (not DPDK port_id) port

Actually it is uint_32_t array for now, but it is easily extandable,
for example, we could add per-port context for interrupt
handler.
	
> 
> >  	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?

Array is needed to deduce the eth_dev from the device port number.
Here is interrupt handler flow:
- entry
- for()
 - get_event()
- get device port (note, this is IB port index, not DPDK port id) from event
- check in the array whether the handler is installed for this port 
  (array member is less than RTE_MAX_ETHPORTS)
-  get DPDK port_id from array()

Array member just indicates whether the handler for  given IB port is
installed. Reference counter is used for rte_intr_callback_register/
rte_intr_callback_unregister calls. 
rte_intr_callback_register() is called when the first handler for the port is
being installed.
rte_intr_callback_unregister() is called when the lastt handler for the port is
being gone away.

> 
> > +		/* 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.

Yes, it is a bug, there should no be "++" at all. Thanks. 

> 
> > +		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