> -----Original Message-----
> From: Shahaf Shuler
> Sent: Thursday, March 21, 2019 14:15
> To: Slava Ovsiienko <viachesl...@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>
> > ---
> > 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
