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 CA587A0C44; Mon, 14 Jun 2021 16:45:09 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 714EC4111E; Mon, 14 Jun 2021 16:44:04 +0200 (CEST) Received: from out1-smtp.messagingengine.com (out1-smtp.messagingengine.com [66.111.4.25]) by mails.dpdk.org (Postfix) with ESMTP id 396014111D for ; Mon, 14 Jun 2021 16:44:03 +0200 (CEST) Received: from compute1.internal (compute1.nyi.internal [10.202.2.41]) by mailout.nyi.internal (Postfix) with ESMTP id A225C5C0182; Mon, 14 Jun 2021 10:44:01 -0400 (EDT) Received: from imap22 ([10.202.2.72]) by compute1.internal (MEProxy); Mon, 14 Jun 2021 10:44:01 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=u256.net; h= mime-version:message-id:in-reply-to:references:date:from:to:cc :subject:content-type; s=fm2; bh=0jUZKjffB2bVRMwS6aYwz04MDpaBOF1 ntW0u4kw1/BU=; b=BKDuCxaEB/uHWDqJ4+qT4BkK00J7ho7z3LM6AxRHtCZIyiw ARNq0JL2+3aHVRyAdCRy5xGLWqulOt9F/cNuuW/wi3IMcfzg5AzxxGkG3OrbM+pv Go27laDmC+mhw77ixZQcPZENaZm7ipeTmD/EDVX5j3tNNEcGdt9hatH9WsNaBuWQ LBGZ5z3kC6fVrn6ojp8duG3P06Dup4+Q9SHCBMdOarYy8VPb901vZPIWrthhHYHM mV2XQz3j2Mj5hyJvElKdb7B7en4heEynjf4npD4JRwZmR2qN+TPhuXAkq/DNO52T MDvc3COrRbkXDdORFOIs495XxZBKTppve/pUkjg== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-type:date:from:in-reply-to :message-id:mime-version:references:subject:to:x-me-proxy :x-me-proxy:x-me-sender:x-me-sender:x-sasl-enc; s=fm3; bh=0jUZKj ffB2bVRMwS6aYwz04MDpaBOF1ntW0u4kw1/BU=; b=TlXzSNBsDMiMmI8TvyDTks hzKfKsROefC6Ku0aRqR+bgqeekO0xy9PD8kmdI8OEltI6KtOfnjspy3ffJZEMBC1 ahfRONxbMw8xFJmumGN21CCCGtMBzDrfVfxfa8pUM8eIYyPdMeHUrzSpZNJq7QS5 ARyKu+niFWLJ3bEfjhssOQojV37JqJ8n62yjXjqh8faSzNr+s8kcOU4ob3rSc4/e pEzQwOpHrjcKnOOwRA8gBiCPSXdtnlh1At6kD2+ROhRd7RPRyWi0yHYG+YHQYMG9 firyq64TkklN0h/K9XbjOyhOTpuULEGgj2jjcfF8Q763c9MdVZzzxHJ+SY2qsYQQ == X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgeduledrfedvhedgjeejucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgen uceurghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmne cujfgurhepofgfggfkjghffffhvffutgesthdtredtreerjeenucfhrhhomhepifgrtoht rghnpgftihhvvghtuceoghhrihhvvgesuhdvheeirdhnvghtqeenucggtffrrghtthgvrh hnpefguefhffelleduueehueetueefgedtieevudduhfegveetuedtleeuffeuheekfeen ucevlhhushhtvghrufhiiigvpedtnecurfgrrhgrmhepmhgrihhlfhhrohhmpehgrhhivh gvsehuvdehiedrnhgvth X-ME-Proxy: Received: by mailuser.nyi.internal (Postfix, from userid 501) id CA4C862C0064; Mon, 14 Jun 2021 10:44:00 -0400 (EDT) X-Mailer: MessagingEngine.com Webmail Interface User-Agent: Cyrus-JMAP/3.5.0-alpha0-519-g27a961944e-fm-20210531.001-g27a96194 Mime-Version: 1.0 Message-Id: In-Reply-To: References: <20210315192722.35490-1-stephen@networkplumber.org> <20210315192722.35490-3-stephen@networkplumber.org> <6747934.v7ilQdk43l@thomas> <20210608084210.73d05f60@hermes.local> <20210608134824.3181b063@hermes.local> Date: Mon, 14 Jun 2021 16:43:40 +0200 From: =?UTF-8?Q?Ga=C3=ABtan_Rivet?= To: "Andrew Rybchenko" , "Stephen Hemminger" Cc: "Thomas Monjalon" , dev@dpdk.org, matan@nvidia.com Content-Type: text/plain 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 Wed, Jun 9, 2021, at 12:04, Andrew Rybchenko wrote: > 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. > On principle the API spec should be respected without assuming a specific implementation. Another way to think about it is that a future dev having zero knowledge of this thread, reading this code and checking the POSIX manual, will also need to check that usual c lib implementations are unlikely to generate an error before concluding that this code is alright. It should not be necessary. -- Gaetan Rivet