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