Re: [dpdk-dev] [PATCH 2/2] net/failsafe: fix primary/secondary mutex

["Gaëtan Rivet" <grive@u256.net>]

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 <andrew.rybchenko@oktetlabs.ru> wrote:
> > 
> >> On 6/8/21 6:42 PM, Stephen Hemminger wrote:
> >>> On Tue, 8 Jun 2021 11:00:37 +0300
> >>> Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru> 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 <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?    
> >>>>
> >>>> +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