Re: [dpdk-dev] [PATCH 4/4] net/mlx5: add mutex for shared DV/DR structures

[Shahaf Shuler <shahafs@mellanox.com>]

Tuesday, April 2, 2019 9:23 AM, Viacheslav Ovsiienko:
> Subject: [PATCH 4/4] net/mlx5: add mutex for shared DV/DR structures

Same comment about title. 

> 
> This patch introduces the mutex for shared DV/DR structures.
> Application may have multiple threads (but single dedicated thread per port).
> Due to sharing the IB context in the multiport IB device configurations we
> should synchronize access to the shared DV/DR flow structures.
> 
> Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
> ---
>  drivers/net/mlx5/mlx5.c         |   6 +++
>  drivers/net/mlx5/mlx5.h         |   1 +
>  drivers/net/mlx5/mlx5_flow_dv.c | 103
> ++++++++++++++++++++++++++++++++++++++--
>  3 files changed, 106 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c index
> 369b698..96ad4c6 100644
> --- a/drivers/net/mlx5/mlx5.c
> +++ b/drivers/net/mlx5/mlx5.c
> @@ -312,6 +312,7 @@ struct mlx5_dev_spawn_data {
> mlx5_alloc_shared_dv(struct mlx5_priv *priv)  {
>  	struct mlx5_ibv_shared *sh = priv->sh;
> +	pthread_mutexattr_t mattr;
>  	int err = 0;
>  	void *ns;
> 
> @@ -336,6 +337,10 @@ struct mlx5_dev_spawn_data {
>  		err = errno;
>  		goto error;
>  	}
> +	pthread_mutexattr_init(&mattr);
> +	pthread_mutexattr_settype(&mattr,
> PTHREAD_MUTEX_RECURSIVE);

Please add inline comment about why you use this type of mutex (so that you can use a single mutex for the entire flow engine logic). 

> +	pthread_mutex_init(&sh->dv_mutex, &mattr);
> +	pthread_mutexattr_destroy(&mattr);
>  	sh->tx_ns = ns;
>  	sh->dv_refcnt++;
>  	priv->dv_shared = 1;
> @@ -381,6 +386,7 @@ struct mlx5_dev_spawn_data {
>  		mlx5dv_dr_destroy_ns(sh->tx_ns);
>  		sh->tx_ns = NULL;
>  	}
> +	pthread_mutex_destroy(&sh->dv_mutex);
>  }
>  #endif
> 
> diff --git a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h index
> e67227f..4f6c1b7 100644
> --- a/drivers/net/mlx5/mlx5.h
> +++ b/drivers/net/mlx5/mlx5.h
> @@ -223,6 +223,7 @@ struct mlx5_ibv_shared {
>  	char ibdev_path[IBV_SYSFS_PATH_MAX]; /* IB device path for
> secondary */
>  	struct ibv_device_attr_ex device_attr; /* Device properties. */
>  	/* Shared DV/DR flow data section. */
> +	pthread_mutex_t dv_mutex; /* DV context mutex. */
>  	uint32_t dv_refcnt; /* DV/DR data reference counter. */
>  	void *rx_ns; /* RX Direct Rules name space handle. */
>  	struct mlx5_flow_tbl_resource rx_tbl[MLX5_MAX_TABLES]; diff --git
> a/drivers/net/mlx5/mlx5_flow_dv.c b/drivers/net/mlx5/mlx5_flow_dv.c
> index 4912fc8..737011a 100644
> --- a/drivers/net/mlx5/mlx5_flow_dv.c
> +++ b/drivers/net/mlx5/mlx5_flow_dv.c
> @@ -125,6 +125,45 @@ struct field_modify_info modify_tcp[] = {  };
> 
>  /**
> + * Acquire the synchronizing object to protect multithreaded access
> + * to shared dv context. Lock occurs only if context is actually
> + * shared, i.e. we have multiport IB device and representors are
> + * created.
> + *
> + * @param[in] dev
> + *   Pointer to the rte_eth_dev structure.
> + */
> +static void
> +flow_d_shared_lock(struct rte_eth_dev *dev) {
> +	struct mlx5_priv *priv = dev->data->dev_private;
> +	struct mlx5_ibv_shared *sh = priv->sh;
> +
> +	if (sh->dv_refcnt > 1) {
> +		int ret;
> +
> +		ret = pthread_mutex_lock(&sh->dv_mutex);
> +		assert(!ret);
> +		(void)ret;
> +	}
> +}
> +
> +static void
> +flow_d_shared_unlock(struct rte_eth_dev *dev) {
> +	struct mlx5_priv *priv = dev->data->dev_private;
> +	struct mlx5_ibv_shared *sh = priv->sh;
> +
> +	if (sh->dv_refcnt > 1) {
> +		int ret;
> +
> +		ret = pthread_mutex_unlock(&sh->dv_mutex);
> +		assert(!ret);
> +		(void)ret;
> +	}
> +}
> +
> +/**
>   * Convert modify-header action to DV specification.
>   *
>   * @param[in] item
> @@ -3958,14 +3997,70 @@ struct field_modify_info modify_tcp[] = {
>  	return ret;
>  }
> 
> +/*
> + * Mutex-protected thunk to flow_dv_translate().
> + */
> +static int
> +flow_d_translate(struct rte_eth_dev *dev,
> +		 struct mlx5_flow *dev_flow,
> +		 const struct rte_flow_attr *attr,
> +		 const struct rte_flow_item items[],
> +		 const struct rte_flow_action actions[],
> +		 struct rte_flow_error *error)
> +{
> +	int ret;
> +
> +	flow_d_shared_lock(dev);
> +	ret = flow_dv_translate(dev, dev_flow, attr, items, actions, error);
> +	flow_d_shared_unlock(dev);
> +	return ret;
> +}
> +
> +/*
> + * Mutex-protected thunk to flow_dv_apply().
> + */
> +static int
> +flow_d_apply(struct rte_eth_dev *dev,
> +	     struct rte_flow *flow,
> +	     struct rte_flow_error *error)
> +{
> +	int ret;
> +
> +	flow_d_shared_lock(dev);
> +	ret = flow_dv_apply(dev, flow, error);
> +	flow_d_shared_unlock(dev);
> +	return ret;
> +}
> +
> +/*
> + * Mutex-protected thunk to flow_dv_remove().
> + */
> +static void
> +flow_d_remove(struct rte_eth_dev *dev, struct rte_flow *flow) {
> +	flow_d_shared_lock(dev);
> +	flow_dv_remove(dev, flow);
> +	flow_d_shared_unlock(dev);
> +}
> +
> +/*
> + * Mutex-protected thunk to flow_dv_destroy().
> + */
> +static void
> +flow_d_destroy(struct rte_eth_dev *dev, struct rte_flow *flow) {
> +	flow_d_shared_lock(dev);
> +	flow_dv_destroy(dev, flow);
> +	flow_d_shared_unlock(dev);
> +}
> 
>  const struct mlx5_flow_driver_ops mlx5_flow_dv_drv_ops = {
>  	.validate = flow_dv_validate,
>  	.prepare = flow_dv_prepare,
> -	.translate = flow_dv_translate,
> -	.apply = flow_dv_apply,
> -	.remove = flow_dv_remove,
> -	.destroy = flow_dv_destroy,
> +	.translate = flow_d_translate,
> +	.apply = flow_d_apply,
> +	.remove = flow_d_remove,
> +	.destroy = flow_d_destroy,
>  	.query = flow_dv_query,
>  };
> 
> --
> 1.8.3.1