[1/2] ethdev: make flow API primary/secondary process safe
Checks
Commit Message
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.
Bugzilla ID: 662
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
Fixes: 80d1a9aff7f6 ("ethdev: make flow API thread safe")
Cc: suanmingm@nvidia.com
---
lib/librte_ethdev/rte_ethdev.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
Comments
Hi Stephen,
> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Tuesday, March 16, 2021 3:27 AM
> To: dev@dpdk.org
> Cc: Stephen Hemminger <stephen@networkplumber.org>; Suanming Mou
> <suanmingm@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.
>
> Bugzilla ID: 662
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> Fixes: 80d1a9aff7f6 ("ethdev: make flow API thread safe")
> Cc: suanmingm@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(ð_dev->data->flow_ops_mutex, NULL);
> +
> + pthread_mutexattr_init(&attr);
> + pthread_mutexattr_setpshared(&attr, PTHREAD_PROCESS_SHARED);
>
> + pthread_mutex_init(ð_dev->data->flow_ops_mutex, &attr);
>
> unlock:
> rte_spinlock_unlock(ð_dev_shared_data->ownership_lock);
> --
> 2.30.2
On Tue, 16 Mar 2021 23:48:57 +0000
Suanming Mou <suanmingm@nvidia.com> wrote:
> Hi Stephen,
>
> > -----Original Message-----
> > From: Stephen Hemminger <stephen@networkplumber.org>
> > Sent: Tuesday, March 16, 2021 3:27 AM
> > To: dev@dpdk.org
> > Cc: Stephen Hemminger <stephen@networkplumber.org>; Suanming Mou
> > <suanmingm@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.
>
OK, that makes sense. Is this in the documentation?
> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Wednesday, March 17, 2021 8:14 AM
> To: Suanming Mou <suanmingm@nvidia.com>
> Cc: dev@dpdk.org
> Subject: Re: [PATCH 1/2] ethdev: make flow API primary/secondary process safe
>
> On Tue, 16 Mar 2021 23:48:57 +0000
> Suanming Mou <suanmingm@nvidia.com> wrote:
>
> > Hi Stephen,
> >
> > > -----Original Message-----
> > > From: Stephen Hemminger <stephen@networkplumber.org>
> > > Sent: Tuesday, March 16, 2021 3:27 AM
> > > To: dev@dpdk.org
> > > Cc: Stephen Hemminger <stephen@networkplumber.org>; Suanming Mou
> > > <suanmingm@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.
> >
>
> OK, that makes sense. Is this in the documentation?
We have the description as below in doc/guides/prog_guide/rte_flow.rst:
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.
On 3/16/2021 11:48 PM, Suanming Mou wrote:
> Hi Stephen,
>
>> -----Original Message-----
>> From: Stephen Hemminger <stephen@networkplumber.org>
>> Sent: Tuesday, March 16, 2021 3:27 AM
>> To: dev@dpdk.org
>> Cc: Stephen Hemminger <stephen@networkplumber.org>; Suanming Mou
>> <suanmingm@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.
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 <stephen@networkplumber.org>
>> Fixes: 80d1a9aff7f6 ("ethdev: make flow API thread safe")
>> Cc: suanmingm@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(ð_dev->data->flow_ops_mutex, NULL);
>> +
>> + pthread_mutexattr_init(&attr);
>> + pthread_mutexattr_setpshared(&attr, PTHREAD_PROCESS_SHARED);
>>
>> + pthread_mutex_init(ð_dev->data->flow_ops_mutex, &attr);
>>
>> unlock:
>> rte_spinlock_unlock(ð_dev_shared_data->ownership_lock);
>> --
>> 2.30.2
>
Hi,
> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Wednesday, April 14, 2021 9:07 PM
> To: Suanming Mou <suanmingm@nvidia.com>; Ori Kam <orika@nvidia.com>;
> Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>; NBU-Contact-Thomas
> Monjalon <thomas@monjalon.net>
> Cc: dev@dpdk.org; Stephen Hemminger <stephen@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 <stephen@networkplumber.org>
> >> Sent: Tuesday, March 16, 2021 3:27 AM
> >> To: dev@dpdk.org
> >> Cc: Stephen Hemminger <stephen@networkplumber.org>; Suanming Mou
> >> <suanmingm@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. (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.
>
>
> 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 <stephen@networkplumber.org>
> >> Fixes: 80d1a9aff7f6 ("ethdev: make flow API thread safe")
> >> Cc: suanmingm@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(ð_dev->data->flow_ops_mutex, NULL);
> >> +
> >> + pthread_mutexattr_init(&attr);
> >> + pthread_mutexattr_setpshared(&attr, PTHREAD_PROCESS_SHARED);
> >>
> >> + pthread_mutex_init(ð_dev->data->flow_ops_mutex, &attr);
> >>
> >> unlock:
> >> rte_spinlock_unlock(ð_dev_shared_data->ownership_lock);
> >> --
> >> 2.30.2
> >
On Thu, 15 Apr 2021 02:55:17 +0000
Suanming Mou <suanmingm@nvidia.com> wrote:
> Hi,
>
> > -----Original Message-----
> > From: Ferruh Yigit <ferruh.yigit@intel.com>
> > Sent: Wednesday, April 14, 2021 9:07 PM
> > To: Suanming Mou <suanmingm@nvidia.com>; Ori Kam <orika@nvidia.com>;
> > Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>; NBU-Contact-Thomas
> > Monjalon <thomas@monjalon.net>
> > Cc: dev@dpdk.org; Stephen Hemminger <stephen@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 <stephen@networkplumber.org>
> > >> Sent: Tuesday, March 16, 2021 3:27 AM
> > >> To: dev@dpdk.org
> > >> Cc: Stephen Hemminger <stephen@networkplumber.org>; Suanming Mou
> > >> <suanmingm@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. (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.
Maybe we need a FLOW_OPS_PROCESS_SAFE flag? and declare all existing drivers as not process safe?
On 4/15/2021 3:55 AM, Suanming Mou wrote:
> Hi,
>
>> -----Original Message-----
>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>> Sent: Wednesday, April 14, 2021 9:07 PM
>> To: Suanming Mou <suanmingm@nvidia.com>; Ori Kam <orika@nvidia.com>;
>> Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>; NBU-Contact-Thomas
>> Monjalon <thomas@monjalon.net>
>> Cc: dev@dpdk.org; Stephen Hemminger <stephen@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 <stephen@networkplumber.org>
>>>> Sent: Tuesday, March 16, 2021 3:27 AM
>>>> To: dev@dpdk.org
>>>> Cc: Stephen Hemminger <stephen@networkplumber.org>; Suanming Mou
>>>> <suanmingm@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 <stephen@networkplumber.org>
>>>> Fixes: 80d1a9aff7f6 ("ethdev: make flow API thread safe")
>>>> Cc: suanmingm@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(ð_dev->data->flow_ops_mutex, NULL);
>>>> +
>>>> + pthread_mutexattr_init(&attr);
>>>> + pthread_mutexattr_setpshared(&attr, PTHREAD_PROCESS_SHARED);
>>>>
>>>> + pthread_mutex_init(ð_dev->data->flow_ops_mutex, &attr);
>>>>
>>>> unlock:
>>>> rte_spinlock_unlock(ð_dev_shared_data->ownership_lock);
>>>> --
>>>> 2.30.2
>>>
>
On Thu, 15 Apr 2021 08:42:39 +0100
Ferruh Yigit <ferruh.yigit@intel.com> wrote:
> > (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?
> >>
Only impacts the contended case.
Setting the PROCESS_SHARED turns the mutex causes the mutex to behave
differently when cmxchg fails. Internally pthread_mutex_lock will
call futex if it has to wait and the SHARED flag impacts whether
FUTEX_PRIVATE_FLAG is set.
FUTEX_PRIVATE_FLAG (since Linux 2.6.22)
This option bit can be employed with all futex operations. It
tells the kernel that the futex is process-private and not
shared with another process (i.e., it is being used for synchro‐
nization only between threads of the same process). This allows
the kernel to make some additional performance optimizations.
> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Thursday, April 15, 2021 3:43 PM
> To: Suanming Mou <suanmingm@nvidia.com>; Ori Kam <orika@nvidia.com>;
> Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>; NBU-Contact-Thomas
> Monjalon <thomas@monjalon.net>
> Cc: dev@dpdk.org; Stephen Hemminger <stephen@networkplumber.org>;
> Matan Azrad <matan@nvidia.com>; Shahaf Shuler <shahafs@nvidia.com>; Ajit
> Khaparde <ajit.khaparde@broadcom.com>; Somnath Kotur
> <somnath.kotur@broadcom.com>
> Subject: Re: [dpdk-dev] [PATCH 1/2] ethdev: make flow API primary/secondary
> process safe
>
> On 4/15/2021 3:55 AM, Suanming Mou wrote:
> > Hi,
> >
> >> -----Original Message-----
> >> From: Ferruh Yigit <ferruh.yigit@intel.com>
> >> Sent: Wednesday, April 14, 2021 9:07 PM
> >> To: Suanming Mou <suanmingm@nvidia.com>; Ori Kam <orika@nvidia.com>;
> >> Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>; NBU-Contact-
> Thomas
> >> Monjalon <thomas@monjalon.net>
> >> Cc: dev@dpdk.org; Stephen Hemminger <stephen@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 <stephen@networkplumber.org>
> >>>> Sent: Tuesday, March 16, 2021 3:27 AM
> >>>> To: dev@dpdk.org
> >>>> Cc: Stephen Hemminger <stephen@networkplumber.org>; Suanming Mou
> >>>> <suanmingm@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?
This depends on the vendor driver. For mlx, it should be safe, not sure others.
>
> >>
> >>
> >> 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?
> >>
> >>>>
We make a test on this patch, test result show that it works fine. Below is the detail:
HW: Kunpeng920 ARM Platform which is ARMv8
NIC: Kunpeng920 SOC NIC
OS: Linux centos-C3 5.12.0-rc4+
DPDK: 21.02
DRV: hns3
Start three process:
./testpmd -w 0000:bd:00.0 -l 67-68 --proc-type=auto -- -i --num-procs=3 --proc-id=0
./testpmd -w 0000:bd:00.0 -l 67-68 --proc-type=auto -- -i --num-procs=3 --proc-id=1
./testpmd -w 0000:bd:00.0 -l 69-70 --proc-type=auto -- -i --num-procs=3 --proc-id=2
Every process execute following steps:
1. create one fdir rule, eg: flow create 0 ingress pattern eth / ipv4 src is 192.168.110.26 / end actions queue index 0 / end
note: each process has different rules, so they will create success
2. create one rss rule
note: all process create the same rules, so they may create fail
3. flush all rules
4. goto step 1, loop again
note: there are +10ms delay after step 1/2/3 because we run VBS script to inject command on windows.
The test lasted 12 hours, and there are no process crashes.
On 2021/4/14 21:06, Ferruh Yigit wrote:
> On 3/16/2021 11:48 PM, Suanming Mou wrote:
>> Hi Stephen,
>>
>>> -----Original Message-----
>>> From: Stephen Hemminger <stephen@networkplumber.org>
>>> Sent: Tuesday, March 16, 2021 3:27 AM
>>> To: dev@dpdk.org
>>> Cc: Stephen Hemminger <stephen@networkplumber.org>; Suanming Mou
>>> <suanmingm@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.
>
>
> 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 <stephen@networkplumber.org>
>>> Fixes: 80d1a9aff7f6 ("ethdev: make flow API thread safe")
>>> Cc: suanmingm@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(ð_dev->data->flow_ops_mutex, NULL);
>>> +
>>> + pthread_mutexattr_init(&attr);
>>> + pthread_mutexattr_setpshared(&attr, PTHREAD_PROCESS_SHARED);
>>>
>>> + pthread_mutex_init(ð_dev->data->flow_ops_mutex, &attr);
>>>
>>> unlock:
>>> rte_spinlock_unlock(ð_dev_shared_data->ownership_lock);
>>> --
>>> 2.30.2
>>
>
>
> .
On Thu, Apr 15, 2021 at 12:42 AM Ferruh Yigit <ferruh.yigit@intel.com> wrote:
>
> On 4/15/2021 3:55 AM, Suanming Mou wrote:
> > Hi,
> >
> >> -----Original Message-----
> >> From: Ferruh Yigit <ferruh.yigit@intel.com>
> >> Sent: Wednesday, April 14, 2021 9:07 PM
> >> To: Suanming Mou <suanmingm@nvidia.com>; Ori Kam <orika@nvidia.com>;
> >> Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>; NBU-Contact-Thomas
> >> Monjalon <thomas@monjalon.net>
> >> Cc: dev@dpdk.org; Stephen Hemminger <stephen@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 <stephen@networkplumber.org>
> >>>> Sent: Tuesday, March 16, 2021 3:27 AM
> >>>> To: dev@dpdk.org
> >>>> Cc: Stephen Hemminger <stephen@networkplumber.org>; Suanming Mou
> >>>> <suanmingm@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?
We have some level of tests on this and so far current code looks safe.
But we will try to increase the coverage and see how it goes.
>
On 4/16/2021 2:41 AM, fengchengwen wrote:
> We make a test on this patch, test result show that it works fine. Below is the detail:
>
> HW: Kunpeng920 ARM Platform which is ARMv8
> NIC: Kunpeng920 SOC NIC
> OS: Linux centos-C3 5.12.0-rc4+
> DPDK: 21.02
> DRV: hns3
>
> Start three process:
> ./testpmd -w 0000:bd:00.0 -l 67-68 --proc-type=auto -- -i --num-procs=3 --proc-id=0
> ./testpmd -w 0000:bd:00.0 -l 67-68 --proc-type=auto -- -i --num-procs=3 --proc-id=1
> ./testpmd -w 0000:bd:00.0 -l 69-70 --proc-type=auto -- -i --num-procs=3 --proc-id=2
>
> Every process execute following steps:
> 1. create one fdir rule, eg: flow create 0 ingress pattern eth / ipv4 src is 192.168.110.26 / end actions queue index 0 / end
> note: each process has different rules, so they will create success
> 2. create one rss rule
> note: all process create the same rules, so they may create fail
> 3. flush all rules
> 4. goto step 1, loop again
> note: there are +10ms delay after step 1/2/3 because we run VBS script to inject command on windows.
>
> The test lasted 12 hours, and there are no process crashes.
>
Thanks Chengwen for testing, appreciated.
>
> On 2021/4/14 21:06, Ferruh Yigit wrote:
>> On 3/16/2021 11:48 PM, Suanming Mou wrote:
>>> Hi Stephen,
>>>
>>>> -----Original Message-----
>>>> From: Stephen Hemminger <stephen@networkplumber.org>
>>>> Sent: Tuesday, March 16, 2021 3:27 AM
>>>> To: dev@dpdk.org
>>>> Cc: Stephen Hemminger <stephen@networkplumber.org>; Suanming Mou
>>>> <suanmingm@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.
>>
>>
>> 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 <stephen@networkplumber.org>
>>>> Fixes: 80d1a9aff7f6 ("ethdev: make flow API thread safe")
>>>> Cc: suanmingm@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(ð_dev->data->flow_ops_mutex, NULL);
>>>> +
>>>> + pthread_mutexattr_init(&attr);
>>>> + pthread_mutexattr_setpshared(&attr, PTHREAD_PROCESS_SHARED);
>>>>
>>>> + pthread_mutex_init(ð_dev->data->flow_ops_mutex, &attr);
>>>>
>>>> unlock:
>>>> rte_spinlock_unlock(ð_dev_shared_data->ownership_lock);
>>>> --
>>>> 2.30.2
>>>
>>
>>
>> .
>
On 3/15/2021 7:27 PM, Stephen Hemminger wrote:
> 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.
>
> Bugzilla ID: 662
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> Fixes: 80d1a9aff7f6 ("ethdev: make flow API thread safe")
> Cc: suanmingm@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(ð_dev->data->flow_ops_mutex, NULL);
> +
> + pthread_mutexattr_init(&attr);
> + pthread_mutexattr_setpshared(&attr, PTHREAD_PROCESS_SHARED);
> + pthread_mutex_init(ð_dev->data->flow_ops_mutex, &attr);
>
> unlock:
> rte_spinlock_unlock(ð_dev_shared_data->ownership_lock);
>
The problem looks legit, and there seems no obvious downside of the patch. Also
Huawei test result was good, hence:
Acked-by: Ferruh Yigit <ferruh.yigit@intel.com>
And the PMDs that set the 'RTE_ETH_DEV_FLOW_OPS_THREAD_SAFE' flag already not
affected from this change at all, but how safe mutli process applications will
be for them is still not clear.
About the title, better to say "multi-process" and do not
commit on being completely safe for the whole API.
It is just fixing the mutex for multi-process,
and this mutex is for driver not being natively multi-thread.
The compilation fails on MinGW:
error: implicit declaration of function ‘pthread_mutexattr_init’
15/03/2021 20:27, Stephen Hemminger:
> 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.
>
> Bugzilla ID: 662
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> Fixes: 80d1a9aff7f6 ("ethdev: make flow API thread safe")
> Cc: suanmingm@nvidia.com
These lines must be sorted.
Please use --cc-cmd devtools/get-maintainer.sh so that maintainers
are Cc'ed. Adding Ferruh and Andrew.
On Mon, 19 Apr 2021 19:14:08 +0200
Thomas Monjalon <thomas@monjalon.net> wrote:
> About the title, better to say "multi-process" and do not
> commit on being completely safe for the whole API.
> It is just fixing the mutex for multi-process,
> and this mutex is for driver not being natively multi-thread.
>
> The compilation fails on MinGW:
> error: implicit declaration of function ‘pthread_mutexattr_init’
Sorry, don't have all the variants. How do you handle the lack of
compatibility on Windows.
19/04/2021 19:45, Stephen Hemminger:
> On Mon, 19 Apr 2021 19:14:08 +0200
> Thomas Monjalon <thomas@monjalon.net> wrote:
> > The compilation fails on MinGW:
> > error: implicit declaration of function ‘pthread_mutexattr_init’
>
> Sorry, don't have all the variants. How do you handle the lack of
> compatibility on Windows.
No idea, I am not the one working in Microsoft :)
Adding more people in Cc to help.
On 3/15/21 10:27 PM, Stephen Hemminger wrote:
> 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.
>
> Bugzilla ID: 662
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> Fixes: 80d1a9aff7f6 ("ethdev: make flow API thread safe")
> Cc: suanmingm@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(ð_dev->data->flow_ops_mutex, NULL);
> +
> + pthread_mutexattr_init(&attr);
> + pthread_mutexattr_setpshared(&attr, PTHREAD_PROCESS_SHARED);
Return value must be checked here. It may return ENOTSUP and
EINVAL. If it fails, IMHO we should do cleanup and return NULL.
> + pthread_mutex_init(ð_dev->data->flow_ops_mutex, &attr);
>
> unlock:
> rte_spinlock_unlock(ð_dev_shared_data->ownership_lock);
>
Please, fix notes from Thomas and above.
Overall these patches LGTM.
I think we should not introduce any flags that flow ops
are multi-process safe. Nobody prevents to call the API in
multi-process case and it must behave consistently.
The patch makes ethdev API safe and it is responsibility
of the driver care about final multi-process safety.
@@ -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(ð_dev->data->flow_ops_mutex, NULL);
+
+ pthread_mutexattr_init(&attr);
+ pthread_mutexattr_setpshared(&attr, PTHREAD_PROCESS_SHARED);
+ pthread_mutex_init(ð_dev->data->flow_ops_mutex, &attr);
unlock:
rte_spinlock_unlock(ð_dev_shared_data->ownership_lock);