Re: [dpdk-dev] [PATCH 1/2] ethdev: make flow API primary/secondary process safe

[Ferruh Yigit]

On 4/15/2021 3:55 AM, Suanming Mou wrote:
Hi,
-----Original Message-----
From: Ferruh Yigit <ferruh.yi...@intel.com>
Sent: Wednesday, April 14, 2021 9:07 PM
To: Suanming Mou <suanmi...@nvidia.com>; Ori Kam <or...@nvidia.com>;
Andrew Rybchenko <andrew.rybche...@oktetlabs.ru>; NBU-Contact-Thomas
Monjalon <tho...@monjalon.net>
Cc: dev@dpdk.org; Stephen Hemminger <step...@networkplumber.org>
Subject: Re: [dpdk-dev] [PATCH 1/2] ethdev: make flow API primary/secondary
process safe

On 3/16/2021 11:48 PM, Suanming Mou wrote:
Hi Stephen,

-----Original Message-----
From: Stephen Hemminger <step...@networkplumber.org>
Sent: Tuesday, March 16, 2021 3:27 AM
To: dev@dpdk.org
Cc: Stephen Hemminger <step...@networkplumber.org>; Suanming Mou
<suanmi...@nvidia.com>
Subject: [PATCH 1/2] ethdev: make flow API primary/secondary process
safe

Posix mutex are not by default safe for protecting for usage from
multiple processes. The flow ops mutex could be used by both primary
and secondary processes.

Process safe is something more widely scope. I assume it should be another
feature but not a bugfix for thread-safe?
And the fag RTE_ETH_DEV_FLOW_OPS_THREAD_SAFE we have added is just
thread safe.


Hi Suanming,

I think 'RTE_ETH_DEV_FLOW_OPS_THREAD_SAFE' flag and what this patch
address are different issues.

'RTE_ETH_DEV_FLOW_OPS_THREAD_SAFE' is to add/remove synchronization
support for flow APIs, that is for thread safety as flag name suggests.

This patch is to solve the problem for multi process, where commit log describes
as posix mutex is not safe for multiple process.

So for PMDs which not set the RTE_ETH_DEV_FLOW_OPS_THREAD_SAFE capability bit, 
they will have the process level protection in multi-process.
For PMDs which set the RTE_ETH_DEV_FLOW_OPS_THREAD_SAFE capability bit, this 
change does not help with these PMDs. If the PMD with 
RTE_ETH_DEV_FLOW_OPS_THREAD_SAFE capability bit  internally does not support 
multi-process, they may still suffer crash etc.

Correct

(If I understand correctly, mlx PMD level now should support multi-process, but 
better to have the confirmation from maintainers with much deeper level).
I assume this patch solves the posix mutex for multi-process only, hard to say 
the flow API primary/secondary process safe after that patch.


I am also not quite sure how PMDs that doesn't require mutex at all, (mlx5, 
bnxt, sfc) behave on multi process. Is calling flow APIs from both 
primary/secondary safe?



Stephen,
Are you aware of any downside setting 'PTHREAD_PROCESS_SHARED' attribute
to the
mutex? Any possible performance implications?

Ori,
Since mlx is heavily using the flow API, is it possible to test this patch? If
there is no negative impact, I think we can get this patch, what do you think?


Bugzilla ID: 662
Signed-off-by: Stephen Hemminger <step...@networkplumber.org>
Fixes: 80d1a9aff7f6 ("ethdev: make flow API thread safe")
Cc: suanmi...@nvidia.com
---
   lib/librte_ethdev/rte_ethdev.c | 6 +++++-
   1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index
6f514c388b4e..d1024df408a5 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -470,6 +470,7 @@ rte_eth_dev_allocate(const char *name)  {
        uint16_t port_id;
        struct rte_eth_dev *eth_dev = NULL;
+       pthread_mutexattr_t attr;
        size_t name_len;

        name_len = strnlen(name, RTE_ETH_NAME_MAX_LEN); @@ -506,7
+507,10 @@ rte_eth_dev_allocate(const char *name)
        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);
+
+       pthread_mutexattr_init(&attr);
+       pthread_mutexattr_setpshared(&attr, PTHREAD_PROCESS_SHARED);

+       pthread_mutex_init(&eth_dev->data->flow_ops_mutex, &attr);

   unlock:
        rte_spinlock_unlock(&eth_dev_shared_data->ownership_lock);
--
2.30.2