Hi Suanming, Small comments, PSB,
Best, Ori > -----Original Message----- > From: Suanming Mou <suanmi...@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 <suanmi...@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(ð_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)); > } > > 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