Re: [PATCH 1/3] multipathd: fix waiter thread cancelling

["Benjamin Marzinski" <bmarzins@redhat.com>]

On Mon, Mar 19, 2018 at 01:59:24PM +0100, Martin Wilck wrote:
> On Fri, 2018-03-16 at 16:31 -0500, Benjamin Marzinski wrote:
> > diff --git a/multipathd/waiter.c b/multipathd/waiter.c
> > index cb9708b..e894294 100644
> > --- a/multipathd/waiter.c
> > +++ b/multipathd/waiter.c
> > @@ -23,6 +23,7 @@
> >  #include "waiter.h"
> >  
> >  pthread_attr_t waiter_attr;
> > +struct mutex_lock waiter_lock = { .mutex = PTHREAD_MUTEX_INITIALIZER
> > };
> >  
> >  static struct event_thread *alloc_waiter (void)
> >  {
> > @@ -59,8 +60,12 @@ void stop_waiter_thread (struct multipath *mpp,
> > struct vectors *vecs)
> >  		mpp->waiter);
> >  	thread = mpp->waiter;
> >  	mpp->waiter = (pthread_t)0;
> > -	pthread_cancel(thread);
> > +	pthread_cleanup_push(cleanup_lock, &waiter_lock);
> > +	lock(&waiter_lock);
> > +	pthread_testcancel();
> 
> What's the purpose of this pthread_testcancel() call?
> 

It's not necessary. I feel like in general, after you get done waiting
on a lock, it's good form to see if you've been cancelled while you were
waiting. However, it's really unlikely that a thread will be waiting
here long, and this pthread_cancel isn't protecting it form accessing
any freed data. If you think it's likely to confuse people, I have no
problem with removing it.

> >  	pthread_kill(thread, SIGUSR2);
> > +	pthread_cancel(thread);
> > +	lock_cleanup_pop(&waiter_lock);
> >  }
> >  
> >  /*
> > @@ -114,8 +119,13 @@ static int waiteventloop (struct event_thread
> > *waiter)
> >  	dm_task_destroy(waiter->dmt);
> >  	waiter->dmt = NULL;
> >  
> > -	if (!r)	/* wait interrupted by signal */
> > -		return -1;
> > +	if (!r)	{ /* wait interrupted by signal. check for
> > cancellation */
> > +		pthread_cleanup_push(cleanup_lock, &waiter_lock);
> > +		lock(&waiter_lock);
> > +		pthread_testcancel();
> > +		lock_cleanup_pop(&waiter_lock);
> 
> Nitpick: I'd prefer pthread_cleanup_pop(1) here.

I have no problem with changing that.

-Ben

> Regards,
> Martin
> 
> 
> > +		return 1; /* If we weren't cancelled, just
> > reschedule */
> > +	}
> >  
> >  	waiter->event_nr++;
> >  
> 
> -- 
> Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
> SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
> HRB 21284 (AG Nürnberg)