Re: [PATCH v2 1/7] libmultipath: fix tur checker locking

[Bart Van Assche <Bart.VanAssche@wdc.com>]

On Fri, 2018-02-09 at 11:26 -0600, Benjamin Marzinski wrote:
> On Fri, Feb 09, 2018 at 04:15:34PM +0000, Bart Van Assche wrote:
> > On Thu, 2018-02-08 at 17:56 -0600, Benjamin Marzinski wrote:
> > >  static void cleanup_func(void *data)
> > >  {
> > > -	int holders;
> > > +	int running, holders;
> > >  	struct tur_checker_context *ct = data;
> > > -	pthread_spin_lock(&ct->hldr_lock);
> > > -	ct->holders--;
> > > -	holders = ct->holders;
> > > -	ct->thread = 0;
> > > -	pthread_spin_unlock(&ct->hldr_lock);
> > > +
> > > +	running = uatomic_xchg(&ct->running, 0);
> > > +	holders = uatomic_sub_return(&ct->holders, 1);
> > >  	if (!holders)
> > >  		cleanup_context(ct);
> > > +	if (!running)
> > > +		pause();
> > >  }
> > 
> > Hello Ben,
> > 
> > Why has the pause() call been added? I think it is safe to call pthread_cancel()
> > for a non-detached thread that has finished so I don't think that pause() call
> > is necessary.
> 
> Martin objected to having the threads getting detached as part of
> cancelling them (I think. I'm a little fuzzy on what he didn't like).
> But he definitely said he preferred the thread to start detached, so in
> this version, it does.  That's why we need the pause().  If he's fine with
> the threads getting detached later, I will happily replace the pause()
> with
> 
> if (running)
> 	pthread_detach(pthread_self());
> 
> and add pthread_detach(ct->thread) after the calls to
> pthread_cancel(ct->thread). Otherwise we need the pause() to solve your
> original bug.

Ah, thanks, I had overlooked that the tur checker detaches the checker thread. Have
you considered to add a comment above the pause() call that explains the purpose of
that call?

Thanks,

Bart.