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

[Shahaf Shuler <shahafs@mellanox.com>]

Thursday, March 21, 2019 4:02 PM, Slava Ovsiienko:
> To: Shahaf Shuler <shahafs@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>

[...]

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

OK, then you need to doc it as such ("per port context for interrupt"). 

> 
> >

[...]

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

OK, it will be much clear to have all the handler patches in a single patch. 

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