From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id DA2BDA0C46; Wed, 9 Jun 2021 12:04:19 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 5AA5A4069B; Wed, 9 Jun 2021 12:04:19 +0200 (CEST) Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [91.220.146.113]) by mails.dpdk.org (Postfix) with ESMTP id 82F6F4003C for ; Wed, 9 Jun 2021 12:04:18 +0200 (CEST) Received: from [192.168.38.17] (aros.oktetlabs.ru [192.168.38.17]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by shelob.oktetlabs.ru (Postfix) with ESMTPSA id 19E827F66E; Wed, 9 Jun 2021 13:04:18 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 shelob.oktetlabs.ru 19E827F66E DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=oktetlabs.ru; s=default; t=1623233058; bh=aFOQtztl2PKTHCSirdIcX3rwPW38bDiacUmd1Hu0IlM=; h=Subject:To:Cc:References:From:Date:In-Reply-To; b=g14hvGeUcG7dfvybkdiucQR+1rHPuk5Jvs5QtggZrSb1snXB1kbM7S+x7IJxg/NCi vDYyJKpTxlgWQYiejiI70w1pFEiJ2Ep4gojgCT7+OPnhPNbfEdPMW7kyU07esbpRAH /cfu75RwHSj26T52HovKC1qS/cRxMP4LypPGUqQM= To: Stephen Hemminger Cc: Thomas Monjalon , dev@dpdk.org, matan@mellanox.com, Gaetan Rivet References: <20210315192722.35490-1-stephen@networkplumber.org> <20210315192722.35490-3-stephen@networkplumber.org> <6747934.v7ilQdk43l@thomas> <20210608084210.73d05f60@hermes.local> <20210608134824.3181b063@hermes.local> From: Andrew Rybchenko Organization: OKTET Labs Message-ID: Date: Wed, 9 Jun 2021 13:04:17 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.10.0 MIME-Version: 1.0 In-Reply-To: <20210608134824.3181b063@hermes.local> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH 2/2] net/failsafe: fix primary/secondary mutex X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On 6/8/21 11:48 PM, Stephen Hemminger wrote: > On Tue, 8 Jun 2021 18:55:17 +0300 > Andrew Rybchenko wrote: > >> On 6/8/21 6:42 PM, Stephen Hemminger wrote: >>> On Tue, 8 Jun 2021 11:00:37 +0300 >>> Andrew Rybchenko wrote: >>> >>>> On 4/19/21 8:08 PM, Thomas Monjalon wrote: >>>>> 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 >>>>>> 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 >>>>> >>>>>> --- >>>>>> --- 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? >>>> >>>> +1 >>>> >>>> I think it would be safer to return an error here. >>> >>> Ok but it never happens. >>> >> >> May I ask why? 'man pthread_mutexattr_setpshared' says that it >> is possible. >> > > The glibc implementation of pthread_mutexattr_setpshared is: > > > int > pthread_mutexattr_setpshared (pthread_mutexattr_t *attr, int pshared) > { > struct pthread_mutexattr *iattr; > > int err = futex_supports_pshared (pshared); > if (err != 0) > return err; > > iattr = (struct pthread_mutexattr *) attr; > > if (pshared == PTHREAD_PROCESS_PRIVATE) > iattr->mutexkind &= ~PTHREAD_MUTEXATTR_FLAG_PSHARED; > else > iattr->mutexkind |= PTHREAD_MUTEXATTR_FLAG_PSHARED; > > return 0; > } > > And > > /* FUTEX_SHARED is always supported by the Linux kernel. */ > static __always_inline int > futex_supports_pshared (int pshared) > { > if (__glibc_likely (pshared == PTHREAD_PROCESS_PRIVATE)) > return 0; > else if (pshared == PTHREAD_PROCESS_SHARED) > return 0; > else > return EINVAL; > } > > > There for the code as written can not return an error. > The check was only because someone could report a bogus > issue from a broken c library. > Many thanks for detailed description. I thought that it is better to follow API definition and it is not that hard to check return code and handle it. Yes, glibc is not the only C library.