From: Ori Kam <orika@nvidia.com> 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" <dev@dpdk.org> Subject: Re: [dpdk-dev] [PATCH 2/2] ethdev: make rte flow API thread safe Date: Wed, 30 Sep 2020 10:56:06 +0000 Message-ID: <MN2PR12MB4286CFC15F886A01F2712F6BD6330@MN2PR12MB4286.namprd12.prod.outlook.com> (raw) In-Reply-To: <1601194817-208834-3-git-send-email-suanmingm@nvidia.com> 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(ð_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
next prev parent reply other threads:[~2020-09-30 10:56 UTC|newest] Thread overview: 40+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-09-27 8:20 [dpdk-dev] [PATCH 0/2] " Suanming Mou 2020-09-27 8:20 ` [dpdk-dev] [PATCH 1/2] eal/windows: add pthread mutex lock Suanming Mou 2020-09-27 15:56 ` Dmitry Kozlyuk 2020-09-28 2:30 ` Suanming Mou 2020-09-27 8:20 ` [dpdk-dev] [PATCH 2/2] ethdev: make rte flow API thread safe Suanming Mou 2020-09-30 10:56 ` Ori Kam [this message] 2020-10-04 23:44 ` Suanming Mou 2020-10-04 23:48 ` [dpdk-dev] [PATCH v2 0/2] ethdev: make rte_flow " Suanming Mou 2020-10-04 23:48 ` [dpdk-dev] [PATCH v2 1/2] eal/windows: add pthread mutex lock Suanming Mou 2020-10-04 23:48 ` [dpdk-dev] [PATCH v2 2/2] ethdev: make rte_flow API thread safe Suanming Mou 2020-10-05 11:28 ` Ori Kam 2020-10-06 23:18 ` Ajit Khaparde 2020-10-07 0:50 ` Suanming Mou 2020-10-07 6:33 ` Ori Kam 2020-10-07 14:17 ` [dpdk-dev] [PATCH v3 0/2] " Suanming Mou 2020-10-07 14:17 ` [dpdk-dev] [PATCH v3 1/2] eal/windows: add pthread mutex lock Suanming Mou 2020-10-07 16:53 ` Dmitry Kozlyuk 2020-10-08 2:46 ` Suanming Mou 2020-10-14 10:02 ` Tal Shnaiderman 2020-10-07 14:17 ` [dpdk-dev] [PATCH v3 2/2] ethdev: make rte_flow API thread safe Suanming Mou 2020-10-07 14:42 ` Ajit Khaparde 2020-10-07 16:37 ` Ori Kam 2020-10-07 20:10 ` Matan Azrad 2020-10-08 2:56 ` Suanming Mou 2020-10-09 1:17 ` [dpdk-dev] [PATCH v4 0/2] " Suanming Mou 2020-10-09 1:17 ` [dpdk-dev] [PATCH v4 1/2] eal/windows: add pthread mutex lock Suanming Mou 2020-10-09 9:19 ` Tal Shnaiderman 2020-10-14 16:45 ` Ranjit Menon 2020-10-15 2:15 ` Narcisa Ana Maria Vasile 2020-10-15 2:18 ` Suanming Mou 2020-10-09 1:17 ` [dpdk-dev] [PATCH v4 2/2] ethdev: make rte_flow API thread safe Suanming Mou 2020-10-14 10:19 ` Thomas Monjalon 2020-10-14 10:41 ` Suanming Mou 2020-10-15 1:07 ` [dpdk-dev] [PATCH v5 0/2] " Suanming Mou 2020-10-15 1:07 ` [dpdk-dev] [PATCH v5 1/2] eal/windows: add pthread mutex lock Suanming Mou 2020-10-15 2:22 ` Narcisa Ana Maria Vasile 2020-10-15 1:07 ` [dpdk-dev] [PATCH v5 2/2] ethdev: make rte_flow API thread safe Suanming Mou 2020-10-15 8:28 ` Thomas Monjalon 2020-10-15 8:52 ` Andrew Rybchenko 2020-10-15 22:43 ` [dpdk-dev] [PATCH v5 0/2] " Thomas Monjalon
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=MN2PR12MB4286CFC15F886A01F2712F6BD6330@MN2PR12MB4286.namprd12.prod.outlook.com \ --to=orika@nvidia.com \ --cc=arybchenko@solarflare.com \ --cc=dev@dpdk.org \ --cc=ferruh.yigit@intel.com \ --cc=john.mcnamara@intel.com \ --cc=marko.kovacevic@intel.com \ --cc=matan@mellanox.com \ --cc=orika@mellanox.com \ --cc=shahafs@mellanox.com \ --cc=suanmingm@nvidia.com \ --cc=thomas@monjalon.net \ --cc=viacheslavo@mellanox.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
DPDK patches and discussions This inbox may be cloned and mirrored by anyone: git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git # If you have public-inbox 1.1+ installed, you may # initialize and index your mirror using the following commands: public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \ dev@dpdk.org public-inbox-index dev Example config snippet for mirrors. Newsgroup available over NNTP: nntp://inbox.dpdk.org/inbox.dpdk.dev AGPL code for this site: git clone https://public-inbox.org/public-inbox.git