[v2,2/2] ethdev: make rte_flow API thread safe
Checks
Commit Message
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(-)
Comments
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
> > +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()
> -----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()
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()
@@ -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
@@ -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(ð_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(ð_dev->data->fts_mutex);
memset(eth_dev->data, 0, sizeof(struct rte_eth_dev_data));
}
@@ -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.
@@ -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;
@@ -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));