[dpdk-dev] [PATCH 0/2] ethdev: make rte flow API thread safe

[dpdk-dev] [PATCH 0/2] ethdev: make rte flow API thread safe

[Suanming Mou]

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.

Suanming Mou (2):
  eal/windows: add pthread mutex lock
  ethdev: make rte flow API thread safe

 doc/guides/prog_guide/rte_flow.rst       |  7 ++-
 drivers/net/mlx5/linux/mlx5_os.c         |  2 +
 lib/librte_eal/windows/include/pthread.h | 46 +++++++++++++++++
 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 ++++++++++++++++++++++++--------
 7 files changed, 124 insertions(+), 23 deletions(-)

-- 
1.8.3.1

[dpdk-dev] [PATCH 1/2] eal/windows: add pthread mutex lock

[Suanming Mou]

Add pthread mutex lock as it is needed for the thread safe rte flow
functions.

Signed-off-by: Suanming Mou <suanmingm@nvidia.com>
---
 lib/librte_eal/windows/include/pthread.h | 46 ++++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/lib/librte_eal/windows/include/pthread.h b/lib/librte_eal/windows/include/pthread.h
index 99013dc..4e2e0b3 100644
--- a/lib/librte_eal/windows/include/pthread.h
+++ b/lib/librte_eal/windows/include/pthread.h
@@ -28,6 +28,10 @@
 /* defining pthread_attr_t type on Windows since there is no in Microsoft libc*/
 typedef void *pthread_attr_t;
 
+typedef void *pthread_mutexattr_t;
+
+typedef HANDLE pthread_mutex_t;
+
 typedef SYNCHRONIZATION_BARRIER pthread_barrier_t;
 
 #define pthread_barrier_init(barrier, attr, count) \
@@ -139,6 +143,48 @@
 	return 0;
 }
 
+static inline int
+pthread_mutex_init(pthread_mutex_t *mutex,
+		   __rte_unused pthread_mutexattr_t *attr)
+{
+	*mutex = CreateMutex(NULL, FALSE, NULL);
+	if (*mutex == NULL) {
+		RTE_LOG_WIN32_ERR("CreateMutex()");
+		return -1;
+	}
+	return 0;
+}
+
+static inline int
+pthread_mutex_lock(pthread_mutex_t *mutex)
+{
+	if (WaitForSingleObject(*mutex, INFINITE) != WAIT_OBJECT_0) {
+		RTE_LOG_WIN32_ERR("WaitForSingleObject()");
+		return -1;
+	}
+	return 0;
+}
+
+static inline int
+pthread_mutex_unlock(pthread_mutex_t *mutex)
+{
+	if (!ReleaseMutex(*mutex)) {
+		RTE_LOG_WIN32_ERR("ReleaseMutex()");
+		return -1;
+	}
+	return 0;
+}
+
+static inline int
+pthread_mutex_destroy(pthread_mutex_t *mutex)
+{
+	if (!CloseHandle(*mutex)) {
+		RTE_LOG_WIN32_ERR("CloseHandle()");
+		return -1;
+	}
+	return 0;
+}
+
 #ifdef __cplusplus
 }
 #endif
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH 1/2] eal/windows: add pthread mutex lock

[Dmitry Kozlyuk]

Hi Suanming,

There's a remark in patch 2/2 and cover letter:

> 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.

Is this property important? To get the described behavior on Windows, you
should've used CRITICAL_SECTION (preferably wrapped in a struct). Mutexes are
kernel objects on Windows and always require syscalls. Otherwise, if mutexes
are sufficient, see a comment inline.

> Add pthread mutex lock as it is needed for the thread safe rte flow
> functions.
> 
> Signed-off-by: Suanming Mou <suanmingm@nvidia.com>
> ---
>  lib/librte_eal/windows/include/pthread.h | 46 ++++++++++++++++++++++++++++++++
>  1 file changed, 46 insertions(+)
> 
> diff --git a/lib/librte_eal/windows/include/pthread.h b/lib/librte_eal/windows/include/pthread.h
> index 99013dc..4e2e0b3 100644
> --- a/lib/librte_eal/windows/include/pthread.h
> +++ b/lib/librte_eal/windows/include/pthread.h
> @@ -28,6 +28,10 @@
>  /* defining pthread_attr_t type on Windows since there is no in Microsoft libc*/
>  typedef void *pthread_attr_t;
>  
> +typedef void *pthread_mutexattr_t;
> +
> +typedef HANDLE pthread_mutex_t;
> +
>  typedef SYNCHRONIZATION_BARRIER pthread_barrier_t;
>  
>  #define pthread_barrier_init(barrier, attr, count) \
> @@ -139,6 +143,48 @@
>  	return 0;
>  }
>  
> +static inline int
> +pthread_mutex_init(pthread_mutex_t *mutex,
> +		   __rte_unused pthread_mutexattr_t *attr)
> +{
> +	*mutex = CreateMutex(NULL, FALSE, NULL);
> +	if (*mutex == NULL) {
> +		RTE_LOG_WIN32_ERR("CreateMutex()");
> +		return -1;
> +	}
> +	return 0;
> +}
> +
> +static inline int
> +pthread_mutex_lock(pthread_mutex_t *mutex)
> +{
> +	if (WaitForSingleObject(*mutex, INFINITE) != WAIT_OBJECT_0) {
> +		RTE_LOG_WIN32_ERR("WaitForSingleObject()");
> +		return -1;

A relevant error code must be returned according to POSIX. Searching the
code for pthread_mutex_lock() calls, I can see that hinic PMD checks for
EOWNERDEAD (corresponding to WAIT_OBJECT_ABANDONED in Windows) and failsafe
PMD supplies return value of pthread_mutex_unlock() to strerror(), i.e. it
should be an errno. Same applies to other functions.

> +	}
> +	return 0;
> +}
> +
> +static inline int
> +pthread_mutex_unlock(pthread_mutex_t *mutex)
> +{
> +	if (!ReleaseMutex(*mutex)) {
> +		RTE_LOG_WIN32_ERR("ReleaseMutex()");
> +		return -1;
> +	}
> +	return 0;
> +}
> +
> +static inline int
> +pthread_mutex_destroy(pthread_mutex_t *mutex)
> +{
> +	if (!CloseHandle(*mutex)) {
> +		RTE_LOG_WIN32_ERR("CloseHandle()");
> +		return -1;
> +	}
> +	return 0;
> +}
> +
>  #ifdef __cplusplus
>  }
>  #endif

Re: [dpdk-dev] [PATCH 1/2] eal/windows: add pthread mutex lock

[Suanming Mou]

Hi Dmitry,

> -----Original Message-----
> From: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
> Sent: Sunday, September 27, 2020 11:57 PM
> To: Suanming Mou <suanmingm@nvidia.com>
> Cc: Narcisa Ana Maria Vasile <navasile@linux.microsoft.com>; Dmitry Malloy
> <dmitrym@microsoft.com>; Pallavi Kadam <pallavi.kadam@intel.com>;
> dev@dpdk.org
> Subject: Re: [PATCH 1/2] eal/windows: add pthread mutex lock
> 
> Hi Suanming,
> 
> There's a remark in patch 2/2 and cover letter:
> 
> > 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.
> 
> Is this property important? To get the described behavior on Windows, you
> should've used CRITICAL_SECTION (preferably wrapped in a struct). Mutexes are
> kernel objects on Windows and always require syscalls. Otherwise, if mutexes
> are sufficient, see a comment inline.

The description was valid only for the standard posix pthread functions. Good to know that there are similar functions on Windows.
I will prefer to change it to CRICTIAL_SECTION functions, in this case the pthread wrap functions on Windows will also have less impact with the current applications.
Thank you very much for the information.

> 
> > Add pthread mutex lock as it is needed for the thread safe rte flow
> > functions.
> >
> > Signed-off-by: Suanming Mou <suanmingm@nvidia.com>
> > ---
> >  lib/librte_eal/windows/include/pthread.h | 46
> > ++++++++++++++++++++++++++++++++
> >  1 file changed, 46 insertions(+)
> >
> > diff --git a/lib/librte_eal/windows/include/pthread.h
> > b/lib/librte_eal/windows/include/pthread.h
> > index 99013dc..4e2e0b3 100644
> > --- a/lib/librte_eal/windows/include/pthread.h
> > +++ b/lib/librte_eal/windows/include/pthread.h
> > @@ -28,6 +28,10 @@
> >  /* defining pthread_attr_t type on Windows since there is no in
> > Microsoft libc*/  typedef void *pthread_attr_t;
> >
> > +typedef void *pthread_mutexattr_t;
> > +
> > +typedef HANDLE pthread_mutex_t;
> > +
> >  typedef SYNCHRONIZATION_BARRIER pthread_barrier_t;
> >
> >  #define pthread_barrier_init(barrier, attr, count) \ @@ -139,6
> > +143,48 @@
> >  	return 0;
> >  }
> >
> > +static inline int
> > +pthread_mutex_init(pthread_mutex_t *mutex,
> > +		   __rte_unused pthread_mutexattr_t *attr) {
> > +	*mutex = CreateMutex(NULL, FALSE, NULL);
> > +	if (*mutex == NULL) {
> > +		RTE_LOG_WIN32_ERR("CreateMutex()");
> > +		return -1;
> > +	}
> > +	return 0;
> > +}
> > +
> > +static inline int
> > +pthread_mutex_lock(pthread_mutex_t *mutex) {
> > +	if (WaitForSingleObject(*mutex, INFINITE) != WAIT_OBJECT_0) {
> > +		RTE_LOG_WIN32_ERR("WaitForSingleObject()");
> > +		return -1;
> 
> A relevant error code must be returned according to POSIX. Searching the code
> for pthread_mutex_lock() calls, I can see that hinic PMD checks for
> EOWNERDEAD (corresponding to WAIT_OBJECT_ABANDONED in Windows) and
> failsafe PMD supplies return value of pthread_mutex_unlock() to strerror(), i.e. it
> should be an errno. Same applies to other functions.

These PMDs should not be valid on Windows now, or the build will be failed as no pthread_mutex on Windows.
I guess we will have a much general solution with the posix APIs support on Windows?
Now the wrap functions solution is much like a WA to fix the build.

> 
> > +	}
> > +	return 0;
> > +}
> > +
> > +static inline int
> > +pthread_mutex_unlock(pthread_mutex_t *mutex) {
> > +	if (!ReleaseMutex(*mutex)) {
> > +		RTE_LOG_WIN32_ERR("ReleaseMutex()");
> > +		return -1;
> > +	}
> > +	return 0;
> > +}
> > +
> > +static inline int
> > +pthread_mutex_destroy(pthread_mutex_t *mutex) {
> > +	if (!CloseHandle(*mutex)) {
> > +		RTE_LOG_WIN32_ERR("CloseHandle()");
> > +		return -1;
> > +	}
> > +	return 0;
> > +}
> > +
> >  #ifdef __cplusplus
> >  }
> >  #endif

[dpdk-dev] [PATCH 2/2] ethdev: make rte flow API thread safe

[Suanming Mou]

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(-)

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.
+
 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;
 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);
+}
+
+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);
+}
+
 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

Re: [dpdk-dev] [PATCH 2/2] ethdev: make rte flow API thread safe

[Ori Kam]

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

Re: [dpdk-dev] [PATCH 2/2] ethdev: make rte flow API thread safe

[Suanming Mou]

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
> 

[dpdk-dev] [PATCH v2 0/2] ethdev: make rte_flow API thread safe

[Suanming Mou]

Currently, the rte_flow functions are not defined as 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 safe 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
safe. As different PMDs have different flow operations, some may
support thread safe 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 safe.

For the PMDs which don't want the default mutex lock, just set the
flag in the PMD, and add the prefer 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. For the standard posix
pthread_mutex, 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.

Suanming Mou (2):
  eal/windows: add pthread mutex lock
  ethdev: make rte_flow API thread safe

---

v2:
 - Using critical section for windows pthread mutex.
 - Update ethdev commnets.

---

 doc/guides/prog_guide/rte_flow.rst       |  9 ++--
 lib/librte_eal/windows/include/pthread.h | 33 +++++++++++++
 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, 111 insertions(+), 23 deletions(-)

-- 
1.8.3.1

[dpdk-dev] [PATCH v2 1/2] eal/windows: add pthread mutex lock

[Suanming Mou]

Add pthread mutex lock as it is needed for the thread safe rte_flow
functions.

Signed-off-by: Suanming Mou <suanmingm@nvidia.com>
---

v2:
 - Using critical section for windows pthread mutex.

---
 lib/librte_eal/windows/include/pthread.h | 33 ++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/lib/librte_eal/windows/include/pthread.h b/lib/librte_eal/windows/include/pthread.h
index 99013dc..644fd49 100644
--- a/lib/librte_eal/windows/include/pthread.h
+++ b/lib/librte_eal/windows/include/pthread.h
@@ -28,6 +28,10 @@
 /* defining pthread_attr_t type on Windows since there is no in Microsoft libc*/
 typedef void *pthread_attr_t;
 
+typedef void *pthread_mutexattr_t;
+
+typedef CRITICAL_SECTION pthread_mutex_t;
+
 typedef SYNCHRONIZATION_BARRIER pthread_barrier_t;
 
 #define pthread_barrier_init(barrier, attr, count) \
@@ -139,6 +143,35 @@
 	return 0;
 }
 
+static inline int
+pthread_mutex_init(pthread_mutex_t *mutex,
+		   __rte_unused pthread_mutexattr_t *attr)
+{
+	InitializeCriticalSection(mutex);
+	return 0;
+}
+
+static inline int
+pthread_mutex_lock(pthread_mutex_t *mutex)
+{
+	EnterCriticalSection(mutex);
+	return 0;
+}
+
+static inline int
+pthread_mutex_unlock(pthread_mutex_t *mutex)
+{
+	LeaveCriticalSection(mutex);
+	return 0;
+}
+
+static inline int
+pthread_mutex_destroy(pthread_mutex_t *mutex)
+{
+	DeleteCriticalSection(mutex);
+	return 0;
+}
+
 #ifdef __cplusplus
 }
 #endif
-- 
1.8.3.1

[dpdk-dev] [PATCH v2 2/2] ethdev: make rte_flow API thread safe

[Suanming Mou]

Currently, the rte_flow functions are not defined as 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 safe 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
safe. As different PMDs have different flow operations, some may
support thread safe 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 safe.

For the PMDs which don't want the default mutex lock, just set the
flag in the PMD, and add the prefer 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. For the standard posix
pthread_mutex, 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>
---

v2:
 - Update commit info and description doc.
 - Add inline for the flow lock and unlock functions.
 - Remove the PMD sample part flag configuration.

---
 doc/guides/prog_guide/rte_flow.rst  |  9 ++--
 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 ++++++++++++++++++++++++++++---------
 5 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..ae2ddb3 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,11 @@ 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. The application can test the dev_flags
+with RTE_ETH_DEV_FLOW_OPS_THREAD_SAFE in struct rte_eth_dev_data to know
+if the rte_flow thread-safe works under rte_flow level or PMD level.
+
 More will be added over time.
 
 Device compatibility
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..2ac966d 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 inline 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);
+}
+
+static inline 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);
+}
+
 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

Re: [dpdk-dev] [PATCH v2 2/2] ethdev: make rte_flow API thread safe

[Ori Kam]

Hi Suanming,

PSB,
Best,
Ori

> -----Original Message-----
> From: Suanming Mou <suanmingm@nvidia.com>
> Subject: [PATCH v2 2/2] ethdev: make rte_flow API thread safe
> 
> Currently, the rte_flow functions are not defined as thread safe.
> DPDK applications either call the functions in single thread or add
> locks around the functions for the critical section.
> 

[Snip ...]

> diff --git a/lib/librte_ethdev/rte_flow.c b/lib/librte_ethdev/rte_flow.c
> index f8fdd68..2ac966d 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 inline void
> +flow_lock(struct rte_eth_dev *dev)

Maybe change the name to flow_safe_enter
Since this function doesn't always lock.
 
> +{
> +	if (!(dev->data->dev_flags &
> RTE_ETH_DEV_FLOW_OPS_THREAD_SAFE))
> +		pthread_mutex_lock(&dev->data->fts_mutex);
> +}
> +
> +static inline void
> +flow_unlock(struct rte_eth_dev *dev)

Maybe change the name flow_safe_leave
Same reason as above.

> +{
> +	if (!(dev->data->dev_flags &
> RTE_ETH_DEV_FLOW_OPS_THREAD_SAFE))
> +		pthread_mutex_unlock(&dev->data->fts_mutex);
> +}
> +
>  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

Re: [dpdk-dev] [PATCH v2 2/2] ethdev: make rte_flow API thread safe

[Ajit Khaparde]

> > +static inline void
> > +flow_lock(struct rte_eth_dev *dev)
>
> Maybe change the name to flow_safe_enter
> Since this function doesn't always lock.
I feel fts_enter() sounds better.

>
> > +{
> > +     if (!(dev->data->dev_flags &
> > RTE_ETH_DEV_FLOW_OPS_THREAD_SAFE))
> > +             pthread_mutex_lock(&dev->data->fts_mutex);
> > +}
> > +
> > +static inline void
> > +flow_unlock(struct rte_eth_dev *dev)
>
> Maybe change the name flow_safe_leave
> Same reason as above.
On the same lines.. fts_exit()

Re: [dpdk-dev] [PATCH v2 2/2] ethdev: make rte_flow API thread safe

[Suanming Mou]

> -----Original Message-----
> From: Ajit Khaparde <ajit.khaparde@broadcom.com>
> Sent: Wednesday, October 7, 2020 7:18 AM
> To: Ori Kam <orika@nvidia.com>
> Cc: Suanming Mou <suanmingm@nvidia.com>; Ori Kam
> <orika@mellanox.com>; John McNamara <john.mcnamara@intel.com>;
> Marko Kovacevic <marko.kovacevic@intel.com>; NBU-Contact-Thomas
> Monjalon <thomas@monjalon.net>; Ferruh Yigit <ferruh.yigit@intel.com>;
> Andrew Rybchenko <arybchenko@solarflare.com>; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2 2/2] ethdev: make rte_flow API thread
> safe
> 
> > > +static inline void
> > > +flow_lock(struct rte_eth_dev *dev)
> >
> > Maybe change the name to flow_safe_enter
> > Since this function doesn't always lock.
> I feel fts_enter() sounds better.

I would prefer to take that one if no other better names.  : )

> 
> >
> > > +{
> > > +     if (!(dev->data->dev_flags &
> > > RTE_ETH_DEV_FLOW_OPS_THREAD_SAFE))
> > > +             pthread_mutex_lock(&dev->data->fts_mutex);
> > > +}
> > > +
> > > +static inline void
> > > +flow_unlock(struct rte_eth_dev *dev)
> >
> > Maybe change the name flow_safe_leave
> > Same reason as above.
> On the same lines.. fts_exit()

Re: [dpdk-dev] [PATCH v2 2/2] ethdev: make rte_flow API thread safe

[Ori Kam]

Hi Suanming,

> -----Original Message-----
> From: Suanming Mou <suanmingm@nvidia.com>
> Sent: Wednesday, October 7, 2020 3:50 AM
> Subject: RE: [dpdk-dev] [PATCH v2 2/2] ethdev: make rte_flow API thread safe
> 
> 
> 
> > -----Original Message-----
> > From: Ajit Khaparde <ajit.khaparde@broadcom.com>
> > Sent: Wednesday, October 7, 2020 7:18 AM
> > Subject: Re: [dpdk-dev] [PATCH v2 2/2] ethdev: make rte_flow API thread
> > safe
> >
> > > > +static inline void
> > > > +flow_lock(struct rte_eth_dev *dev)
> > >
> > > Maybe change the name to flow_safe_enter
> > > Since this function doesn't always lock.
> > I feel fts_enter() sounds better.
> 
> I would prefer to take that one if no other better names.  : )
> 

Good suggestion.

> >
> > >
> > > > +{
> > > > +     if (!(dev->data->dev_flags &
> > > > RTE_ETH_DEV_FLOW_OPS_THREAD_SAFE))
> > > > +             pthread_mutex_lock(&dev->data->fts_mutex);
> > > > +}
> > > > +
> > > > +static inline void
> > > > +flow_unlock(struct rte_eth_dev *dev)
> > >
> > > Maybe change the name flow_safe_leave
> > > Same reason as above.
> > On the same lines.. fts_exit()

[dpdk-dev] [PATCH v3 0/2] ethdev: make rte_flow API thread safe

[Suanming Mou]

Currently, the rte_flow functions are not defined as 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 safe 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
safe. As different PMDs have different flow operations, some may
support thread safe 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 safe.

For the PMDs which don't want the default mutex lock, just set the
flag in the PMD, and add the prefer 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. For the standard posix
pthread_mutex, 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.

Suanming Mou (2):
  eal/windows: add pthread mutex lock
  ethdev: make rte_flow API thread safe

---

v3:
 - update flow_lock/unlock -> fts_enter/exit

v2:
 - Using critical section for windows pthread mutex.
 - Update ethdev commnets.

---

 doc/guides/prog_guide/rte_flow.rst       |  9 ++--
 lib/librte_eal/windows/include/pthread.h | 33 +++++++++++++
 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, 111 insertions(+), 23 deletions(-)

-- 
1.8.3.1

[dpdk-dev] [PATCH v3 1/2] eal/windows: add pthread mutex lock

[Suanming Mou]

Add pthread mutex lock as it is needed for the thread safe rte_flow
functions.

Signed-off-by: Suanming Mou <suanmingm@nvidia.com>
---

v3:
 - No updates.

v2:
 - Using critical section for windows pthread mutex.

---

 lib/librte_eal/windows/include/pthread.h | 33 ++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/lib/librte_eal/windows/include/pthread.h b/lib/librte_eal/windows/include/pthread.h
index 99013dc..644fd49 100644
--- a/lib/librte_eal/windows/include/pthread.h
+++ b/lib/librte_eal/windows/include/pthread.h
@@ -28,6 +28,10 @@
 /* defining pthread_attr_t type on Windows since there is no in Microsoft libc*/
 typedef void *pthread_attr_t;
 
+typedef void *pthread_mutexattr_t;
+
+typedef CRITICAL_SECTION pthread_mutex_t;
+
 typedef SYNCHRONIZATION_BARRIER pthread_barrier_t;
 
 #define pthread_barrier_init(barrier, attr, count) \
@@ -139,6 +143,35 @@
 	return 0;
 }
 
+static inline int
+pthread_mutex_init(pthread_mutex_t *mutex,
+		   __rte_unused pthread_mutexattr_t *attr)
+{
+	InitializeCriticalSection(mutex);
+	return 0;
+}
+
+static inline int
+pthread_mutex_lock(pthread_mutex_t *mutex)
+{
+	EnterCriticalSection(mutex);
+	return 0;
+}
+
+static inline int
+pthread_mutex_unlock(pthread_mutex_t *mutex)
+{
+	LeaveCriticalSection(mutex);
+	return 0;
+}
+
+static inline int
+pthread_mutex_destroy(pthread_mutex_t *mutex)
+{
+	DeleteCriticalSection(mutex);
+	return 0;
+}
+
 #ifdef __cplusplus
 }
 #endif
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH v3 1/2] eal/windows: add pthread mutex lock

[Dmitry Kozlyuk]

On Wed,  7 Oct 2020 22:17:28 +0800, Suanming Mou wrote:
> Add pthread mutex lock as it is needed for the thread safe rte_flow
> functions.
> 
> Signed-off-by: Suanming Mou <suanmingm@nvidia.com>

Acked-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>

Re: [dpdk-dev] [PATCH v3 1/2] eal/windows: add pthread mutex lock

[Suanming Mou]

Hi Dmitry,

Thank you very much.
I also got some messages that they wish the PTHREAD_MUTEX_INITIALIZER macro can also be supported.

Refer to [1], we found that with critical section solution, maybe the macro can be defined as this:
#define PTHREAD_MUTEX_INITIALIZER {(void*)-1,-1,0,0,0,0}

We have tested that works, so I would prefer to add that if the macro is also OK with you.
What do you think about that? 

The explanation from [1]:
" The pthreads API has an initialization macro that has no correspondence to anything in the windows API. By investigating the internal definition of the critical section type, one may work out how to initialize one without calling InitializeCriticalSection(). The trick here is that InitializeCriticalSection() is not allowed to fail. It tries to allocate a critical section debug object, but if no memory is available, it sets the pointer to a specific value. (One would expect that value to be NULL, but it is actually (void *)-1 for some reason.) Thus we can use this special value for that pointer, and the critical section code will work.

The other important part of the critical section type to initialize is the number of waiters. This controls whether or not the mutex is locked. Fortunately, this part of the critical section is unlikely to change. Apparently, many programs already test critical sections to see if they are locked using this value, so Microsoft felt that it was necessary to keep it set at -1 for an unlocked critical section, even when they changed the underlying algorithm to be more scalable. The final parts of the critical section object are unimportant, and can be set to zero for their defaults."

[1] https://locklessinc.com/articles/pthreads_on_windows/

BR,
SuanmingMou

> -----Original Message-----
> From: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
> Sent: Thursday, October 8, 2020 12:54 AM
> To: Suanming Mou <suanmingm@nvidia.com>
> Cc: Narcisa Ana Maria Vasile <navasile@linux.microsoft.com>; Dmitry Malloy
> <dmitrym@microsoft.com>; Pallavi Kadam <pallavi.kadam@intel.com>;
> dev@dpdk.org
> Subject: Re: [PATCH v3 1/2] eal/windows: add pthread mutex lock
> 
> On Wed,  7 Oct 2020 22:17:28 +0800, Suanming Mou wrote:
> > Add pthread mutex lock as it is needed for the thread safe rte_flow
> > functions.
> >
> > Signed-off-by: Suanming Mou <suanmingm@nvidia.com>
> 
> Acked-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>

Re: [dpdk-dev] [PATCH v3 1/2] eal/windows: add pthread mutex lock

[Tal Shnaiderman]

Hi DmitryM, Naty,

We would like to know if the macro added below for PTHREAD_MUTEX_INITIALIZER is a safe initialization of a CRITICAL_SECTION.

Can you please review the code and explanation below and check it internally?  

> Subject: Re: [dpdk-dev] [PATCH v3 1/2] eal/windows: add pthread mutex
> lock
> 
> External email: Use caution opening links or attachments
> 
> 
> Hi Dmitry,
> 
> Thank you very much.
> I also got some messages that they wish the PTHREAD_MUTEX_INITIALIZER
> macro can also be supported.
> 
> Refer to [1], we found that with critical section solution, maybe the macro
> can be defined as this:
> #define PTHREAD_MUTEX_INITIALIZER {(void*)-1,-1,0,0,0,0}
> 
> We have tested that works, so I would prefer to add that if the macro is also
> OK with you.
> What do you think about that?
> 
> The explanation from [1]:
> " The pthreads API has an initialization macro that has no correspondence to
> anything in the windows API. By investigating the internal definition of the
> critical section type, one may work out how to initialize one without calling
> InitializeCriticalSection(). The trick here is that InitializeCriticalSection() is not
> allowed to fail. It tries to allocate a critical section debug object, but if no
> memory is available, it sets the pointer to a specific value. (One would expect
> that value to be NULL, but it is actually (void *)-1 for some reason.) Thus we
> can use this special value for that pointer, and the critical section code will
> work.
> 
> The other important part of the critical section type to initialize is the number
> of waiters. This controls whether or not the mutex is locked. Fortunately, this
> part of the critical section is unlikely to change. Apparently, many programs
> already test critical sections to see if they are locked using this value, so
> Microsoft felt that it was necessary to keep it set at -1 for an unlocked critical
> section, even when they changed the underlying algorithm to be more
> scalable. The final parts of the critical section object are unimportant, and can
> be set to zero for their defaults."
> 
> [1]
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flockl
> essinc.com%2Farticles%2Fpthreads_on_windows%2F&data=02%7C01%
> 7Ctalshn%40nvidia.com%7C4533970f5c964751eeea08d86b3483e3%7C43083d
> 15727340c1b7db39efd9ccc17a%7C0%7C0%7C637377220576713708&sdat
> a=qELFpXSw7%2Fk8m9FMIidiQsnTq%2BeyJRII8C1sDyrHJe4%3D&reserv
> ed=0
> 
> BR,
> SuanmingMou
> 
> > -----Original Message-----
> > From: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
> > Sent: Thursday, October 8, 2020 12:54 AM
> > To: Suanming Mou <suanmingm@nvidia.com>
> > Cc: Narcisa Ana Maria Vasile <navasile@linux.microsoft.com>; Dmitry
> > Malloy <dmitrym@microsoft.com>; Pallavi Kadam
> > <pallavi.kadam@intel.com>; dev@dpdk.org
> > Subject: Re: [PATCH v3 1/2] eal/windows: add pthread mutex lock
> >
> > On Wed,  7 Oct 2020 22:17:28 +0800, Suanming Mou wrote:
> > > Add pthread mutex lock as it is needed for the thread safe rte_flow
> > > functions.
> > >
> > > Signed-off-by: Suanming Mou <suanmingm@nvidia.com>
> >
> > Acked-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>

[dpdk-dev] [PATCH v3 2/2] ethdev: make rte_flow API thread safe

[Suanming Mou]

Currently, the rte_flow functions are not defined as 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 safe 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
safe. As different PMDs have different flow operations, some may
support thread safe 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 safe.

For the PMDs which don't want the default mutex lock, just set the
flag in the PMD, and add the prefer 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. For the standard posix
pthread_mutex, 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>
---

v3:
 - update flow_lock/unlock -> fts_enter/exit

v2:
 - Update commit info and description doc.
 - Add inline for the flow lock and unlock functions.
 - Remove the PMD sample part flag configuration.

---

 doc/guides/prog_guide/rte_flow.rst  |  9 ++--
 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 ++++++++++++++++++++++++++++---------
 5 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..ae2ddb3 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,11 @@ 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. The application can test the dev_flags
+with RTE_ETH_DEV_FLOW_OPS_THREAD_SAFE in struct rte_eth_dev_data to know
+if the rte_flow thread-safe works under rte_flow level or PMD level.
+
 More will be added over time.
 
 Device compatibility
diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index 0f56541..60677fe 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);
@@ -564,6 +565,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 d2bf74f..03612fd 100644
--- a/lib/librte_ethdev/rte_ethdev.h
+++ b/lib/librte_ethdev/rte_ethdev.h
@@ -1664,6 +1664,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..6823458 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 inline void
+fts_enter(struct rte_eth_dev *dev)
+{
+	if (!(dev->data->dev_flags & RTE_ETH_DEV_FLOW_OPS_THREAD_SAFE))
+		pthread_mutex_lock(&dev->data->fts_mutex);
+}
+
+static inline void
+fts_exit(struct rte_eth_dev *dev)
+{
+	if (!(dev->data->dev_flags & RTE_ETH_DEV_FLOW_OPS_THREAD_SAFE))
+		pthread_mutex_unlock(&dev->data->fts_mutex);
+}
+
 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)) {
+		fts_enter(dev);
+		ret = ops->validate(dev, attr, pattern, actions, error);
+		fts_exit(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)) {
+		fts_enter(dev);
 		flow = ops->create(dev, attr, pattern, actions, error);
+		fts_exit(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)) {
+		fts_enter(dev);
+		ret = ops->destroy(dev, flow, error);
+		fts_exit(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)) {
+		fts_enter(dev);
+		ret = ops->flush(dev, error);
+		fts_exit(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)) {
+		fts_enter(dev);
+		ret = ops->query(dev, flow, action, data, error);
+		fts_exit(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)) {
+		fts_enter(dev);
+		ret = ops->isolate(dev, set, error);
+		fts_exit(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)) {
+		fts_enter(dev);
+		ret = ops->dev_dump(dev, file, error);
+		fts_exit(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)) {
+		fts_enter(dev);
+		ret = ops->get_aged_flows(dev, contexts, nb_contexts, error);
+		fts_exit(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

Re: [dpdk-dev] [PATCH v3 2/2] ethdev: make rte_flow API thread safe

[Ajit Khaparde]

On Wed, Oct 7, 2020 at 7:18 AM Suanming Mou <suanmingm@nvidia.com> wrote:
>
> Currently, the rte_flow functions are not defined as 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 safe 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
> safe. As different PMDs have different flow operations, some may
> support thread safe 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 safe.
>
> For the PMDs which don't want the default mutex lock, just set the
> flag in the PMD, and add the prefer 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. For the standard posix
> pthread_mutex, 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>
Acked-by: Ajit Khaparde <ajit.khaparde@broadcom.com>


> ---
>
> v3:
>  - update flow_lock/unlock -> fts_enter/exit
>
> v2:
>  - Update commit info and description doc.
>  - Add inline for the flow lock and unlock functions.
>  - Remove the PMD sample part flag configuration.
>
> ---
>
>  doc/guides/prog_guide/rte_flow.rst  |  9 ++--
>  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 ++++++++++++++++++++++++++++---------
>  5 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..ae2ddb3 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,11 @@ 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. The application can test the dev_flags
> +with RTE_ETH_DEV_FLOW_OPS_THREAD_SAFE in struct rte_eth_dev_data to know
> +if the rte_flow thread-safe works under rte_flow level or PMD level.
> +
>  More will be added over time.
>
>  Device compatibility
> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index 0f56541..60677fe 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);
> @@ -564,6 +565,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 d2bf74f..03612fd 100644
> --- a/lib/librte_ethdev/rte_ethdev.h
> +++ b/lib/librte_ethdev/rte_ethdev.h
> @@ -1664,6 +1664,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..6823458 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 inline void
> +fts_enter(struct rte_eth_dev *dev)
> +{
> +       if (!(dev->data->dev_flags & RTE_ETH_DEV_FLOW_OPS_THREAD_SAFE))
> +               pthread_mutex_lock(&dev->data->fts_mutex);
> +}
> +
> +static inline void
> +fts_exit(struct rte_eth_dev *dev)
> +{
> +       if (!(dev->data->dev_flags & RTE_ETH_DEV_FLOW_OPS_THREAD_SAFE))
> +               pthread_mutex_unlock(&dev->data->fts_mutex);
> +}
> +
>  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)) {
> +               fts_enter(dev);
> +               ret = ops->validate(dev, attr, pattern, actions, error);
> +               fts_exit(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)) {
> +               fts_enter(dev);
>                 flow = ops->create(dev, attr, pattern, actions, error);
> +               fts_exit(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)) {
> +               fts_enter(dev);
> +               ret = ops->destroy(dev, flow, error);
> +               fts_exit(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)) {
> +               fts_enter(dev);
> +               ret = ops->flush(dev, error);
> +               fts_exit(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)) {
> +               fts_enter(dev);
> +               ret = ops->query(dev, flow, action, data, error);
> +               fts_exit(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)) {
> +               fts_enter(dev);
> +               ret = ops->isolate(dev, set, error);
> +               fts_exit(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)) {
> +               fts_enter(dev);
> +               ret = ops->dev_dump(dev, file, error);
> +               fts_exit(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)) {
> +               fts_enter(dev);
> +               ret = ops->get_aged_flows(dev, contexts, nb_contexts, error);
> +               fts_exit(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
>

Re: [dpdk-dev] [PATCH v3 2/2] ethdev: make rte_flow API thread safe

[Ori Kam]

Hi Suanming,


> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Ajit Khaparde
> Sent: Wednesday, October 7, 2020 5:43 PM
> Subject: Re: [dpdk-dev] [PATCH v3 2/2] ethdev: make rte_flow API thread safe
> 
> On Wed, Oct 7, 2020 at 7:18 AM Suanming Mou <suanmingm@nvidia.com>
> wrote:
> >
> > Currently, the rte_flow functions are not defined as 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 safe 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
> > safe. As different PMDs have different flow operations, some may
> > support thread safe 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 safe.
> >
> > For the PMDs which don't want the default mutex lock, just set the
> > flag in the PMD, and add the prefer 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. For the standard posix
> > pthread_mutex, 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>
> Acked-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
> 

Acked-by: Ori Kam <orika@nvidia.com>
Thanks,
Ori

Re: [dpdk-dev] [PATCH v3 2/2] ethdev: make rte_flow API thread safe

[Matan Azrad]

From: Suanming Mou
> Currently, the rte_flow functions are not defined as thread safe.
> DPDK applications either call the functions in single thread or add locks
> around the functions for the critical section.
> 

Better to write here "or protect any concurrent calling for the rte_flow operations using a lock."

> 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 safe not guaranteed for the rte_flow functions

not => is not

> also limits the applications' expectation.
> 
> This feature is going to change the rte_flow functions to be thread safe. As
> different PMDs have different flow operations, some may support thread
> safe 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
> safe.
> 
> For the PMDs which don't want the default mutex lock, just set the flag in
> the PMD, and add the prefer 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. For the standard posix
> pthread_mutex, 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>
> ---
> 
> v3:
>  - update flow_lock/unlock -> fts_enter/exit
> 
> v2:
>  - Update commit info and description doc.
>  - Add inline for the flow lock and unlock functions.
>  - Remove the PMD sample part flag configuration.
> 
> ---
> 
>  doc/guides/prog_guide/rte_flow.rst  |  9 ++--
>  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 ++++++++++++++++++++++++++++---
> ------
>  5 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..ae2ddb3 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,11 @@ 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. The application can test

Good to mention mutex per port.

> +the dev_flags with RTE_ETH_DEV_FLOW_OPS_THREAD_SAFE in struct
> +rte_eth_dev_data to know if the rte_flow thread-safe works under
> rte_flow level or PMD level.
> +

If think it will be helpful to add here a note saying that it is unsafe to call other control commands In parallel to rte_flow operations. 

>  More will be added over time.
> 
>  Device compatibility
> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index 0f56541..60677fe 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);
> @@ -564,6 +565,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 d2bf74f..03612fd 100644
> --- a/lib/librte_ethdev/rte_ethdev.h
> +++ b/lib/librte_ethdev/rte_ethdev.h
> @@ -1664,6 +1664,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..6823458 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 inline void
> +fts_enter(struct rte_eth_dev *dev)
> +{
> +	if (!(dev->data->dev_flags &
> RTE_ETH_DEV_FLOW_OPS_THREAD_SAFE))
> +		pthread_mutex_lock(&dev->data->fts_mutex);
> +}
> +
> +static inline void
> +fts_exit(struct rte_eth_dev *dev)
> +{
> +	if (!(dev->data->dev_flags &
> RTE_ETH_DEV_FLOW_OPS_THREAD_SAFE))
> +		pthread_mutex_unlock(&dev->data->fts_mutex);
> +}
> +
>  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)) {
> +		fts_enter(dev);
> +		ret = ops->validate(dev, attr, pattern, actions, error);
> +		fts_exit(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)) {
> +		fts_enter(dev);
>  		flow = ops->create(dev, attr, pattern, actions, error);
> +		fts_exit(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)) {
> +		fts_enter(dev);
> +		ret = ops->destroy(dev, flow, error);
> +		fts_exit(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)) {
> +		fts_enter(dev);
> +		ret = ops->flush(dev, error);
> +		fts_exit(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)) {
> +		fts_enter(dev);
> +		ret = ops->query(dev, flow, action, data, error);
> +		fts_exit(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)) {
> +		fts_enter(dev);
> +		ret = ops->isolate(dev, set, error);
> +		fts_exit(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)) {
> +		fts_enter(dev);
> +		ret = ops->dev_dump(dev, file, error);
> +		fts_exit(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)) {
> +		fts_enter(dev);
> +		ret = ops->get_aged_flows(dev, contexts, nb_contexts,
> error);
> +		fts_exit(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

Besides the above (not must) suggestion, looks good,
Acked-by: Matan Azrad <matan@nvidia.com>

Re: [dpdk-dev] [PATCH v3 2/2] ethdev: make rte_flow API thread safe

[Suanming Mou]

> -----Original Message-----
> From: Matan Azrad <matan@nvidia.com>
> Sent: Thursday, October 8, 2020 4:11 AM
> To: Suanming Mou <suanmingm@nvidia.com>; Ori Kam <orika@nvidia.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: [dpdk-dev] [PATCH v3 2/2] ethdev: make rte_flow API thread safe
> 
> 
> 
> From: Suanming Mou
> > Currently, the rte_flow functions are not defined as thread safe.
> > DPDK applications either call the functions in single thread or add
> > locks around the functions for the critical section.
> >
> 
> Better to write here "or protect any concurrent calling for the rte_flow
> operations using a lock."
> 
> > 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 safe not guaranteed for the rte_flow
> > functions
> 
> not => is not
> 
> > also limits the applications' expectation.
> >
> > This feature is going to change the rte_flow functions to be thread
> > safe. As different PMDs have different flow operations, some may
> > support thread safe 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 safe.
> >
> > For the PMDs which don't want the default mutex lock, just set the
> > flag in the PMD, and add the prefer 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. For the standard posix
> > pthread_mutex, 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>
> > ---
> >
> > v3:
> >  - update flow_lock/unlock -> fts_enter/exit
> >
> > v2:
> >  - Update commit info and description doc.
> >  - Add inline for the flow lock and unlock functions.
> >  - Remove the PMD sample part flag configuration.
> >
> > ---
> >
> >  doc/guides/prog_guide/rte_flow.rst  |  9 ++--
> >  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 ++++++++++++++++++++++++++++---
> > ------
> >  5 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..ae2ddb3 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,11 @@ 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. The application can
> > +test
> 
> Good to mention mutex per port.
> 
> > +the dev_flags with RTE_ETH_DEV_FLOW_OPS_THREAD_SAFE in struct
> > +rte_eth_dev_data to know if the rte_flow thread-safe works under
> > rte_flow level or PMD level.
> > +
> 
> If think it will be helpful to add here a note saying that it is unsafe to call other
> control commands In parallel to rte_flow operations.
> 
> >  More will be added over time.
> >
> >  Device compatibility

[snip]

> > 1.8.3.1
> 
> Besides the above (not must) suggestion, looks good,
> Acked-by: Matan Azrad <matan@nvidia.com>

Thanks for the suggestions, I will update that in the next version.

[dpdk-dev] [PATCH v4 0/2] ethdev: make rte_flow API thread safe

[Suanming Mou]

Currently, the rte_flow functions are not defined as 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 safe 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
safe. As different PMDs have different flow operations, some may
support thread safe 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 safe.

For the PMDs which don't want the default mutex lock, just set the
flag in the PMD, and add the prefer 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. For the standard posix
pthread_mutex, 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.

Suanming Mou (2):
  eal/windows: add pthread mutex lock
  ethdev: make rte_flow API thread safe

---

v4:
 - Add PTHREAD_MUTEX_INITIALIZER for windows pthread mutex.
 - Change RTE_ETH_DEV_FLOW_OPS_THREAD_SAFE to 0x0001, since
   RTE_ETH_DEV_CLOSE_REMOVE has been removed.
 - Update small comments.
 - Update release notes.

v3:
 - Update flow_lock/unlock -> fts_enter/exit.

v2:
 - Using critical section for windows pthread mutex.
 - Update ethdev commnets.

---

 doc/guides/prog_guide/rte_flow.rst       | 11 +++--
 doc/guides/rel_notes/release_20_11.rst   |  6 +++
 lib/librte_eal/windows/include/pthread.h | 35 +++++++++++++
 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 ++++++++++++++++++++++++--------
 7 files changed, 121 insertions(+), 23 deletions(-)

-- 
1.8.3.1

[dpdk-dev] [PATCH v4 1/2] eal/windows: add pthread mutex lock

[Suanming Mou]

Add pthread mutex lock as it is needed for the thread safe rte_flow
functions.

Signed-off-by: Suanming Mou <suanmingm@nvidia.com>
Acked-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
---

v4:
 - Add PTHREAD_MUTEX_INITIALIZER macro.

v3:
 - No updates.

v2:
 - Using critical section for windows pthread mutex.

---

 lib/librte_eal/windows/include/pthread.h | 35 ++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/lib/librte_eal/windows/include/pthread.h b/lib/librte_eal/windows/include/pthread.h
index 99013dc..c62251f 100644
--- a/lib/librte_eal/windows/include/pthread.h
+++ b/lib/librte_eal/windows/include/pthread.h
@@ -28,6 +28,12 @@
 /* defining pthread_attr_t type on Windows since there is no in Microsoft libc*/
 typedef void *pthread_attr_t;
 
+typedef void *pthread_mutexattr_t;
+
+typedef CRITICAL_SECTION pthread_mutex_t;
+
+#define PTHREAD_MUTEX_INITIALIZER {(void *)-1, -1, 0, 0, 0, 0}
+
 typedef SYNCHRONIZATION_BARRIER pthread_barrier_t;
 
 #define pthread_barrier_init(barrier, attr, count) \
@@ -139,6 +145,35 @@
 	return 0;
 }
 
+static inline int
+pthread_mutex_init(pthread_mutex_t *mutex,
+		   __rte_unused pthread_mutexattr_t *attr)
+{
+	InitializeCriticalSection(mutex);
+	return 0;
+}
+
+static inline int
+pthread_mutex_lock(pthread_mutex_t *mutex)
+{
+	EnterCriticalSection(mutex);
+	return 0;
+}
+
+static inline int
+pthread_mutex_unlock(pthread_mutex_t *mutex)
+{
+	LeaveCriticalSection(mutex);
+	return 0;
+}
+
+static inline int
+pthread_mutex_destroy(pthread_mutex_t *mutex)
+{
+	DeleteCriticalSection(mutex);
+	return 0;
+}
+
 #ifdef __cplusplus
 }
 #endif
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH v4 1/2] eal/windows: add pthread mutex lock

[Tal Shnaiderman]

> Subject: [dpdk-dev] [PATCH v4 1/2] eal/windows: add pthread mutex lock
> 
> Add pthread mutex lock as it is needed for the thread safe rte_flow
> functions.
> 
> Signed-off-by: Suanming Mou <suanmingm@nvidia.com>
> Acked-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
> ---
> 	
> v4:
>  - Add PTHREAD_MUTEX_INITIALIZER macro.
> 
> v3:
>  - No updates.
> 
> v2:
>  - Using critical section for windows pthread mutex.
> 
> ---
> 
>  lib/librte_eal/windows/include/pthread.h | 35
> ++++++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
> 
> diff --git a/lib/librte_eal/windows/include/pthread.h
> b/lib/librte_eal/windows/include/pthread.h
> index 99013dc..c62251f 100644
> --- a/lib/librte_eal/windows/include/pthread.h
> +++ b/lib/librte_eal/windows/include/pthread.h
> @@ -28,6 +28,12 @@
>  /* defining pthread_attr_t type on Windows since there is no in Microsoft
> libc*/  typedef void *pthread_attr_t;
> 
> +typedef void *pthread_mutexattr_t;
> +
> +typedef CRITICAL_SECTION pthread_mutex_t;
> +
> +#define PTHREAD_MUTEX_INITIALIZER {(void *)-1, -1, 0, 0, 0, 0}
> +
>  typedef SYNCHRONIZATION_BARRIER pthread_barrier_t;
> 
>  #define pthread_barrier_init(barrier, attr, count) \ @@ -139,6 +145,35 @@
>  	return 0;
>  }
> 
> +static inline int
> +pthread_mutex_init(pthread_mutex_t *mutex,
> +		   __rte_unused pthread_mutexattr_t *attr) {
> +	InitializeCriticalSection(mutex);
> +	return 0;
> +}
> +
> +static inline int
> +pthread_mutex_lock(pthread_mutex_t *mutex) {
> +	EnterCriticalSection(mutex);
> +	return 0;
> +}
> +
> +static inline int
> +pthread_mutex_unlock(pthread_mutex_t *mutex) {
> +	LeaveCriticalSection(mutex);
> +	return 0;
> +}
> +
> +static inline int
> +pthread_mutex_destroy(pthread_mutex_t *mutex) {
> +	DeleteCriticalSection(mutex);
> +	return 0;
> +}
> +
>  #ifdef __cplusplus
>  }
>  #endif
> --
> 1.8.3.1

Tested-by: Tal Shnaiderman <talshn@nvidia.com>
Acked-by: Tal Shnaiderman <talshn@nvidia.com>

Re: [dpdk-dev] [PATCH v4 1/2] eal/windows: add pthread mutex lock

[Ranjit Menon]

On 10/8/2020 6:17 PM, Suanming Mou wrote:
> Add pthread mutex lock as it is needed for the thread safe rte_flow
> functions.
>
> Signed-off-by: Suanming Mou <suanmingm@nvidia.com>
> Acked-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
> ---
>
> v4:
>   - Add PTHREAD_MUTEX_INITIALIZER macro.
>
> v3:
>   - No updates.
>
> v2:
>   - Using critical section for windows pthread mutex.
>
> ---
>
>   lib/librte_eal/windows/include/pthread.h | 35 ++++++++++++++++++++++++++++++++
>   1 file changed, 35 insertions(+)
>
> diff --git a/lib/librte_eal/windows/include/pthread.h b/lib/librte_eal/windows/include/pthread.h
> index 99013dc..c62251f 100644
> --- a/lib/librte_eal/windows/include/pthread.h
> +++ b/lib/librte_eal/windows/include/pthread.h
> @@ -28,6 +28,12 @@
>   /* defining pthread_attr_t type on Windows since there is no in Microsoft libc*/
>   typedef void *pthread_attr_t;
>   
> +typedef void *pthread_mutexattr_t;
> +
> +typedef CRITICAL_SECTION pthread_mutex_t;
> +
> +#define PTHREAD_MUTEX_INITIALIZER {(void *)-1, -1, 0, 0, 0, 0}
> +
>   typedef SYNCHRONIZATION_BARRIER pthread_barrier_t;
>   
>   #define pthread_barrier_init(barrier, attr, count) \
> @@ -139,6 +145,35 @@
>   	return 0;
>   }
>   
> +static inline int
> +pthread_mutex_init(pthread_mutex_t *mutex,
> +		   __rte_unused pthread_mutexattr_t *attr)
> +{
> +	InitializeCriticalSection(mutex);
> +	return 0;
> +}
> +
> +static inline int
> +pthread_mutex_lock(pthread_mutex_t *mutex)
> +{
> +	EnterCriticalSection(mutex);
> +	return 0;
> +}
> +
> +static inline int
> +pthread_mutex_unlock(pthread_mutex_t *mutex)
> +{
> +	LeaveCriticalSection(mutex);
> +	return 0;
> +}
> +
> +static inline int
> +pthread_mutex_destroy(pthread_mutex_t *mutex)
> +{
> +	DeleteCriticalSection(mutex);
> +	return 0;
> +}
> +
>   #ifdef __cplusplus
>   }
>   #endif

Acked-by: Ranjit Menon <ranjit.menon@intel.com>

Re: [dpdk-dev] [PATCH v4 1/2] eal/windows: add pthread mutex lock

[Narcisa Ana Maria Vasile]

On Fri, Oct 09, 2020 at 09:17:22AM +0800, Suanming Mou wrote:
> Add pthread mutex lock as it is needed for the thread safe rte_flow
> functions.
> 
> Signed-off-by: Suanming Mou <suanmingm@nvidia.com>
> Acked-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
> ---
> 
> v4:
>  - Add PTHREAD_MUTEX_INITIALIZER macro.
> 
> v3:
>  - No updates.
> 
> v2:
>  - Using critical section for windows pthread mutex.
> 
> ---
> 
>  lib/librte_eal/windows/include/pthread.h | 35 ++++++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
> 
> diff --git a/lib/librte_eal/windows/include/pthread.h b/lib/librte_eal/windows/include/pthread.h
> index 99013dc..c62251f 100644
> --- a/lib/librte_eal/windows/include/pthread.h
> +++ b/lib/librte_eal/windows/include/pthread.h
> @@ -28,6 +28,12 @@
>  /* defining pthread_attr_t type on Windows since there is no in Microsoft libc*/
>  typedef void *pthread_attr_t;
>  
> +typedef void *pthread_mutexattr_t;
> +
> +typedef CRITICAL_SECTION pthread_mutex_t;
> +
> +#define PTHREAD_MUTEX_INITIALIZER {(void *)-1, -1, 0, 0, 0, 0}
> +

Regarding the question on the static initializer, adding the guidance from DmitryM:
"If you choose to do the static initializer, you will be relying on implementation specifics,
(which may have not changed ever, or may not ever change, or may change in the next release).
This would be a hack (although potentially long term)."

Otherwise,
Acked-by: Narcisa Vasile <navasile@linux.microsoft.com>

Re: [dpdk-dev] [PATCH v4 1/2] eal/windows: add pthread mutex lock

[Suanming Mou]

> -----Original Message-----
> From: Narcisa Ana Maria Vasile <navasile@linux.microsoft.com>
> Sent: Thursday, October 15, 2020 10:16 AM
> To: Suanming Mou <suanmingm@nvidia.com>
> Cc: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>; Dmitry Malloy
> <dmitrym@microsoft.com>; Pallavi Kadam <pallavi.kadam@intel.com>;
> dev@dpdk.org
> Subject: Re: [PATCH v4 1/2] eal/windows: add pthread mutex lock
> 
> On Fri, Oct 09, 2020 at 09:17:22AM +0800, Suanming Mou wrote:
> > Add pthread mutex lock as it is needed for the thread safe rte_flow
> > functions.
> >
> > Signed-off-by: Suanming Mou <suanmingm@nvidia.com>
> > Acked-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
> > ---
> >
> > v4:
> >  - Add PTHREAD_MUTEX_INITIALIZER macro.
> >
> > v3:
> >  - No updates.
> >
> > v2:
> >  - Using critical section for windows pthread mutex.
> >
> > ---
> >
> >  lib/librte_eal/windows/include/pthread.h | 35
> > ++++++++++++++++++++++++++++++++
> >  1 file changed, 35 insertions(+)
> >
> > diff --git a/lib/librte_eal/windows/include/pthread.h
> > b/lib/librte_eal/windows/include/pthread.h
> > index 99013dc..c62251f 100644
> > --- a/lib/librte_eal/windows/include/pthread.h
> > +++ b/lib/librte_eal/windows/include/pthread.h
> > @@ -28,6 +28,12 @@
> >  /* defining pthread_attr_t type on Windows since there is no in
> > Microsoft libc*/  typedef void *pthread_attr_t;
> >
> > +typedef void *pthread_mutexattr_t;
> > +
> > +typedef CRITICAL_SECTION pthread_mutex_t;
> > +
> > +#define PTHREAD_MUTEX_INITIALIZER {(void *)-1, -1, 0, 0, 0, 0}
> > +
> 
> Regarding the question on the static initializer, adding the guidance from
> DmitryM:
> "If you choose to do the static initializer, you will be relying on implementation
> specifics, (which may have not changed ever, or may not ever change, or may
> change in the next release).
> This would be a hack (although potentially long term)."

Yes, thanks. So we have removed that hack in the latest v5.
https://patches.dpdk.org/patch/80814/

> 
> Otherwise,
> Acked-by: Narcisa Vasile <navasile@linux.microsoft.com>

[dpdk-dev] [PATCH v4 2/2] ethdev: make rte_flow API thread safe

[Suanming Mou]

Currently, the rte_flow functions are not defined as thread safe.
DPDK applications either call the functions in single thread or
protect any concurrent calling for the rte_flow operations using
a lock.

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 safe is 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
safe. As different PMDs have different flow operations, some may
support thread safe 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 safe.

For the PMDs which don't want the default mutex lock, just set the
flag in the PMD, and add the prefer 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. For the standard posix
pthread_mutex, 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>
Acked-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
Acked-by: Ori Kam <orika@nvidia.com>
Acked-by: Matan Azrad <matan@nvidia.com>
---

v4:
 - Change RTE_ETH_DEV_FLOW_OPS_THREAD_SAFE to 0x0001, since
   RTE_ETH_DEV_CLOSE_REMOVE has been removed.
 - Update small comments.
 - Update release notes.

v3:
 - update flow_lock/unlock -> fts_enter/exit

v2:
 - Update commit info and description doc.
 - Add inline for the flow lock and unlock functions.
 - Remove the PMD sample part flag configuration.

---

 doc/guides/prog_guide/rte_flow.rst     | 11 +++--
 doc/guides/rel_notes/release_20_11.rst |  6 +++
 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, 86 insertions(+), 23 deletions(-)

diff --git a/doc/guides/prog_guide/rte_flow.rst b/doc/guides/prog_guide/rte_flow.rst
index 119b128..b4ae9cc 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,13 @@ 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 per port. The application can test the
+dev_flags with RTE_ETH_DEV_FLOW_OPS_THREAD_SAFE in struct rte_eth_dev_data
+to know if the rte_flow thread-safe works under rte_flow level or PMD level.
+Please note that the mutex only protects rte_flow level functions, other
+control path functions are not in scope.
+
 More will be added over time.
 
 Device compatibility
diff --git a/doc/guides/rel_notes/release_20_11.rst b/doc/guides/rel_notes/release_20_11.rst
index 0b2a370..364306d 100644
--- a/doc/guides/rel_notes/release_20_11.rst
+++ b/doc/guides/rel_notes/release_20_11.rst
@@ -109,6 +109,12 @@ New Features
   * Extern objects and functions can be plugged into the pipeline.
   * Transaction-oriented table updates.
 
+* **Added thread safe support to rte_flow functions.**
+
+  Added ``RTE_ETH_DEV_FLOW_OPS_THREAD_SAFE`` device flag to indicate
+  if PMD support thread safe operations. If PMD doesn't set the flag,
+  rte_flow level functions will protect the flow operations with mutex.
+
 
 Removed Items
 -------------
diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index 0f56541..60677fe 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);
@@ -564,6 +565,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 d2bf74f..4dc2b85 100644
--- a/lib/librte_ethdev/rte_ethdev.h
+++ b/lib/librte_ethdev/rte_ethdev.h
@@ -1654,6 +1654,8 @@ struct rte_eth_dev_owner {
 	char name[RTE_ETH_MAX_OWNER_NAME_LEN]; /**< The owner name. */
 };
 
+/** Device PMD supports thread safety flow operation */
+#define RTE_ETH_DEV_FLOW_OPS_THREAD_SAFE  0x0001
 /** Device supports link state interrupt */
 #define RTE_ETH_DEV_INTR_LSC     0x0002
 /** Device is a bonded slave */
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..6823458 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 inline void
+fts_enter(struct rte_eth_dev *dev)
+{
+	if (!(dev->data->dev_flags & RTE_ETH_DEV_FLOW_OPS_THREAD_SAFE))
+		pthread_mutex_lock(&dev->data->fts_mutex);
+}
+
+static inline void
+fts_exit(struct rte_eth_dev *dev)
+{
+	if (!(dev->data->dev_flags & RTE_ETH_DEV_FLOW_OPS_THREAD_SAFE))
+		pthread_mutex_unlock(&dev->data->fts_mutex);
+}
+
 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)) {
+		fts_enter(dev);
+		ret = ops->validate(dev, attr, pattern, actions, error);
+		fts_exit(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)) {
+		fts_enter(dev);
 		flow = ops->create(dev, attr, pattern, actions, error);
+		fts_exit(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)) {
+		fts_enter(dev);
+		ret = ops->destroy(dev, flow, error);
+		fts_exit(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)) {
+		fts_enter(dev);
+		ret = ops->flush(dev, error);
+		fts_exit(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)) {
+		fts_enter(dev);
+		ret = ops->query(dev, flow, action, data, error);
+		fts_exit(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)) {
+		fts_enter(dev);
+		ret = ops->isolate(dev, set, error);
+		fts_exit(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)) {
+		fts_enter(dev);
+		ret = ops->dev_dump(dev, file, error);
+		fts_exit(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)) {
+		fts_enter(dev);
+		ret = ops->get_aged_flows(dev, contexts, nb_contexts, error);
+		fts_exit(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

Re: [dpdk-dev] [PATCH v4 2/2] ethdev: make rte_flow API thread safe

[Thomas Monjalon]

09/10/2020 03:17, Suanming Mou:
> --- a/doc/guides/prog_guide/rte_flow.rst
> +++ b/doc/guides/prog_guide/rte_flow.rst
> +If PMD interfaces do not support re-entrancy/multi-thread safety, rte_flow

"API" should be inserted here to make clear which layer we talk about.

> +level functions will do it by mutex per port. The application can test the

"do it" is too vague. I suggest "protect threads".

> +dev_flags with RTE_ETH_DEV_FLOW_OPS_THREAD_SAFE in struct rte_eth_dev_data

The application access it through dev_info.

> +to know if the rte_flow thread-safe works under rte_flow level or PMD level.

Again insert "API": rte_flow API level

This sentence can be confusing. Better to say explicitly that
if RTE_ETH_DEV_FLOW_OPS_THREAD_SAFE is set, it means the protection
at API level is disabled.

> +Please note that the mutex only protects rte_flow level functions, other
> +control path functions are not in scope.

Please find a complete rewording with sentences split after punctuation:

If PMD interfaces do not support re-entrancy/multi-thread safety,
the rte_flow API functions will protect threads by mutex per port.
The application can check whether ``RTE_ETH_DEV_FLOW_OPS_THREAD_SAFE``
is set in ``dev_flags``, meaning the PMD is thread-safe regarding rte_flow,
so the API level protection is disabled.
Please note that this API-level mutex protects only rte_flow functions,
other control path functions are not in scope.


[...]
> --- a/doc/guides/rel_notes/release_20_11.rst
> +++ b/doc/guides/rel_notes/release_20_11.rst
> @@ -109,6 +109,12 @@ New Features
>    * Extern objects and functions can be plugged into the pipeline.
>    * Transaction-oriented table updates.
>  
> +* **Added thread safe support to rte_flow functions.**
> +
> +  Added ``RTE_ETH_DEV_FLOW_OPS_THREAD_SAFE`` device flag to indicate
> +  if PMD support thread safe operations. If PMD doesn't set the flag,
> +  rte_flow level functions will protect the flow operations with mutex.
> +

Should be sorted before drivers with other ethdev features if any.

[...]
> --- a/lib/librte_ethdev/rte_ethdev.h
> +++ b/lib/librte_ethdev/rte_ethdev.h
> +/** Device PMD supports thread safety flow operation */

"Device" is useless
safety -> safe (adjective before "flow operation")

It becomes:
/** PMD supports thread-safe flow operations */

> +#define RTE_ETH_DEV_FLOW_OPS_THREAD_SAFE  0x0001

OK for the name of the flag.

[...]
> --- a/lib/librte_ethdev/rte_ethdev_core.h
> +++ b/lib/librte_ethdev/rte_ethdev_core.h
> @@ -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. */

"ts" or "safety" is redundant for a mutex.
I suggest "flow_ops_mutex" as a name.

Re: [dpdk-dev] [PATCH v4 2/2] ethdev: make rte_flow API thread safe

[Suanming Mou]

Hi Thomas,

Thanks, will update the next version when Windows mutex macro solution is confirmed.

BR,
SuanmingMou

> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Wednesday, October 14, 2020 6:19 PM
> To: Suanming Mou <suanmingm@nvidia.com>
> Cc: Ori Kam <orika@nvidia.com>; Ferruh Yigit <ferruh.yigit@intel.com>; Andrew
> Rybchenko <arybchenko@solarflare.com>; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v4 2/2] ethdev: make rte_flow API thread safe
> 
> 09/10/2020 03:17, Suanming Mou:
> > --- a/doc/guides/prog_guide/rte_flow.rst
> > +++ b/doc/guides/prog_guide/rte_flow.rst
> > +If PMD interfaces do not support re-entrancy/multi-thread safety,
> > +rte_flow
> 
> "API" should be inserted here to make clear which layer we talk about.
> 
> > +level functions will do it by mutex per port. The application can
> > +test the
> 
> "do it" is too vague. I suggest "protect threads".
> 
> > +dev_flags with RTE_ETH_DEV_FLOW_OPS_THREAD_SAFE in struct
> > +rte_eth_dev_data
> 
> The application access it through dev_info.
> 
> > +to know if the rte_flow thread-safe works under rte_flow level or PMD level.
> 
> Again insert "API": rte_flow API level
> 
> This sentence can be confusing. Better to say explicitly that if
> RTE_ETH_DEV_FLOW_OPS_THREAD_SAFE is set, it means the protection at API
> level is disabled.
> 
> > +Please note that the mutex only protects rte_flow level functions,
> > +other control path functions are not in scope.
> 
> Please find a complete rewording with sentences split after punctuation:
> 
> If PMD interfaces do not support re-entrancy/multi-thread safety, the rte_flow
> API functions will protect threads by mutex per port.
> The application can check whether ``RTE_ETH_DEV_FLOW_OPS_THREAD_SAFE``
> is set in ``dev_flags``, meaning the PMD is thread-safe regarding rte_flow, so the
> API level protection is disabled.
> Please note that this API-level mutex protects only rte_flow functions, other
> control path functions are not in scope.
> 
> 
> [...]
> > --- a/doc/guides/rel_notes/release_20_11.rst
> > +++ b/doc/guides/rel_notes/release_20_11.rst
> > @@ -109,6 +109,12 @@ New Features
> >    * Extern objects and functions can be plugged into the pipeline.
> >    * Transaction-oriented table updates.
> >
> > +* **Added thread safe support to rte_flow functions.**
> > +
> > +  Added ``RTE_ETH_DEV_FLOW_OPS_THREAD_SAFE`` device flag to indicate
> > + if PMD support thread safe operations. If PMD doesn't set the flag,
> > + rte_flow level functions will protect the flow operations with mutex.
> > +
> 
> Should be sorted before drivers with other ethdev features if any.
> 
> [...]
> > --- a/lib/librte_ethdev/rte_ethdev.h
> > +++ b/lib/librte_ethdev/rte_ethdev.h
> > +/** Device PMD supports thread safety flow operation */
> 
> "Device" is useless
> safety -> safe (adjective before "flow operation")
> 
> It becomes:
> /** PMD supports thread-safe flow operations */
> 
> > +#define RTE_ETH_DEV_FLOW_OPS_THREAD_SAFE  0x0001
> 
> OK for the name of the flag.
> 
> [...]
> > --- a/lib/librte_ethdev/rte_ethdev_core.h
> > +++ b/lib/librte_ethdev/rte_ethdev_core.h
> > @@ -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. */
> 
> "ts" or "safety" is redundant for a mutex.
> I suggest "flow_ops_mutex" as a name.
> 
> 

[dpdk-dev] [PATCH v5 0/2] ethdev: make rte_flow API thread safe

[Suanming Mou]

Currently, the rte_flow functions are not defined as 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 safe 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
safe. As different PMDs have different flow operations, some may
support thread safe 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 safe.

For the PMDs which don't want the default mutex lock, just set the
flag in the PMD, and add the prefer 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. For the standard posix
pthread_mutex, 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.


Suanming Mou (2):
  eal/windows: add pthread mutex lock
  ethdev: make rte_flow API thread safe

---

v5:
 - Update ethdev patch commnets, doc and release notes.
 - Update fts_mutex -> flow_ops_mutex.
 - Remove PTHREAD_MUTEX_INITIALIZER for windows added in v4.

v4:
 - Add PTHREAD_MUTEX_INITIALIZER for windows pthread mutex.
 - Change RTE_ETH_DEV_FLOW_OPS_THREAD_SAFE to 0x0001, since
   RTE_ETH_DEV_CLOSE_REMOVE has been removed.
 - Update small comments.
 - Update release notes.

v3:
 - Update flow_lock/unlock -> fts_enter/exit.

v2:
 - Using critical section for windows pthread mutex.
 - Update ethdev commnets.

---

 doc/guides/prog_guide/rte_flow.rst       | 12 +++--
 doc/guides/rel_notes/release_20_11.rst   |  6 +++
 lib/librte_eal/windows/include/pthread.h | 33 +++++++++++++
 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 ++++++++++++++++++++++++--------
 7 files changed, 120 insertions(+), 23 deletions(-)

-- 
1.8.3.1

[dpdk-dev] [PATCH v5 1/2] eal/windows: add pthread mutex lock

[Suanming Mou]

Add pthread mutex lock as it is needed for the thread safe rte_flow
functions.

Signed-off-by: Suanming Mou <suanmingm@nvidia.com>
Tested-by: Tal Shnaiderman <talshn@nvidia.com>
Acked-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
Acked-by: Ranjit Menon <ranjit.menon@intel.com>
---

v5:
 - Remove PTHREAD_MUTEX_INITIALIZER macro added in v4.

v4:
 - Add PTHREAD_MUTEX_INITIALIZER macro.

v3:
 - No updates.

v2:
 - Using critical section for windows pthread mutex.

---

 lib/librte_eal/windows/include/pthread.h | 33 ++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/lib/librte_eal/windows/include/pthread.h b/lib/librte_eal/windows/include/pthread.h
index 99013dc..644fd49 100644
--- a/lib/librte_eal/windows/include/pthread.h
+++ b/lib/librte_eal/windows/include/pthread.h
@@ -28,6 +28,10 @@
 /* defining pthread_attr_t type on Windows since there is no in Microsoft libc*/
 typedef void *pthread_attr_t;
 
+typedef void *pthread_mutexattr_t;
+
+typedef CRITICAL_SECTION pthread_mutex_t;
+
 typedef SYNCHRONIZATION_BARRIER pthread_barrier_t;
 
 #define pthread_barrier_init(barrier, attr, count) \
@@ -139,6 +143,35 @@
 	return 0;
 }
 
+static inline int
+pthread_mutex_init(pthread_mutex_t *mutex,
+		   __rte_unused pthread_mutexattr_t *attr)
+{
+	InitializeCriticalSection(mutex);
+	return 0;
+}
+
+static inline int
+pthread_mutex_lock(pthread_mutex_t *mutex)
+{
+	EnterCriticalSection(mutex);
+	return 0;
+}
+
+static inline int
+pthread_mutex_unlock(pthread_mutex_t *mutex)
+{
+	LeaveCriticalSection(mutex);
+	return 0;
+}
+
+static inline int
+pthread_mutex_destroy(pthread_mutex_t *mutex)
+{
+	DeleteCriticalSection(mutex);
+	return 0;
+}
+
 #ifdef __cplusplus
 }
 #endif
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH v5 1/2] eal/windows: add pthread mutex lock

[Narcisa Ana Maria Vasile]

On Thu, Oct 15, 2020 at 09:07:46AM +0800, Suanming Mou wrote:
> Add pthread mutex lock as it is needed for the thread safe rte_flow
> functions.
> 
> Signed-off-by: Suanming Mou <suanmingm@nvidia.com>
> Tested-by: Tal Shnaiderman <talshn@nvidia.com>
> Acked-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
> Acked-by: Ranjit Menon <ranjit.menon@intel.com>
> ---
> 
> v5:
>  - Remove PTHREAD_MUTEX_INITIALIZER macro added in v4.
> 
> v4:
>  - Add PTHREAD_MUTEX_INITIALIZER macro.
> 
> v3:
>  - No updates.
> 
> v2:
>  - Using critical section for windows pthread mutex.
> 
> ---
> 
>  lib/librte_eal/windows/include/pthread.h | 33 ++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
> 

Acked-by: Narcisa Vasile <navasile@linux.microsoft.com>

[dpdk-dev] [PATCH v5 2/2] ethdev: make rte_flow API thread safe

[Suanming Mou]

Currently, the rte_flow functions are not defined as thread safe.
DPDK applications either call the functions in single thread or
protect any concurrent calling for the rte_flow operations using
a lock.

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 safe is 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
safe. As different PMDs have different flow operations, some may
support thread safe 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 safe.

For the PMDs which don't want the default mutex lock, just set the
flag in the PMD, and add the prefer 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. For the standard posix
pthread_mutex, 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>
Acked-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
Acked-by: Ori Kam <orika@nvidia.com>
Acked-by: Matan Azrad <matan@nvidia.com>
---

v5:
 - Update ethdev patch commnets, doc and release notes.
 - Update fts_mutex -> flow_ops_mutex.

v4:
 - Change RTE_ETH_DEV_FLOW_OPS_THREAD_SAFE to 0x0001, since
   RTE_ETH_DEV_CLOSE_REMOVE has been removed.
 - Update small comments.
 - Update release notes.

v3:
 - update flow_lock/unlock -> fts_enter/exit

v2:
 - Update commit info and description doc.
 - Add inline for the flow lock and unlock functions.
 - Remove the PMD sample part flag configuration.

---

 doc/guides/prog_guide/rte_flow.rst     | 12 +++--
 doc/guides/rel_notes/release_20_11.rst |  6 +++
 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, 87 insertions(+), 23 deletions(-)

diff --git a/doc/guides/prog_guide/rte_flow.rst b/doc/guides/prog_guide/rte_flow.rst
index f26a6c2..a446b51 100644
--- a/doc/guides/prog_guide/rte_flow.rst
+++ b/doc/guides/prog_guide/rte_flow.rst
@@ -3088,10 +3088,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
@@ -3143,6 +3139,14 @@ This interface additionally defines the following helper function:
 - ``rte_flow_ops_get()``: get generic flow operations structure from a
   port.
 
+If PMD interfaces don't support re-entrancy/multi-thread safety,
+the rte_flow API functions will protect threads by mutex per port.
+The application can check whether ``RTE_ETH_DEV_FLOW_OPS_THREAD_SAFE``
+is set in ``dev_flags``, meaning the PMD is thread-safe regarding rte_flow,
+so the API level protection is disabled.
+Please note that this API-level mutex protects only rte_flow functions,
+other control path functions are not in scope.
+
 More will be added over time.
 
 Device compatibility
diff --git a/doc/guides/rel_notes/release_20_11.rst b/doc/guides/rel_notes/release_20_11.rst
index 30db8f2..ae3381f 100644
--- a/doc/guides/rel_notes/release_20_11.rst
+++ b/doc/guides/rel_notes/release_20_11.rst
@@ -67,6 +67,12 @@ New Features
   Added the FEC API which provides functions for query FEC capabilities and
   current FEC mode from device. Also, API for configuring FEC mode is also provided.
 
+* **Added thread safe support to rte_flow functions.**
+
+  Added ``RTE_ETH_DEV_FLOW_OPS_THREAD_SAFE`` device flag to indicate
+  if PMD support thread safe operations. If PMD doesn't set the flag,
+  rte_flow API level functions will protect the flow operations with mutex.
+
 * **Updated Broadcom bnxt driver.**
 
   Updated the Broadcom bnxt driver with new features and improvements, including:
diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index 5b7979a..7817224 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->flow_ops_mutex, NULL);
 
 unlock:
 	rte_spinlock_unlock(&rte_eth_dev_shared_data->ownership_lock);
@@ -564,6 +565,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->flow_ops_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 f4cc591..f07dbc4 100644
--- a/lib/librte_ethdev/rte_ethdev.h
+++ b/lib/librte_ethdev/rte_ethdev.h
@@ -1684,6 +1684,8 @@ struct rte_eth_dev_owner {
 	char name[RTE_ETH_MAX_OWNER_NAME_LEN]; /**< The owner name. */
 };
 
+/** PMD supports thread-safe flow operations */
+#define RTE_ETH_DEV_FLOW_OPS_THREAD_SAFE  0x0001
 /** Device supports link state interrupt */
 #define RTE_ETH_DEV_INTR_LSC     0x0002
 /** Device is a bonded slave */
diff --git a/lib/librte_ethdev/rte_ethdev_core.h b/lib/librte_ethdev/rte_ethdev_core.h
index fd3bf92..918a34e 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 flow_ops_mutex; /**< rte_flow ops 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 0a8a7a2..1b0370f 100644
--- a/lib/librte_ethdev/rte_flow.c
+++ b/lib/librte_ethdev/rte_flow.c
@@ -208,6 +208,20 @@ struct rte_flow_desc_data {
 	return -rte_errno;
 }
 
+static inline void
+fts_enter(struct rte_eth_dev *dev)
+{
+	if (!(dev->data->dev_flags & RTE_ETH_DEV_FLOW_OPS_THREAD_SAFE))
+		pthread_mutex_lock(&dev->data->flow_ops_mutex);
+}
+
+static inline void
+fts_exit(struct rte_eth_dev *dev)
+{
+	if (!(dev->data->dev_flags & RTE_ETH_DEV_FLOW_OPS_THREAD_SAFE))
+		pthread_mutex_unlock(&dev->data->flow_ops_mutex);
+}
+
 static int
 flow_err(uint16_t port_id, int ret, struct rte_flow_error *error)
 {
@@ -254,12 +268,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)) {
+		fts_enter(dev);
+		ret = ops->validate(dev, attr, pattern, actions, error);
+		fts_exit(dev);
+		return flow_err(port_id, ret, error);
+	}
 	return rte_flow_error_set(error, ENOSYS,
 				  RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
 				  NULL, rte_strerror(ENOSYS));
@@ -280,7 +298,9 @@ struct rte_flow *
 	if (unlikely(!ops))
 		return NULL;
 	if (likely(!!ops->create)) {
+		fts_enter(dev);
 		flow = ops->create(dev, attr, pattern, actions, error);
+		fts_exit(dev);
 		if (flow == NULL)
 			flow_err(port_id, -rte_errno, error);
 		return flow;
@@ -298,12 +318,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)) {
+		fts_enter(dev);
+		ret = ops->destroy(dev, flow, error);
+		fts_exit(dev);
+		return flow_err(port_id, ret, error);
+	}
 	return rte_flow_error_set(error, ENOSYS,
 				  RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
 				  NULL, rte_strerror(ENOSYS));
@@ -316,11 +340,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)) {
+		fts_enter(dev);
+		ret = ops->flush(dev, error);
+		fts_exit(dev);
+		return flow_err(port_id, ret, error);
+	}
 	return rte_flow_error_set(error, ENOSYS,
 				  RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
 				  NULL, rte_strerror(ENOSYS));
@@ -336,12 +365,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)) {
+		fts_enter(dev);
+		ret = ops->query(dev, flow, action, data, error);
+		fts_exit(dev);
+		return flow_err(port_id, ret, error);
+	}
 	return rte_flow_error_set(error, ENOSYS,
 				  RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
 				  NULL, rte_strerror(ENOSYS));
@@ -355,11 +388,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)) {
+		fts_enter(dev);
+		ret = ops->isolate(dev, set, error);
+		fts_exit(dev);
+		return flow_err(port_id, ret, error);
+	}
 	return rte_flow_error_set(error, ENOSYS,
 				  RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
 				  NULL, rte_strerror(ENOSYS));
@@ -963,12 +1001,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)) {
+		fts_enter(dev);
+		ret = ops->dev_dump(dev, file, error);
+		fts_exit(dev);
+		return flow_err(port_id, ret, error);
+	}
 	return rte_flow_error_set(error, ENOSYS,
 				  RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
 				  NULL, rte_strerror(ENOSYS));
@@ -980,12 +1022,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)) {
+		fts_enter(dev);
+		ret = ops->get_aged_flows(dev, contexts, nb_contexts, error);
+		fts_exit(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

Re: [dpdk-dev] [PATCH v5 2/2] ethdev: make rte_flow API thread safe

[Thomas Monjalon]

15/10/2020 03:07, Suanming Mou:
> Currently, the rte_flow functions are not defined as thread safe.
> DPDK applications either call the functions in single thread or
> protect any concurrent calling for the rte_flow operations using
> a lock.
> 
> 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 safe is 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
> safe. As different PMDs have different flow operations, some may
> support thread safe 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 safe.
> 
> For the PMDs which don't want the default mutex lock, just set the
> flag in the PMD, and add the prefer 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. For the standard posix
> pthread_mutex, 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>
> Acked-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
> Acked-by: Ori Kam <orika@nvidia.com>
> Acked-by: Matan Azrad <matan@nvidia.com>
Acked-by: Thomas Monjalon <thomas@monjalon.net>

Re: [dpdk-dev] [PATCH v5 2/2] ethdev: make rte_flow API thread safe

[Andrew Rybchenko]

On 10/15/20 11:28 AM, Thomas Monjalon wrote:
> 15/10/2020 03:07, Suanming Mou:
>> Currently, the rte_flow functions are not defined as thread safe.
>> DPDK applications either call the functions in single thread or
>> protect any concurrent calling for the rte_flow operations using
>> a lock.
>>
>> 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 safe is 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
>> safe. As different PMDs have different flow operations, some may
>> support thread safe 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 safe.
>>
>> For the PMDs which don't want the default mutex lock, just set the
>> flag in the PMD, and add the prefer 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. For the standard posix
>> pthread_mutex, 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>
>> Acked-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
>> Acked-by: Ori Kam <orika@nvidia.com>
>> Acked-by: Matan Azrad <matan@nvidia.com>
> Acked-by: Thomas Monjalon <thomas@monjalon.net>

Acked-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>

Re: [dpdk-dev] [PATCH v5 0/2] ethdev: make rte_flow API thread safe

[Thomas Monjalon]

15/10/2020 03:07, Suanming Mou:
> Currently, the rte_flow functions are not defined as 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 safe 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
> safe. As different PMDs have different flow operations, some may
> support thread safe 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 safe.
> 
> For the PMDs which don't want the default mutex lock, just set the
> flag in the PMD, and add the prefer 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. For the standard posix
> pthread_mutex, 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.
> 
> 
> Suanming Mou (2):
>   eal/windows: add pthread mutex lock
>   ethdev: make rte_flow API thread safe

Applied, thanks