diff mbox series

[2/2] net/failsafe: fix primary/secondary mutex

Message ID 20210315192722.35490-3-stephen@networkplumber.org (mailing list archive)
State Deferred
Delegated to: Ferruh Yigit
Headers show
Series Mark shared pthread mutex | expand

Checks

Context Check Description
ci/intel-Testing success Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-mellanox-Functional success Functional Testing PASS
ci/iol-testing fail Testing issues
ci/github-robot success github build: passed
ci/travis-robot success travis build: passed
ci/iol-abi-testing success Testing PASS
ci/Intel-compilation fail Compilation issues
ci/checkpatch success coding style OK

Commit Message

Stephen Hemminger March 15, 2021, 7:27 p.m. UTC
Set mutex used in failsafe driver to protect when used by
both primary and secondary process. Without this fix, the failsafe
lock is not really locking when there are multiple secondary processes.

Bugzilla ID: 662
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
Fixes: 655fcd68c7d2 ("net/failsafe: fix hotplug races")
Cc: matan@mellanox.com
---
 drivers/net/failsafe/failsafe.c | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Ferruh Yigit April 14, 2021, 1:10 p.m. UTC | #1
On 3/15/2021 7:27 PM, Stephen Hemminger wrote:
> Set mutex used in failsafe driver to protect when used by
> both primary and secondary process. Without this fix, the failsafe
> lock is not really locking when there are multiple secondary processes.
> 
> Bugzilla ID: 662
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> Fixes: 655fcd68c7d2 ("net/failsafe: fix hotplug races")
> Cc: matan@mellanox.com
> ---
>   drivers/net/failsafe/failsafe.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/net/failsafe/failsafe.c b/drivers/net/failsafe/failsafe.c
> index e3bda0df2bf9..5b7e560dbc08 100644
> --- a/drivers/net/failsafe/failsafe.c
> +++ b/drivers/net/failsafe/failsafe.c
> @@ -140,6 +140,11 @@ fs_mutex_init(struct fs_priv *priv)
>   		ERROR("Cannot initiate mutex attributes - %s", strerror(ret));
>   		return ret;
>   	}
> +	/* Allow mutex to protect primary/secondary */
> +	ret = pthread_mutexattr_setpshared(&attr, PTHREAD_PROCESS_SHARED);
> +	if (ret)
> +		ERROR("Cannot set mutex shared - %s", strerror(ret));
> +
>   	/* Allow mutex relocks for the thread holding the mutex. */
>   	ret = pthread_mutexattr_settype(&attr, PTHREAD_MUTEX_RECURSIVE);
>   	if (ret) {
> 

Overall looks good to me.

Gaetan, Matan,

Can you please test the patch? To be sure it is not causing any unexpected 
functional/performance issues.

Thanks,
ferruh
Ferruh Yigit April 16, 2021, 8:19 a.m. UTC | #2
On 4/14/2021 2:10 PM, Ferruh Yigit wrote:
> On 3/15/2021 7:27 PM, Stephen Hemminger wrote:
>> Set mutex used in failsafe driver to protect when used by
>> both primary and secondary process. Without this fix, the failsafe
>> lock is not really locking when there are multiple secondary processes.
>>
>> Bugzilla ID: 662
>> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
>> Fixes: 655fcd68c7d2 ("net/failsafe: fix hotplug races")
>> Cc: matan@mellanox.com
>> ---
>>   drivers/net/failsafe/failsafe.c | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/net/failsafe/failsafe.c b/drivers/net/failsafe/failsafe.c
>> index e3bda0df2bf9..5b7e560dbc08 100644
>> --- a/drivers/net/failsafe/failsafe.c
>> +++ b/drivers/net/failsafe/failsafe.c
>> @@ -140,6 +140,11 @@ fs_mutex_init(struct fs_priv *priv)
>>           ERROR("Cannot initiate mutex attributes - %s", strerror(ret));
>>           return ret;
>>       }
>> +    /* Allow mutex to protect primary/secondary */
>> +    ret = pthread_mutexattr_setpshared(&attr, PTHREAD_PROCESS_SHARED);
>> +    if (ret)
>> +        ERROR("Cannot set mutex shared - %s", strerror(ret));
>> +
>>       /* Allow mutex relocks for the thread holding the mutex. */
>>       ret = pthread_mutexattr_settype(&attr, PTHREAD_MUTEX_RECURSIVE);
>>       if (ret) {
>>
> 
> Overall looks good to me.
> 
> Gaetan, Matan,
> 
> Can you please test the patch? To be sure it is not causing any unexpected 
> functional/performance issues.
> 

Ping.
Thomas Monjalon April 19, 2021, 5:08 p.m. UTC | #3
About the title, better to speak about multi-process,
it is less confusing than primary/secondary.

15/03/2021 20:27, Stephen Hemminger:
> Set mutex used in failsafe driver to protect when used by
> both primary and secondary process. Without this fix, the failsafe
> lock is not really locking when there are multiple secondary processes.
> 
> Bugzilla ID: 662
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> Fixes: 655fcd68c7d2 ("net/failsafe: fix hotplug races")
> Cc: matan@mellanox.com

The correct order for above lines is:

Bugzilla ID: 662
Fixes: 655fcd68c7d2 ("net/failsafe: fix hotplug races")

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>

> ---
> --- a/drivers/net/failsafe/failsafe.c
> +++ b/drivers/net/failsafe/failsafe.c
> @@ -140,6 +140,11 @@ fs_mutex_init(struct fs_priv *priv)
>  		ERROR("Cannot initiate mutex attributes - %s", strerror(ret));
>  		return ret;
>  	}
> +	/* Allow mutex to protect primary/secondary */
> +	ret = pthread_mutexattr_setpshared(&attr, PTHREAD_PROCESS_SHARED);
> +	if (ret)
> +		ERROR("Cannot set mutex shared - %s", strerror(ret));

Why not returning an error here?
diff mbox series

Patch

diff --git a/drivers/net/failsafe/failsafe.c b/drivers/net/failsafe/failsafe.c
index e3bda0df2bf9..5b7e560dbc08 100644
--- a/drivers/net/failsafe/failsafe.c
+++ b/drivers/net/failsafe/failsafe.c
@@ -140,6 +140,11 @@  fs_mutex_init(struct fs_priv *priv)
 		ERROR("Cannot initiate mutex attributes - %s", strerror(ret));
 		return ret;
 	}
+	/* Allow mutex to protect primary/secondary */
+	ret = pthread_mutexattr_setpshared(&attr, PTHREAD_PROCESS_SHARED);
+	if (ret)
+		ERROR("Cannot set mutex shared - %s", strerror(ret));
+
 	/* Allow mutex relocks for the thread holding the mutex. */
 	ret = pthread_mutexattr_settype(&attr, PTHREAD_MUTEX_RECURSIVE);
 	if (ret) {