[12/14] net/mlx5: update install/uninstall int handler routines
Checks
Commit Message
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(-)
Comments
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
> -----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
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
@@ -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;
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.
*/
@@ -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. */
@@ -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) {
+ /* The handler is already installed for this port. */
+ assert(sh->intr_cnt++);
+ 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",