[2/2] ethdev: make rte flow API thread safe

  Currently, the rte flow functions are not defined as thread safety.
DPDK applications either call the functions in single thread or add
locks around the functions for the critical section.

For PMDs support the flow operations thread safe natively, the
redundant protection in application hurts the performance of the
rte flow operation functions.

And the restriction of thread safety not guaranteed for the rte
flow functions also limits the applications' expectation.

This feature is going to change the rte flow functions to be thread
safety. As different PMDs have different flow operations, some may
support thread safety already and others may not. For PMDs don't
support flow thread safe operation, a new lock is defined in ethdev
in order to protects thread unsafe PMDs from rte flow level.

A new RTE_ETH_DEV_FLOW_OPS_THREAD_SAFE device flag is added to
determine whether the PMD supports thread safe flow operation or not.
For PMDs support thread safe flow operations, set the
RTE_ETH_DEV_FLOW_OPS_THREAD_SAFE flag, rte flow level functions will
skip the thread safe helper lock for these PMDs. Again the rte flow
level thread safe lock only works when PMD operation functions are
not thread safety.

For the PMDs which don't want the default mutex lock, just set the
flag in the PMD, and add the perfer type of lock in the PMD. Then
the default mutex lock is easily replaced by the PMD level lock.

The change has no effect on the current DPDK applications. No change
is required for the current DPDK applications. If no lock contention
with the added rte flow level mutex, the mutex only does the atomic
increasing in pthread_mutex_lock() and decreasing in
pthread_mutex_unlock(). No futex() syscall will be involved.

Signed-off-by: Suanming Mou <suanmingm@nvidia.com>
---
 doc/guides/prog_guide/rte_flow.rst  |  7 ++--
 drivers/net/mlx5/linux/mlx5_os.c    |  2 +
 lib/librte_ethdev/rte_ethdev.c      |  2 +
 lib/librte_ethdev/rte_ethdev.h      |  2 +
 lib/librte_ethdev/rte_ethdev_core.h |  4 ++
 lib/librte_ethdev/rte_flow.c        | 84 ++++++++++++++++++++++++++++---------
 6 files changed, 78 insertions(+), 23 deletions(-)
  

Hi Suanming,

Small comments,
PSB,

Best,
Ori

> -----Original Message-----
> From: Suanming Mou <suanmingm@nvidia.com>
> Sent: Sunday, September 27, 2020 11:20 AM
> Cc: dev@dpdk.org
> Subject: [PATCH 2/2] ethdev: make rte flow API thread safe
> 
> Currently, the rte flow functions are not defined as thread safety.

For a given port. (between ports they are thread safe)

> DPDK applications either call the functions in single thread or add
> locks around the functions for the critical section.
> 
> For PMDs support the flow operations thread safe natively, the
> redundant protection in application hurts the performance of the
> rte flow operation functions.
> 
> And the restriction of thread safety not guaranteed for the rte
> flow functions also limits the applications' expectation.
> 
> This feature is going to change the rte flow functions to be thread
> safety. As different PMDs have different flow operations, some may

Safety => safe I think this is true also to other places in this patch.

> support thread safety already and others may not. For PMDs don't
> support flow thread safe operation, a new lock is defined in ethdev
> in order to protects thread unsafe PMDs from rte flow level.
> 
> A new RTE_ETH_DEV_FLOW_OPS_THREAD_SAFE device flag is added to
> determine whether the PMD supports thread safe flow operation or not.
> For PMDs support thread safe flow operations, set the
> RTE_ETH_DEV_FLOW_OPS_THREAD_SAFE flag, rte flow level functions will
> skip the thread safe helper lock for these PMDs. Again the rte flow
> level thread safe lock only works when PMD operation functions are
> not thread safety.
safe
> 
> For the PMDs which don't want the default mutex lock, just set the
> flag in the PMD, and add the perfer type of lock in the PMD. Then
> the default mutex lock is easily replaced by the PMD level lock.
> 
> The change has no effect on the current DPDK applications. No change
> is required for the current DPDK applications. If no lock contention
> with the added rte flow level mutex, the mutex only does the atomic
> increasing in pthread_mutex_lock() and decreasing in
> pthread_mutex_unlock(). No futex() syscall will be involved.
> 
> Signed-off-by: Suanming Mou <suanmingm@nvidia.com>
> ---
>  doc/guides/prog_guide/rte_flow.rst  |  7 ++--
>  drivers/net/mlx5/linux/mlx5_os.c    |  2 +
>  lib/librte_ethdev/rte_ethdev.c      |  2 +
>  lib/librte_ethdev/rte_ethdev.h      |  2 +
>  lib/librte_ethdev/rte_ethdev_core.h |  4 ++
>  lib/librte_ethdev/rte_flow.c        | 84 ++++++++++++++++++++++++++++---------
>  6 files changed, 78 insertions(+), 23 deletions(-)
> 
> diff --git a/doc/guides/prog_guide/rte_flow.rst
> b/doc/guides/prog_guide/rte_flow.rst
> index 119b128..6f7997a 100644
> --- a/doc/guides/prog_guide/rte_flow.rst
> +++ b/doc/guides/prog_guide/rte_flow.rst
> @@ -3046,10 +3046,6 @@ Caveats
>  - API operations are synchronous and blocking (``EAGAIN`` cannot be
>    returned).
> 
> -- There is no provision for re-entrancy/multi-thread safety, although nothing
> -  should prevent different devices from being configured at the same
> -  time. PMDs may protect their control path functions accordingly.
> -
>  - Stopping the data path (TX/RX) should not be necessary when managing flow
>    rules. If this cannot be achieved naturally or with workarounds (such as
>    temporarily replacing the burst function pointers), an appropriate error
> @@ -3101,6 +3097,9 @@ This interface additionally defines the following
> helper function:
>  - ``rte_flow_ops_get()``: get generic flow operations structure from a
>    port.
> 
> +If PMD interfaces do not support re-entrancy/multi-thread safety, rte flow
> +level functions will do it by mutex.
> +

I think you should add which flag the app should test to see if the PMD
support thread safe.

>  More will be added over time.
> 
>  Device compatibility
> diff --git a/drivers/net/mlx5/linux/mlx5_os.c
> b/drivers/net/mlx5/linux/mlx5_os.c
> index 2a4beb0..daefb7d 100644
> --- a/drivers/net/mlx5/linux/mlx5_os.c
> +++ b/drivers/net/mlx5/linux/mlx5_os.c
> @@ -1351,6 +1351,8 @@
>  			goto error;
>  		}
>  	}
> +	if (priv->config.dv_flow_en)
> +		eth_dev->data->dev_flags =
> RTE_ETH_DEV_FLOW_OPS_THREAD_SAFE;
>  	return eth_dev;

I don't think this code is relevant for this patch.
I also think you are missing the | when assigning the value.

>  error:
>  	if (priv) {
> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index dfe5c1b..d6fd17e 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -500,6 +500,7 @@ struct rte_eth_dev *
>  	strlcpy(eth_dev->data->name, name, sizeof(eth_dev->data->name));
>  	eth_dev->data->port_id = port_id;
>  	eth_dev->data->mtu = RTE_ETHER_MTU;
> +	pthread_mutex_init(&eth_dev->data->fts_mutex, NULL);
> 
>  unlock:
>  	rte_spinlock_unlock(&rte_eth_dev_shared_data->ownership_lock);
> @@ -562,6 +563,7 @@ struct rte_eth_dev *
>  		rte_free(eth_dev->data->mac_addrs);
>  		rte_free(eth_dev->data->hash_mac_addrs);
>  		rte_free(eth_dev->data->dev_private);
> +		pthread_mutex_destroy(&eth_dev->data->fts_mutex);
>  		memset(eth_dev->data, 0, sizeof(struct rte_eth_dev_data));
>  	}
> 
> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> index 645a186..61bd09f 100644
> --- a/lib/librte_ethdev/rte_ethdev.h
> +++ b/lib/librte_ethdev/rte_ethdev.h
> @@ -1669,6 +1669,8 @@ struct rte_eth_dev_owner {
>  #define RTE_ETH_DEV_REPRESENTOR  0x0010
>  /** Device does not support MAC change after started */
>  #define RTE_ETH_DEV_NOLIVE_MAC_ADDR  0x0020
> +/** Device PMD supports thread safety flow operation */
> +#define RTE_ETH_DEV_FLOW_OPS_THREAD_SAFE  0x0040
> 
>  /**
>   * Iterates over valid ethdev ports owned by a specific owner.
> diff --git a/lib/librte_ethdev/rte_ethdev_core.h
> b/lib/librte_ethdev/rte_ethdev_core.h
> index fd3bf92..89df65a 100644
> --- a/lib/librte_ethdev/rte_ethdev_core.h
> +++ b/lib/librte_ethdev/rte_ethdev_core.h
> @@ -5,6 +5,9 @@
>  #ifndef _RTE_ETHDEV_CORE_H_
>  #define _RTE_ETHDEV_CORE_H_
> 
> +#include <pthread.h>
> +#include <sys/types.h>
> +
>  /**
>   * @file
>   *
> @@ -180,6 +183,7 @@ struct rte_eth_dev_data {
>  			 *   Valid if RTE_ETH_DEV_REPRESENTOR in dev_flags.
>  			 */
> 
> +	pthread_mutex_t fts_mutex; /**< rte flow ops thread safety mutex. */
>  	uint64_t reserved_64s[4]; /**< Reserved for future fields */
>  	void *reserved_ptrs[4];   /**< Reserved for future fields */
>  } __rte_cache_aligned;
> diff --git a/lib/librte_ethdev/rte_flow.c b/lib/librte_ethdev/rte_flow.c
> index f8fdd68..ca80b12 100644
> --- a/lib/librte_ethdev/rte_flow.c
> +++ b/lib/librte_ethdev/rte_flow.c
> @@ -207,6 +207,20 @@ struct rte_flow_desc_data {
>  	return -rte_errno;
>  }
> 
> +static void
> +flow_lock(struct rte_eth_dev *dev)
> +{
> +	if (!(dev->data->dev_flags &
> RTE_ETH_DEV_FLOW_OPS_THREAD_SAFE))
> +		pthread_mutex_lock(&dev->data->fts_mutex);
> +}
Why not inline function?
Also I don't think you should check the PMD support in this function
when this function is called it should lock.

> +
> +static void
> +flow_unlock(struct rte_eth_dev *dev)
> +{
> +	if (!(dev->data->dev_flags &
> RTE_ETH_DEV_FLOW_OPS_THREAD_SAFE))
> +		pthread_mutex_unlock(&dev->data->fts_mutex);
> +}

Same comments as above.

> +
>  static int
>  flow_err(uint16_t port_id, int ret, struct rte_flow_error *error)
>  {
> @@ -346,12 +360,16 @@ struct rte_flow_desc_data {
>  {
>  	const struct rte_flow_ops *ops = rte_flow_ops_get(port_id, error);
>  	struct rte_eth_dev *dev = &rte_eth_devices[port_id];
> +	int ret;
> 
>  	if (unlikely(!ops))
>  		return -rte_errno;
> -	if (likely(!!ops->validate))
> -		return flow_err(port_id, ops->validate(dev, attr, pattern,
> -						       actions, error), error);
> +	if (likely(!!ops->validate)) {
> +		flow_lock(dev);
> +		ret = ops->validate(dev, attr, pattern, actions, error);
> +		flow_unlock(dev);
> +		return flow_err(port_id, ret, error);
> +	}
>  	return rte_flow_error_set(error, ENOSYS,
>  				  RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
>  				  NULL, rte_strerror(ENOSYS));
> @@ -372,7 +390,9 @@ struct rte_flow *
>  	if (unlikely(!ops))
>  		return NULL;
>  	if (likely(!!ops->create)) {
> +		flow_lock(dev);
>  		flow = ops->create(dev, attr, pattern, actions, error);
> +		flow_unlock(dev);
>  		if (flow == NULL)
>  			flow_err(port_id, -rte_errno, error);
>  		return flow;
> @@ -390,12 +410,16 @@ struct rte_flow *
>  {
>  	struct rte_eth_dev *dev = &rte_eth_devices[port_id];
>  	const struct rte_flow_ops *ops = rte_flow_ops_get(port_id, error);
> +	int ret;
> 
>  	if (unlikely(!ops))
>  		return -rte_errno;
> -	if (likely(!!ops->destroy))
> -		return flow_err(port_id, ops->destroy(dev, flow, error),
> -				error);
> +	if (likely(!!ops->destroy)) {
> +		flow_lock(dev);
> +		ret = ops->destroy(dev, flow, error);
> +		flow_unlock(dev);
> +		return flow_err(port_id, ret, error);
> +	}
>  	return rte_flow_error_set(error, ENOSYS,
>  				  RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
>  				  NULL, rte_strerror(ENOSYS));
> @@ -408,11 +432,16 @@ struct rte_flow *
>  {
>  	struct rte_eth_dev *dev = &rte_eth_devices[port_id];
>  	const struct rte_flow_ops *ops = rte_flow_ops_get(port_id, error);
> +	int ret;
> 
>  	if (unlikely(!ops))
>  		return -rte_errno;
> -	if (likely(!!ops->flush))
> -		return flow_err(port_id, ops->flush(dev, error), error);
> +	if (likely(!!ops->flush)) {
> +		flow_lock(dev);
> +		ret = ops->flush(dev, error);
> +		flow_unlock(dev);
> +		return flow_err(port_id, ret, error);
> +	}
>  	return rte_flow_error_set(error, ENOSYS,
>  				  RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
>  				  NULL, rte_strerror(ENOSYS));
> @@ -428,12 +457,16 @@ struct rte_flow *
>  {
>  	struct rte_eth_dev *dev = &rte_eth_devices[port_id];
>  	const struct rte_flow_ops *ops = rte_flow_ops_get(port_id, error);
> +	int ret;
> 
>  	if (!ops)
>  		return -rte_errno;
> -	if (likely(!!ops->query))
> -		return flow_err(port_id, ops->query(dev, flow, action, data,
> -						    error), error);
> +	if (likely(!!ops->query)) {
> +		flow_lock(dev);
> +		ret = ops->query(dev, flow, action, data, error);
> +		flow_unlock(dev);
> +		return flow_err(port_id, ret, error);
> +	}
>  	return rte_flow_error_set(error, ENOSYS,
>  				  RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
>  				  NULL, rte_strerror(ENOSYS));
> @@ -447,11 +480,16 @@ struct rte_flow *
>  {
>  	struct rte_eth_dev *dev = &rte_eth_devices[port_id];
>  	const struct rte_flow_ops *ops = rte_flow_ops_get(port_id, error);
> +	int ret;
> 
>  	if (!ops)
>  		return -rte_errno;
> -	if (likely(!!ops->isolate))
> -		return flow_err(port_id, ops->isolate(dev, set, error), error);
> +	if (likely(!!ops->isolate)) {
> +		flow_lock(dev);
> +		ret = ops->isolate(dev, set, error);
> +		flow_unlock(dev);
> +		return flow_err(port_id, ret, error);
> +	}
>  	return rte_flow_error_set(error, ENOSYS,
>  				  RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
>  				  NULL, rte_strerror(ENOSYS));
> @@ -1224,12 +1262,16 @@ enum rte_flow_conv_item_spec_type {
>  {
>  	struct rte_eth_dev *dev = &rte_eth_devices[port_id];
>  	const struct rte_flow_ops *ops = rte_flow_ops_get(port_id, error);
> +	int ret;
> 
>  	if (unlikely(!ops))
>  		return -rte_errno;
> -	if (likely(!!ops->dev_dump))
> -		return flow_err(port_id, ops->dev_dump(dev, file, error),
> -				error);
> +	if (likely(!!ops->dev_dump)) {
> +		flow_lock(dev);
> +		ret = ops->dev_dump(dev, file, error);
> +		flow_unlock(dev);
> +		return flow_err(port_id, ret, error);
> +	}
>  	return rte_flow_error_set(error, ENOSYS,
>  				  RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
>  				  NULL, rte_strerror(ENOSYS));
> @@ -1241,12 +1283,16 @@ enum rte_flow_conv_item_spec_type {
>  {
>  	struct rte_eth_dev *dev = &rte_eth_devices[port_id];
>  	const struct rte_flow_ops *ops = rte_flow_ops_get(port_id, error);
> +	int ret;
> 
>  	if (unlikely(!ops))
>  		return -rte_errno;
> -	if (likely(!!ops->get_aged_flows))
> -		return flow_err(port_id, ops->get_aged_flows(dev, contexts,
> -				nb_contexts, error), error);
> +	if (likely(!!ops->get_aged_flows)) {
> +		flow_lock(dev);
> +		ret = ops->get_aged_flows(dev, contexts, nb_contexts, error);
> +		flow_unlock(dev);
> +		return flow_err(port_id, ret, error);
> +	}
>  	return rte_flow_error_set(error, ENOTSUP,
>  				  RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
>  				  NULL, rte_strerror(ENOTSUP));
> --
> 1.8.3.1
  

Hi Ori,

Thanks for the comments.
Will update the next V2 version.

BR,
SuanmingMou

> -----Original Message-----
> From: Ori Kam <orika@nvidia.com>
> Sent: Wednesday, September 30, 2020 6:56 PM
> To: Suanming Mou <suanmingm@nvidia.com>; Ori Kam
> <orika@mellanox.com>; John McNamara <john.mcnamara@intel.com>;
> Marko Kovacevic <marko.kovacevic@intel.com>; Matan Azrad
> <matan@mellanox.com>; Shahaf Shuler <shahafs@mellanox.com>;
> Viacheslav Ovsiienko <viacheslavo@mellanox.com>; NBU-Contact-Thomas
> Monjalon <thomas@monjalon.net>; Ferruh Yigit <ferruh.yigit@intel.com>;
> Andrew Rybchenko <arybchenko@solarflare.com>
> Cc: dev@dpdk.org
> Subject: RE: [PATCH 2/2] ethdev: make rte flow API thread safe
> 
> Hi Suanming,
> 
> Small comments,
> PSB,
> 
> Best,
> Ori
>